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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.

LGTM, thanks for all the changes @llunak!



================
Comment at: llvm/lib/Support/ThreadPool.cpp:155
+  // possible deadlock.
+  processTasks(&Group);
 }
----------------
llunak wrote:
> aganea wrote:
> > One more thing perhaps: tasks `.wait()`-ing will be "suspended" by re-entering `processTasks`. Any remaining tasks in the queue will be scheduled randomly over the waiting task(s), and this could easily create stack-overflows, since the scheduling is non-deterministic (so we could have several tasks waiting, piled on the top of each other). Depending on the stack size per platform, and how much stack each task eats up, this could potentially cause random crashes in production.
> > 
> > Probably the less intrusive approach would be to use fibers for suspended tasks. I suppose we could do that as a secondary step after this patch.
> Yes, but this is going to cost some resource no matter what, so it's just a choice of what resource it will be. And note that the recursion will be limited to the number of groups, since waiting loops are not allowed.
> 
I think it is fine for now, let's see first if this is really a problem in practice. Worst case, it could be fixed by ensuring that the scope calling the `.wait` doesn't hold on too many stack variables.


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

https://reviews.llvm.org/D123225