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

[release/7.0] Gradual decommit in wks #76306

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 28, 2022

Backport of #73620 to release/7.0

/cc @mangod9 @PeterSolMS

Customer Impact

This addresses a performance regression from .NET 6.0 - with regions enabled, we decommit more aggressively, which causes us to recommit more memory later and incur page faults when that memory is touched for the first time. The effect has been seen with micro benchmarks, but will likely impact customer scenarios as well.

Testing

Ran GCPerfSim scenarios and thousands of micro benchmarks with this change and verified it mostly (but not altogether completely) gets rid of the regression caused by more agressive decommit.

Risk

Risk is low because the fix only impacts when free regions get returned to the region allocator. When the allocator needs a new region, it can get it either from the list of free regions, or from the region allocator. Both code paths are well tested. Because the fix delays decommitting free regions, some increase in working set may be observed temporarily.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

This is the regions version modeled after the behavior of the segments version.

Idea is simply to limit the amount of decommitted memory based on the time since the last GC.
…es the logic for the WKS decommit more straightforward.
…_segment_pages for WKS, some changes in distribute_free_regions as a consequence.
@ghost
Copy link

ghost commented Sep 28, 2022

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

Issue Details

Backport of #73620 to release/7.0

/cc @mangod9 @PeterSolMS

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@PeterSolMS
Copy link
Contributor

Customer Impact

This addresses a performance regression from .NET 6.0 - with regions enabled, we decommit more aggressively, which causes us to recommit more memory later and incur page faults when that memory is touched for the first time. The effect has been seen with micro benchmarks, but will likely impact customer scenarios as well.

Testing

Ran GCPerfSim scenarios and thousands of micro benchmarks with this change and verified it mostly (but not altogether completely) gets rid of the regression caused by more agressive decommit.

Risk

Risk is low because the fix only impacts when free regions get returned to the region allocator. When the allocator needs a new region, it can get it either from the list of free regions, or from the region allocator. Both code paths are well tested. Because the fix delays decommitting free regions, some increase in working set may be observed temporarily.

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 take for consideration in 7 ga.

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Sep 28, 2022
@jeffschwMSFT jeffschwMSFT added this to the 7.0.0 milestone Sep 28, 2022
@carlossanlop
Copy link
Member

@PeterSolMS can you please send an email to Tactics requesting approval?

@mangod9
Copy link
Member

mangod9 commented Sep 28, 2022

Installer failures look to be unrelated, but rerunning them now.

@Maoni0
Copy link
Member

Maoni0 commented Sep 28, 2022

I emailed tactics for @PeterSolMS

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 29, 2022
@carlossanlop
Copy link
Member

Approved by Tactics via email. CI is green after re-run. Signed off. Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 9c7b5a8 into release/7.0 Sep 29, 2022
@carlossanlop carlossanlop deleted the backport/pr-73620-to-release/7.0 branch September 29, 2022 00:55
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2022
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.

5 participants