Skip to content

Clear entire range on decommit #128032

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.

Sign up for GitHub

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

Closed
BenV wants to merge 1 commit into from
+1 −1
Closed

Clear entire range on decommit #128032

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/gc/memory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ size_t gc_heap::decommit_region (heap_segment* region, int bucket, int h_number)
decommit_succeeded_p));
if (require_clearing_memory_p)
{
uint8_t* clear_end = never_decommit_p ? heap_segment_used (region) : heap_segment_committed (region);
Copy link
Copy Markdown
Member

@janvorli janvorli May 11, 2026

Choose a reason for hiding this comment

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

@BenV I am not 100% sure the actual problem is at this place. Last week I've tried to add clearing to all places where we skip it for large pages. And I've found that if I keep it at all the other places except of this one, it still passed the test you've shared. I am going to look into it more today, but that seems to indicate that the clearing we are adding here may be just fixing what should have been done at another place.
I didn't have a chance to read through @cshung's analysis yet though, I am going to do that first.

Copy link
Copy Markdown
Member

@janvorli janvorli May 11, 2026

Choose a reason for hiding this comment

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

@cshung after reading your analysis (thank you for that!), I wonder - why is clearing the memory here better than clearing the necessary part after the compaction / replanning instead? Wouldn't we end up zeroing less memory that way?

Copy link
Copy Markdown
Author

@BenV BenV May 11, 2026

Choose a reason for hiding this comment

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

That sounds good, let me know what you find and if you need anything else from us. Thanks again.

Copy link
Copy Markdown
Contributor

@cshung cshung May 11, 2026

Choose a reason for hiding this comment

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

@janvorli, the change will stop the bleeding for now, we can definitely try to optimize to zero less memory.

Copy link
Copy Markdown
Author

@BenV BenV May 11, 2026

Choose a reason for hiding this comment

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

I am admittedly biased but I agree it would be great to get the simple fix in and optimize later (particularly if we could backport this to .NET 10) - I'd rather zero a bit of extra memory if it is guaranteed to avoid corruption as GCLargePages is essentially a landmine right now. That being said if some additional investigation reveals a similarly simple fix elsewhere I would be all for that as well.

Copy link
Copy Markdown
Author

@BenV BenV May 14, 2026

Choose a reason for hiding this comment

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

Amazing, thanks @cshung - if @janvorli is able to verify it in .NET 10 I'm assuming we should close this out in favor of that change instead, right?

Copy link
Copy Markdown
Member

@janvorli janvorli May 15, 2026

Choose a reason for hiding this comment

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

@BenV, @cshung I can confirm that @cshung's change combined with the gc_heap::decommit_heap_segment early out change I've mentioned above passed the test that @BenV has shared.

Copy link
Copy Markdown
Author

@BenV BenV May 15, 2026

Choose a reason for hiding this comment

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

@janvorli Excellent, so would the plan be to backport your proposed gc_heap::decommit_heap_segment combined with #128217 to .NET 10 (and close this out)?

Copy link
Copy Markdown
Member

@janvorli janvorli May 18, 2026

Choose a reason for hiding this comment

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

Yes, that's the plan.

Copy link
Copy Markdown
Member

@janvorli janvorli May 18, 2026

Choose a reason for hiding this comment

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

@BenV, @cshung I've just merged the #128217 and I'll prepare the backport later today or tomorrow.

uint8_t* clear_end = heap_segment_committed (region);
size_t clear_size = clear_end - page_start;
memclr (page_start, clear_size);
heap_segment_used (region) = heap_segment_mem (region);
Expand Down
Loading