This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Add normalized equivalent of YieldProcessor, retune some spin loops #13670
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Author
|
Numbers from Thread.SpinWait divide count by 7 experiment: Xeon E5-1650 (Sandy Bridge, 6-core, 12-thread): Core i7-6700 (Skylake, 4-core, 8-thread): |
Author
|
Numbers from this PR: Xeon E5-1650 (Sandy Bridge, 6-core, 12-thread): Core i7-6700 (Skylake, 4-core, 8-thread): |
stephentoub
reviewed
Aug 30, 2017
| /// when the resource becomes available. | ||
| /// </summary> | ||
| internal static readonly int SpinCountforSpinBeforeWait = PlatformHelper.IsSingleProcessor ? 1 : 35; | ||
| internal const int Sleep1ThresholdForSpinBeforeWait = 40; // should be greater than MaxSpinCountBeforeWait |
Member
There was a problem hiding this comment.
What is MaxSpinCountBeforeWait?
stephentoub
reviewed
Aug 30, 2017
| // (_count - YieldThreshold) % 2 == 0: The purpose of this check is to interleave Thread.Yield/Sleep(0) with | ||
| // Thread.SpinWait. Otherwise, the following issues occur: | ||
| // - When there are no threads to switch to, Yield and Sleep(0) become no-op and it turns the spin loop into a | ||
| // busy -spin that may quickly reach the max spin count and cause the thread to enter a wait state, or may |
Member
There was a problem hiding this comment.
Nit: extra space in "busy -spin"
stephentoub
reviewed
Aug 30, 2017
| // contention), they may switch between one another, delaying work that can make progress. | ||
| if (( | ||
| _count >= YieldThreshold && | ||
| (_count >= sleep1Threshold || (_count - YieldThreshold) % 2 == 0) |
Member
There was a problem hiding this comment.
Nit: the formatting here reads strangely to me
Author
There was a problem hiding this comment.
It's formatted similarly to:
if (a ||
b)where
a ==
(
c &&
d
)This is how I typically format multi-line expressions, trying to align parentheses and putting each type of expression (&& or ||) separately, one condition per line unless the whole expression fits on one line. What would you suggest instead? I can separate parts of it into locals if you prefer.
Member
|
Thanks, @kouvel. Do you have any throughput numbers on the thread pool with this change? |
Author
|
The only use of Thread.SpinWait I found in the thread pool is in RegisteredWaitHandleSafe.Unregister, which I don't think is interesting. I have not measured the perf for Task.SpinWait, I can do that if you would like. |
Author
|
Code used for Spin perf: |
Author
|
Code used for ReaderWriterLockSlim perf: var sw = new Stopwatch();
var scores = new double[16];
var startThreads = new ManualResetEvent(false);
bool stop = false;
var counts = new int[64];
var readerThreads = new Thread[readerThreadCount];
ThreadStart readThreadStart =
() =>
{
startThreads.WaitOne();
while (!stop)
{
rw.EnterReadLock();
rw.ExitReadLock();
Interlocked.Increment(ref counts[16]);
}
};
for (int i = 0; i < readerThreadCount; ++i)
{
readerThreads[i] = new Thread(readThreadStart);
readerThreads[i].IsBackground = true;
readerThreads[i].Start();
}
var writeLockAcquireAndReleasedInnerIterationCountTimes = new AutoResetEvent(false);
var writerThreads = new Thread[writerThreadCount];
ThreadStart writeThreadStart =
() =>
{
startThreads.WaitOne();
while (!stop)
{
rw.EnterWriteLock();
rw.ExitWriteLock();
Interlocked.Increment(ref counts[32]);
}
};
for (int i = 0; i < writerThreadCount; ++i)
{
writerThreads[i] = new Thread(writeThreadStart);
writerThreads[i].IsBackground = true;
writerThreads[i].Start();
}
startThreads.Set();
// Warmup
Thread.Sleep(4000);
// Actual run
for(int i = 0; i < scores.Length; ++i)
{
counts[16] = 0;
counts[32] = 0;
Interlocked.MemoryBarrier();
sw.Restart();
Thread.Sleep(500);
sw.Stop();
int readCount = counts[16];
int writeCount = counts[32];
double elapsedMs = sw.Elapsed.TotalMilliseconds;
scores[i] =
new double[]
{
Math.Max(1, (readCount + writeCount) / elapsedMs),
Math.Max(1, writeCount / elapsedMs)
}.GeometricMean(readerThreadCount, writerThreadCount);
}
return scores; |
Member
ThreadPool's global queue is a ConcurrentQueue, and CQ uses System.Threading.SpinWait when there are contentions on various operations, including dequeues. |
Author
|
Ah ok, I included ConcurrentQueue, I'll add a test for thread pool as well |
Author
|
Updated code above with the added thread pool throughput test. Looks like there's no change: Xeon E5-1650 (Sandy Bridge, 6-core, 12-thread): Core i7-6700 (Skylake, 4-core, 8-thread): |
Author
|
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness |
Author
|
@dotnet-bot test Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness |
Author
|
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness |
Author
|
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness |
tarekgh
reviewed
Aug 31, 2017
| /// A suggested number of spin iterations before doing a proper wait, such as waiting on an event that becomes signaled | ||
| /// when the resource becomes available. | ||
| /// </summary> | ||
| internal static readonly int SpinCountforSpinBeforeWait = PlatformHelper.IsSingleProcessor ? 1 : 35; |
Member
There was a problem hiding this comment.
35 [](start = 105, length = 2)
did we get this number from experimenting different scenarios? just curious how we come up with this number. and it doesn't matter the number of processors?
Author
There was a problem hiding this comment.
I experimented with ManualResetEventSlim to get an initial number, applied the same number to other similar situations, and then tweaked up and down to see what was working. Spinning less can lead to early waiting and more context switching, spinning more can decrease latency but may use up some CPU time unnecessarily. Depends on the situation too, like for SemaphoreSlim I had to double the spin iterations because the waiting there is a lot more expensive. Also depends on the likelihood of the spin being successful and how long the wait would be but those are not accounted for here.
Author
There was a problem hiding this comment.
I don't think including number of processors (N) works well. Multiplying by N increases spinning on each thread by N, so total spinning across N threads is increased by N^2. When there are more processors contending on a resource, it may even be better to spin less and wait sooner to reduce contention since with more processors something like a mutex has the natural possibility of having more contention.
tarekgh
reviewed
Aug 31, 2017
| // usually better for that. | ||
| // | ||
| int n = RuntimeThread.OptimalMaxSpinWaitsPerSpinIteration; | ||
| if (_count <= 30 && (1 << _count) < n) |
Member
There was a problem hiding this comment.
30 [](start = 30, length = 2)
would be nice to comment how we choose this number.
Author
There was a problem hiding this comment.
I'll add a reference to Thread::InitializeYieldProcessorNormalized that describes and calculates it
tarekgh
reviewed
Aug 31, 2017
| { | ||
| get | ||
| { | ||
| if (s_optimalMaxSpinWaitsPerSpinIteration != 0) |
Member
There was a problem hiding this comment.
s_optimalMaxSpinWaitsPerSpinIteration [](start = 20, length = 37)
Looks this one can be converted to readonly field initialized with GetOptimalMaxSpinWaitsPerSpinIterationInternal() so we can avoid checking 0 value.
Author
There was a problem hiding this comment.
I didn't want do that since the first call would trigger the measurement that takes about 10 ms. Static construction of RuntimeThread probably happens during startup for most apps.
tarekgh
reviewed
Aug 31, 2017
| } | ||
|
|
||
| return IsCompleted; | ||
| return false; |
Member
There was a problem hiding this comment.
return false; [](start = 11, length = 14)
Is it possible between exiting the loop and executing the return, the task can get into completed state? I am asking to know if we should keep returning IsCompleted
Author
There was a problem hiding this comment.
Functionally it doesn't make any difference, the caller will do the right thing. Previously it made sense to check IsCompleted before returning because the loop would have stopped immediately after a wait. But previously it was redundant to check IsCompleted first in the loop because it was already checked immediately before before the loop. So I just changed the loop to wait first and check later, now the loop exits right after checking IsCompleted and it would be redundant to check it again before returning.
kouvel
added
the
* NO MERGE *
kouvel
added a commit
to dotnet/corert
that referenced
this pull request
Aug 31, 2017
PR dotnet/coreclr#13670 adds a new member to RuntimeThread is that used by SpinWait. SpinWait is shared, so this PR is adding the new member to CoreRT first to avoid a break.
kouvel
removed
the
* NO MERGE *
Author
|
Are there any other concerns with this change that should be addressed? As with any change to spin heuristics, there probably will be some regressions here and there since there are tradeoffs, which may have to be dealt with. If we're ok with that I'll go ahead and merge. |
Member
|
no concerns from my side. |
kouvel
added a commit
that referenced
this pull request
Sep 5, 2017
In #13670, by mistake I made the spin loop infinite, that is now fixed. As a result the numbers I had provided in that PR for SemaphoreSlim were skewed, and fixing it caused the throughput to get even lower. To compensate, I have found and fixed one culprit for the low throughput problem: - Every release wakes up a waiter. Effectively, when there is a thread acquiring and releasing the semaphore, waiters don't get to remain in a wait state. - Added a field to keep track of how many waiters were pulsed to wake but have not yet woken, and took that into account in Release() to not wake up more waiters than necessary. - Retuned and increased the number of spin iterations. The total spin delay is still less than before the above PR.
This was referenced Sep 7, 2017
kouvel
added a commit
that referenced
this pull request
Sep 26, 2017
- Removed asm helpers on Windows and used portable C++ helpers instead
- Rearranged fast path code to improve them a bit and match the asm more closely
Perf:
- The asm helpers are a bit faster. The code generated for the portable helpers is almost the same now, the remaining differences are:
- There were some layout issues where hot paths were in the wrong place and return paths were not cloned. Instrumenting some of the tests below with PGO on x64 resolved all of the layout issues. I couldn't get PGO instrumentation to work on x86 but I imagine it would be the same there.
- Register usage
- x64: All of the Enter functions are using one or two (TryEnter is using two) callee-saved registers for no apparent reason, forcing them to be saved and restored. r10 and r11 seem to be available but they're not being used.
- x86: Similarly to x64, the compiled functions are pushing and popping 2-3 additional registers in the hottest fast paths.
- I believe this is the main remaining gap and PGO is not helping with this
- On Linux, perf is >= before for the most part
- Perf tests used for below are updated in PR #13670
MichalStrehovsky
added a commit
to MichalStrehovsky/corert
that referenced
this pull request
Apr 13, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of fix for https://github.com/dotnet/coreclr/issues/13388
Normalized equivalent of YieldProcessor
Thread.SpinWait divide count by 7 experiment
Spin tuning