Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[WIP] Threadpool exploration RFC #18158

Closed
wants to merge 4 commits into from
Closed

[WIP] Threadpool exploration RFC #18158

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cd5d40
Threadpool exploration
benaadams May 29, 2018
dbbfa48
Add Volatile.Read guard
benaadams May 29, 2018
c705310
Go back to full random
benaadams May 30, 2018
b60449c
Feedback
benaadams May 30, 2018
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
32 changes: 7 additions & 25 deletions src/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 @@ -19,7 +19,6 @@
using System.Runtime.CompilerServices;
using System.Runtime.ConstrainedExecution;
using System.Runtime.InteropServices;
using System.Security;
using Microsoft.Win32;

namespace System.Threading
Expand All @@ -30,8 +29,6 @@ internal static class ThreadPoolGlobals
//requests in the current domain.
public const uint TP_QUANTUM = 30U;

public static readonly int processorCount = Environment.ProcessorCount;

public static volatile bool vmTpInitialized;
public static bool enableWorkerTracking;

Expand Down Expand Up @@ -380,7 +377,7 @@ public IThreadPoolWorkItem TrySteal(ref bool missedSteal)

private Internal.PaddingFor32 pad1;

private volatile int numOutstandingThreadRequests = 0;
private int threadRequestOutstanding = 0;

private Internal.PaddingFor32 pad2;

Expand All @@ -395,23 +392,14 @@ public ThreadPoolWorkQueueThreadLocals EnsureCurrentThreadHasQueue() =>

internal void EnsureThreadRequested()
{
//
// If we have not yet requested #procs threads from the VM, then request a new thread
// as needed
// If we have not yet a thread request outstanding from the VM, then request a new thread
//
// Note that there is a separate count in the VM which will also be incremented in this case,
// which is handled by RequestWorkerThread.
//
int count = numOutstandingThreadRequests;
while (count < ThreadPoolGlobals.processorCount)
if (Volatile.Read(ref threadRequestOutstanding) == 0 &&
Interlocked.Exchange(ref threadRequestOutstanding, 1) == 0)
{
int prev = Interlocked.CompareExchange(ref numOutstandingThreadRequests, count + 1, count);
if (prev == count)
{
ThreadPool.RequestWorkerThread();
break;
}
count = prev;
ThreadPool.RequestWorkerThread();
Copy link
Member

Choose a reason for hiding this comment

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

An issue with this is going to be that when an idle thread pool gets a burst of work items in quick succession, only one thread would be requested, and only one thread then will be available to process all of that work. It would be fine (maybe even better) if those work items are very short, but they may not be and it may be beneficial to have multiple threads processing them. I have some thoughts on how to approach this issue and other perf issues here, but I'd have to prototype them to validate it. I think it will need some design changes to solve these problems.

Copy link
Member Author

@benaadams benaadams May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

An issue with this is going to be that when an idle thread pool gets a burst of work items in quick succession, only one thread would be requested, and only one thread then will be available to process all of that work.

Except... one of the things we were trying to fix in the reverted PR https://github.com/dotnet/coreclr/issues/14017 was the over requesting of threads; because the very first thing Dispatch if it dequeues a work item; before running it, is to request another thread (as does Enqueue - so double requesting):

coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.cs

Lines 561 to 564 in 208ea16

// If we found work, there may be more work. Ask for another thread so that the other work can be processed
// in parallel. Note that this will only ask for a max of #procs threads, so it's safe to call it for every dequeue.
//
workQueue.EnsureThreadRequested();

Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than trying to fix that, which was problematic and difficult to get round the races; idea is to work with it. So it will always (except between wake-up and dequeue gap) have a request active while it is successfully dequeueing; though only 1 rather than # cpu count?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the over-requesting is currently necessary because the thread request count tracking is fragile, mainly to reduce the chance of failure to near-zero (but not zero), theoretically. @sdmaclea tried to limit the requesting to the right amount but it ended up having races - I don't think there was anything wrong with the solution, but because of existing issues, it didn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but this takes a different approach by instead working with the over-requesting?

So with this change, while there is any items in the queue there will always[1] be one thread request active (plus when there are no more items, but one has just been dequeued).

Whereas currently it will always very quickly grow to always ProcessorCount thread requests active (as the number will never decrease while items are dequeued); as well as having a hot CompareExchange loop to decrease and increase the number.

[1] There will be a slight gap between thread being activated and dequeing an item, where it will drop to 0

}
}

Expand All @@ -423,15 +411,9 @@ internal void MarkThreadRequestSatisfied()
// Note that there is a separate count in the VM which has already been decremented by the VM
// by the time we reach this point.
//
int count = numOutstandingThreadRequests;
while (count > 0)
if (Volatile.Read(ref threadRequestOutstanding) == 1)
{
int prev = Interlocked.CompareExchange(ref numOutstandingThreadRequests, count - 1, count);
if (prev == count)
{
break;
}
count = prev;
Volatile.Write(ref threadRequestOutstanding, 0);
}
}

Expand Down