Skip to content

Commit 1752d7b

Browse files
committed
Fixed reused slot counting in implicit producer hash table which can again cause the 0.75 load invariant to be broken (almost certainly the main cause of #288)
1 parent e4245f8 commit 1752d7b

1 file changed

Lines changed: 9 additions & 19 deletions

File tree

concurrentqueue.h

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ namespace moodycamel { namespace details {
218218
// Finally, iOS/ARM doesn't have support for it either, and g++/ARM allows it to compile but it's unconfirmed to actually work
219219
#if (!defined(_MSC_VER) || _MSC_VER >= 1900) && (!defined(__MINGW32__) && !defined(__MINGW64__) || !defined(__WINPTHREADS_VERSION)) && (!defined(__GNUC__) || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8)) && (!defined(__APPLE__) || !TARGET_OS_IPHONE) && !defined(__arm__) && !defined(_M_ARM) && !defined(__aarch64__)
220220
// Assume `thread_local` is fully supported in all other C++11 compilers/platforms
221-
#define MOODYCAMEL_CPP11_THREAD_LOCAL_SUPPORTED // tentatively enabled for now; years ago several users report having problems with it on
221+
//#define MOODYCAMEL_CPP11_THREAD_LOCAL_SUPPORTED // tentatively enabled for now; years ago several users report having problems with it on
222222
#endif
223223
#endif
224224
#endif
@@ -3211,12 +3211,6 @@ class ConcurrentQueue
32113211
//////////////////////////////////
32123212

32133213
ProducerBase* recycle_or_create_producer(bool isExplicit)
3214-
{
3215-
bool recycled;
3216-
return recycle_or_create_producer(isExplicit, recycled);
3217-
}
3218-
3219-
ProducerBase* recycle_or_create_producer(bool isExplicit, bool& recycled)
32203214
{
32213215
#ifdef MCDBGQ_NOLOCKFREE_IMPLICITPRODHASH
32223216
debug::DebugLock lock(implicitProdMutex);
@@ -3227,13 +3221,11 @@ class ConcurrentQueue
32273221
bool expected = true;
32283222
if (ptr->inactive.compare_exchange_strong(expected, /* desired */ false, std::memory_order_acquire, std::memory_order_relaxed)) {
32293223
// We caught one! It's been marked as activated, the caller can have it
3230-
recycled = true;
32313224
return ptr;
32323225
}
32333226
}
32343227
}
3235-
3236-
recycled = false;
3228+
32373229
return add_producer(isExplicit ? static_cast<ProducerBase*>(create<ExplicitProducer>(this)) : create<ImplicitProducer>(this));
32383230
}
32393231

@@ -3486,15 +3478,11 @@ class ConcurrentQueue
34863478
// to finish being allocated by another thread (and if we just finished allocating above, the condition will
34873479
// always be true)
34883480
if (newCount < (mainHash->capacity >> 1) + (mainHash->capacity >> 2)) {
3489-
bool recycled;
3490-
auto producer = static_cast<ImplicitProducer*>(recycle_or_create_producer(false, recycled));
3481+
auto producer = static_cast<ImplicitProducer*>(recycle_or_create_producer(false));
34913482
if (producer == nullptr) {
34923483
implicitProducerHashCount.fetch_sub(1, std::memory_order_relaxed);
34933484
return nullptr;
34943485
}
3495-
if (recycled) {
3496-
implicitProducerHashCount.fetch_sub(1, std::memory_order_relaxed);
3497-
}
34983486

34993487
#ifdef MOODYCAMEL_CPP11_THREAD_LOCAL_SUPPORTED
35003488
producer->threadExitListener.callback = &ConcurrentQueue::implicit_producer_thread_exited_callback;
@@ -3508,11 +3496,13 @@ class ConcurrentQueue
35083496
auto empty = details::invalid_thread_id;
35093497
#ifdef MOODYCAMEL_CPP11_THREAD_LOCAL_SUPPORTED
35103498
auto reusable = details::invalid_thread_id2;
3511-
if (mainHash->entries[index].key.compare_exchange_strong(empty, id, std::memory_order_seq_cst, std::memory_order_relaxed) ||
3512-
mainHash->entries[index].key.compare_exchange_strong(reusable, id, std::memory_order_seq_cst, std::memory_order_relaxed)) {
3513-
#else
3514-
if (mainHash->entries[index].key.compare_exchange_strong(empty, id, std::memory_order_seq_cst, std::memory_order_relaxed)) {
3499+
if (mainHash->entries[index].key.compare_exchange_strong(reusable, id, std::memory_order_seq_cst, std::memory_order_relaxed)) {
3500+
implicitProducerHashCount.fetch_sub(1, std::memory_order_relaxed); // already counted as a used slot
3501+
mainHash->entries[index].value = producer;
3502+
break;
3503+
}
35153504
#endif
3505+
if (mainHash->entries[index].key.compare_exchange_strong(empty, id, std::memory_order_seq_cst, std::memory_order_relaxed)) {
35163506
mainHash->entries[index].value = producer;
35173507
break;
35183508
}

0 commit comments

Comments
 (0)