[release/9.0] Revert "[GC] Avoid OOM in large-allocation-only workloads (#105521)"#107712
Merged
jeffschwMSFT merged 3 commits intodotnet:release/9.0from Sep 13, 2024
Merged
[release/9.0] Revert "[GC] Avoid OOM in large-allocation-only workloads (#105521)"#107712jeffschwMSFT merged 3 commits intodotnet:release/9.0from
jeffschwMSFT merged 3 commits intodotnet:release/9.0from
Conversation
…otnet#105521)" This partially reverts commit b27a808. This is done instead of a full revert because there were merge conflicts with bfb674e. This also minimizes diffs with main in case of future backports. This reverts the distribute_free_regions but keeps the factoring. It also keeps the aging of huge free regions, but nothing is done with those ages.
ghost
added
the
Contributor
|
Tagging subscribers to this area: @dotnet/gc |
…tnet#105521)" This reverts the rest of commit b27a808 except for the age helper.
markples
changed the title
markples
changed the title
markples
changed the title
jeffschwMSFT
approved these changes
Sep 13, 2024
Member
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
There was a problem hiding this comment.
approved. please get a code review. we can merge when ready
jeffschwMSFT
added
Servicing-approved
Maoni0
approved these changes
Sep 13, 2024
Member
|
@markples please take a look at the PR failures |
Contributor
Author
@jeffschwMSFT It's one known test error (and known to be on the 9.0 branch). I don't know what else build analysis looks at, but I verified that the pipeline reports a total error count of 1, has 1 failing step, and the log for that step reports just the one test. |
2 tasks
markples
added a commit
to markples/runtime
that referenced
this pull request
Sep 30, 2024
…location-only workloads (dotnet#105521)" (dotnet#107712)', which is b27a808 The 9.0 revert already handled merge conflicts with bfb674e, which is in main and 9.0. However, this keeps the changes to age huge free regions. Nothing is done with those ages, but this avoids churn to put it back in later.
markples
added a commit
that referenced
this pull request
Oct 8, 2024
…ent strategy (#108417) * Revert "[GC] Allow distribute_free_regions to decommit and redistribute (#106414)" This reverts commit 566a3bd. * Cherry-pick part of '[release/9.0] Revert "[GC] Avoid OOM in large-allocation-only workloads (#105521)" (#107712)', which is b27a808 The 9.0 revert already handled merge conflicts with bfb674e, which is in main and 9.0. However, this keeps the changes to age huge free regions. Nothing is done with those ages, but this avoids churn to put it back in later.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revert "[GC] Avoid OOM in large-allocation-only workloads (#105521)", which is commit b27a808.
The revert had merge conflicts with bfb674e. That commit changed the ordering of distribute_free_regions/age_free_regions with other code, so it was simpler to leave the new age_free_regions in place. The original factoring into age_free_regions also adding aging of huge regions. This change reverts that part in order to restore the original behavior.
Customer Impact
#105521 was intended to be followed by #106414 and possibly another improvement, but subsequent study has shown that the existing bookkeeping for large vs. huge regions is inconsistent, and these changes can trigger some strange behavior. In various cases, it can cause
The solution is going to involve a bigger change to distribute_free_regions if not something else like estimate_gen_growth as well, so this goes back to the original behavior until that can happen in 10.
Regression
Testing
distribute_free_region was factored into a standalone C++ project in order to run it in various scenarios. Due to the issues listed above, the original behavior was chosen for .NET 9.
Risk
Low - this reverts from newly added behavior to what had been in place for months.