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

[Luboš Luňák via Phabricator via llvm-commits]

llunak marked 4 inline comments as done.
llunak added inline comments.


================
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 =
----------------
aganea wrote:
> 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?
> As an alternative, given your current usage, the tasks could be even queued in a `std::vector` prior, and passed to the `TaskGroup` constructor.

Why? That seems unnecessary and inconsistent with how ThreadPool is used now.



================
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 =
----------------
llunak wrote:
> aganea wrote:
> > 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?
> > As an alternative, given your current usage, the tasks could be even queued in a `std::vector` prior, and passed to the `TaskGroup` constructor.
> 
> Why? That seems unnecessary and inconsistent with how ThreadPool is used now.
> 
> 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?

I see no reason to do it differently from ThreadPool, and ThreadPool currently says it's an error to add tasks while waiting but then does nothing about it (and AFAICT it's actually an unnecessary restriction).



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:94
+  /// It is possible to recursively wait even inside a task, if the group is
+  /// different. There may be only one active wait() call for a given group.
+  /// It is possible to add new tasks while blocking on this call, if those
----------------
mehdi_amini wrote:
> Do we have a way to assert on this?
Not without extra variables added just for detecting that. Is that something that would be done in LLVM code?



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:106
   bool isWorkerThread() const;
 
 private:
----------------
clayborg wrote:
> mehdi_amini wrote:
> > I think you need more documentation for the groups, at minima:
> > - in particular ownership model / lifetime expectation for the `TaskGroup` objects. 
> > - the existing APIs that don't take group should like be updated to be documented with respect to groups (is there a concept of a "default group" represented by the nullptr?).
> > 
> That would be good. Maybe add headerdoc before the TaskGroup class. If we end up making the TaskGroup constructor take a reference to the ThreadPool, we should mention that the ThreadPool must outlive the TaskGroup as far as lifetime goes. More documentation or examples would be nice for:
> - Making a TaskGroup 1 and then during work for TaskGroup 1 creating a TaskGroup 2 and then waiting on that within a worker thread
> - details on if you can add to a group while work is ongoing for that group
> 
I'm confused about this part about documentation, because I think all of this is either documented or obvious. Did you miss those or are they not as obvious as I find them to be? It seems to me that you expect this change to be more complex than it is - it's just an ability to tag tasks with a TaskGroup pointer and then wait on tasks with a specific tag. The only somewhat complex thing here is making sure wait() called from within a task doesn't not deadlock, and that's not an API thing.

> - in particular ownership model / lifetime expectation for the `TaskGroup` objects.

The TaskGroup objects are passed by reference, so I don't see how there could be any ownership. And TaskGroup objects are groups, so lifetime of TaskGroup objects and lifetime of groups are the same thing.

> - the existing APIs that don't take group should like be updated to be documented with respect to groups (is there a concept of a "default group" represented by the nullptr?).

Tasks without a group are tasks without a group. It really matters only for wait(), which already says that it waits for all threads.

> - Making a TaskGroup 1 and then during work for TaskGroup 1 creating a TaskGroup 2 and then waiting on that within a worker thread

This is done by simply doing it, there's nothing special about it from the API point of view, and wait() documentation says that this is possible. What more documentation would you need?

> - details on if you can add to a group while work is ongoing for that group

The description of wait() already says that no. What other details do you need?



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:108
 private:
+  typedef std::deque<std::pair<std::function<void()>, TaskGroup *>> TaskQueue;
+
----------------
clayborg wrote:
> clayborg wrote:
> > mehdi_amini wrote:
> > > Why the change to `deque` in this patch?
> > This is the new code that is adding the ability to run work in the groups
> Ignore my comment, I see that this type was used below where a queue was being used,.
std::queue has only front() and back(), which is insufficient for checking only tasks in a specific group.




================
Comment at: llvm/include/llvm/Support/ThreadPool.h:194
   /// Signaling for job completion
   std::condition_variable CompletionCondition;
 
----------------
clayborg wrote:
> Is this needed now that we have the TaskGroup objects? Or does this get signaled when ever all TaskGroups complete all of their work? Maybe update the documentation stating this is used for ThreadPool::wait() only for when all work is done?
Yes, it is still needed. TaskGroup are dumb IDs, so they change nothing about this. This gets signalled when either all tasks in a group get finished, or when all tasks get finished (I'll add this to the description).





================
Comment at: llvm/lib/Support/ThreadPool.cpp:47
+    std::function<void()> Task;
+    TaskGroup *GroupOfTask;
+    {
----------------
clayborg wrote:
> initialize to nullptr
It gets set on all paths (=only one) before it's used, and if it wouldn't, then initializing it here would prevent a warning about the mistake from -Wsometimes-uninitialized.



================
Comment at: llvm/lib/Support/ThreadPool.cpp:91
+      NotifyGroup =
+          GroupOfTask != nullptr && workCompletedUnlocked(GroupOfTask);
+    }
----------------
clayborg wrote:
> mehdi_amini wrote:
> > Seems like if `GroupOfTask` is non-null you're calling `workCompletedUnlocked` twice, why?
> Yeah this seems is should just be:
> ```
> NotifyGroup = GroupOfTask != nullptr;
> ```
Because 'Notify' is to be done if work is done for the group or for all tasks (nullptr), while "NotifyGroup' is to be done if the work is done for the group. But I can replace the second call with 'Notify' since it's the same value.



================
Comment at: llvm/lib/Support/ThreadPool.cpp:129-131
+  // Handle the case of recursive call from another task in a different group,
+  // in which case process tasks while waiting to keep the thread busy and avoid
+  // possible deadlock.
----------------
clayborg wrote:
> Can we detect recursive calls to wait on the same group here?
Not without extra debug variables (see a similar question for wait()). Unless you count a guaranteed deadlock as detecting.



================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:41
 #include <algorithm>
+#include <queue>
 
----------------
aganea wrote:
> 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 :-)
Yes.



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

https://reviews.llvm.org/D123225