Skip to content

Conversation

@mrsharm
Copy link
Member

@mrsharm mrsharm commented May 4, 2022

Summary

This PR is to address #68839 where it's clear that SetYieldProcessorScalingFactor is called multiple times thereby increasing the value of yp_spin_count_unit and as a result the spin counts are monotonically increasing each time SetYieldProcessorScalingFactor is called; this function was previously assumed to be called only once but that changed with the following PR: #55295.

The fix is to store the original spin count unit and use that value to set the yp_spin_count_unit rather than adjusting the value based on the last yp_spin_count_unit.

…t_unit rather than a continually increasing value
@ghost ghost added the area-GC-coreclr label May 4, 2022
@ghost ghost assigned mrsharm May 4, 2022
@ghost
Copy link

ghost commented May 4, 2022

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

Issue Details

Summary

This PR is to address #68839 where it's clear that SetYieldProcessorScalingFactor is called multiple times thereby increasing the value of yp_spin_count_unit and as a result the spin counts are monotonically increasing each time SetYieldProcessorScalingFactor is called; this function was previously assumed to be called only once but that changed with the following PR: #55295.

The fix is to store the original spin count unit and use that value to set the yp_spin_count_unit rather than adjusting the value based on the last yp_spin_count_unit.

Author: mrsharm
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@Maoni0
Copy link
Member

Maoni0 commented May 5, 2022

I would put a cap on the check too 'cause we don't want to spin too much -

if ((yp_spin_count_unit == 0) || (yp_spin_count_unit > 32768))

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

LGTM!

@kouvel
Copy link
Contributor

kouvel commented May 5, 2022

Ah I seem to have missed this issue in my other change. Thanks for fixing it @mrsharm!

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.

3 participants