From 857e3b3ec88fd609c80024e6e62523460f97b950 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 22 Dec 2022 10:51:23 -0800 Subject: [PATCH 1/4] Decommit mark array when decommitting region --- src/coreclr/gc/gc.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index bf7d5a07c12b2..7475e48219676 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -41072,6 +41072,12 @@ bool gc_heap::decommit_step (uint64_t step_milliseconds) if (!use_large_pages_p) { decommit_succeeded_p = virtual_decommit(page_start, size, recorded_committed_free_bucket); +#ifdef MULTIPLE_HEAPS + gc_heap* hp = heap_segment_heap (region); +#else + gc_heap* hp = __this; +#endif + hp->decommit_mark_array_by_seg (region); dprintf(REGIONS_LOG, ("decommitted region %p(%p-%p) (%zu bytes) - success: %d", region, page_start, From 697d7134f43eb23e7b1296564370033c4f61e38d Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 22 Dec 2022 15:56:11 -0800 Subject: [PATCH 2/4] Decommit region before delete region --- src/coreclr/gc/gc.cpp | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 7475e48219676..a2eaf0575b723 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -20325,22 +20325,31 @@ bool gc_heap::try_get_new_free_region() bool gc_heap::init_table_for_region (int gen_number, heap_segment* region) { #ifdef BACKGROUND_GC - 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, ("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))); - // 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. + int h_number = +#ifdef MULTIPLE_HEAPS + heap_number; +#else + 0; +#endif + gc_oh_num oh = gen_to_oh (gen_number); + size_t size = heap_segment_committed(region) - heap_segment_mem (region); + virtual_decommit(heap_segment_mem (region), size, oh, h_number); + 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); + } #endif //BACKGROUND_GC if (gen_number <= max_generation) From 3497400c86d63140e2ac50ef92d82e515f02b915 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Fri, 6 Jan 2023 15:54:31 -0800 Subject: [PATCH 3/4] Code Review Feedback --- src/coreclr/gc/gc.cpp | 97 +++++++++++++++++++++++++---------------- src/coreclr/gc/gcpriv.h | 6 +++ 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index a2eaf0575b723..dbee5b368a235 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -3988,6 +3988,8 @@ bool region_allocator::allocate_large_region (int gen_num, uint8_t** start, uint return allocate_region (gen_num, 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(); @@ -20334,16 +20336,8 @@ bool gc_heap::init_table_for_region (int gen_number, heap_segment* region) 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. - int h_number = -#ifdef MULTIPLE_HEAPS - heap_number; -#else - 0; -#endif - gc_oh_num oh = gen_to_oh (gen_number); - size_t size = heap_segment_committed(region) - heap_segment_mem (region); - virtual_decommit(heap_segment_mem (region), size, oh, h_number); - global_region_allocator.delete_region (get_region_start (region)); + decommit_region (region, gen_to_oh (gen_number), heap_number); + delete_region (region); return false; } if ((region->flags & heap_segment_flags_ma_committed) != 0) @@ -41073,37 +41067,15 @@ 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); + size_t size = decommit_region (region, recorded_committed_free_bucket, -1); #ifdef MULTIPLE_HEAPS - gc_heap* hp = heap_segment_heap (region); + gc_heap* hp = heap_segment_heap (region); #else - gc_heap* hp = __this; + gc_heap* hp = pGenGCHeap; #endif - hp->decommit_mark_array_by_seg (region); - dprintf(REGIONS_LOG, ("decommitted region %p(%p-%p) (%zu bytes) - success: %d", - region, - page_start, - end, - size, - decommit_succeeded_p)); - } - if (!decommit_succeeded_p) - { - memclr(page_start, size); - dprintf(REGIONS_LOG, ("cleared region %p(%p-%p) (%zu bytes)", - region, - page_start, - end, - size)); - } - global_region_allocator.delete_region(get_region_start(region)); + hp->decommit_mark_array_by_seg (region); + region->flags &= ~(heap_segment_flags_ma_committed); + delete_region (region); decommit_size += size; if (decommit_size >= max_decommit_step_size) { @@ -41130,6 +41102,55 @@ 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)); + } + return size; +} + +void gc_heap::delete_region (heap_segment* region) +{ + 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)); +} +#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 6514729366da2..89b4e6a4d2fe2 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2075,6 +2075,12 @@ 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); + PER_HEAP_ISOLATED + void delete_region (heap_segment* region); +#endif //USE_REGIONS PER_HEAP void decommit_heap_segment (heap_segment* seg); PER_HEAP_ISOLATED From 88f14dcefe08a73f76c27f4b3610e9d7654dbd4a Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Tue, 10 Jan 2023 16:34:11 -0800 Subject: [PATCH 4/4] Merge methods --- src/coreclr/gc/gc.cpp | 26 +++++++++++++------------- src/coreclr/gc/gcpriv.h | 2 -- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index dbee5b368a235..f66d9d7ab7771 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -20337,7 +20337,6 @@ bool gc_heap::init_table_for_region (int gen_number, heap_segment* region) // 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); - delete_region (region); return false; } if ((region->flags & heap_segment_flags_ma_committed) != 0) @@ -41068,14 +41067,6 @@ bool gc_heap::decommit_step (uint64_t step_milliseconds) { heap_segment* region = global_regions_to_decommit[kind].unlink_region_front(); size_t size = decommit_region (region, recorded_committed_free_bucket, -1); -#ifdef MULTIPLE_HEAPS - gc_heap* hp = heap_segment_heap (region); -#else - gc_heap* hp = pGenGCHeap; -#endif - hp->decommit_mark_array_by_seg (region); - region->flags &= ~(heap_segment_flags_ma_committed); - delete_region (region); decommit_size += size; if (decommit_size >= max_decommit_step_size) { @@ -41133,11 +41124,18 @@ size_t gc_heap::decommit_region (heap_segment* region, int bucket, int h_number) end, size)); } - return size; -} -void gc_heap::delete_region (heap_segment* region) -{ + if ((region->flags & heap_segment_flags_ma_committed) != 0) + { +#ifdef MULTIPLE_HEAPS + gc_heap* hp = heap_segment_heap (region); +#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)); @@ -41148,6 +41146,8 @@ void gc_heap::delete_region (heap_segment* region) } assert ((region->flags & heap_segment_flags_ma_committed) == 0); global_region_allocator.delete_region (get_region_start (region)); + + return size; } #endif //USE_REGIONS diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index 89b4e6a4d2fe2..09f2a9de208c2 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2078,8 +2078,6 @@ class gc_heap #ifdef USE_REGIONS PER_HEAP_ISOLATED size_t decommit_region (heap_segment* region, int bucket, int h_number); - PER_HEAP_ISOLATED - void delete_region (heap_segment* region); #endif //USE_REGIONS PER_HEAP void decommit_heap_segment (heap_segment* seg);