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

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

llunak added a comment.

Let's first deal with the conceptual stuff, no point in dealing with the small code things as long as there's not agreement that this is the way to implement it.

In D123225#3434132 <https://reviews.llvm.org/D123225#3434132>, @aganea wrote:

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

I used what CODE_OWNERS.txt lists for 'Support'.

In D123225#3435887 <https://reviews.llvm.org/D123225#3435887>, @aganea wrote:

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

I think that would be possible only with non-recursive use of ThreadPool. But D122975 <https://reviews.llvm.org/D122975> requires creating running tasks that themselves run tasks and wait for them, which requires two things:

- Waiting just for a specific subset of tasks, otherwise a task could deadlock waiting for itself. This requires the queue-processing code to signal such state.
- Not wasting thread pool slots on waiting, otherwise they all could end up waiting for tasks that wouldn't have slots to run and deadlock. My approach handles that by letting such threads process tasks while waiting for a group. Another approach could be launching additional temporary threads, but I don't see how that would make anything simpler or better.

I don't see how either of these could be done without altering the queue-processing code in ThreadPool itself. So unless you have a good idea there, I think the most I can move to TaskGroup is syntactic sugar.


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

https://reviews.llvm.org/D123225