Skip to content

Only ensure the ThreadPool is active when queuing tasks #35760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
benaadams wants to merge 1 commit into from
Closed

Only ensure the ThreadPool is active when queuing tasks #35760

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,28 @@ private ThreadPoolWorkQueueThreadLocals CreateThreadLocals()
return ThreadPoolWorkQueueThreadLocals.threadLocals = new ThreadPoolWorkQueueThreadLocals(this);
}

internal void EnsureThreadPoolActive()
{
// Used for queuing.

// If we have not yet requested a thread, then request a new thread so the ThreadPool is active.
int count = numOutstandingThreadRequests;
while (count < 1)
Copy link
Copy Markdown
Contributor

@kouvel kouvel May 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@kouvel kouvel May 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edited above

Copy link
Copy Markdown
Member Author

@benaadams benaadams May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Member

@davidfowl davidfowl May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Copy Markdown
Member

@davidfowl davidfowl May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kouvel where's your branch?

Copy link
Copy Markdown
Contributor

@kouvel kouvel May 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@kouvel kouvel May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh that's the wrong one

Copy link
Copy Markdown
Contributor

@kouvel kouvel May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated inline above

Copy link
Copy Markdown
Contributor

@kouvel kouvel May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have a fix for this issue yet, I have a prototype that I'll be adding to that branch

Copy link
Copy Markdown
Member

@davidfowl davidfowl May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I'll be stalking these changes 😄 .

{
int prev = Interlocked.CompareExchange(ref numOutstandingThreadRequests, count + 1, count);
if (prev == count)
{
ThreadPool.RequestWorkerThread();
break;
}
count = prev;
}
}

internal void EnsureThreadRequested()
{
//
// Used when running work items with a non-empty queue.

// If we have not yet requested #procs threads, then request a new thread.
//
// CoreCLR: Note that there is a separate count in the VM which has already been incremented
Expand Down Expand Up @@ -486,7 +505,7 @@ public void Enqueue(object callback, bool forceGlobal)
workItems.Enqueue(callback);
}

EnsureThreadRequested();
EnsureThreadPoolActive();
}

internal bool LocalFindAndPop(object callback)
Expand Down