Uh oh!
There was an error while loading. Please reload this page.
Fix issue 41311 - Assert failure ephemeral_heap_segment->saved_committed == heap_segment_committed - #41441
Conversation
…mmitted == heap_segment_committed This is an interaction problem between gradual decommit and entering a no GC region. Details: - When we enter a no GC region, we are manipulating our commit goals outside of GC, we usually are planning to commit more, so it makes sense to stop any gradual decommit in progress. - Even if we need to do a GC in preparation for a no GC region, any decommit target established by decommit_ephemeral_segment_pages is likely to be invalid, so we do an early out. - add an assert to grow_heap_segment so we can catch future issues in this area better. We may want to trigger the gradual decommit logic in set_allocations_for_no_gc if we discover that after accounting for the planned allocation, we still have substantial decommits. I'm leaving this for a later PR though.
ghost
commented
Aug 27, 2020
Tagging subscribers to this area: @dotnet/gc |
| assert (!gradual_decommit_in_progress_p || | ||
| heap_segment_decommit_target (seg) == nullptr || | ||
| heap_segment_committed (seg) <= heap_segment_decommit_target (seg)); | ||
| #endif // MULTIPLE_HEAPS |
There was a problem hiding this comment.
I must be missing something - isn't decommit_target smaller than committed? we keep shrinking committed till we reach decommit_tagert when we do the decommit steps.
also when gradual_decommit_in_progress_p is TRUE, shouldn't decommit_target always be non NULL?
nit -
assert (!gradual_decommit_in_progress_p ||
(heap_segment_decommit_target (seg) == nullptr) ||
(heap_segment_committed (seg) <= heap_segment_decommit_target (seg)));
There was a problem hiding this comment.
For some heaps, decommit_target may be bigger than committed, and that's fine - it just means that we won't decommit.
When gradual_decommit_in_progress_p is TRUE, decommit_target will be non NULL for the ephemeral_heap_segment, but may be NULL for other segments - the condition was meant to filter out the other segments. Now that I think about it, I realize this isn't a good test - I should compare against ephemeral_heap_segment directly. So I fixed the assert as follows:
// we should never increase committed beyond decommit target when gradual
// decommit is in progress - if we do, this means commit and decommit are
// going on at the same time.
assert (!gradual_decommit_in_progress_p ||
(seg != ephemeral_heap_segment) ||
(heap_segment_committed (seg) <= heap_segment_decommit_target (seg)));
There was a problem hiding this comment.
oh right, I was only thinking about eph seg. and yes the point is if we actually need to commit more, the decommit target had better be bigger otherwise we have a problem. thanks! I agree the new assert looks better.
| void gc_heap::decommit_ephemeral_segment_pages() | ||
| { | ||
| if (settings.concurrent || use_large_pages_p) | ||
| if (settings.concurrent || use_large_pages_p || settings.pause_mode == pause_no_gc) |
There was a problem hiding this comment.
if (settings.concurrent || use_large_pages_p || settings.pause_mode == pause_no_gc) [](start = 4, length = 83)
nit
if (settings.concurrent || use_large_pages_p || (settings.pause_mode == pause_no_gc))
PeterSolMS
commented
Sep 1, 2020
/backport to release/5.0 |
Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/234190938 |
This is an interaction problem between gradual decommit and entering a no GC region.
Details:
We may want to trigger the gradual decommit logic in set_allocations_for_no_gc if we discover that after accounting for the planned allocation, we still have substantial decommits. I'm leaving this for a later PR
cc @xiangzhai