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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for contributing this. A few questions and a few minor nits.


================
Comment at: include/llvm/Support/thread.h:46
@@ -45,1 +45,3 @@
 
+#include <utility>
+
----------------
Is this change needed? No other change here.

================
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());
----------------
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.

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

================
Comment at: lib/Support/ThreadPool.cpp:50
@@ +49,3 @@
+        Task();
+        --ActiveThreads;
+
----------------
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.

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

================
Comment at: lib/Support/ThreadPool.cpp:86
@@ +85,3 @@
+ThreadPool::~ThreadPool() {
+  EnableFlag = false;
+  QueueCondition.notify_all();
----------------
Do we need to get QueueLock before modifying EnableFlag? Same comment as earlier about ActiveThread (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.
)

================
Comment at: lib/Support/ThreadPool.cpp:117
@@ +116,3 @@
+  auto Future = std::async(std::launch::deferred, std::move(Task)).share();
+  // Wrap the future so that both Pool::wait() can operate and the returned
+  // future can be sync'ed on.
----------------
nit: ThreadPool::wait()

================
Comment at: unittests/Support/ThreadPool.cpp:31
@@ +30,3 @@
+  }
+  ASSERT_EQ(0, checked_in);
+  Pool.wait();
----------------
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?


http://reviews.llvm.org/D15464