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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Some suggestions about the way you package things and manage `std::future` stuff below.

Some higher level comments here...

The term I think you're looking for is "synchronized" or "serial". So I would call this a `SynchronizedTaskQueue` or `SerializedTaskQueue`. Given the other use of the term "serialized", I'd lean towards `Synchronized` or `Sync` if that's too long to type. Ultimately, the name needs to make it clear that it enforces that the tasks scheduled here execute without overlap.

I also wonder whether you want to decouple this serialized execution from the thread creation and management. Could you accept a thread pool instead, and enqueue work on the thread pool? You can do this by having each task on the thread pool, on completion, check the queue for more work, and if there is some, schedule a new task on the thread pool. The reason why this is interesting is because if you have, say, a system thread pool that maybe gets a lot of other work, you can intersperse these tasks on it without overloading the system. It also allows the client of this class to dictate which threads are allowable threads for these tasks to run by providing a correct thread pool. My hope is that worst case, creating a one-thread thread pool and handing it to this class would be the equivalent of what you have here, but with a more clean separation of roles. I understand it would add some complexity, for example you'd have to handle the case where no work is currently in-flight when `asyncImpl` finishes adding the task to the queue and start a new batch. But this seems reasonably tolerable.



================
Comment at: llvm/include/llvm/Support/TaskQueue.h:35-41
+  using TaskFunctionTy = std::function<void()>;
+
+#if LLVM_ENABLE_THREADS
+  using TaskTy = std::packaged_task<void()>;
+#else
+  using TaskTy = std::shared_future<void>;
+#endif
----------------
Rather than these types, I would suggest you build your task more like the following:

```
template <typename Callable>
std::future<void> async(Callable &&C) {
  // Because we don't have init capture to use move-only local variables that
  // are captured into a lambda, we create the promise inside an explicit callable
  // struct. We want to do as much of the wrapping in the type-specialized domain
  // (before type erasure) and then erase this into a std::function.
  struct Task {
    Callable C;
    std::promise<void> P;

    void operator()() noexcept {
      // Note, we don't catch exceptions and store them in the promise here because
      // we don't support exception handling in LLVM.
      C();
      P.set_value(C());
    }
  };

  Task T = {std::forward<Callable>(C)};
  std::future<void> F = T.P.get_future();
  asyncImpl(std::move(T));
  return F;
}
```

This is somewhat similar to what `std::packaged_task` does, but due to how `std::packaged_task` works, this will probably end up a bit lighter weight (specifically, `std::packaged_task` type erases the callable separately from the promise, while this packages the callable and the promise together ahead of time and then type erases it afterward.

The end result is that your task queue operates in a super simple closure type: `std::function<void()>`. This will be true even when you produce a value from the callable and store it into the promise. And this should also work even when you have no threading support at all. `std::shared_future` is a lot more expensive ultimately than `std::future`.

Does all of this make sense?


And FWIW, this is where C++14 is actually helpful, as there the above goop looks a lot cleaner:
```
template <typename Callable>
std::future<void> async(Callable &&C) {
  std::promise<void> P;
  std::future<void> F = P.get_future();
  asyncImpl([C = std::forward<Callable>(C), P = std::move(P)] {
      // Note, we don't catch exceptions and store them in the promise here because
      // we don't support exception handling in LLVM.
      C();
      P.set_value(C());
  });

  return F;
}
```


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:49-57
+  /// Asynchronous submission of a task to the queue. The returned future can be
+  /// used to wait for the task (and all previous tasks that have not yet
+  /// completed) to finish.
+  template <typename Function, typename... Args>
+  inline std::shared_future<void> async(Function &&F, Args &&... ArgList) {
+    auto Task =
+        std::bind(std::forward<Function>(F), std::forward<Args>(ArgList)...);
----------------
I regularly find std::bind to be problematic and hard to understand. Rather than expose this at all, I would force clients to handle it by passing you a lambda (or a std::bind if they so choose) that handles all of the argument binding. Then you just have a callable (the overload below).


================
Comment at: llvm/include/llvm/Support/TaskQueue.h:62-63
+  /// completed) to finish.
+  template <typename Function>
+  inline std::shared_future<void> async(Function &&F) {
+    return asyncImpl(std::forward<Function>(F));
----------------
I would term this a `Callable` as it may not be a function at all. The most common case I suspect will be a lambda.


https://reviews.llvm.org/D48240