[llvm] r256087 - ThreadPool unittest: reimplement concurrency test, deterministically this time.

[Mehdi Amini via llvm-commits]

Sent from my iPhone

> On Dec 18, 2015, at 8:30 PM, Vedant Kumar <vsk at apple.com> wrote:
> 
> Thanks :).
> 
>> +
>> +  /// Make sure this thread not progress faster than the main thread.
>> +  void waitForMainThread() {
>> +    while (!MainThreadReady) {
>> +      std::unique_lock<std::mutex> LockGuard(WaitMainThreadMutex);
>> +      WaitMainThread.wait(LockGuard, [&] { return MainThreadReady; });
> 
> Nit: wait(CV, Predicate) is guaranteed to not wake up spuriously. The surrounding while loop here is redundant.
> 
> 
>> +    }
>> +  }
>> +  std::condition_variable WaitMainThread;
>> +  std::mutex WaitMainThreadMutex;
>> +  bool MainThreadReady;
>> +
>> };
>> 
>> #define CHECK_UNSUPPORTED() \
>> @@ -68,12 +80,17 @@ TEST_F(ThreadPoolTest, AsyncBarrier) {
>> 
>>  std::atomic_int checked_in{0};
>> 
>> +  MainThreadReady = false;
>>  ThreadPool Pool;
>>  for (size_t i = 0; i < 5; ++i) {
>> -    Pool.async([&checked_in, i] {
>> +    Pool.async([this, &checked_in, i] {
>> +      waitForMainThread();
>>      ++checked_in;
>>    });
>>  }
>> +  ASSERT_EQ(0, checked_in);
>> +  MainThreadReady = true;
> 
> Paranoid nit: Can this assignment be reordered before the assert? If so, the main thread could context-switch to an async worker that's awake.

It's even worse: you're supposed to get the mutex before notifying a condition variable. I always forget that, Teresa already caught this on the original patch.



> Later, the assert will fail. Wdyt of changing this to --
> 
>  void setMainThreadReadyState(bool Ready) {
>    std::unique_lock<std::mutex> LockGuard(WaitMainThreadMutex);
>    MainThreadReady = Ready;
>  }
> 
>  setMainThreadReadyState(true);
> 
> That should still test the async behavior.

Would need the notify_all while holding the mutex. Otherwise LGTM.


Mehdi

> 
> vedant
> 
>> +  WaitMainThread.notify_all();
>>  Pool.wait();
>>  ASSERT_EQ(5, checked_in);
>> }
>> @@ -97,10 +114,15 @@ TEST_F(ThreadPoolTest, Async) {
>>  CHECK_UNSUPPORTED();
>>  ThreadPool Pool;
>>  std::atomic_int i{0};
>> -  Pool.async([&i] {
>> +  MainThreadReady = false;
>> +  Pool.async([this, &i] {
>> +    waitForMainThread();
>>    ++i;
>>  });
>>  Pool.async([&i] { ++i; });
>> +  ASSERT_NE(2, i.load());
>> +  MainThreadReady = true;
>> +  WaitMainThread.notify_all();
>>  Pool.wait();
>>  ASSERT_EQ(2, i.load());
>> }
>> @@ -109,11 +131,16 @@ TEST_F(ThreadPoolTest, GetFuture) {
>>  CHECK_UNSUPPORTED();
>>  ThreadPool Pool;
>>  std::atomic_int i{0};
>> -  Pool.async([&i] {
>> +  MainThreadReady = false;
>> +  Pool.async([this, &i] {
>> +    waitForMainThread();
>>    ++i;
>>  });
>>  // Force the future using get()
>>  Pool.async([&i] { ++i; }).get();
>> +  ASSERT_NE(2, i.load());
>> +  MainThreadReady = true;
>> +  WaitMainThread.notify_all();
>>  Pool.wait();
>>  ASSERT_EQ(2, i.load());
>> }
>> @@ -122,14 +149,18 @@ TEST_F(ThreadPoolTest, PoolDestruction)
>>  CHECK_UNSUPPORTED();
>>  // Test that we are waiting on destruction
>>  std::atomic_int checked_in{0};
>> -
>>  {
>> +    MainThreadReady = false;
>>    ThreadPool Pool;
>>    for (size_t i = 0; i < 5; ++i) {
>> -      Pool.async([&checked_in, i] {
>> +      Pool.async([this, &checked_in, i] {
>> +        waitForMainThread();
>>        ++checked_in;
>>      });
>>    }
>> +    ASSERT_EQ(0, checked_in);
>> +    MainThreadReady = true;
>> +    WaitMainThread.notify_all();
>>  }
>>  ASSERT_EQ(5, checked_in);
>> }
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>