[PATCH] D148728: [Support][Parallel] Add sequential mode to TaskGroup::spawn().

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: llvm/lib/Support/Parallel.cpp:16
 #include <future>
-#include <stack>
+#include <queue>
 #include <thread>
----------------
Unfortunately, at least in my testing on Windows, this change causes a drop in performance of around 3-4%. I believe that's why `std::stack` is used.


================
Comment at: llvm/lib/Support/Parallel.cpp:102-106
       std::lock_guard<std::mutex> Lock(Mutex);
-      WorkStack.push(std::move(F));
+      if (Sequential)
+        WorkQueueSequential.emplace(std::move(F));
+      else
+        WorkQueue.emplace(std::move(F));
----------------
Would this be nicer?


================
Comment at: llvm/lib/Support/Parallel.cpp:112
 private:
+  bool hasSequentialTasks() {
+    return !WorkQueueSequential.empty() && !SequentialQueueIsLocked;
----------------
Make `const`?


================
Comment at: llvm/lib/Support/Parallel.cpp:116
+
+  bool hasGeneralTasks() { return !WorkQueue.empty(); }
+
----------------
Make `const`?


================
Comment at: llvm/lib/Support/Parallel.cpp:133
+        Lock.unlock();
+        Task();
+        SequentialQueueIsLocked = false;
----------------
MaskRay wrote:
> After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
Yes, this can happen but I believe the key point is that it can only be a thread with a different "index". However, one subtlety is that "sequential" tasks will run on different threads and thus use a different "index".


================
Comment at: llvm/lib/Support/Parallel.cpp:147
+  std::queue<std::function<void()>> WorkQueue;
+  std::atomic<bool> SequentialQueueIsLocked{false};
+  std::queue<std::function<void()>> WorkQueueSequential;
----------------
I think that moving this up after the other `std::atomic` would feel better.


================
Comment at: llvm/unittests/Support/ParallelTest.cpp:97-121
+TEST(Parallel, TaskGroupSequential) {
+  size_t Count = 0;
+  {
+    parallel::TaskGroup tg;
+
+    tg.spawn([&]() { Count++; }, true);
+
----------------
I'm not entirely sure what this test case adds in terms of coverage that isn't covered by the test case that follows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148728