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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In https://reviews.llvm.org/D48240#1138646, @zturner wrote:

> Updated with suggestions from chandler.  I'm not sure why all my tests passed even despite having that bug that he noticed. Would be nice if there were a way to write a test for that case, but I'm not sure how.


I think what happened was that all tasks were inserted into the scheduler unconditionally. But, since that was a one-thread scheduler which happens to process tasks in sequence, everything worked as it should.

It would be interesting to have a test with scheduler with at least two threads which verifies that the tasks are still processed serially. You could have the first task block and then validate that the second task hasn't been started.



================
Comment at: llvm/include/llvm/Support/TaskQueue.h:37
+#if LLVM_ENABLE_THREADS
+  template <typename T> friend class Task;
+  // Because we don't have init capture to use move-only local variables that
----------------
`Task` uses different tags  here (class) and in the definition (struct). Also, I think this friend declaration isn't really needed as Task should have access to TaskQueue internals anyway.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:52-66
+      // We just completed a task.  If there are no more tasks in the queue,
+      // update IsTaskInFlight to false and stop doing work.  Otherwise
+      // schedule the next task (while not holding the lock).
+      std::function<void()> Continuation;
+      {
+        std::lock_guard<std::mutex> Lock(Parent->QueueLock);
+        if (Parent->Tasks.empty()) {
----------------
Maybe this part could be moved to a function in the TaskQueue class? It means less code would need to be duplicated between different Task instantiations, and the management of TaskQueue internal data structures would stay within that class.


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:131-132
+        Tasks.front()();
+        Tasks.pop_front();
+      } while (NextPtr != ElementPtr);
+    };
----------------
Here you're using NextPtr **after** you have popped the element it points to off the queue. I think this is technically UB. Instead of that you could just have bool `Done` variable which you set at the beginning of the loop.

After that ABA should not be a problem since you're only ever using the `ElementPtr` pointer while it is live.


https://reviews.llvm.org/D48240