Skip to content
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.

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

This fixes an issue with gradual decommit in scenarios where we have … #84975

Merged
merged 3 commits into from May 22, 2023

Conversation

PeterSolMS
Copy link
Contributor

…almost exclusively background GCs.

The problem is that we turn off the gradual_decommit_in_progress_p flag and rely on the distribute_free_regions method to turn it back on. If we only trigger a BGC however, distribute_free_regions won't get called and the flag will stay off, despite the global_regions_to_decommit list being non-empty.

This will cause regions to accumulate in the global_regions_to_decommit list, and eventual will cause the process to run out of memory.

The fix is to remember the setting of the gradual_decommit_in_progress_p flag in a local variable and turn gradual_decommit_in_progress_p back on if only a BGC was triggered.

This a partial fix to issue #78959.

…almost exclusively background GCs.

The problem is that we turn off the gradual_decommit_in_progress_p flag and rely on the distribute_free_regions flag to turn it back on. If we only trigger a BGC however, distribute_free_regions won't get called and the flag will stay off, despite the global_regions_to_decommit list being non-empty.

This will cause regions to accumulate in the global_regions_to_decommit list, and eventual will cause the process to run out of memory.
@Maoni0
Copy link
Member

Maoni0 commented May 3, 2023

what if we don't set gradual_decommit_in_progress_p before triggering a GC, and in distribute_free_regions we set it to FALSE if we have nothing on the global decommit list? so if we only triggered a BGC, gradual_decommit_in_progress_p would naturally be preserved?

@PeterSolMS
Copy link
Contributor Author

There's a complication in that decommit_ephemeral_segment_pages also sets gradual_decommit_in_progress_p, so it's more natural to reset gradual_decommit_in_progress_p at the beginning of a GC, and set it when we discover we have something to decommit. So maybe the better solution is to just move resetting gradual_decommit_in_progress_p to a place where we are already sure we're going to do a blocking GC. We have to keep no GC regions in mind though to make sure decommitting and allocating don't conflict in this case.

@Maoni0
Copy link
Member

Maoni0 commented May 4, 2023

yeh, you could just always reset it during a blocking GC, eg, during the gc_join_decide_on_compaction join. I think this would actually make no GC region work better because currently gradual_decommit_in_progress_p is always reset to false even when we do a GC for no GC region where we just allocate a bunch of regions but don't go into plan phase and later which will change the gradual_decommit_in_progress_p value. so if we had stuff to decommit it means it wouldn't continue when we get out of the GC that was done for no GC region.

@PeterSolMS
Copy link
Contributor Author

The better solution appears to be to just test for the no gc region case in decommit_step.

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.

LGTM!

@PeterSolMS PeterSolMS merged commit dd390e1 into dotnet:main May 22, 2023
109 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 21, 2023
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.

None yet

2 participants