[release/8.0-staging] Reapply distribute free regions #117736
Conversation
This change was missing from the initial port.
|
Tagging subscribers to this area: @dotnet/gc |
There was a problem hiding this comment.
Pull Request Overview
This PR reapplies the "distribute free regions" functionality to the garbage collection system, along with a small missing change from the original backport. The changes primarily involve refactoring and restructuring the region distribution logic in the CoreCLR garbage collector.
Key changes include:
- Restructuring the free region distribution algorithm with improved organization and separation of concerns
- Adding new region aging thresholds for different region types (basic, large, huge)
- Introducing new helper methods for region management and memory load calculations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/gc/gcpriv.h | Updates enum definitions, adds new method declarations, and defines region aging constants |
| src/coreclr/gc/gc.cpp | Major refactoring of distribute_free_regions logic with new helper methods and improved organization |
Comments suppressed due to low confidence (1)
| // just to reduce the number of #ifdefs in the code below | ||
| const int i = 0; | ||
| #endif //MULTIPLE_HEAPS | ||
| ptrdiff_t budget_gen = max (hp->estimate_gen_growth (gen), (ptrdiff_t)0); |
There was a problem hiding this comment.
[nitpick] The explicit cast to (ptrdiff_t)0 is unnecessary since max() will handle the type promotion automatically. Consider using just 0 for better readability.
| ptrdiff_t budget_gen = max (hp->estimate_gen_growth (gen), (ptrdiff_t)0); | |
| ptrdiff_t budget_gen = max (hp->estimate_gen_growth (gen), 0); |
Copilot uses AI. Check for mistakes.
| { | ||
| gc_last_ephemeral_decommit_time = dd_time_clock (dd0); | ||
| size_t decommit_step_milliseconds = min (ephemeral_elapsed, (10*1000)); | ||
| size_t decommit_step_milliseconds = min (ephemeral_elapsed, (size_t)(10 * 1000)); |
There was a problem hiding this comment.
The magic number '10 * 1000' should be defined as a named constant to improve code maintainability and clarity about what this timeout represents.
| size_t decommit_step_milliseconds = min (ephemeral_elapsed, (size_t)(10 * 1000)); | |
| size_t decommit_step_milliseconds = min (ephemeral_elapsed, (size_t)(MAX_DECOMMIT_TIME_MILLISECONDS)); |
Copilot uses AI. Check for mistakes.
jeffschwMSFT
added
the
Servicing-consider
jeffschwMSFT
changed the title
leecow
added
Servicing-approved
4 participants
Port change for better free-list management. This is a back port of #109431, and also applies a fix which was missing from the .NET 8 port.
Customer Impact
Memory utilization regression as part of Regions Enablement. Reported by a customer here: #103582.
The fix is to improve distribute_free_regions where aged regions are added to decommit list to ultimately free.
Regression
Yes in memory utilization. For certain customers who were running with dense containers they would observe an OOM occasionally.
Testing
Verified with internal performance testing. Provided a private to the customer to try out and they confirmed their memory utilization improved after the fix.
Risk
Medium, this back port had caused a regression which required a revert in 8.19. We have done more validation to ensure the fix is working well.