From 9fb2f8ff5751c1d3308174ada923c0091c9854c3 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Wed, 8 Feb 2023 17:09:19 -0800 Subject: [PATCH] Avoid using a null heap to decommit the mark array (#80640) (#81295) --- src/coreclr/gc/gc.cpp | 122 ++++++++++++++++++++++++++-------------- src/coreclr/gc/gcpriv.h | 4 ++ 2 files changed, 85 insertions(+), 41 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 6742ea79497d1..4f23aabbddbba 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -3971,6 +3971,8 @@ bool region_allocator::allocate_large_region (uint8_t** start, uint8_t** end, al return allocate_region (size, start, end, direction, fn); } +// Whenever a region is deleted, it is expected that the memory and the mark array +// of the region is decommitted already. void region_allocator::delete_region (uint8_t* region_start) { enter_spin_lock(); @@ -20308,25 +20310,25 @@ bool gc_heap::try_get_new_free_region() bool gc_heap::init_table_for_region (int gen_number, heap_segment* region) { #ifdef BACKGROUND_GC - if (is_bgc_in_progress()) + if (is_bgc_in_progress()) + { + dprintf (GC_TABLE_LOG, ("new seg %Ix, mark_array is %Ix", + heap_segment_mem (region), mark_array)); + if (((region->flags & heap_segment_flags_ma_committed) == 0) && + !commit_mark_array_new_seg (__this, region)) { - dprintf (GC_TABLE_LOG, ("new seg %Ix, mark_array is %Ix", - heap_segment_mem (region), mark_array)); - if (((region->flags & heap_segment_flags_ma_committed) == 0) && - !commit_mark_array_new_seg (__this, region)) - { - dprintf (GC_TABLE_LOG, ("failed to commit mark array for the new region %Ix-%Ix", - get_region_start (region), heap_segment_reserved (region))); + dprintf (GC_TABLE_LOG, ("failed to commit mark array for the new region %Ix-%Ix", + get_region_start (region), heap_segment_reserved (region))); - // We don't have memory to commit the mark array so we cannot use the new region. - global_region_allocator.delete_region (get_region_start (region)); - return false; - } - } - if ((region->flags & heap_segment_flags_ma_committed) != 0) - { - bgc_verify_mark_array_cleared (region, true); + // We don't have memory to commit the mark array so we cannot use the new region. + decommit_region (region, gen_to_oh (gen_number), heap_number); + return false; } + } + if ((region->flags & heap_segment_flags_ma_committed) != 0) + { + bgc_verify_mark_array_cleared (region, true); + } #endif //BACKGROUND_GC if (gen_number <= max_generation) @@ -40970,31 +40972,7 @@ bool gc_heap::decommit_step (uint64_t step_milliseconds) while (global_regions_to_decommit[kind].get_num_free_regions() > 0) { heap_segment* region = global_regions_to_decommit[kind].unlink_region_front(); - - uint8_t* page_start = align_lower_page(get_region_start(region)); - uint8_t* end = use_large_pages_p ? heap_segment_used(region) : heap_segment_committed(region); - size_t size = end - page_start; - bool decommit_succeeded_p = false; - if (!use_large_pages_p) - { - decommit_succeeded_p = virtual_decommit(page_start, size, recorded_committed_free_bucket); - dprintf(REGIONS_LOG, ("decommitted region %Ix(%Ix-%Ix) (%Iu bytes) - success: %d", - region, - page_start, - end, - size, - decommit_succeeded_p)); - } - if (!decommit_succeeded_p) - { - memclr(page_start, size); - dprintf(REGIONS_LOG, ("cleared region %Ix(%Ix-%Ix) (%Iu bytes)", - region, - page_start, - end, - size)); - } - global_region_allocator.delete_region(get_region_start(region)); + size_t size = decommit_region (region, recorded_committed_free_bucket, -1); decommit_size += size; if (decommit_size >= max_decommit_step_size) { @@ -41021,6 +40999,68 @@ bool gc_heap::decommit_step (uint64_t step_milliseconds) return (decommit_size != 0); } +#ifdef USE_REGIONS +size_t gc_heap::decommit_region (heap_segment* region, int bucket, int h_number) +{ + uint8_t* page_start = align_lower_page (get_region_start (region)); + uint8_t* end = use_large_pages_p ? heap_segment_used (region) : heap_segment_committed (region); + size_t size = end - page_start; + bool decommit_succeeded_p = false; + if (!use_large_pages_p) + { + decommit_succeeded_p = virtual_decommit (page_start, size, bucket, h_number); + } + dprintf (REGIONS_LOG, ("decommitted region %p(%p-%p) (%zu bytes) - success: %d", + region, + page_start, + end, + size, + decommit_succeeded_p)); + if (decommit_succeeded_p) + { + heap_segment_committed (region) = heap_segment_mem (region); + } + else + { + memclr (page_start, size); + heap_segment_used (region) = heap_segment_mem (region); + dprintf(REGIONS_LOG, ("cleared region %p(%p-%p) (%zu bytes)", + region, + page_start, + end, + size)); + } + + // Under USE_REGIONS, mark array is never partially committed. So we are only checking for this + // flag here. + if ((region->flags & heap_segment_flags_ma_committed) != 0) + { +#ifdef MULTIPLE_HEAPS + // In return_free_region, we set heap_segment_heap (region) to nullptr so we cannot use it here. + // but since all heaps share the same mark array we simply pick the 0th heap to use.  + gc_heap* hp = g_heaps [0]; +#else + gc_heap* hp = pGenGCHeap; +#endif + hp->decommit_mark_array_by_seg (region); + region->flags &= ~(heap_segment_flags_ma_committed); + } + + if (use_large_pages_p) + { + assert (heap_segment_used (region) == heap_segment_mem (region)); + } + else + { + assert (heap_segment_committed (region) == heap_segment_mem (region)); + } + assert ((region->flags & heap_segment_flags_ma_committed) == 0); + global_region_allocator.delete_region (get_region_start (region)); + + return size; +} +#endif //USE_REGIONS + #ifdef MULTIPLE_HEAPS // return the decommitted size size_t gc_heap::decommit_ephemeral_segment_pages_step () diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index ebbe1bf077851..995a6f14b586e 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2081,6 +2081,10 @@ class gc_heap size_t decommit_heap_segment_pages_worker (heap_segment* seg, uint8_t *new_committed); PER_HEAP_ISOLATED bool decommit_step (uint64_t step_milliseconds); +#ifdef USE_REGIONS + PER_HEAP_ISOLATED + size_t decommit_region (heap_segment* region, int bucket, int h_number); +#endif //USE_REGIONS PER_HEAP void decommit_heap_segment (heap_segment* seg); PER_HEAP_ISOLATED