Only ensure the ThreadPool is active when queuing tasks #35760
Conversation
Rather than double requesting threads on both enqueue and run, only ensure there is 1 or more requests outstanding at enqueue, and let dispatch request up to ProcessorCount threads; rather than doing it in both places
|
|
||
| // If we have not yet requested a thread, then request a new thread so the ThreadPool is active. | ||
| int count = numOutstandingThreadRequests; | ||
| while (count < 1) |
There was a problem hiding this comment.
I believe this speculative check is insufficient to guarantee that the thread pool would be active on an enqueue.
I'm already looking into this problem and I think I have a decent approach that would be correct along with compensating for the extra overhead of a non-speculative check here. I'm currently looking at doing that along with my changes to switch the coreclr runtime to use the portable thread pool (WIP changes in my branch ThreadPool at the moment), as this type of change would involve a fair bit of testing that I would need to do with those changes anyway. If for some reason the portable thread pool change cannot be merged for 5.0 then there are several portions of that branch that I would plan to submit separately including a fix for this issue. Let me try a fix for it and see where it goes. If you have a scenario in mind where this type of change would be impactful I would greatly appreciate it if you could help to verify those, though that branch is not ready for verification yet. I'll ping on on the PR once I have it up.
There was a problem hiding this comment.
If for some reason the portable thread pool change cannot be merged for 5.0 then there are several portions of that branch that I would plan to submit separately including a fix for this issue. Let me try a fix for it and see where it goes. If you have a scenario in mind where this type of change would be impactful I would greatly appreciate it if you could help to verify those, though that branch is not ready for verification yet. I'll ping on on the PR once I have it up.
No negative thoughts, we'll get it in for 5.0 😄
There was a problem hiding this comment.
It's this one: https://github.com/kouvel/runtime/tree/ThreadPool
I will likely update the various commits with force-push so the commits may change over time (to make it easier to review) until I put up a PR.
There was a problem hiding this comment.
It doesn't have a fix for this issue yet, I have a prototype that I'll be adding to that branch
There was a problem hiding this comment.
Awesome! I'll be stalking these changes 😄 .
ghost
locked as resolved and limited conversation to collaborators
4 participants
Rather than double requesting threads on both
EnqueueandDispatch, only ensure there is >= 1 requests outstanding at enqueue, and let dispatch request up toProcessorCountthreads; rather than havingProcessorCountas the cap in both places.Idea from @kouvel's comment in #35330 (comment)