Skip to content

Conversation

@PeterSolMS
Copy link
Contributor

As it turns out, distribute_free_regions does not do a very good job distributing free regions if there are fewer than the budget demands. There are two reasons:

  • there is a bug in the adjustment_per_heap computation that causes the value to be too large - that is because dividing a negative number already rounds towards zero, and hence towards positive infinity, thus adding (n_heaps-1) before dividing is wrong.
  • apart from that, if the overall budget is not a multiple of the number of heaps, trying to fill each heap to the budget will allow heaps with higher numbers to have significant shortfalls.

The fix corrects the computation and (somewhat inelegantly) adds yet another pass to the distribution algorithm:

  • Now, the first pass trims the regions in every heap to budget-1
  • The seconds pass fills the regions in every heap to budget-1
  • The third pass fills the regions in every heap to budget, as long as we have surplus regions.

This should guarantee that each heap has either budget-1 or budget number of regions.

I also adjusted the computation of min_gen0_balance_delta - that is subject to further testing.

@PeterSolMS PeterSolMS requested review from Maoni0 and cshung September 1, 2022 10:05
@ghost ghost assigned PeterSolMS Sep 1, 2022
@ghost ghost added the area-GC-coreclr label Sep 1, 2022
@ghost
Copy link

ghost commented Sep 1, 2022

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

Issue Details

As it turns out, distribute_free_regions does not do a very good job distributing free regions if there are fewer than the budget demands. There are two reasons:

  • there is a bug in the adjustment_per_heap computation that causes the value to be too large - that is because dividing a negative number already rounds towards zero, and hence towards positive infinity, thus adding (n_heaps-1) before dividing is wrong.
  • apart from that, if the overall budget is not a multiple of the number of heaps, trying to fill each heap to the budget will allow heaps with higher numbers to have significant shortfalls.

The fix corrects the computation and (somewhat inelegantly) adds yet another pass to the distribution algorithm:

  • Now, the first pass trims the regions in every heap to budget-1
  • The seconds pass fills the regions in every heap to budget-1
  • The third pass fills the regions in every heap to budget, as long as we have surplus regions.

This should guarantee that each heap has either budget-1 or budget number of regions.

I also adjusted the computation of min_gen0_balance_delta - that is subject to further testing.

Author: PeterSolMS
Assignees: PeterSolMS
Labels:

area-GC-coreclr

Milestone: -

…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.
size_t num_decommit_regions_by_time = 0;
size_t size_decommit_regions_by_time = 0;
size_t heap_budget_in_region_units[MAX_SUPPORTED_CPUS][kind_count];
size_t min_heap_budget_in_region_units[MAX_SUPPORTED_CPUS][kind_count];
Copy link
Member

Choose a reason for hiding this comment

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

since we are only using this for SOH, it seems like this can just be declared as

size_t min_heap_budget_in_region_units[MAX_SUPPORTED_CPUS];

MAX_SUPPORTED_CPUS is 1024 so this is quite a large data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we are using it for UOH as well, but it's always 0 in this case, so we can use an if-statement or conditional expression.

I'll fix it...

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.

other than the comment I have above, the rest LGTM

@Maoni0
Copy link
Member

Maoni0 commented Sep 29, 2022

new changes LGTM!

@PeterSolMS PeterSolMS merged commit ed10bc0 into dotnet:main Sep 29, 2022
@PeterSolMS
Copy link
Contributor Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3153046300

@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants