[PATCH] D48240: Try again to implement a FIFO task queue

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

> Another example might be if you were writing a debugger which was debugging N processes simultaneously, and due to various OS restrictions, each of these N processes was being debugged by a separate thread.

We've had something similar to this in lldb (it was called ThreadStateCoordinator), but it was removed when we made the whole lldb-server process single-threaded.



================
Comment at: llvm/include/llvm/Support/TaskQueue.h:41-43
+    std::shared_future<void> ParentFuture;
+    std::shared_future<void> ThisFuture;
+    std::function<void()> ThisTask;
----------------
Are all of these really needed? The only use of ParentFuture I see is in the lambda in ThisFuture, and that one already has it captured. `ThisTask` is not captured by that lambda, but it could easily be, at which point the whole Task struct could just become `std::shared_future<void>`.


================
Comment at: llvm/lib/Support/TaskQueue.cpp:79
+    // Don't allow enqueueing after disabling the pool
+    assert(EnableFlag && "Queuing a thread during ThreadPool destruction");
+
----------------
The assert message could be updated to reflect the new home.


================
Comment at: llvm/lib/Support/TaskQueue.cpp:102-107
+  // Sequential implementation running the tasks
+  while (!Tasks.empty()) {
+    auto Task = std::move(Tasks.front());
+    Tasks.pop();
+    Task.ThisTask();
+  }
----------------
This could be implemented as `Tasks.back().wait()`, avoiding the direct access into the ThisTask function and making sure the tasks are always invoked the same way.


================
Comment at: llvm/unittests/Support/TaskQueueTest.cpp:64-70
+#if !LLVM_ENABLE_THREADS
+  // We can't check this in the LLVM_ENABLE_THREADS case since the work thread
+  // proceeds independently of the main thread, so this may or may not still
+  // be true.
+  ASSERT_EQ(0, Y);
+  ASSERT_EQ(0, Z);
+#endif
----------------
I'm not sure if it's necessary, but if you wanted to have the test be uniform for both kinds of builds, you could replace MainThreadReady with an integer variable and then have the tasks wait until that variable is >= some_value.


https://reviews.llvm.org/D48240