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

Decommit region tails #66008

Merged
merged 10 commits into from
Mar 7, 2022
Merged

Conversation

PeterSolMS
Copy link
Contributor

I observed that with gen 1 regions, we often get into the situation that gen 1 is much smaller per heap than a region. So it makes sense to decommit the tail end of the last region in an ephemeral generation guided by the budget for that generation.

To implement this, I reactivated decommit_target for regions and have decommit_step call decommit_ephemeral_segment_pages_step which in the regions case needs to synchronize with the allocator. This is done by taking the more space lock.

Note that with default settings, this decommitting logic will usually only apply to gen 1 because normally gen 0 is larger than a region. It can still happen for gen 0 though if gen 0 has pins and thus already has enough space to satisfy the budget. Then we will decommit the tail end of the last region in gen 0.

@ghost
Copy link

ghost commented Mar 1, 2022

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

I observed that with gen 1 regions, we often get into the situation that gen 1 is much smaller per heap than a region. So it makes sense to decommit the tail end of the last region in an ephemeral generation guided by the budget for that generation.

To implement this, I reactivated decommit_target for regions and have decommit_step call decommit_ephemeral_segment_pages_step which in the regions case needs to synchronize with the allocator. This is done by taking the more space lock.

Note that with default settings, this decommitting logic will usually only apply to gen 1 because normally gen 0 is larger than a region. It can still happen for gen 0 though if gen 0 has pins and thus already has enough space to satisfy the budget. Then we will decommit the tail end of the last region in gen 0.

Author: PeterSolMS
Assignees: PeterSolMS
Labels:

area-GC-coreclr

Milestone: -

@@ -39572,14 +39570,55 @@ void gc_heap::decommit_ephemeral_segment_pages()
}

#if defined(MULTIPLE_HEAPS) && defined(USE_REGIONS)
// for regions, this is done at the regions level
return;
for (int gen_number = soh_gen0; gen_number <= soh_gen1; gen_number++)
Copy link
Member

@Maoni0 Maoni0 Mar 2, 2022

Choose a reason for hiding this comment

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

if we are not even doing a gen1 GC, would we need to goto soh_gen1 here?

if we are doing a gen1 GC we would have called decommit_heap_segment_pages and decommitted (almost) everything after heap_segment_allocated for gen1 regions -

            if (gen_num != 0)
            {
                dprintf (REGIONS_LOG, ("  gen%d decommit end of region %Ix(%Ix)",
                    gen_num, current_region, heap_segment_mem (current_region)));
                decommit_heap_segment_pages (current_region, 0);
            }

if dt_high_memory_load_p() is true. so you could avoid doing the gen1 region in that case. however, I'm also fine if you want to avoid complicating this code path here.

Copy link
Member

Choose a reason for hiding this comment

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

now we are not decommitting end of region space for gen2 since it's only doing the decommitting when memory load is high, this could still make a difference for small benchmarks (as in, regions would use more memory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two reasons for considering the situation in soh_gen1 even if we are not doing a gen 1 GC:

  • The free list situation in gen 1 could have changed.
  • We are smoothing the ramp down, and gen 1 GCs may happen too infrequently. The alternative would be to not do the smoothing, but I think that would likely perform worse in cases where the work load changes behavior.

We need to consider what to do with gen 2 (and UOH as well), agreed. This should be a separate PR, I think.

Copy link
Member

Choose a reason for hiding this comment

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

actually I think for now we should keep the behavior for decommit_heap_segment_pages for now, for gen2/UOH - there's no reason not to because that's only done during a full blocking GC. and it's the same behavior for segments. we can make a separate PR to improve this behavior.

if ((allocated <= decommit_target) && (decommit_target < committed))
{
#ifdef USE_REGIONS
enter_spin_lock (&more_space_lock_soh);
Copy link
Member

Choose a reason for hiding this comment

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

I would not take the msl if gen_number is soh_gen1 since it's unnecessary. taking the msl here is kind of worrisome for perf - this thread is running at THREAD_PRIORITY_HIGHEST which means it will actually likely keep running till it's done with all the decommit work here (this also means the decommit for regions on the decommit list might occupy heap0's core for quite some time before it can be used for anything else). while lower priority threads can run on other cores (and most likely will get to), it's still good to be conscientious about not holding up one core for too long. we do call SetThreadIdealProcessorEx on heap0's proc when we want to allocate on heap0 but that's just the ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I skipped taking the msl lock for soh_gen1.

Regarding tying up GC thread 0, hopefully we won't be tying it up for very long, because we limit the amount of decommit to max_decommit_step_size.

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.

the rest LGTM

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.

new changes look good to me

- add initialization for heap_segment_decommit_target
- check for use_large_pages_p in decommit_step
- for the ephemeral_heap_segment, need to get allocated from the heap, not from the segment
- calling enter_spin_lock may deadlock at the start of a GC, replaced by try_enter_spin_lock
@Maoni0
Copy link
Member

Maoni0 commented Mar 4, 2022

I agree with both fixes. thanks!

@Maoni0
Copy link
Member

Maoni0 commented Mar 4, 2022

one minor thing we could do is to save the tail_region that GC saw and in case it changed, we can simply stop gradual decommit instead of checking the new tail_region.

@PeterSolMS
Copy link
Contributor Author

one minor thing we could do is to save the tail_region that GC saw and in case it changed, we can simply stop gradual decommit instead of checking the new tail_region.

If we added a new tail region, then I think this region must have gone through init_heap_segment which sets heap_segment_decommit_target to heap_segment_reserved. So we shouldn't decommit in this case. So the net effect of not proceeding with decommit should already happen.

@PeterSolMS PeterSolMS merged commit 942430c into dotnet:main Mar 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 6, 2022
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.

2 participants