[PATCH] D78856: [Support] Simplify and optimize ThreadPool

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

> No. Only the thread(s) calling ThreadPool::Wait() waits on CompletionCondition. When it gets stuck, it will not awake spuriously. Note, before, we took locks 3 times in the loop body. Now it is 2...

I wasn't worried about contention with the waiting threads, but between working threads: they are taking the QueueLock more often now (it was taken once or twice per iteration of the loop body before and is now taken an extra time).
I am not sure we'd be able to measure any difference here, just curious how you thought about this tradeoff!

Originally I think I started writing all this code with `ActiveThreads` being an atomic so that the working threads can increment/decrement it without taking any lock: however adding the "wait()" then I couldn't see how to avoid taking the `CompletionLock` because of how std::condition_variable is setup. 
That said since the waiting thread takes the QueueLock with your change, would leaving the  `ActiveThreads` atomic allow to not take the Queue lock on task completion? 
I'm wondering about something like this:

  // Notify task completion if this is the last active thread, in case
  // someone waits on ThreadPool::wait().
  bool Notify = --ActiveThreads == 0;
  if (Notify)
    CompletionCondition.notify_all();





================
Comment at: llvm/lib/Support/ThreadPool.cpp:57
+          std::lock_guard<std::mutex> LockGuard(QueueLock);
+          Notify = --ActiveThreads == 0;
         }
----------------
mehdi_amini wrote:
> Can you expand this over different variables/statements? Or at minima add explicit parentheses? I am sure the compiler parses it, but not me :)
Can you add parentheses or split the expression?
```
          Notify = (--ActiveThreads == 0) && Tasks.empty();
```
or
```
          --ActiveThreads;
          Notify = !ActiveThreads && Tasks.empty();
```
The latter avoid the side effect on ActiveThreads in the expression, making the second line "pure" and easier to read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78856/new/

https://reviews.llvm.org/D78856