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

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 6 15:25:24 PDT 2016


joker.eph added a comment.

> 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?


Well, one can use exactly the same sentence to argue against threading at all in the first place :)
This implementation is pretty dumb anyway and I don't expect it to scale, it is convenient for coarse grain tasks, so I am indeed not really worried about the contention part here.
There is always a tradeoff to be acknowledged, I wanted to make it clear here, because I felt it wasn't from the original description. 
(i.e. threads will now take the lock and release it both when popping from the queue, and when the task is completed).


================
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
----------------
jlebar wrote:
> 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.  :)
This does not seems unnecessary to me, and this is in line with the request I usually receive on my patches. `git blame` and `git show` are a lot more friendly to analyze why the code is the way this way. As we say `written once, read many`...
(also minimizing diff helps the review)

As of the cost you're mentioning, you don't have to carry small patches in your queue as you can commit the NFC/renaming change immediately without going through any review. 
It seems totally "free" to me so I don't really see why not doing it?




http://reviews.llvm.org/D18811





More information about the llvm-commits mailing list