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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Really nice approach for THREADS=0 w/ the deferred future that checks that "enough" of the queue has been processed. I like it.

If you want to avoid the address stability stuff, you could always keep an integer next to the things in the queue and track the integers...

I want to think a bit harder to understand if using the address creates an A-B-A problem. I feel like it does, but my brain is exhausted and not able to pin it down yet.

Unrelated comments inline....



================
Comment at: llvm/include/llvm/Support/TaskQueue.h:32-33
+namespace llvm {
+/// TaskQueue executes work on exactly 1 thread in a first in first out
+/// fashion.  All work is guaranteed to execute in the order it is received.
+class TaskQueue {
----------------
Documentation needs updating based on the new design.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:47
+#if LLVM_ENABLE_THREADS
+    Scheduler.wait();
+#else
----------------
Assert that the queue is empty at this point?

Also, rather than waiting on the entire thread-pool, maybe enqueue a no-op future into the queue and wait on that? We don't need the underlying executor to finish, just the queue to empty.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:65-68
+      if (IsTaskInFlight)
+        Tasks.push_back(std::move(Func));
+    }
+    Scheduler.async(std::move(Func));
----------------
This doesn't seem right...

Only if there are *no* tasks in flight do you want to mov this into the scheduler's async. And when you do, you want to set tasks in flight to be true.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:89
+  std::pair<std::function<void()>, std::future<void>> CreateTask(Callable &&C) {
+    auto P = std::make_shared<std::promise<void>>();
+
----------------
Add a FIXME about the std::function issue? D48349 has unique_function to eventually fix this.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:96
+    // std::function.
+    std::function<void()> WrappedTask = [this, C, P] {
+      C();
----------------
FWIW, the copy of std::function, and the eventual move of llvm::unique_function won't be free. It may be worthwhile to keep this in a non-type-erased state by making CreateTask here a lambda within the async function above. =/ Just something to think about.


https://reviews.llvm.org/D48240