Page MenuHomePhabricator

Add a C++11 ThreadPool implementation in LLVM
ClosedPublic

Authored by mehdi_amini on Dec 11 2015, 1:45 PM.

Details

Summary

This is a very simple implementation of a thread pool using C++11
thread. It accepts any std::function<void()> for asynchronous
execution. Individual task can be synchronize using the returned
future, or the client can block on the full queue completion.

In case LLVM is configured with Threading disabled, it falls back
to sequential execution using std::async with launch:deferred.

This is intended to support parallelism for ThinLTO processing in
linker plugin, but is generic enough to be for any other uses.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Add a C++11 ThreadPool implementation in LLVM.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini updated this object.
mehdi_amini added a subscriber: llvm-commits.

Fix some comments

tejohnson edited edge metadata.Dec 11 2015, 10:58 PM

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

include/llvm/Support/thread.h
46

Is this change needed? No other change here.

lib/Support/ThreadPool.cpp
45

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.

49

Typo: grabbed

51

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.

53

nit: ThreadPool::wait()

87

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.

)

118

nit: ThreadPool::wait()

unittests/Support/ThreadPool.cpp
32

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?

mehdi_amini edited edge metadata.

Update following Teresa's comments.

mehdi_amini marked 3 inline comments as done.Dec 12 2015, 12:04 AM

Thanks for the review!

include/llvm/Support/thread.h
46

Yes it is needed for std::forward, the unittests can't be compiled with LLVM_ENABLE_THREADS=OFF without this.

lib/Support/ThreadPool.cpp
45

done

49

oops

51

I tried to be too clever :(

Added another lock

53

done

unittests/Support/ThreadPool.cpp
32

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.

tejohnson accepted this revision.Dec 12 2015, 1:59 PM
tejohnson edited edge metadata.

Thanks! Will change my gold-plugin patch to use the thread pool once this is in.

lib/Support/ThreadPool.cpp
45

Nit: typo "popping"

139

Is this just for completeness? It doesn't look like this flag is used when ! LLVM_ENABLE_THREADS.

unittests/Support/ThreadPool.cpp
32

Ok, that seems reasonable. We can reevaluate if the test behaves flakily.

This revision is now accepted and ready to land.Dec 12 2015, 1:59 PM
mehdi_amini closed this revision.Dec 12 2015, 2:58 PM
mehdi_amini marked 2 inline comments as done.

r255444

lib/Support/ThreadPool.cpp
139

It is to silent a compiler warning about "member variable not used" when compiling with LLVM_ENABLE_THREAD=OFF

Here's the fix I mentioned on IRC:

diff --git a/include/llvm/Support/ThreadPool.h b/include/llvm/Support/ThreadPool.h
index c388c07..4b29392 100644

  • a/include/llvm/Support/ThreadPool.h

+++ b/include/llvm/Support/ThreadPool.h
@@ -50,7 +50,7 @@ public:

template <typename Function, typename... Args>
inline std::shared_future<void> async(Function &&F, Args &&... ArgList) {
  auto Task =
  • std::bind(std::forward<Function>(F), std::forward<Args...>(ArgList...));

+ std::bind(std::forward<Function>(F), std::forward<Args>(ArgList)...);

  return asyncImpl(Task);
}

diff --git a/unittests/Support/ThreadPool.cpp b/unittests/Support/ThreadPool.cpp
index d36341e..5457cdc 100644

  • a/unittests/Support/ThreadPool.cpp

+++ b/unittests/Support/ThreadPool.cpp
@@ -44,6 +44,20 @@ TEST(ThreadPoolTest, AsyncBarrier) {

ASSERT_EQ(5, checked_in);

}

+static void TestFunc(std::atomic_int &checked_in, int i) { checked_in += i; }
+
+TEST(ThreadPoolTest, AsyncBarrierArgs) {
+ // Test that async works with a function requiring multiple parameters.
+ std::atomic_int checked_in{0};
+
+ ThreadPool Pool;
+ for (size_t i = 0; i < 5; ++i) {
+ Pool.async(TestFunc, std::ref(checked_in), i);
+ }
+ Pool.wait();
+ ASSERT_EQ(10, checked_in);
+}
+
TEST(ThreadPoolTest, Async) {

ThreadPool Pool;

recommitted r255593, trying to please MSVC.

Teresa: your patch LGTM, feel free to commit on top of it.

This breaks the LLVM build with mingw-w64, which doesn't have C++11 threading support as far as I know. It would be nice if the threading-disabled version didn't use any of that stuff (mutexes, futures, etc) such that LLVM can be built with mingw-w64.

There is no inclusion of <thread> when threading is disabled right now. However #include <future> is necessary even without threading.
If some toolchains can't provide std::future and std::async (with std::launch::deferred) then someone needs to provide a patch to implement a include/llvm/Support/future.h on the model of include/llvm/Support/future.h. I'll be happy to review.