Skip to content

[release/7.0] Fix distribute free regions #76391

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

Merged
merged 6 commits into from
Sep 30, 2022

Conversation

github-actions[bot]
Copy link
Contributor

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

Backport of #74916 to release/7.0

/cc @PeterSolMS

Customer Impact

This addresses an issue with SVR GC where suboptimal distribution of free regions to heaps after GC may cause excessive decommits/recommits/page faults. This is mostly relevant for scenarios with many processors/heaps.

Testing

Extensive testing has been done with both GCPerfSim and first party customer scenarios to make sure the fix addresses the issue.

Risk

Risk is low because the fix will only change the distribution of free regions and - indirectly - how many regions get decommitted. When a new region is needed, the allocator will get it either from the free list or the region allocator. Both code paths are unchanged and well tested, so correctness should not be impacted.

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.

…e the correction evenly over all the heaps. This makes the budget correction pass more expensive, but enables us to remove extra pass at the end.
…heaps, disregard higher generation budgets if the free regions are only sufficient for lower ones.

Enhance instrumentation for decommit_ephemeral_segment_pages.
…t per heap which is the budget for the highest generation covered by the available free regions. When distributing a budget shortfall for a higher generation, avoid going lower than the minimum.

Example: assume we have 21 free regions available, heap 0 needs 10 in gen 0 and 0 in gen 1, while heap 1 needs 10 in gen 0 and 3 in gen 1. So our budget for gen 0 is 20 regions, which is covered by the available free regions, while the budget for gen 0 + gen 1 is 23 regions, so we have a shortfall of 2 regions compared to the available free regions. As the gen 1 budget is less reliable and its use further in the future, it's better to give heap 0 10 regions and heap 1 11 regions, rather than reducing the budget by 1 region each, which would mean giving 9 regions to heap 0 and 12 regions to heap 1.
…wice as large as it needs to be as the slots for kind == 1 will always be 0.
@ghost ghost added the area-GC-coreclr label Sep 29, 2022
@ghost
Copy link

ghost commented Sep 29, 2022

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

Issue Details

Backport of #74916 to release/7.0

/cc @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: -

@carlossanlop
Copy link
Contributor

@PeterSolMS when this is ready, can you please fill out the template, add the servicing-consider label, then send an email to Tactics requesting approval?

@PeterSolMS PeterSolMS added the Servicing-consider Issue for next servicing release review label Sep 30, 2022
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. please get a code review and we can consider for 7 ga.

@jeffschwMSFT jeffschwMSFT added this to the 7.0.0 milestone Sep 30, 2022
@Maoni0
Copy link
Member

Maoni0 commented Sep 30, 2022

I don't think we should take this part for 7.0 -

gc_heap::min_gen0_balance_delta = (dd_min_size (gen0_dd) >> 6);

as this is a tuning change that was not well tested, could affect pause time. the rest is fine and only affects regions.

@Maoni0 Maoni0 added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 30, 2022
@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) Servicing-consider Issue for next servicing release review labels Sep 30, 2022
@carlossanlop
Copy link
Contributor

Approved by Tactics via email.

@jeffschwMSFT
Copy link
Member

@carlossanlop the 'no-merge' as on this one as we were still discussing. @Maoni0 with your signoff is it all good?

@Maoni0
Copy link
Member

Maoni0 commented Sep 30, 2022

all good, thanks!

@carlossanlop
Copy link
Contributor

@jeffschwMSFT yep, I should've clarified that in my previous comment. I checked with @Maoni0 before changing the labels. The no-merge label was added while we waited for Tactics approval.

@carlossanlop
Copy link
Contributor

CI finished and is all green. Let's get this merged. :shipit:

@carlossanlop carlossanlop merged commit 809b421 into release/7.0 Sep 30, 2022
@carlossanlop carlossanlop deleted the backport/pr-74916-to-release/7.0 branch September 30, 2022 23:25
@ghost ghost locked as resolved and limited conversation to collaborators Oct 31, 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.

4 participants