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

[Mehdi Amini via llvm-commits]

> On Dec 19, 2015, at 9:50 AM, Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
> 
> 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)

Fixed in r256111, along with some restructuring.

— 
Mehdi