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

Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

mrsharm
Copy link
Member

@mrsharm mrsharm commented Apr 7, 2023

Added the ability to specify a Spin Count Unit via a GC Configuration for the release/7.0-staging branch and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

This PR is port of #84339 as automatic merging failed.

Customer impact

There were performance regressions experienced by customers related to this fix that resulted in an increase in the Spin Count Unit. This PR allows the ability to hardcode that value.

Testing

Tested manually and verified by a customer that the fix reverted the regression.

Risk

Low. The setting is not enabled by default and has to be explicitly set by the user for the conditional logic to work.

@ghost
Copy link

ghost commented Apr 7, 2023

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration for the release/7.0 branch and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

This PR is port of #84339 as automatic merging failed.

Author: mrsharm
Assignees: mrsharm
Labels:

area-GC-coreclr

Milestone: -

@mrsharm mrsharm added the Servicing-consider Issue for next servicing release review label Apr 7, 2023
@teo-tsirpanis
Copy link
Contributor

@mrsharm you have to target the release/7.0-staging branch.

@teo-tsirpanis teo-tsirpanis added this to the 7.0.x milestone Apr 7, 2023
@JulieLeeMSFT JulieLeeMSFT removed the Servicing-consider Issue for next servicing release review label Apr 7, 2023
@JulieLeeMSFT
Copy link
Member

@mrsharm, I am removing the servicing consider label until it is reviewed and all the tests are passed.

@mrsharm mrsharm changed the base branch from release/7.0 to release/7.0-staging April 8, 2023 01:14
@mrsharm mrsharm marked this pull request as ready for review April 8, 2023 01:16
@carlossanlop
Copy link
Member

carlossanlop commented Apr 10, 2023

@mrsharm today is code complete. If you want this fix included in the May Servicing release, please get it ready by 4pm PT today:

  • Get the Tactics approval ASAP (and once approved, please add the Servicing-approved label to unblock the check-service-labels CI action)
  • Confirm the CI failures are unrelated
  • Get a sign-off from another area owner

No OOB changes needed since this is a native code change.

@JulieLeeMSFT
Copy link
Member

cc @jeffschwMSFT for servicing consider.

@Maoni0
Copy link
Member

Maoni0 commented Apr 10, 2023

both @mrsharm and @mangod9 are out of office and I was not informed this needed to be in for the next servicing release when we were going through issues last week. so I will assume this does not need to get in right now and let them handle it when they are back.

@JulieLeeMSFT
Copy link
Member

@mrsharm, please update the issue following the template for servicing fix (refer #84444).

@mangod9
Copy link
Member

mangod9 commented Apr 11, 2023

Correct, we are still waiting for customer validation on this and will merge when we have confirmation.

@mrsharm mrsharm added the Servicing-consider Issue for next servicing release review label Apr 26, 2023
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will bring for consideration in 7.0.x

@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.7 Apr 27, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 27, 2023
@mrsharm mrsharm merged commit 3a80b29 into dotnet:release/7.0-staging Apr 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants