[PATCH] D15464: Add a C++11 ThreadPool implementation in LLVM

[Mehdi AMINI via llvm-commits]

joker.eph marked 3 inline comments as done.
joker.eph added a comment.

Thanks for the review!


================
Comment at: include/llvm/Support/thread.h:46
@@ -45,1 +45,3 @@
 
+#include <utility>
+
----------------
tejohnson wrote:
> Is this change needed? No other change here.
Yes it is needed for std::forward, the unittests can't be compiled with LLVM_ENABLE_THREADS=OFF without this.

================
Comment at: lib/Support/ThreadPool.cpp:44
@@ +43,3 @@
+          // Yeah, we have a task, grab it and release the lock on the queue
+          ++ActiveThreads;
+          Task = std::move(Tasks.front());
----------------
tejohnson wrote:
> Might want to note that the order of operations is important here. The increment of ActiveThreads needs to happen before the pop() or the CompletionCondition in wait() might trigger early.
done

================
Comment at: lib/Support/ThreadPool.cpp:48
@@ +47,3 @@
+        }
+        // Run the task we just grabed
+        Task();
----------------
tejohnson wrote:
> Typo: grabbed
oops

================
Comment at: lib/Support/ThreadPool.cpp:50
@@ +49,3 @@
+        Task();
+        --ActiveThreads;
+
----------------
tejohnson wrote:
> Do we need to get a lock before modifying ActiveThreads (and use the same lock to guard the wait on CompletionCondition further down? According to http://en.cppreference.com/w/cpp/thread/condition_variable:
> 
> > Even if the shared variable is atomic, it must be modified under the mutex in order to correctly publish the modification to the waiting thread.
I tried to be too clever :(

Added another lock

================
Comment at: lib/Support/ThreadPool.cpp:52
@@ +51,3 @@
+
+        // Notify task completion, in case someone waits on Pool::wait()
+        CompletionCondition.notify_all();
----------------
tejohnson wrote:
> nit: ThreadPool::wait()
done

================
Comment at: unittests/Support/ThreadPool.cpp:31
@@ +30,3 @@
+  }
+  ASSERT_EQ(0, checked_in);
+  Pool.wait();
----------------
tejohnson wrote:
> This is expected to be 0 because the sleep in the threads is expected to delay their execution sufficiently? It seems like that's how all of the tests work. I wonder if that will always be enough to guarantee that the first assert executes before the first thread increments. I.e could the main thread ever get delayed under some circumstance, and a child thread update first? Wondering if the pre-wait() asserts are really necessary.
> 
> Why are some of the sleeps for 50ms and some for 500ms?
Yes, with LLVM_ENABLE_THREADS=OFF it is deterministic, but not with real threads.
I tried to improve it using added `std::this_thread::yield()`, even if it still theoretically possible to break the test, it should be unlikely enough to not happen in practice.


http://reviews.llvm.org/D15464