[PATCH] D78856: [Support] Simplify and optimize ThreadPool

[Fangrui Song via Phabricator via llvm-commits]

MaskRay updated this revision to Diff 260115.
MaskRay added a comment.

Avoid spurious wake-up of waiter


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78856/new/

https://reviews.llvm.org/D78856

Files:
  llvm/include/llvm/Support/ThreadPool.h
  llvm/lib/Support/ThreadPool.cpp


Index: llvm/lib/Support/ThreadPool.cpp
===================================================================
--- llvm/lib/Support/ThreadPool.cpp
+++ llvm/lib/Support/ThreadPool.cpp
@@ -21,8 +21,7 @@
 #if LLVM_ENABLE_THREADS
 
 ThreadPool::ThreadPool(ThreadPoolStrategy S)
-    : ActiveThreads(0), EnableFlag(true),
-      ThreadCount(S.compute_thread_count()) {
+    : ThreadCount(S.compute_thread_count()) {
   // Create ThreadCount threads that will loop forever, wait on QueueCondition
   // for tasks to be queued or the Pool to be destroyed.
   Threads.reserve(ThreadCount);
@@ -44,24 +43,23 @@
           // We first need to signal that we are active before popping the queue
           // in order for wait() to properly detect that even if the queue is
           // empty, there is still a task in flight.
-          {
-            std::unique_lock<std::mutex> LockGuard(CompletionLock);
-            ++ActiveThreads;
-          }
+          ++ActiveThreads;
           Task = std::move(Tasks.front());
           Tasks.pop();
         }
         // Run the task we just grabbed
         Task();
 
+        bool Notify;
         {
           // Adjust `ActiveThreads`, in case someone waits on ThreadPool::wait()
-          std::unique_lock<std::mutex> LockGuard(CompletionLock);
-          --ActiveThreads;
+          std::lock_guard<std::mutex> LockGuard(QueueLock);
+          Notify = --ActiveThreads == 0 && Tasks.empty();
         }
-
-        // Notify task completion, in case someone waits on ThreadPool::wait()
-        CompletionCondition.notify_all();
+        // Notify task completion if this is the last active thread, in case
+        // someone waits on ThreadPool::wait().
+        if (Notify)
+          CompletionCondition.notify_all();
       }
     });
   }
@@ -69,7 +67,7 @@
 
 void ThreadPool::wait() {
   // Wait for all threads to complete and the queue to be empty
-  std::unique_lock<std::mutex> LockGuard(CompletionLock);
+  std::unique_lock<std::mutex> LockGuard(QueueLock);
   // The order of the checks for ActiveThreads and Tasks.empty() matters because
   // any active threads might be modifying the Tasks queue, and this would be a
   // race.
@@ -109,7 +107,7 @@
 
 // No threads are launched, issue a warning if ThreadCount is not 0
 ThreadPool::ThreadPool(ThreadPoolStrategy S)
-    : ActiveThreads(0), ThreadCount(S.compute_thread_count()) {
+    : ThreadCount(S.compute_thread_count()) {
   if (ThreadCount != 1) {
     errs() << "Warning: request a ThreadPool with " << ThreadCount
            << " threads, but LLVM_ENABLE_THREADS has been turned off\n";
Index: llvm/include/llvm/Support/ThreadPool.h
===================================================================
--- llvm/include/llvm/Support/ThreadPool.h
+++ llvm/include/llvm/Support/ThreadPool.h
@@ -86,16 +86,15 @@
   std::mutex QueueLock;
   std::condition_variable QueueCondition;
 
-  /// Locking and signaling for job completion
-  std::mutex CompletionLock;
+  /// Signaling for job completion
   std::condition_variable CompletionCondition;
 
   /// Keep track of the number of thread actually busy
-  std::atomic<unsigned> ActiveThreads;
+  unsigned ActiveThreads = 0;
 
 #if LLVM_ENABLE_THREADS // avoids warning for unused variable
   /// Signal for the destruction of the pool, asking thread to exit.
-  bool EnableFlag;
+  bool EnableFlag = true;
 #endif
 
   unsigned ThreadCount;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78856.260115.patch
Type: text/x-patch
Size: 3403 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200425/66b7382b/attachment.bin>