From 71c0cdb49b589529c337bb1ad59c82d984c34a4c Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 14 Jul 2022 12:53:53 -0700 Subject: [PATCH 1/4] Fixing the per object hard limit support for regions --- src/coreclr/gc/gc.cpp | 223 +++++++++++++++++++++++++++++++++++++--- src/coreclr/gc/gcpriv.h | 23 +++-- 2 files changed, 225 insertions(+), 21 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index cd938431d70ed..72e0133ba5eee 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -2198,7 +2198,7 @@ CLRCriticalSection gc_heap::check_commit_cs; size_t gc_heap::current_total_committed = 0; -size_t gc_heap::committed_by_oh[total_oh_count] = {0, 0, 0, 0}; +size_t gc_heap::committed_by_oh[gc_oh_num::total_bucket_count] = {0, 0, 0, 0, 0}; size_t gc_heap::current_total_committed_bookkeeping = 0; @@ -2275,7 +2275,7 @@ uint64_t gc_heap::entry_available_physical_mem = 0; size_t gc_heap::heap_hard_limit = 0; -size_t gc_heap::heap_hard_limit_oh[total_oh_count - 1] = {0, 0, 0}; +size_t gc_heap::heap_hard_limit_oh[gc_oh_num::total_oh_count] = {0, 0, 0}; #ifdef USE_REGIONS size_t gc_heap::regions_range = 0; @@ -2370,7 +2370,7 @@ oom_history gc_heap::oomhist_per_heap[max_oom_history_count]; fgm_history gc_heap::fgm_result; -size_t gc_heap::allocated_since_last_gc[gc_oh_num::total_oh_count - 1]; +size_t gc_heap::allocated_since_last_gc[gc_oh_num::total_oh_count]; BOOL gc_heap::ro_segments_in_range; @@ -2412,7 +2412,7 @@ uint8_t* gc_heap::last_gen1_pin_end; gen_to_condemn_tuning gc_heap::gen_to_condemn_reasons; -size_t gc_heap::etw_allocation_running_amount[gc_oh_num::total_oh_count - 1]; +size_t gc_heap::etw_allocation_running_amount[gc_oh_num::total_oh_count]; uint64_t gc_heap::total_alloc_bytes_soh = 0; @@ -6815,6 +6815,8 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu assert (heap_hard_limit == 0); #endif //!HOST_64BIT + dprintf(3, ("commit-accounting: commit in %d [%Ix, %Ix) for heap %d", oh, address, ((uint8_t*)address + size), h_number)); + if (heap_hard_limit) { check_commit_cs.Enter(); @@ -6839,6 +6841,13 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu if (!exceeded_p) { +#if defined(_DEBUG) && defined(MULTIPLE_HEAPS) + if (oh < gc_oh_num::free) + { + assert (h_number != -1); + g_heaps[h_number]->committed_by_oh_per_heap[oh] += size; + } +#endif // _DEBUG && MULTIPLE_HEAPS committed_by_oh[oh] += size; current_total_committed += size; if (h_number < 0) @@ -6867,7 +6876,14 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu { check_commit_cs.Enter(); committed_by_oh[oh] -= size; - +#if defined(_DEBUG) && defined(MULTIPLE_HEAPS) + if (oh < gc_oh_num::free) + { + assert (h_number != -1); + assert (g_heaps[h_number]->committed_by_oh_per_heap[oh] >= size); + g_heaps[h_number]->committed_by_oh_per_heap[oh] -= size; + } +#endif // _DEBUG && MULTIPLE_HEAPS dprintf (1, ("commit failed, updating %Id to %Id", current_total_committed, (current_total_committed - size))); current_total_committed -= size; @@ -6887,10 +6903,21 @@ bool gc_heap::virtual_decommit (void* address, size_t size, gc_oh_num oh, int h_ bool decommit_succeeded_p = GCToOSInterface::VirtualDecommit (address, size); + dprintf(3, ("commit-accounting: decommit in %d [%Ix, %Ix) for heap %d", oh, address, ((uint8_t*)address + size), h_number)); + if (decommit_succeeded_p && heap_hard_limit) { check_commit_cs.Enter(); + assert (committed_by_oh[oh] >= size); committed_by_oh[oh] -= size; +#if defined(_DEBUG) && defined(MULTIPLE_HEAPS) + if (oh < gc_oh_num::free) + { + assert (h_number != -1); + assert (g_heaps[h_number]->committed_by_oh_per_heap[oh] >= size); + g_heaps[h_number]->committed_by_oh_per_heap[oh] -= size; + } +#endif // _DEBUG && MULTIPLE_HEAPS current_total_committed -= size; if (h_number < 0) current_total_committed_bookkeeping -= size; @@ -11117,6 +11144,24 @@ void gc_heap::clear_region_info (heap_segment* region) // REGIONS PERF TODO: should decommit if needed. void gc_heap::return_free_region (heap_segment* region) { + gc_oh_num oh = heap_segment_oh (region); + dprintf(3, ("commit-accounting: from %d to free [%Ix, %Ix) for heap %d", oh, get_region_start (region), heap_segment_committed (region), heap_number)); + if (heap_hard_limit_oh[soh]) + { + size_t committed = heap_segment_committed (region) - get_region_start (region); + if (committed > 0) + { + check_commit_cs.Enter(); + assert (committed_by_oh[oh] >= committed); + committed_by_oh[oh] -= committed; + committed_by_oh[free] += committed; +#if defined(_DEBUG) && defined(MULTIPLE_HEAPS) + assert (committed_by_oh_per_heap[oh] >= committed); + committed_by_oh_per_heap[oh] -= committed; +#endif // _DEBUG && MULTIPLE_HEAPS + check_commit_cs.Leave(); + } + } clear_region_info (region); region_free_list::add_region_descending (region, free_regions); @@ -11184,6 +11229,25 @@ heap_segment* gc_heap::get_free_region (int gen_number, size_t size) init_heap_segment (region, __this, region_start, (region_end - region_start), gen_number); + + gc_oh_num oh = gen_to_oh (gen_number); + dprintf(3, ("commit-accounting: from free to %d [%Ix, %Ix) for heap %d", oh, get_region_start (region), heap_segment_committed (region), heap_number)); + if (heap_hard_limit_oh[soh]) + { + size_t committed = heap_segment_committed (region) - get_region_start (region); + if (committed > 0) + { + check_commit_cs.Enter(); + committed_by_oh[oh] += committed; + assert (committed_by_oh[free] >= committed); + committed_by_oh[free] -= committed; +#if defined(_DEBUG) && defined(MULTIPLE_HEAPS) + committed_by_oh_per_heap[oh] += committed; +#endif // _DEBUG && MULTIPLE_HEAPS + check_commit_cs.Leave(); + } + } + dprintf (REGIONS_LOG, ("h%d GFR get region %Ix (%Ix-%Ix) for gen%d", heap_number, (size_t)region, region_start, region_end, @@ -13746,13 +13810,21 @@ void gc_heap::add_saved_spinlock_info ( } int -gc_heap::init_gc_heap (int h_number) +gc_heap::init_gc_heap (int h_number) { #ifdef MULTIPLE_HEAPS +#ifdef _DEBUG + for (int i = 0; i < gc_oh_num::total_oh_count; i++) + { + committed_by_oh_per_heap[i] = 0; + } +#endif + + g_heaps [h_number] = this; time_bgc_last = 0; - for (int oh_index = 0; oh_index < (gc_oh_num::total_oh_count - 1); oh_index++) + for (int oh_index = 0; oh_index < gc_oh_num::total_oh_count; oh_index++) allocated_since_last_gc[oh_index] = 0; #ifdef SPINLOCK_HISTORY @@ -14002,7 +14074,7 @@ gc_heap::init_gc_heap (int h_number) generation_of (loh_generation)->free_list_allocator = allocator(NUM_LOH_ALIST, BASE_LOH_ALIST_BITS, loh_alloc_list); generation_of (poh_generation)->free_list_allocator = allocator(NUM_POH_ALIST, BASE_POH_ALIST_BITS, poh_alloc_list); - for (int oh_index = 0; oh_index < (gc_oh_num::total_oh_count - 1); oh_index++) + for (int oh_index = 0; oh_index < gc_oh_num::total_oh_count; oh_index++) etw_allocation_running_amount[oh_index] = 0; total_alloc_bytes_soh = 0; @@ -14061,8 +14133,6 @@ gc_heap::init_gc_heap (int h_number) if (!create_gc_thread ()) return 0; - g_heaps [heap_number] = this; - #endif //MULTIPLE_HEAPS #ifdef FEATURE_PREMORTEM_FINALIZATION @@ -22616,6 +22686,18 @@ heap_segment* gc_heap::unlink_first_rw_region (int gen_idx) assert (!heap_segment_read_only_p (region)); dprintf (REGIONS_LOG, ("unlink_first_rw_region on heap: %d gen: %d region: %Ix", heap_number, gen_idx, heap_segment_mem (region))); + int old_oh = heap_segment_oh (region); + int old_heap = heap_segment_heap (region)->heap_number; + dprintf(3, ("commit-accounting: from %d to temp [%Ix, %Ix) for heap %d", old_oh, get_region_start (region), heap_segment_committed (region), old_heap)); + +#ifdef _DEBUG + size_t committed = heap_segment_committed (region) - get_region_start (region); + check_commit_cs.Enter(); + assert (g_heaps[old_heap]->committed_by_oh_per_heap[old_oh] >= committed); + g_heaps[old_heap]->committed_by_oh_per_heap[old_oh] -= committed; + check_commit_cs.Leave(); +#endif // _DEBUG + set_heap_for_contained_basic_regions (region, nullptr); return region; @@ -22638,6 +22720,18 @@ void gc_heap::thread_rw_region_front (int gen_idx, heap_segment* region) } dprintf (REGIONS_LOG, ("thread_rw_region_front on heap: %d gen: %d region: %Ix", heap_number, gen_idx, heap_segment_mem (region))); + int new_oh = gen_to_oh (gen_idx); + int new_heap = this->heap_number; + dprintf(3, ("commit-accounting: from temp to %d [%Ix, %Ix) for heap %d", new_oh, get_region_start (region), heap_segment_committed (region), new_heap)); + +#ifdef _DEBUG + size_t committed = heap_segment_committed (region) - get_region_start (region); + check_commit_cs.Enter(); + assert (heap_segment_heap (region) == nullptr); + g_heaps[new_heap]->committed_by_oh_per_heap[new_oh] += committed; + check_commit_cs.Leave(); +#endif // _DEBUG + set_heap_for_contained_basic_regions (region, this); } #endif // MULTIPLE_HEAPS @@ -29838,6 +29932,59 @@ void gc_heap::plan_phase (int condemned_gen_number) } #endif //FEATURE_EVENT_TRACE +#if defined(_DEBUG) && defined(USE_REGIONS) + if (heap_hard_limit_oh[soh]) + { + size_t committed = 0; + for (int i = 0; i < total_generation_count; i++) + { + int oh = i - max_generation; +#ifdef MULTIPLE_HEAPS + for (int hn = 0; hn < gc_heap::n_heaps; hn++) + { + gc_heap* hp = gc_heap::g_heaps[hn]; +#else + { + gc_heap* hp = nullptr; +#endif // MULTIPLE_HEAPS + heap_segment* region = generation_start_segment (hp->generation_of (i)); + while (region) + { + committed += heap_segment_committed (region) - get_region_start (region); + region = heap_segment_next (region); + } + + if (oh == soh) + { + heap_segment* freeable = freeable_soh_segment; + while (freeable) + { + committed += (heap_segment_committed (freeable) - get_region_start (freeable)); + freeable = heap_segment_next (freeable); + } + } + else + { + heap_segment* freeable = freeable_uoh_segment; + while (freeable) + { + if (heap_segment_oh (freeable) == oh) + { + committed += (heap_segment_committed (freeable) - get_region_start (freeable)); + } + freeable = heap_segment_next (freeable); + } + } + } + if (i >= max_generation) + { + assert (committed_by_oh[oh] == committed); + committed = 0; + } + } + } +#endif // _DEBUG && USE_REGIONS + #ifdef MULTIPLE_HEAPS //join all threads to make sure they are synchronized dprintf(3, ("Restarting after Promotion granted")); @@ -30558,7 +30705,8 @@ heap_segment* gc_heap::get_new_region (int gen_number, size_t size) heap_segment_next (generation_tail_region (gen)) = new_region; generation_tail_region (gen) = new_region; - verify_regions (gen_number, false, settings.concurrent); + size_t unused_total_committed; + verify_regions (gen_number, false, settings.concurrent, unused_total_committed); } return new_region; @@ -39895,7 +40043,7 @@ bool gc_heap::decommit_step () bool decommit_succeeded_p = false; if (!use_large_pages_p) { - decommit_succeeded_p = virtual_decommit(page_start, size, heap_segment_oh(region), 0); + decommit_succeeded_p = virtual_decommit(page_start, size, gc_oh_num::free, -1); dprintf(REGIONS_LOG, ("decommitted region %Ix(%Ix-%Ix) (%Iu bytes) - success: %d", region, page_start, @@ -43138,7 +43286,7 @@ gc_heap::verify_free_lists () } } -void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail) +void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t& total_committed) { #ifdef USE_REGIONS // For the given generation, verify that @@ -43157,6 +43305,7 @@ void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_ while (seg_in_gen) { + total_committed += (heap_segment_committed (seg_in_gen) - get_region_start (seg_in_gen)); if (can_verify_gen_num) { if (heap_segment_gen_num (seg_in_gen) != min (gen_number, max_generation)) @@ -43220,10 +43369,56 @@ inline bool is_user_alloc_gen (int gen_number) void gc_heap::verify_regions (bool can_verify_gen_num, bool concurrent_p) { #ifdef USE_REGIONS + size_t total_committed = 0; for (int i = 0; i < total_generation_count; i++) { bool can_verify_tail = (concurrent_p ? !is_user_alloc_gen (i) : true); - verify_regions (i, can_verify_gen_num, can_verify_tail); + verify_regions (i, can_verify_gen_num, can_verify_tail, total_committed); + + if (can_verify_gen_num && + can_verify_tail && + (i >= max_generation) && + (heap_hard_limit_oh[soh] > 0)) + { + int oh = i - max_generation; + if (oh == soh) + { + heap_segment* freeable = freeable_soh_segment; + while (freeable) + { + total_committed += (heap_segment_committed (freeable) - get_region_start (freeable)); + freeable = heap_segment_next (freeable); + } + } + else + { + heap_segment* freeable = freeable_uoh_segment; + while (freeable) + { + if (heap_segment_oh (freeable) == oh) + { + total_committed += (heap_segment_committed (freeable) - get_region_start (freeable)); + } + freeable = heap_segment_next (freeable); + } + } +#ifdef MULTIPLE_HEAPS +#ifdef _DEBUG + size_t total_accounted = committed_by_oh_per_heap[i - max_generation]; +#else + size_t total_accounted = total_committed; +#endif +#else + size_t total_accounted = committed_by_oh[i - max_generation]; +#endif + if (total_committed != total_accounted) + { + FATAL_GC_ERROR(); + } + dprintf(3, ("commit-accounting: checkpoint for %d for heap %d", oh, heap_number)); + total_committed = 0; + } + } #endif //USE_REGIONS } diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index c146f31cbfd3c..ce054616ede49 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -419,8 +419,10 @@ enum gc_oh_num soh = 0, loh = 1, poh = 2, - none = 3, - total_oh_count = 4 + free = 3, + none = 4, + total_oh_count = 3, + total_bucket_count = 5 }; gc_oh_num gen_to_oh (int gen); @@ -1296,7 +1298,7 @@ class gc_heap PER_HEAP void verify_free_lists(); PER_HEAP - void verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail); + void verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t& total_committed); PER_HEAP void verify_regions (bool can_verify_gen_num, bool concurrent_p); PER_HEAP_ISOLATED @@ -3803,7 +3805,7 @@ class gc_heap gen_to_condemn_tuning gen_to_condemn_reasons; PER_HEAP - size_t etw_allocation_running_amount[gc_oh_num::total_oh_count - 1]; + size_t etw_allocation_running_amount[gc_oh_num::total_oh_count]; PER_HEAP uint64_t total_alloc_bytes_soh; @@ -4007,7 +4009,7 @@ class gc_heap size_t heap_hard_limit; PER_HEAP_ISOLATED - size_t heap_hard_limit_oh[total_oh_count - 1]; + size_t heap_hard_limit_oh[gc_oh_num::total_oh_count]; PER_HEAP_ISOLATED CLRCriticalSection check_commit_cs; @@ -4016,7 +4018,14 @@ class gc_heap size_t current_total_committed; PER_HEAP_ISOLATED - size_t committed_by_oh[total_oh_count]; + size_t committed_by_oh[gc_oh_num::total_bucket_count]; + +#ifdef _DEBUG +#ifdef MULTIPLE_HEAPS + PER_HEAP + size_t committed_by_oh_per_heap[gc_oh_num::total_oh_count]; +#endif +#endif // This is what GC uses for its own bookkeeping. PER_HEAP_ISOLATED @@ -4818,7 +4827,7 @@ class gc_heap size_t num_provisional_triggered; PER_HEAP - size_t allocated_since_last_gc[gc_oh_num::total_oh_count - 1]; + size_t allocated_since_last_gc[gc_oh_num::total_oh_count]; #ifdef BACKGROUND_GC PER_HEAP_ISOLATED From 0dd7c148035c857fa1d385897a88a22138d17d81 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Tue, 26 Jul 2022 14:20:57 -0700 Subject: [PATCH 2/4] Fix per heap testing --- src/coreclr/gc/gc.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 72e0133ba5eee..77127955adcf5 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -29956,7 +29956,7 @@ void gc_heap::plan_phase (int condemned_gen_number) if (oh == soh) { - heap_segment* freeable = freeable_soh_segment; + heap_segment* freeable = hp->freeable_soh_segment; while (freeable) { committed += (heap_segment_committed (freeable) - get_region_start (freeable)); @@ -29965,7 +29965,7 @@ void gc_heap::plan_phase (int condemned_gen_number) } else { - heap_segment* freeable = freeable_uoh_segment; + heap_segment* freeable = hp->freeable_uoh_segment; while (freeable) { if (heap_segment_oh (freeable) == oh) From cebb65f13e1bdf5bc1bd3b4ccc0af4edca5644f6 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Wed, 27 Jul 2022 13:48:03 -0700 Subject: [PATCH 3/4] Code Review Feedback --- src/coreclr/gc/gc.cpp | 120 ++++++++++++++++++++-------------------- src/coreclr/gc/gcpriv.h | 32 +++++------ 2 files changed, 77 insertions(+), 75 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 77127955adcf5..8f96ca0d80f83 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -241,6 +241,7 @@ gc_oh_num gen_to_oh(int gen) case poh_generation: return gc_oh_num::poh; default: + assert(false); return gc_oh_num::none; } } @@ -2198,7 +2199,7 @@ CLRCriticalSection gc_heap::check_commit_cs; size_t gc_heap::current_total_committed = 0; -size_t gc_heap::committed_by_oh[gc_oh_num::total_bucket_count] = {0, 0, 0, 0, 0}; +size_t gc_heap::committed_by_oh[recorded_committed_bucket_counts]; size_t gc_heap::current_total_committed_bookkeeping = 0; @@ -2275,7 +2276,7 @@ uint64_t gc_heap::entry_available_physical_mem = 0; size_t gc_heap::heap_hard_limit = 0; -size_t gc_heap::heap_hard_limit_oh[gc_oh_num::total_oh_count] = {0, 0, 0}; +size_t gc_heap::heap_hard_limit_oh[total_oh_count]; #ifdef USE_REGIONS size_t gc_heap::regions_range = 0; @@ -2370,7 +2371,7 @@ oom_history gc_heap::oomhist_per_heap[max_oom_history_count]; fgm_history gc_heap::fgm_result; -size_t gc_heap::allocated_since_last_gc[gc_oh_num::total_oh_count]; +size_t gc_heap::allocated_since_last_gc[total_oh_count]; BOOL gc_heap::ro_segments_in_range; @@ -2412,7 +2413,7 @@ uint8_t* gc_heap::last_gen1_pin_end; gen_to_condemn_tuning gc_heap::gen_to_condemn_reasons; -size_t gc_heap::etw_allocation_running_amount[gc_oh_num::total_oh_count]; +size_t gc_heap::etw_allocation_running_amount[total_oh_count]; uint64_t gc_heap::total_alloc_bytes_soh = 0; @@ -5627,7 +5628,7 @@ gc_heap::soh_get_segment_to_expand() heap_segment* gc_heap::get_segment (size_t size, gc_oh_num oh) { - assert(oh != gc_oh_num::none); + assert(oh < total_oh_count); BOOL uoh_p = (oh == gc_oh_num::loh) || (oh == gc_oh_num::poh); if (heap_hard_limit) return NULL; @@ -6809,22 +6810,26 @@ bool gc_heap::virtual_alloc_commit_for_heap (void* addr, size_t size, int h_numb return GCToOSInterface::VirtualCommit(addr, size); } -bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_number, bool* hard_limit_exceeded_p) +bool gc_heap::virtual_commit (void* address, size_t size, int bucket, int h_number, bool* hard_limit_exceeded_p) { #ifndef HOST_64BIT assert (heap_hard_limit == 0); #endif //!HOST_64BIT - dprintf(3, ("commit-accounting: commit in %d [%Ix, %Ix) for heap %d", oh, address, ((uint8_t*)address + size), h_number)); + assert(0 <= bucket && bucket < recorded_committed_bucket_counts); + assert(bucket < total_oh_count || h_number == -1); + assert(bucket != recorded_committed_free_bucket); + + dprintf(3, ("commit-accounting: commit in %d [%Ix, %Ix) for heap %d", bucket, address, ((uint8_t*)address + size), h_number)); if (heap_hard_limit) { check_commit_cs.Enter(); bool exceeded_p = false; - if (heap_hard_limit_oh[oh] != 0) + if (heap_hard_limit_oh[bucket] != 0) { - if ((oh != gc_oh_num::none) && (committed_by_oh[oh] + size) > heap_hard_limit_oh[oh]) + if ((bucket < total_oh_count) && (committed_by_oh[bucket] + size) > heap_hard_limit_oh[bucket]) { exceeded_p = true; } @@ -6842,13 +6847,12 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu if (!exceeded_p) { #if defined(_DEBUG) && defined(MULTIPLE_HEAPS) - if (oh < gc_oh_num::free) + if (bucket < total_oh_count) { - assert (h_number != -1); - g_heaps[h_number]->committed_by_oh_per_heap[oh] += size; + g_heaps[h_number]->committed_by_oh_per_heap[bucket] += size; } #endif // _DEBUG && MULTIPLE_HEAPS - committed_by_oh[oh] += size; + committed_by_oh[bucket] += size; current_total_committed += size; if (h_number < 0) current_total_committed_bookkeeping += size; @@ -6875,13 +6879,12 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu if (!commit_succeeded_p && heap_hard_limit) { check_commit_cs.Enter(); - committed_by_oh[oh] -= size; + committed_by_oh[bucket] -= size; #if defined(_DEBUG) && defined(MULTIPLE_HEAPS) - if (oh < gc_oh_num::free) + if (bucket < total_oh_count) { - assert (h_number != -1); - assert (g_heaps[h_number]->committed_by_oh_per_heap[oh] >= size); - g_heaps[h_number]->committed_by_oh_per_heap[oh] -= size; + assert (g_heaps[h_number]->committed_by_oh_per_heap[bucket] >= size); + g_heaps[h_number]->committed_by_oh_per_heap[bucket] -= size; } #endif // _DEBUG && MULTIPLE_HEAPS dprintf (1, ("commit failed, updating %Id to %Id", @@ -6895,27 +6898,29 @@ bool gc_heap::virtual_commit (void* address, size_t size, gc_oh_num oh, int h_nu return commit_succeeded_p; } -bool gc_heap::virtual_decommit (void* address, size_t size, gc_oh_num oh, int h_number) +bool gc_heap::virtual_decommit (void* address, size_t size, int bucket, int h_number) { #ifndef HOST_64BIT assert (heap_hard_limit == 0); #endif //!HOST_64BIT + assert(0 <= bucket && bucket < recorded_committed_bucket_counts); + assert(bucket < total_oh_count || h_number == -1); + bool decommit_succeeded_p = GCToOSInterface::VirtualDecommit (address, size); - dprintf(3, ("commit-accounting: decommit in %d [%Ix, %Ix) for heap %d", oh, address, ((uint8_t*)address + size), h_number)); + dprintf(3, ("commit-accounting: decommit in %d [%Ix, %Ix) for heap %d", bucket, address, ((uint8_t*)address + size), h_number)); if (decommit_succeeded_p && heap_hard_limit) { check_commit_cs.Enter(); - assert (committed_by_oh[oh] >= size); - committed_by_oh[oh] -= size; + assert (committed_by_oh[bucket] >= size); + committed_by_oh[bucket] -= size; #if defined(_DEBUG) && defined(MULTIPLE_HEAPS) - if (oh < gc_oh_num::free) + if (bucket < total_oh_count) { - assert (h_number != -1); - assert (g_heaps[h_number]->committed_by_oh_per_heap[oh] >= size); - g_heaps[h_number]->committed_by_oh_per_heap[oh] -= size; + assert (g_heaps[h_number]->committed_by_oh_per_heap[bucket] >= size); + g_heaps[h_number]->committed_by_oh_per_heap[bucket] -= size; } #endif // _DEBUG && MULTIPLE_HEAPS current_total_committed -= size; @@ -8747,7 +8752,7 @@ bool gc_heap::inplace_commit_card_table (uint8_t* from, uint8_t* to) bool succeed; if (commit_sizes[i] > 0) { - succeed = virtual_commit (commit_begins[i], commit_sizes[i], gc_oh_num::none); + succeed = virtual_commit (commit_begins[i], commit_sizes[i], recorded_committed_bookkeeping_bucket); if (!succeed) { failed_commit = i; @@ -8769,7 +8774,7 @@ bool gc_heap::inplace_commit_card_table (uint8_t* from, uint8_t* to) bool succeed; if (commit_sizes[i] > 0) { - succeed = virtual_decommit (commit_begins[i], commit_sizes[i], gc_oh_num::none); + succeed = virtual_decommit (commit_begins[i], commit_sizes[i], recorded_committed_bookkeeping_bucket); assert (succeed); } } @@ -8822,7 +8827,7 @@ uint32_t* gc_heap::make_card_table (uint8_t* start, uint8_t* end) // in case of background gc, the mark array will be committed separately (per segment). size_t commit_size = card_table_element_layout[seg_mapping_table_element + 1]; - if (!virtual_commit (mem, commit_size, gc_oh_num::none)) + if (!virtual_commit (mem, commit_size, recorded_committed_bookkeeping_bucket)) { dprintf (1, ("Card table commit failed")); GCToOSInterface::VirtualRelease (mem, alloc_size); @@ -8991,7 +8996,7 @@ int gc_heap::grow_brick_card_tables (uint8_t* start, // in case of background gc, the mark array will be committed separately (per segment). size_t commit_size = card_table_element_layout[seg_mapping_table_element + 1]; - if (!virtual_commit (mem, commit_size, gc_oh_num::none)) + if (!virtual_commit (mem, commit_size, recorded_committed_bookkeeping_bucket)) { dprintf (GC_TABLE_LOG, ("Table commit failed")); set_fgm_result (fgm_commit_table, commit_size, uoh_p); @@ -11154,7 +11159,7 @@ void gc_heap::return_free_region (heap_segment* region) check_commit_cs.Enter(); assert (committed_by_oh[oh] >= committed); committed_by_oh[oh] -= committed; - committed_by_oh[free] += committed; + committed_by_oh[recorded_committed_free_bucket] += committed; #if defined(_DEBUG) && defined(MULTIPLE_HEAPS) assert (committed_by_oh_per_heap[oh] >= committed); committed_by_oh_per_heap[oh] -= committed; @@ -11239,8 +11244,8 @@ heap_segment* gc_heap::get_free_region (int gen_number, size_t size) { check_commit_cs.Enter(); committed_by_oh[oh] += committed; - assert (committed_by_oh[free] >= committed); - committed_by_oh[free] -= committed; + assert (committed_by_oh[recorded_committed_free_bucket] >= committed); + committed_by_oh[recorded_committed_free_bucket] -= committed; #if defined(_DEBUG) && defined(MULTIPLE_HEAPS) committed_by_oh_per_heap[oh] += committed; #endif // _DEBUG && MULTIPLE_HEAPS @@ -13814,19 +13819,13 @@ gc_heap::init_gc_heap (int h_number) { #ifdef MULTIPLE_HEAPS #ifdef _DEBUG - for (int i = 0; i < gc_oh_num::total_oh_count; i++) - { - committed_by_oh_per_heap[i] = 0; - } -#endif + memset (committed_by_oh_per_heap, 0, sizeof (committed_by_oh_per_heap)); +#endif g_heaps [h_number] = this; time_bgc_last = 0; - for (int oh_index = 0; oh_index < gc_oh_num::total_oh_count; oh_index++) - allocated_since_last_gc[oh_index] = 0; - #ifdef SPINLOCK_HISTORY spinlock_info_index = 0; memset (last_spinlock_info, 0, sizeof(last_spinlock_info)); @@ -13922,6 +13921,8 @@ gc_heap::init_gc_heap (int h_number) heap_number = h_number; #endif //MULTIPLE_HEAPS + memset (etw_allocation_running_amount, 0, sizeof (etw_allocation_running_amount)); + memset (allocated_since_last_gc, 0, sizeof (allocated_since_last_gc)); memset (&oom_info, 0, sizeof (oom_info)); memset (&fgm_result, 0, sizeof (fgm_result)); memset (oomhist_per_heap, 0, sizeof (oomhist_per_heap)); @@ -14074,9 +14075,6 @@ gc_heap::init_gc_heap (int h_number) generation_of (loh_generation)->free_list_allocator = allocator(NUM_LOH_ALIST, BASE_LOH_ALIST_BITS, loh_alloc_list); generation_of (poh_generation)->free_list_allocator = allocator(NUM_POH_ALIST, BASE_POH_ALIST_BITS, poh_alloc_list); - for (int oh_index = 0; oh_index < gc_oh_num::total_oh_count; oh_index++) - etw_allocation_running_amount[oh_index] = 0; - total_alloc_bytes_soh = 0; total_alloc_bytes_uoh = 0; @@ -22686,11 +22684,11 @@ heap_segment* gc_heap::unlink_first_rw_region (int gen_idx) assert (!heap_segment_read_only_p (region)); dprintf (REGIONS_LOG, ("unlink_first_rw_region on heap: %d gen: %d region: %Ix", heap_number, gen_idx, heap_segment_mem (region))); +#ifdef _DEBUG int old_oh = heap_segment_oh (region); int old_heap = heap_segment_heap (region)->heap_number; dprintf(3, ("commit-accounting: from %d to temp [%Ix, %Ix) for heap %d", old_oh, get_region_start (region), heap_segment_committed (region), old_heap)); -#ifdef _DEBUG size_t committed = heap_segment_committed (region) - get_region_start (region); check_commit_cs.Enter(); assert (g_heaps[old_heap]->committed_by_oh_per_heap[old_oh] >= committed); @@ -22720,11 +22718,11 @@ void gc_heap::thread_rw_region_front (int gen_idx, heap_segment* region) } dprintf (REGIONS_LOG, ("thread_rw_region_front on heap: %d gen: %d region: %Ix", heap_number, gen_idx, heap_segment_mem (region))); +#ifdef _DEBUG int new_oh = gen_to_oh (gen_idx); int new_heap = this->heap_number; dprintf(3, ("commit-accounting: from temp to %d [%Ix, %Ix) for heap %d", new_oh, get_region_start (region), heap_segment_committed (region), new_heap)); -#ifdef _DEBUG size_t committed = heap_segment_committed (region) - get_region_start (region); check_commit_cs.Enter(); assert (heap_segment_heap (region) == nullptr); @@ -29945,7 +29943,7 @@ void gc_heap::plan_phase (int condemned_gen_number) gc_heap* hp = gc_heap::g_heaps[hn]; #else { - gc_heap* hp = nullptr; + gc_heap* hp = pGenGCHeap; #endif // MULTIPLE_HEAPS heap_segment* region = generation_start_segment (hp->generation_of (i)); while (region) @@ -30705,8 +30703,7 @@ heap_segment* gc_heap::get_new_region (int gen_number, size_t size) heap_segment_next (generation_tail_region (gen)) = new_region; generation_tail_region (gen) = new_region; - size_t unused_total_committed; - verify_regions (gen_number, false, settings.concurrent, unused_total_committed); + verify_regions (gen_number, false, settings.concurrent); } return new_region; @@ -33708,7 +33705,7 @@ BOOL gc_heap::commit_mark_array_by_range (uint8_t* begin, uint8_t* end, uint32_t size)); #endif //SIMPLE_DPRINTF - if (virtual_commit (commit_start, size, gc_oh_num::none)) + if (virtual_commit (commit_start, size, recorded_committed_bookkeeping_bucket)) { // We can only verify the mark array is cleared from begin to end, the first and the last // page aren't necessarily all cleared 'cause they could be used by other segments or @@ -33933,7 +33930,7 @@ void gc_heap::decommit_mark_array_by_seg (heap_segment* seg) if (decommit_start < decommit_end) { - if (!virtual_decommit (decommit_start, size, gc_oh_num::none)) + if (!virtual_decommit (decommit_start, size, recorded_committed_bookkeeping_bucket)) { dprintf (GC_TABLE_LOG, ("decommit on %Ix for %Id bytes failed", decommit_start, size)); @@ -40043,7 +40040,7 @@ bool gc_heap::decommit_step () bool decommit_succeeded_p = false; if (!use_large_pages_p) { - decommit_succeeded_p = virtual_decommit(page_start, size, gc_oh_num::free, -1); + 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, @@ -43286,7 +43283,7 @@ gc_heap::verify_free_lists () } } -void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t& total_committed) +void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t* p_total_committed) { #ifdef USE_REGIONS // For the given generation, verify that @@ -43305,7 +43302,10 @@ void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_ while (seg_in_gen) { - total_committed += (heap_segment_committed (seg_in_gen) - get_region_start (seg_in_gen)); + if (p_total_committed) + { + *p_total_committed += (heap_segment_committed (seg_in_gen) - get_region_start (seg_in_gen)); + } if (can_verify_gen_num) { if (heap_segment_gen_num (seg_in_gen) != min (gen_number, max_generation)) @@ -43373,7 +43373,7 @@ void gc_heap::verify_regions (bool can_verify_gen_num, bool concurrent_p) for (int i = 0; i < total_generation_count; i++) { bool can_verify_tail = (concurrent_p ? !is_user_alloc_gen (i) : true); - verify_regions (i, can_verify_gen_num, can_verify_tail, total_committed); + verify_regions (i, can_verify_gen_num, can_verify_tail, &total_committed); if (can_verify_gen_num && can_verify_tail && @@ -43405,12 +43405,12 @@ void gc_heap::verify_regions (bool can_verify_gen_num, bool concurrent_p) #ifdef MULTIPLE_HEAPS #ifdef _DEBUG size_t total_accounted = committed_by_oh_per_heap[i - max_generation]; -#else +#else // _DEBUG size_t total_accounted = total_committed; -#endif -#else +#endif // _DEBUG +#else // MULTIPLE_HEAPS size_t total_accounted = committed_by_oh[i - max_generation]; -#endif +#endif // MULTIPLE_HEAPS if (total_committed != total_accounted) { FATAL_GC_ERROR(); @@ -44051,6 +44051,8 @@ HRESULT GCHeap::Initialize() { HRESULT hr = S_OK; + memset (gc_heap::committed_by_oh, 0, sizeof (gc_heap::committed_by_oh)); + qpf = (uint64_t)GCToOSInterface::QueryPerformanceFrequency(); qpf_ms = 1000.0 / (double)qpf; qpf_us = 1000.0 * 1000.0 / (double)qpf; diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index ce054616ede49..3ce9b71dbe22c 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -419,12 +419,14 @@ enum gc_oh_num soh = 0, loh = 1, poh = 2, - free = 3, - none = 4, - total_oh_count = 3, - total_bucket_count = 5 + none = -1, }; +const int total_oh_count = gc_oh_num::poh + 1; +const int recorded_committed_free_bucket = total_oh_count; +const int recorded_committed_bookkeeping_bucket = recorded_committed_free_bucket + 1; +const int recorded_committed_bucket_counts = recorded_committed_bookkeeping_bucket + 1; + gc_oh_num gen_to_oh (int gen); #if defined(TRACE_GC) && defined(BACKGROUND_GC) @@ -1298,7 +1300,7 @@ class gc_heap PER_HEAP void verify_free_lists(); PER_HEAP - void verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t& total_committed); + void verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail, size_t* p_total_committed = nullptr); PER_HEAP void verify_regions (bool can_verify_gen_num, bool concurrent_p); PER_HEAP_ISOLATED @@ -2035,9 +2037,9 @@ class gc_heap PER_HEAP_ISOLATED bool virtual_alloc_commit_for_heap (void* addr, size_t size, int h_number); PER_HEAP_ISOLATED - bool virtual_commit (void* address, size_t size, gc_oh_num oh, int h_number=-1, bool* hard_limit_exceeded_p=NULL); + bool virtual_commit (void* address, size_t size, int bucket, int h_number=-1, bool* hard_limit_exceeded_p=NULL); PER_HEAP_ISOLATED - bool virtual_decommit (void* address, size_t size, gc_oh_num oh, int h_number=-1); + bool virtual_decommit (void* address, size_t size, int bucket, int h_number=-1); PER_HEAP_ISOLATED void virtual_free (void* add, size_t size, heap_segment* sg=NULL); PER_HEAP @@ -3805,7 +3807,7 @@ class gc_heap gen_to_condemn_tuning gen_to_condemn_reasons; PER_HEAP - size_t etw_allocation_running_amount[gc_oh_num::total_oh_count]; + size_t etw_allocation_running_amount[total_oh_count]; PER_HEAP uint64_t total_alloc_bytes_soh; @@ -4009,7 +4011,7 @@ class gc_heap size_t heap_hard_limit; PER_HEAP_ISOLATED - size_t heap_hard_limit_oh[gc_oh_num::total_oh_count]; + size_t heap_hard_limit_oh[total_oh_count]; PER_HEAP_ISOLATED CLRCriticalSection check_commit_cs; @@ -4018,14 +4020,12 @@ class gc_heap size_t current_total_committed; PER_HEAP_ISOLATED - size_t committed_by_oh[gc_oh_num::total_bucket_count]; + size_t committed_by_oh[recorded_committed_bucket_counts]; -#ifdef _DEBUG -#ifdef MULTIPLE_HEAPS +#if defined (_DEBUG) && defined (MULTIPLE_HEAPS) PER_HEAP - size_t committed_by_oh_per_heap[gc_oh_num::total_oh_count]; -#endif -#endif + size_t committed_by_oh_per_heap[total_oh_count]; +#endif // _DEBUG && MULTIPLE_HEAPS // This is what GC uses for its own bookkeeping. PER_HEAP_ISOLATED @@ -4827,7 +4827,7 @@ class gc_heap size_t num_provisional_triggered; PER_HEAP - size_t allocated_since_last_gc[gc_oh_num::total_oh_count]; + size_t allocated_since_last_gc[total_oh_count]; #ifdef BACKGROUND_GC PER_HEAP_ISOLATED From 7f40a1be0d172fa29450c85a5cba33a96ff821c9 Mon Sep 17 00:00:00 2001 From: Andrew Au Date: Thu, 28 Jul 2022 17:11:17 -0700 Subject: [PATCH 4/4] More Code Review Feedback --- src/coreclr/gc/gc.cpp | 8 ++++---- src/coreclr/gc/gcpriv.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 8f96ca0d80f83..25301254e2826 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -242,7 +242,7 @@ gc_oh_num gen_to_oh(int gen) return gc_oh_num::poh; default: assert(false); - return gc_oh_num::none; + return gc_oh_num::unknown; } } @@ -5628,7 +5628,7 @@ gc_heap::soh_get_segment_to_expand() heap_segment* gc_heap::get_segment (size_t size, gc_oh_num oh) { - assert(oh < total_oh_count); + assert(oh != gc_oh_num::unknown); BOOL uoh_p = (oh == gc_oh_num::loh) || (oh == gc_oh_num::poh); if (heap_hard_limit) return NULL; @@ -44051,8 +44051,6 @@ HRESULT GCHeap::Initialize() { HRESULT hr = S_OK; - memset (gc_heap::committed_by_oh, 0, sizeof (gc_heap::committed_by_oh)); - qpf = (uint64_t)GCToOSInterface::QueryPerformanceFrequency(); qpf_ms = 1000.0 / (double)qpf; qpf_us = 1000.0 * 1000.0 / (double)qpf; @@ -44079,6 +44077,8 @@ HRESULT GCHeap::Initialize() gc_heap::heap_hard_limit_oh[loh] = (size_t)GCConfig::GetGCHeapHardLimitLOH(); gc_heap::heap_hard_limit_oh[poh] = (size_t)GCConfig::GetGCHeapHardLimitPOH(); + memset (gc_heap::committed_by_oh, 0, sizeof (gc_heap::committed_by_oh)); + gc_heap::use_large_pages_p = GCConfig::GetGCLargePages(); if (gc_heap::heap_hard_limit_oh[soh] || gc_heap::heap_hard_limit_oh[loh] || gc_heap::heap_hard_limit_oh[poh]) diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index 3ce9b71dbe22c..54d909a526079 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -419,7 +419,7 @@ enum gc_oh_num soh = 0, loh = 1, poh = 2, - none = -1, + unknown = -1, }; const int total_oh_count = gc_oh_num::poh + 1;