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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

That's a pretty nice improvement! :)

Reading the patch, I'm not sure I have a good grasp about all the interactions of this with the existing invariants of this class: concurrency is always complex...

> 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 :)



================
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
----------------
Do we have a way to assert on this?


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



================
Comment at: llvm/include/llvm/Support/ThreadPool.h:108
 private:
+  typedef std::deque<std::pair<std::function<void()>, TaskGroup *>> TaskQueue;
+
----------------
Why the change to `deque` in this patch?


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:135
 
-  bool workCompletedUnlocked() { return !ActiveThreads && Tasks.empty(); }
+  bool workCompletedUnlocked(TaskGroup *Group) const;
 
----------------
Can you document it please?


================
Comment at: llvm/lib/Support/ThreadPool.cpp:91
+      NotifyGroup =
+          GroupOfTask != nullptr && workCompletedUnlocked(GroupOfTask);
+    }
----------------
Seems like if `GroupOfTask` is non-null you're calling `workCompletedUnlocked` twice, why?


================
Comment at: llvm/lib/Support/ThreadPool.cpp:97
+      CompletionCondition.notify_all();
+    // If this was a task in a group, notify also waiting for tasks in this
+    // function, to make a recursive wait() return after the group it's been
----------------
" notify also **threads** waiting" ?


================
Comment at: llvm/lib/Support/ThreadPool.cpp:98
+    // If this was a task in a group, notify also waiting for tasks in this
+    // function, to make a recursive wait() return after the group it's been
+    // waiting for has finished.
----------------
What does "this function" means here? 


================
Comment at: llvm/lib/Support/ThreadPool.cpp:111
+           return T.second == Group;
+         }) == Tasks.end();
+}
----------------
Nit: seems like The `find_if(...) == end()` could be replaced by `!llvm::any_of(...)`


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


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

https://reviews.llvm.org/D123225