Skip to content
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

Fix Lock spin-waiting on single-proc machines #101513

Merged
merged 2 commits into from
May 2, 2024
Merged

Fix Lock spin-waiting on single-proc machines #101513

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b97a31e
Fix Lock spin-waiting on single-proc machines
kouvel Mar 28, 2024
ae4ad93
Add a couple of comments to clarify
kouvel Apr 25, 2024
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
4 changes: 2 additions & 2 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Lock.NativeAot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public sealed partial class Lock
private static int s_staticsInitializationStage;
private static bool s_isSingleProcessor;
private static short s_maxSpinCount;
private static short s_minSpinCount;
private static short s_minSpinCountForAdaptiveSpin;

/// <summary>
/// Initializes a new instance of the <see cref="Lock"/> class.
Expand Down Expand Up @@ -191,7 +191,7 @@ private static bool TryInitializeStatics()
// If the stage is PartiallyComplete, these will have already been initialized
s_isSingleProcessor = Environment.IsSingleProcessor;
s_maxSpinCount = DetermineMaxSpinCount();
s_minSpinCount = DetermineMinSpinCount();
s_minSpinCountForAdaptiveSpin = DetermineMinSpinCountForAdaptiveSpin();
}

// Also initialize some types that are used later to prevent potential class construction cycles. If
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Threading/Lock.NonNativeAot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace System.Threading
public sealed partial class Lock
{
private static readonly short s_maxSpinCount = DetermineMaxSpinCount();
private static readonly short s_minSpinCount = DetermineMinSpinCount();
private static readonly short s_minSpinCountForAdaptiveSpin = DetermineMinSpinCountForAdaptiveSpin();

/// <summary>
/// Initializes a new instance of the <see cref="Lock"/> class.
Expand Down
39 changes: 23 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ private void ExitImpl()
}
}

private static bool IsAdaptiveSpinEnabled(short minSpinCount) => minSpinCount <= 0;
private static bool IsAdaptiveSpinEnabled(short minSpinCountForAdaptiveSpin) => minSpinCountForAdaptiveSpin <= 0;

[MethodImpl(MethodImplOptions.NoInlining)]
private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
Expand Down Expand Up @@ -339,20 +339,19 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
goto Locked;
}

bool isSingleProcessor = IsSingleProcessor;
short maxSpinCount = s_maxSpinCount;
if (maxSpinCount == 0)
{
goto Wait;
}

short minSpinCount = s_minSpinCount;
short minSpinCountForAdaptiveSpin = s_minSpinCountForAdaptiveSpin;
short spinCount = _spinCount;
if (spinCount < 0)
{
// When negative, the spin count serves as a counter for contentions such that a spin-wait can be attempted
// periodically to see if it would be beneficial. Increment the spin count and skip spin-waiting.
Debug.Assert(IsAdaptiveSpinEnabled(minSpinCount));
Debug.Assert(IsAdaptiveSpinEnabled(minSpinCountForAdaptiveSpin));
_spinCount = (short)(spinCount + 1);
goto Wait;
}
Expand All @@ -377,7 +376,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)

for (short spinIndex = 0; ;)
{
LowLevelSpinWaiter.Wait(spinIndex, SpinSleep0Threshold, isSingleProcessor);
LowLevelSpinWaiter.Wait(spinIndex, SpinSleep0Threshold, isSingleProcessor: false);

if (++spinIndex >= spinCount)
{
Expand All @@ -394,7 +393,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)

if (tryLockResult == TryLockResult.Locked)
{
if (isFirstSpinner && IsAdaptiveSpinEnabled(minSpinCount))
if (isFirstSpinner && IsAdaptiveSpinEnabled(minSpinCountForAdaptiveSpin))
{
// Since the first spinner does a full-length spin-wait, and to keep upward and downward changes to the
// spin count more balanced, only the first spinner adjusts the spin count
Expand All @@ -415,7 +414,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)

// Unregister the spinner and try to acquire the lock
tryLockResult = State.TryLockAfterSpinLoop(this);
if (isFirstSpinner && IsAdaptiveSpinEnabled(minSpinCount))
if (isFirstSpinner && IsAdaptiveSpinEnabled(minSpinCountForAdaptiveSpin))
{
// Since the first spinner does a full-length spin-wait, and to keep upward and downward changes to the
// spin count more balanced, only the first spinner adjusts the spin count
Expand All @@ -433,7 +432,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
// number of contentions, the first spinner will attempt a spin-wait again to see if it is effective.
Debug.Assert(tryLockResult == TryLockResult.Wait);
spinCount = _spinCount;
_spinCount = spinCount > 0 ? (short)(spinCount - 1) : minSpinCount;
_spinCount = spinCount > 0 ? (short)(spinCount - 1) : minSpinCountForAdaptiveSpin;
}
}

Expand Down Expand Up @@ -504,7 +503,7 @@ private ThreadId TryEnterSlow(int timeoutMs, ThreadId currentThreadId)
break;
}

LowLevelSpinWaiter.Wait(spinIndex, SpinSleep0Threshold, isSingleProcessor);
LowLevelSpinWaiter.Wait(spinIndex, SpinSleep0Threshold, isSingleProcessor: false);
}

if (acquiredLock)
Expand Down Expand Up @@ -648,14 +647,22 @@ internal nint LockIdForEvents

internal ulong OwningThreadId => _owningThreadId;

private static short DetermineMaxSpinCount() =>
AppContextConfigHelper.GetInt16Config(
"System.Threading.Lock.SpinCount",
"DOTNET_Lock_SpinCount",
DefaultMaxSpinCount,
allowNegative: false);
private static short DetermineMaxSpinCount()
{
if (IsSingleProcessor)
{
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is this the actual fix? - to force "no spin" in singleproc case, even if user specified something max spin count.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea that's the actual fix

}

return
AppContextConfigHelper.GetInt16Config(
"System.Threading.Lock.SpinCount",
"DOTNET_Lock_SpinCount",
DefaultMaxSpinCount,
allowNegative: false);
}

private static short DetermineMinSpinCount()
private static short DetermineMinSpinCountForAdaptiveSpin()
Copy link
Member

Choose a reason for hiding this comment

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

If there is a Min and Max spin count - the actual spincount is obviously computed to be somewhere in the range. Not sure the name change makes it more or less clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

The min spin count with adaptive spin is just used as a limit for how low the _spinCount field can go, it can be negative, in which case it's just used as a counter for contentions while skipping spin-waiting until it reaches a value where it tries spin-waiting again. Maybe there's a better name, I can add a comment as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is ok either way.

_spinCount has dual purpose, but the limit is just a limit.
There are two ways to disable adaptiveness - by setting Min to be equal to Max or by setting it to a negative number. I think it was kind of obvious already.

{
// The config var can be set to -1 to disable adaptive spin
short adaptiveSpinPeriod =
Expand Down
Loading