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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:106
   bool isWorkerThread() const;
 
 private:
----------------
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



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:108
 private:
+  typedef std::deque<std::pair<std::function<void()>, TaskGroup *>> TaskQueue;
+
----------------
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


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:194
   /// Signaling for job completion
   std::condition_variable CompletionCondition;
 
----------------
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?


================
Comment at: llvm/lib/Support/ThreadPool.cpp:47
+    std::function<void()> Task;
+    TaskGroup *GroupOfTask;
+    {
----------------
initialize to nullptr


================
Comment at: llvm/lib/Support/ThreadPool.cpp:91
+      NotifyGroup =
+          GroupOfTask != nullptr && workCompletedUnlocked(GroupOfTask);
+    }
----------------
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;
```


================
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.
----------------
Can we detect recursive calls to wait on the same group here?


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

https://reviews.llvm.org/D123225