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.

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

Conversation

kouvel
Copy link
Member

@kouvel kouvel commented Apr 24, 2024

Small fix to check for a single proc during initialization. Also renamed things referring to "minSpinCount" to clarify it a bit.

Small fix to check for a single proc during initialization. Also renamed things referring to "minSpinCount" to clarify it a bit.
@kouvel kouvel added this to the 9.0.0 milestone Apr 24, 2024
@kouvel kouvel requested a review from mangod9 April 24, 2024 19:07
@kouvel kouvel self-assigned this Apr 24, 2024
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.


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.

{
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

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* Fix Lock spin-waiting on single-proc machines

Small fix to check for a single proc during initialization. Also renamed things referring to "minSpinCount" to clarify it a bit.
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* Fix Lock spin-waiting on single-proc machines

Small fix to check for a single proc during initialization. Also renamed things referring to "minSpinCount" to clarify it a bit.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants