[PATCH] D123225: [ThreadPool] add ability to group tasks into separate groups

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D123225#3435142 <https://reviews.llvm.org/D123225#3435142>, @mehdi_amini wrote:

>> In essence the call stack would be: the thread loop in ThreadPool::grow() calls Task() which would call the TaskGroup logic lambda, which would call the user lambda. Regular non-TaskGroup would not go through that logic.
>
> I don't quite get what you mean? Can you elaborate a bit? 
> I agree that the complexity increase is worrisome, so any idea to manage it is welcome :)

I'm just saying that we shouldn't be modifying `ThreadPool::grow` and instead implement the `TaskGroup` logic into a intermediate function, if that's possible. Instead of this currently:

  template <typename Func>
  auto async(TaskGroup &Group, Func &&F) -> std::shared_future<decltype(F())> {
    return asyncImpl(std::function<decltype(F())()>(std::forward<Func>(F)),
                     &Group);
  }

instead we could have this:

  template <typename Func>
  auto TaskGroup::async(TaskGroup &Group, Func &&F) -> std::shared_future<decltype(F())> {
    return asyncImpl([]() {
       // ...TaskGroup logic...
       F();
       // ...TaskGroup logic... (for example, notify the local TaskGroup condition when all group tasks are done)
    }, &Group);
  }

thus it makes sense to make `async()` and `wait()` members of `TaskGroup` and store the necessary state in the `TaskGroup` object itself (such a task counter -- currently stored by `DenseMap<TaskGroup *, unsigned> ActiveGroups`, a condition variable, etc.).
I feel the `TaskGroup`'s logic is a subset of the generalized case we already have.



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:68
+  template <typename Function, typename... Args>
+  inline auto async(TaskGroup &Group, Function &&F, Args &&...ArgList) {
+    auto Task =
----------------
clayborg wrote:
> aganea wrote:
> > Do you think we could have all these `TaskGroup`-specific functions inside the `TaskGroup` class instead? As an alternative, given your current usage, the tasks could be even queued in a `std::vector` prior, and passed to the `TaskGroup` constructor.
> I do like that idea, but if we do that it might be best to have TaskGroup take a "ThreadPool &" as a constructor argument so that it can ensure we always use the right ThreadPool if we ask the TaskGroup to wait. 
> 
> Do we need to lock down the task group once "wait()" is called with a TaskGroup or can users keep adding things to the TaskGroup even if work is currently going on? Should we freeze the contents of a TaskGroup once we start waiting on this?
@clayborg Right now the freezing is implicit through the use of a `condition_variable` & a `mutex`, see `llvm/lib/Support/ThreadPool.cpp`, L124.  It is a interesting question, should we make it support dynamic additions on the fly, while waiting? I would be tempted to wait for a practical use-case, but perhaps there's already one?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:41
 #include <algorithm>
+#include <queue>
 
----------------
mehdi_amini wrote:
> aganea wrote:
> > Is this related to the current patch?
> I suspect this file was depending on this header transitively and the include was removed from the ThreadPool.h header.
Ah, good catch, thanks :-)


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

https://reviews.llvm.org/D123225