[release/10.0] Fix heap_segment_used watermark after compaction#128342
Open
janvorli wants to merge 1 commit into
Open
[release/10.0] Fix heap_segment_used watermark after compaction#128342janvorli wants to merge 1 commit into
janvorli wants to merge 1 commit into
Conversation
After compact_phase, heap_segment_used can be stale — lower than the actual end of live data - because `plan_phase` sets `plan_allocated` beyond used for regions that receive relocated objects. When `decommit_region` later clears memory only up to used instead of committed (the large-pages / never_decommit_p path), the gap between used and plan_allocated retains dirty data from a previous region lifetime, causing heap corruption on the next GC cycle. Fix: At the end of `compact_phase`, bump heap_segment_used to `max(used, plan_allocated)` for every non-read-only region in the condemned generations and one generation above (the maximum compaction target range). The fix cost is zero when no compaction occurs. When compaction does occur, it avoids unnecessary `memclr` in `decommit_region` by keeping the used watermark accurate, so only truly unused memory is cleared.
janvorli
added
area-GC-coreclr
Servicing-consider
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/gc |
Contributor
There was a problem hiding this comment.
Pull request overview
This backport addresses a GC regions + large pages correctness issue where stale heap_segment_used values after compaction could cause decommit_region to clear an insufficient range, allowing dirty memory to be reused and leading to heap corruption/crashes.
Changes:
- In
compact_phase(regions mode), updates each affected region’sheap_segment_usedto covermax(used, plan_allocated)for condemned generations and one generation above. - In
decommit_heap_segment(regions mode), skips decommitting when large pages are enabled to avoid incorrect “logical decommit” behavior (large-page decommit is a no-op).
Member
Author
|
cc: @BenV |
Open
3 tasks
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport of #128217 to release/10.0
Customer Impact
GC with large pages enabled in regions mode can lead to intermittent crashes due to non-zeroed memory being returned for an allocation request that expects the memory to be zeroed.
Regression
Testing
CI tests, local testing using targeted repro app from the customer, GC tests
Risk
Low. It adds maintaining
heap_segment_usedwatermark after compaction so that it covers all the touched memory in a region. Before this change, it was stale (lower) for regions that receive relocated objects.