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

[Mehdi Amini via llvm-commits]

Sent from my iPhone

> On Dec 18, 2015, at 11:59 PM, Vedant Kumar <vsk at apple.com> wrote:
> 
> 
>>> 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.
> 
> Done, r256096.
> 

Thanks!

(But I was terribly wrong about the condition variable and the lock, I'll fix it later. We need to hold the lock while changing the flag used in the condition::wait, but not during the notify)

Mehdi

> vedant