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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added reviewers: mehdi_amini, MaskRay, aganea.
aganea added a comment.
Herald added a subscriber: StephenFan.

Not sure @chandlerc will be able to review your patch, adding some people that have been working in this space recently.

I think this is an interesting change, but I'm a bit worried that it adds complexity to the the thread task loop. I am wondering if this problem couldn't be solved by packaging the `TaskGroup` logic in a lambda. 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.



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:50
+  /// that run on the same threadpool but can be waited for separately.
+  struct TaskGroup {
+    TaskGroup() = default;
----------------
Can you move this above the `ThreadPool`? It'll be easier for future code readers I think.


================
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 =
----------------
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.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:41
 #include <algorithm>
+#include <queue>
 
----------------
Is this related to the current patch?


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

https://reviews.llvm.org/D123225