[PATCH] D18811: Fix a race condition in support library ThreadPool

[Justin Lebar via llvm-commits]

jlebar added a comment.

> One lock is simpler to understand, however it adds unnecessary contention, which is what I was trying to avoid originally.


My preference is that we write simple, correct code and worry about performance separately, when we have explicit use-cases, benchmarks, etc.  Otherwise we're just prematurely optimizing.  Does that sound OK to you?


================
Comment at: include/llvm/Support/ThreadPool.h:130
@@ -131,3 +129,3 @@
   /// Signal for the destruction of the pool, asking thread to exit.
-  bool EnableFlag;
+  bool InDestructorFlag;
 #endif
----------------
joker.eph wrote:
> Is this related to the changes here? If not then split it out to a separate NFC patch, reducing the change in this patch to the bug it is actually fixing. Same for renaming `QueueCondition ` to `SubmissionCondition`.
This feels pretty unnecessary to me.  We're already changing a bunch of things in this class; who cares if we rename one additional variable in this patch as opposed to in a separate one?  All of these changes are related in that they're attempting to make it less likely that this class will have bugs in the future.

There's a cost to carrying small patches in your queue that are tightly intermingled with a base patch; every time you update the base patch, you have to rebase the dependent patch.

I like small patches too, but there is a line.  :)

================
Comment at: lib/Support/ThreadPool.cpp:39
@@ -40,1 +38,3 @@
+          SubmissionCondition.wait(
+              LockGuard, [&] { return InDestructorFlag || !Tasks.empty(); });
           // Exit condition
----------------
Oh, indeed, you're right; it's &&, not ||.  Carry on.  :)


http://reviews.llvm.org/D18811