From 821246ac4e784051304b6a66016e014eac7031b6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 27 Sep 2022 17:33:08 +0200 Subject: [PATCH 01/16] CheckPromoted used to mark weakref to a frozen object in SyncBlock as unreachable because it was not promoted --- src/coreclr/gc/gcscan.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/gcscan.cpp b/src/coreclr/gc/gcscan.cpp index 69f81dda6e9dd2..4f7ef991cc50a1 100644 --- a/src/coreclr/gc/gcscan.cpp +++ b/src/coreclr/gc/gcscan.cpp @@ -113,7 +113,7 @@ static void CALLBACK CheckPromoted(_UNCHECKED_OBJECTREF *pObjRef, uintptr_t * /* LOG((LF_GC, LL_INFO100000, LOG_HANDLE_OBJECT_CLASS("Checking referent of Weak-", pObjRef, "to ", *pObjRef))); Object **pRef = (Object **)pObjRef; - if (!g_theGCHeap->IsPromoted(*pRef)) + if (!g_theGCHeap->IsPromoted(*pRef) && !g_theGCHeap->IsInFrozenSegment(*pRef)) { LOG((LF_GC, LL_INFO100, LOG_HANDLE_OBJECT_CLASS("Severing Weak-", pObjRef, "to unreachable ", *pObjRef))); From 77d91a7e472033a2f433016dafafd7181a918552 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 27 Sep 2022 18:42:19 +0200 Subject: [PATCH 02/16] Move check to gc.cpp --- src/coreclr/gc/gc.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 93c6715b565478..96ba25004371d0 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -45164,6 +45164,13 @@ HRESULT GCHeap::Initialize() // GC callback functions bool GCHeap::IsPromoted(Object* object) { +#ifdef FEATURE_BASICFREEZE + if (gc_heap::frozen_object_p (object)) + { + return true; + } +#endif + uint8_t* o = (uint8_t*)object; bool is_marked; From 7d88147a323658f022a636d9dcffbbc7a239f359 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 27 Sep 2022 18:42:45 +0200 Subject: [PATCH 03/16] clean up --- src/coreclr/gc/gcscan.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/gcscan.cpp b/src/coreclr/gc/gcscan.cpp index 4f7ef991cc50a1..69f81dda6e9dd2 100644 --- a/src/coreclr/gc/gcscan.cpp +++ b/src/coreclr/gc/gcscan.cpp @@ -113,7 +113,7 @@ static void CALLBACK CheckPromoted(_UNCHECKED_OBJECTREF *pObjRef, uintptr_t * /* LOG((LF_GC, LL_INFO100000, LOG_HANDLE_OBJECT_CLASS("Checking referent of Weak-", pObjRef, "to ", *pObjRef))); Object **pRef = (Object **)pObjRef; - if (!g_theGCHeap->IsPromoted(*pRef) && !g_theGCHeap->IsInFrozenSegment(*pRef)) + if (!g_theGCHeap->IsPromoted(*pRef)) { LOG((LF_GC, LL_INFO100, LOG_HANDLE_OBJECT_CLASS("Severing Weak-", pObjRef, "to unreachable ", *pObjRef))); From d3594904aa07077d2e1fa71db0bdaeb91cb21416 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 27 Sep 2022 19:33:57 +0200 Subject: [PATCH 04/16] Use safer IsInFrozenSegment --- src/coreclr/gc/gc.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 96ba25004371d0..21a83d8f2b92c3 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -45164,12 +45164,10 @@ HRESULT GCHeap::Initialize() // GC callback functions bool GCHeap::IsPromoted(Object* object) { -#ifdef FEATURE_BASICFREEZE - if (gc_heap::frozen_object_p (object)) + if (IsInFrozenSegment (object)) { return true; } -#endif uint8_t* o = (uint8_t*)object; From 86dc10cdd32a9423ba0807b4c4ee6f323bba1d6a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 27 Sep 2022 20:32:24 +0200 Subject: [PATCH 05/16] IsInFrozenSegment returns true for nullptr and nullptr is possible here it seems --- src/coreclr/gc/gc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 21a83d8f2b92c3..3e8977d4a5126c 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -45164,7 +45164,7 @@ HRESULT GCHeap::Initialize() // GC callback functions bool GCHeap::IsPromoted(Object* object) { - if (IsInFrozenSegment (object)) + if (object != nullptr && IsInFrozenSegment (object)) { return true; } From 555e1dd26495f6151f70420c2167699bdca3a0d0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 28 Sep 2022 15:41:52 +0200 Subject: [PATCH 06/16] Initial impl of "mark all ro segments" --- src/coreclr/gc/gc.cpp | 83 ++++++++++++++++++++++++++++++++++++++--- src/coreclr/gc/gcpriv.h | 6 +++ 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 3e8977d4a5126c..7eb6bc3f5ca1a9 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -11013,6 +11013,18 @@ void gc_heap::seg_clear_mark_array_bits_soh (heap_segment* seg) } } +inline +void gc_heap::seg_mark_array_bits_soh (heap_segment* seg) +{ + uint8_t* range_beg = 0; + uint8_t* range_end = 0; + if (bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) + { + // TODO: + // mark_array_set_marked (range_beg) + } +} + void gc_heap::bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) { if ((start < background_saved_highest_address) && @@ -26160,6 +26172,19 @@ void gc_heap::mark_phase (int condemned_gen_number, BOOL mark_only_p) gen_to_init = total_generation_count - 1; } +#ifdef FEATURE_BASICFREEZE +#ifdef USE_REGIONS + assert (!ro_segments_in_range); +#else // USE_REGIONS + if ((generation_start_segment (generation_of (condemned_gen_number)) != ephemeral_heap_segment) && + ro_segments_in_range) + { + generation* max_gen = generation_of (max_generation); + mark_ro_segments (generation_start_segment (max_gen)); + } +#endif // !USE_REGIONS +#endif // !FEATURE_BASICFREEZE + for (int gen_idx = 0; gen_idx <= gen_to_init; gen_idx++) { dynamic_data* dd = dynamic_data_of (gen_idx); @@ -27433,6 +27458,17 @@ void gc_heap::process_ephemeral_boundaries (uint8_t* x, } #endif //!USE_REGIONS +inline +void gc_heap::seg_set_mark_bits (heap_segment* seg) +{ + uint8_t* o = heap_segment_mem (seg); + while (o < heap_segment_allocated (seg)) + { + set_marked (o); + o = o + Align (size(o)); + } +} + inline void gc_heap::seg_clear_mark_bits (heap_segment* seg) { @@ -27443,11 +27479,38 @@ void gc_heap::seg_clear_mark_bits (heap_segment* seg) { clear_marked (o); } - o = o + Align (size (o)); + o = o + Align (size (o)); } } #ifdef FEATURE_BASICFREEZE +void gc_heap::mark_ro_segments (heap_segment* start_seg) +{ + //go through all of the segment in range and set the mark bit + heap_segment* seg = start_seg; + + while (seg) + { + if (heap_segment_read_only_p (seg) && + heap_segment_in_range_p (seg)) + { +#ifdef BACKGROUND_GC + if (settings.concurrent) + { + // seg_mark_array_bits_soh (seg); + } + else + { + seg_set_mark_bits (seg); + } +#else //BACKGROUND_GC + seg_set_mark_bits (seg); +#endif //BACKGROUND_GC + } + seg = heap_segment_next (seg); + } +} + void gc_heap::sweep_ro_segments (heap_segment* start_seg) { //go through all of the segment in range and reset the mark bit @@ -34582,6 +34645,13 @@ void gc_heap::background_mark_phase () dprintf(3,("BGC: stack marking")); sc.concurrent = TRUE; +#if defined(FEATURE_BASICFREEZE) && !defined(USE_REGIONS) + if ((generation_start_segment(gen) != ephemeral_heap_segment) && ro_segments_in_range) + { + mark_ro_segments(generation_start_segment(gen)); + } +#endif // FEATURE_BASICFREEZE + GCScan::GcScanRoots(background_promote_callback, max_generation, max_generation, &sc); @@ -45164,11 +45234,6 @@ HRESULT GCHeap::Initialize() // GC callback functions bool GCHeap::IsPromoted(Object* object) { - if (object != nullptr && IsInFrozenSegment (object)) - { - return true; - } - uint8_t* o = (uint8_t*)object; bool is_marked; @@ -45210,6 +45275,12 @@ bool GCHeap::IsPromoted(Object* object) { ((CObjectHeader*)o)->Validate(TRUE, TRUE, is_marked); } + + if (!is_marked && object != nullptr && IsInFrozenSegment (object)) + { + // Frozen objects aren't expected to be "not promoted" here + __UNREACHABLE(); + } #endif //_DEBUG return is_marked; diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index b6c12ccd0ced7a..f5cbc455bbad36 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2209,6 +2209,8 @@ class gc_heap PER_HEAP void seg_clear_mark_array_bits_soh (heap_segment* seg); PER_HEAP + void seg_mark_array_bits_soh (heap_segment* seg); + PER_HEAP void bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end); #ifdef VERIFY_HEAP PER_HEAP @@ -2863,6 +2865,10 @@ class gc_heap PER_HEAP void seg_clear_mark_bits (heap_segment* seg); PER_HEAP + void seg_set_mark_bits (heap_segment* seg); + PER_HEAP + void mark_ro_segments (heap_segment* start_seg); + PER_HEAP void sweep_ro_segments (heap_segment* start_seg); PER_HEAP void convert_to_pinned_plug (BOOL& last_npinned_plug_p, From 40f58e06da04cc0e80b0e6f3386cf1b7d0790073 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 28 Sep 2022 15:59:37 +0200 Subject: [PATCH 07/16] Add test --- .../Github/Runtime_76219/Runtime_76219.cs | 44 +++++++++++++++++++ .../Github/Runtime_76219/Runtime_76219.csproj | 24 ++++++++++ 2 files changed, 68 insertions(+) create mode 100644 src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs create mode 100644 src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.csproj diff --git a/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs new file mode 100644 index 00000000000000..21376b4a3f4202 --- /dev/null +++ b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs @@ -0,0 +1,44 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +public class Runtime_76219 +{ + [MethodImpl(MethodImplOptions.Synchronized)] + public static int Main() + { + for (int i = 0; i < 100; i++) + { + string alloc = i.ToString(); + + Test(i); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + return 100; + } + + [MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)] + static void Test() + { + CallConsume("hello"); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CallConsume(string str) + { + lock (str) + { + Consume(str); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Consume(string str) + { + } +} diff --git a/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.csproj b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.csproj new file mode 100644 index 00000000000000..392859ac40db8e --- /dev/null +++ b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.csproj @@ -0,0 +1,24 @@ + + + Exe + True + + + + + + + + + From dc66006da3e13a349eae9985485f4e2abd039886 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 28 Sep 2022 16:03:48 +0200 Subject: [PATCH 08/16] clean up --- src/coreclr/gc/gc.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 7eb6bc3f5ca1a9..f64d571d2ea1d7 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -27497,7 +27497,7 @@ void gc_heap::mark_ro_segments (heap_segment* start_seg) #ifdef BACKGROUND_GC if (settings.concurrent) { - // seg_mark_array_bits_soh (seg); + seg_mark_array_bits_soh (seg); } else { @@ -34645,10 +34645,10 @@ void gc_heap::background_mark_phase () dprintf(3,("BGC: stack marking")); sc.concurrent = TRUE; -#if defined(FEATURE_BASICFREEZE) && !defined(USE_REGIONS) - if ((generation_start_segment(gen) != ephemeral_heap_segment) && ro_segments_in_range) +#if defined (FEATURE_BASICFREEZE) && !defined (USE_REGIONS) + if ((generation_start_segment (gen) != ephemeral_heap_segment) && ro_segments_in_range) { - mark_ro_segments(generation_start_segment(gen)); + mark_ro_segments (generation_start_segment (gen)); } #endif // FEATURE_BASICFREEZE From 42b56d29945efc5a21e8651be17562ebce32fda3 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 28 Sep 2022 16:29:29 +0200 Subject: [PATCH 09/16] Update Runtime_76219.cs --- src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs index 21376b4a3f4202..91ebca02e7d44b 100644 --- a/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs +++ b/src/tests/GC/Regressions/Github/Runtime_76219/Runtime_76219.cs @@ -13,7 +13,7 @@ public static int Main() { string alloc = i.ToString(); - Test(i); + Test(); GC.Collect(); GC.WaitForPendingFinalizers(); From 633e52a6121480051ca23ae162f35a86c1df9238 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 28 Sep 2022 20:15:43 +0200 Subject: [PATCH 10/16] is this a correct impl for background mark? --- src/coreclr/gc/gc.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index f64d571d2ea1d7..df67dbe29b657a 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -11018,10 +11018,21 @@ void gc_heap::seg_mark_array_bits_soh (heap_segment* seg) { uint8_t* range_beg = 0; uint8_t* range_end = 0; - if (bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) + if (gc_can_use_concurrent && bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) { - // TODO: - // mark_array_set_marked (range_beg) + assert(range_beg == align_on_mark_word(range_beg)); + if ((range_beg <= background_saved_highest_address) && (range_beg >= background_saved_lowest_address)) + { + size_t beg_word = mark_word_of (align_on_mark_word (range_beg)); + size_t end_word = mark_word_of (align_on_mark_word (range_beg)); + + uint8_t* op = range_beg; + while (op < mark_word_address (beg_word)) + { + mark_array_set_marked (op); + op += mark_bit_pitch; + } + } } } From 5de363ae08e93641d5448c95d2372f3b14995b6a Mon Sep 17 00:00:00 2001 From: Maoni0 Date: Sun, 2 Oct 2022 15:30:01 -0700 Subject: [PATCH 11/16] a bit of refactoring, added an explanation why we need mark_ro_segments --- src/coreclr/gc/gc.cpp | 76 +++++++++++++++++++---------------------- src/coreclr/gc/gcpriv.h | 25 ++++++++------ 2 files changed, 50 insertions(+), 51 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index df67dbe29b657a..988fde2811bdbe 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -8458,12 +8458,9 @@ uint32_t* translate_mark_array (uint32_t* ma) return (uint32_t*)((uint8_t*)ma - size_mark_array_of (0, g_gc_lowest_address)); } -// from and end must be page aligned addresses. -void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL check_only/*=TRUE*/ #ifdef FEATURE_BASICFREEZE - , BOOL read_only/*=FALSE*/ -#endif // FEATURE_BASICFREEZE - ) +// from and end must be page aligned addresses. +void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL read_only/*=FALSE*/) { if(!gc_can_use_concurrent) return; @@ -8487,22 +8484,20 @@ void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL check_only/*=T size_t beg_word = mark_word_of (align_on_mark_word (from)); //align end word to make sure to cover the address size_t end_word = mark_word_of (align_on_mark_word (end)); - dprintf (3, ("Calling clearing mark array [%Ix, %Ix[ for addresses [%Ix, %Ix[(%s)", + dprintf (3, ("Calling clearing mark array [%Ix, %Ix[ for addresses [%Ix, %Ix[", (size_t)mark_word_address (beg_word), (size_t)mark_word_address (end_word), - (size_t)from, (size_t)end, - (check_only ? "check_only" : "clear"))); - if (!check_only) - { - uint8_t* op = from; - while (op < mark_word_address (beg_word)) - { - mark_array_clear_marked (op); - op += mark_bit_pitch; - } + (size_t)from, (size_t)end)); - memset (&mark_array[beg_word], 0, (end_word - beg_word)*sizeof (uint32_t)); + uint8_t* op = from; + while (op < mark_word_address (beg_word)) + { + mark_array_clear_marked (op); + op += mark_bit_pitch; } + + memset (&mark_array[beg_word], 0, (end_word - beg_word)*sizeof (uint32_t)); + #ifdef _DEBUG else { @@ -8526,6 +8521,7 @@ void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL check_only/*=T #endif //_DEBUG } } +#endif // FEATURE_BASICFREEZE #endif //BACKGROUND_GC //These work on untranslated card tables @@ -9475,7 +9471,7 @@ BOOL gc_heap::insert_ro_segment (heap_segment* seg) return FALSE; } - //insert at the head of the segment list + //insert at the head of the max_generation segment list generation* gen2 = generation_of (max_generation); heap_segment* oldhead = generation_start_segment (gen2); heap_segment_next (seg) = oldhead; @@ -9513,13 +9509,12 @@ BOOL gc_heap::insert_ro_segment (heap_segment* seg) // which portion of the mark array was committed and only decommit that. void gc_heap::remove_ro_segment (heap_segment* seg) { -//clear the mark bits so a new segment allocated in its place will have a clear mark bits + //clear the mark bits so a new segment allocated in its place will have a clear mark bits #ifdef BACKGROUND_GC if (gc_can_use_concurrent) { clear_mark_array (align_lower_mark_word (max (heap_segment_mem (seg), lowest_address)), - align_on_card_word (min (heap_segment_allocated (seg), highest_address)), - false); // read_only segments need the mark clear + align_on_card_word (min (heap_segment_allocated (seg), highest_address))); } #endif //BACKGROUND_GC @@ -10998,6 +10993,7 @@ inline size_t my_get_size (Object* ob) #endif //COLLECTIBLE_CLASS #ifdef BACKGROUND_GC +#ifdef FEATURE_BASICFREEZE inline void gc_heap::seg_clear_mark_array_bits_soh (heap_segment* seg) { @@ -11005,16 +11001,12 @@ void gc_heap::seg_clear_mark_array_bits_soh (heap_segment* seg) uint8_t* range_end = 0; if (bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) { - clear_mark_array (range_beg, align_on_mark_word (range_end), FALSE -#ifdef FEATURE_BASICFREEZE - , TRUE -#endif // FEATURE_BASICFREEZE - ); + clear_mark_array (range_beg, align_on_mark_word (range_end), TRUE); } } inline -void gc_heap::seg_mark_array_bits_soh (heap_segment* seg) +void gc_heap::seg_set_mark_array_bits_soh (heap_segment* seg) { uint8_t* range_beg = 0; uint8_t* range_end = 0; @@ -11035,6 +11027,7 @@ void gc_heap::seg_mark_array_bits_soh (heap_segment* seg) } } } +#endif //FEATURE_BASICFREEZE void gc_heap::bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) { @@ -27495,28 +27488,31 @@ void gc_heap::seg_clear_mark_bits (heap_segment* seg) } #ifdef FEATURE_BASICFREEZE +// We have to do this for in range ro segments because these objects' life time isn't accurately +// expressed. The expectation is all objects on ro segs are live. So we just artifically mark +// all of them on the in range ro segs. void gc_heap::mark_ro_segments (heap_segment* start_seg) { - //go through all of the segment in range and set the mark bit + //go through all of the ro segment in range and set the mark bit heap_segment* seg = start_seg; while (seg) { - if (heap_segment_read_only_p (seg) && - heap_segment_in_range_p (seg)) + if (!heap_segment_read_only_p (seg)) + break; + + if (heap_segment_in_range_p (seg)) { #ifdef BACKGROUND_GC if (settings.concurrent) { - seg_mark_array_bits_soh (seg); + seg_set_mark_array_bits_soh (seg); } else +#endif //BACKGROUND_GC { seg_set_mark_bits (seg); } -#else //BACKGROUND_GC - seg_set_mark_bits (seg); -#endif //BACKGROUND_GC } seg = heap_segment_next (seg); } @@ -27524,13 +27520,15 @@ void gc_heap::mark_ro_segments (heap_segment* start_seg) void gc_heap::sweep_ro_segments (heap_segment* start_seg) { - //go through all of the segment in range and reset the mark bit + //go through all of the ro segment in range and reset the mark bit heap_segment* seg = start_seg; while (seg) { - if (heap_segment_read_only_p (seg) && - heap_segment_in_range_p (seg)) + if (!heap_segment_read_only_p (seg)) + break; + + if (heap_segment_in_range_p (seg)) { #ifdef BACKGROUND_GC if (settings.concurrent) @@ -27538,12 +27536,10 @@ void gc_heap::sweep_ro_segments (heap_segment* start_seg) seg_clear_mark_array_bits_soh (seg); } else +#endif //BACKGROUND_GC { seg_clear_mark_bits (seg); } -#else //BACKGROUND_GC - seg_clear_mark_bits (seg); -#endif //BACKGROUND_GC } seg = heap_segment_next (seg); } diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index f5cbc455bbad36..d46ae2cff491c1 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -52,8 +52,8 @@ inline void FATAL_GC_ERROR() // This means any empty regions can be freely used for any generation. For // Server GC we will balance regions between heaps. // For now disable regions for StandAlone GC, NativeAOT and MacOS builds -#if defined (HOST_64BIT) && !defined (BUILD_AS_STANDALONE) && !defined(__APPLE__) && !defined(FEATURE_NATIVEAOT) -#define USE_REGIONS +#if defined (HOST_64BIT) && defined (BUILD_AS_STANDALONE) && !defined(__APPLE__) && !defined(FEATURE_NATIVEAOT) +//#define USE_REGIONS #endif //HOST_64BIT && BUILD_AS_STANDALONE #ifdef USE_REGIONS @@ -2200,16 +2200,16 @@ class gc_heap BOOL mark_array_bit_set (size_t mark_bit); PER_HEAP void mark_array_clear_marked (uint8_t* add); - PER_HEAP - void clear_mark_array (uint8_t* from, uint8_t* end, BOOL check_only=TRUE + #ifdef FEATURE_BASICFREEZE - , BOOL read_only=FALSE -#endif // FEATURE_BASICFREEZE - ); PER_HEAP - void seg_clear_mark_array_bits_soh (heap_segment* seg); + void seg_set_mark_array_bits_soh (heap_segment* seg); + PER_HEAP + void clear_mark_array (uint8_t* from, uint8_t* end, BOOL read_only=FALSE); PER_HEAP - void seg_mark_array_bits_soh (heap_segment* seg); + void seg_clear_mark_array_bits_soh (heap_segment* seg); +#endif // FEATURE_BASICFREEZE + PER_HEAP void bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end); #ifdef VERIFY_HEAP @@ -2862,14 +2862,17 @@ class gc_heap BOOL& allocate_in_condemned); #endif //!USE_REGIONS - PER_HEAP - void seg_clear_mark_bits (heap_segment* seg); +#ifdef FEATURE_BASICFREEZE PER_HEAP void seg_set_mark_bits (heap_segment* seg); PER_HEAP + void seg_clear_mark_bits (heap_segment* seg); + PER_HEAP void mark_ro_segments (heap_segment* start_seg); PER_HEAP void sweep_ro_segments (heap_segment* start_seg); +#endif // FEATURE_BASICFREEZE + PER_HEAP void convert_to_pinned_plug (BOOL& last_npinned_plug_p, BOOL& last_pinned_plug_p, From d7caee348e4ae146d528220ff45f74a1d18cdabd Mon Sep 17 00:00:00 2001 From: Maoni0 Date: Sun, 2 Oct 2022 17:06:22 -0700 Subject: [PATCH 12/16] + actually setting the mark array bits for ro segments + moved where we are calling mark_ro_segments to the right places - for BGC this needs to be in the 2nd non concurrent phase) for blocking GCs I just moved it to a more appropriate place + for full blocking GCs it's not enough to do mark_ro_segments when gen start seg is not ephemeral. when we are doing a gen2 GC, even if we have acquired a new seg for that heap, we still need to mark all in range ro segs because our logic in IsPromoted expects when there's in range ro segs we need the mark bit to tell us these ro objects are marked. + got rid of some unnecessary checks Note that I do make the assumption that ro segs are always threaded at the beginning of gen2. We actually always thread these into heap 0's gen2 seg list but I can see a chance of that changing, if we want a better balancing of the mark_ro_segments work. --- src/coreclr/gc/gc.cpp | 212 ++++++++++++++++++++-------------------- src/coreclr/gc/gcpriv.h | 4 +- 2 files changed, 107 insertions(+), 109 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 988fde2811bdbe..1d9af15f040602 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -8459,15 +8459,12 @@ uint32_t* translate_mark_array (uint32_t* ma) } #ifdef FEATURE_BASICFREEZE -// from and end must be page aligned addresses. +// end must be page aligned addresses. void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL read_only/*=FALSE*/) { - if(!gc_can_use_concurrent) - return; + assert (gc_can_use_concurrent); -#ifdef FEATURE_BASICFREEZE if (!read_only) -#endif // FEATURE_BASICFREEZE { assert (from == align_on_mark_word (from)); } @@ -8499,24 +8496,21 @@ void gc_heap::clear_mark_array (uint8_t* from, uint8_t* end, BOOL read_only/*=FA memset (&mark_array[beg_word], 0, (end_word - beg_word)*sizeof (uint32_t)); #ifdef _DEBUG - else + //Beware, it is assumed that the mark array word straddling + //start has been cleared before + //verify that the array is empty. + size_t markw = mark_word_of (align_on_mark_word (from)); + size_t markw_end = mark_word_of (align_on_mark_word (end)); + while (markw < markw_end) { - //Beware, it is assumed that the mark array word straddling - //start has been cleared before - //verify that the array is empty. - size_t markw = mark_word_of (align_on_mark_word (from)); - size_t markw_end = mark_word_of (align_on_mark_word (end)); - while (markw < markw_end) - { - assert (!(mark_array [markw])); - markw++; - } - uint8_t* p = mark_word_address (markw_end); - while (p < end) - { - assert (!(mark_array_marked (p))); - p++; - } + assert (!(mark_array [markw])); + markw++; + } + uint8_t* p = mark_word_address (markw_end); + while (p < end) + { + assert (!(mark_array_marked (p))); + p++; } #endif //_DEBUG } @@ -9452,6 +9446,7 @@ void gc_heap::copy_brick_card_table() } #ifdef FEATURE_BASICFREEZE +// Note that we always insert at the head of the max_generation segment list. BOOL gc_heap::insert_ro_segment (heap_segment* seg) { #ifdef FEATURE_EVENT_TRACE @@ -9471,7 +9466,6 @@ BOOL gc_heap::insert_ro_segment (heap_segment* seg) return FALSE; } - //insert at the head of the max_generation segment list generation* gen2 = generation_of (max_generation); heap_segment* oldhead = generation_start_segment (gen2); heap_segment_next (seg) = oldhead; @@ -9514,7 +9508,7 @@ void gc_heap::remove_ro_segment (heap_segment* seg) if (gc_can_use_concurrent) { clear_mark_array (align_lower_mark_word (max (heap_segment_mem (seg), lowest_address)), - align_on_card_word (min (heap_segment_allocated (seg), highest_address))); + align_on_card_word (min (heap_segment_allocated (seg), highest_address))); } #endif //BACKGROUND_GC @@ -11010,21 +11004,19 @@ void gc_heap::seg_set_mark_array_bits_soh (heap_segment* seg) { uint8_t* range_beg = 0; uint8_t* range_end = 0; - if (gc_can_use_concurrent && bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) + if (bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) { - assert(range_beg == align_on_mark_word(range_beg)); - if ((range_beg <= background_saved_highest_address) && (range_beg >= background_saved_lowest_address)) - { - size_t beg_word = mark_word_of (align_on_mark_word (range_beg)); - size_t end_word = mark_word_of (align_on_mark_word (range_beg)); + size_t beg_word = mark_word_of (align_on_mark_word (range_beg)); + size_t end_word = mark_word_of (align_on_mark_word (range_beg)); - uint8_t* op = range_beg; - while (op < mark_word_address (beg_word)) - { - mark_array_set_marked (op); - op += mark_bit_pitch; - } + uint8_t* op = range_beg; + while (op < mark_word_address (beg_word)) + { + mark_array_set_marked (op); + op += mark_bit_pitch; } + + memset (&mark_array[beg_word], 0xFF, (end_word - beg_word)*sizeof (uint32_t)); } } #endif //FEATURE_BASICFREEZE @@ -26176,19 +26168,6 @@ void gc_heap::mark_phase (int condemned_gen_number, BOOL mark_only_p) gen_to_init = total_generation_count - 1; } -#ifdef FEATURE_BASICFREEZE -#ifdef USE_REGIONS - assert (!ro_segments_in_range); -#else // USE_REGIONS - if ((generation_start_segment (generation_of (condemned_gen_number)) != ephemeral_heap_segment) && - ro_segments_in_range) - { - generation* max_gen = generation_of (max_generation); - mark_ro_segments (generation_start_segment (max_gen)); - } -#endif // !USE_REGIONS -#endif // !FEATURE_BASICFREEZE - for (int gen_idx = 0; gen_idx <= gen_to_init; gen_idx++) { dynamic_data* dd = dynamic_data_of (gen_idx); @@ -26417,6 +26396,19 @@ void gc_heap::mark_phase (int condemned_gen_number, BOOL mark_only_p) } } +#ifdef FEATURE_BASICFREEZE +#ifdef USE_REGIONS + assert (!ro_segments_in_range); +#else //USE_REGIONS + if (ro_segments_in_range) + { + dprintf(3,("Marking in range ro segments")); + mark_ro_segments(); + // Should fire an ETW event here. + } +#endif USE_REGIONS +#endif //FEATURE_BASICFREEZE + dprintf(3,("Marking Roots")); GCScan::GcScanRoots(GCHeap::Promote, @@ -27462,6 +27454,7 @@ void gc_heap::process_ephemeral_boundaries (uint8_t* x, } #endif //!USE_REGIONS +#ifdef FEATURE_BASICFREEZE inline void gc_heap::seg_set_mark_bits (heap_segment* seg) { @@ -27487,62 +27480,73 @@ void gc_heap::seg_clear_mark_bits (heap_segment* seg) } } -#ifdef FEATURE_BASICFREEZE // We have to do this for in range ro segments because these objects' life time isn't accurately // expressed. The expectation is all objects on ro segs are live. So we just artifically mark // all of them on the in range ro segs. -void gc_heap::mark_ro_segments (heap_segment* start_seg) +void gc_heap::mark_ro_segments() { - //go through all of the ro segment in range and set the mark bit - heap_segment* seg = start_seg; - - while (seg) +#ifdef USE_REGIONS + assert (!ro_segments_in_range); +#else //USE_REGIONS + if ((settings.condemned_generation == max_generation) && ro_segments_in_range) { - if (!heap_segment_read_only_p (seg)) - break; + heap_segment* seg = generation_start_segment (generation_of (max_generation)); - if (heap_segment_in_range_p (seg)) + while (seg) { -#ifdef BACKGROUND_GC - if (settings.concurrent) + if (!heap_segment_read_only_p (seg)) + break; + + if (heap_segment_in_range_p (seg)) { - seg_set_mark_array_bits_soh (seg); - } - else +#ifdef BACKGROUND_GC + if (settings.concurrent) + { + seg_set_mark_array_bits_soh (seg); + } + else #endif //BACKGROUND_GC - { - seg_set_mark_bits (seg); + { + seg_set_mark_bits (seg); + } } + seg = heap_segment_next (seg); } - seg = heap_segment_next (seg); } +#endif //USE_REGIONS } -void gc_heap::sweep_ro_segments (heap_segment* start_seg) +void gc_heap::sweep_ro_segments() { - //go through all of the ro segment in range and reset the mark bit - heap_segment* seg = start_seg; - - while (seg) +#ifdef USE_REGIONS + assert (!ro_segments_in_range); +#else //USE_REGIONS + if ((settings.condemned_generation == max_generation) && ro_segments_in_range) { - if (!heap_segment_read_only_p (seg)) - break; + heap_segment* seg = generation_start_segment (generation_of (max_generation));; - if (heap_segment_in_range_p (seg)) + while (seg) { -#ifdef BACKGROUND_GC - if (settings.concurrent) + if (!heap_segment_read_only_p (seg)) + break; + + if (heap_segment_in_range_p (seg)) { - seg_clear_mark_array_bits_soh (seg); - } - else +#ifdef BACKGROUND_GC + if (settings.concurrent) + { + seg_clear_mark_array_bits_soh (seg); + } + else #endif //BACKGROUND_GC - { - seg_clear_mark_bits (seg); + { + seg_clear_mark_bits (seg); + } } + seg = heap_segment_next (seg); } - seg = heap_segment_next (seg); } +#endif //USE_REGIONS } #endif // FEATURE_BASICFREEZE @@ -28921,16 +28925,8 @@ void gc_heap::plan_phase (int condemned_gen_number) } #ifdef FEATURE_BASICFREEZE -#ifdef USE_REGIONS - assert (!ro_segments_in_range); -#else //USE_REGIONS - if ((generation_start_segment (condemned_gen1) != ephemeral_heap_segment) && - ro_segments_in_range) - { - sweep_ro_segments (generation_start_segment (condemned_gen1)); - } -#endif //USE_REGIONS -#endif // FEATURE_BASICFREEZE + sweep_ro_segments(); +#endif //FEATURE_BASICFREEZE #ifndef MULTIPLE_HEAPS int condemned_gen_index = get_stop_generation_index (condemned_gen_number); @@ -34652,13 +34648,6 @@ void gc_heap::background_mark_phase () dprintf(3,("BGC: stack marking")); sc.concurrent = TRUE; -#if defined (FEATURE_BASICFREEZE) && !defined (USE_REGIONS) - if ((generation_start_segment (gen) != ephemeral_heap_segment) && ro_segments_in_range) - { - mark_ro_segments (generation_start_segment (gen)); - } -#endif // FEATURE_BASICFREEZE - GCScan::GcScanRoots(background_promote_callback, max_generation, max_generation, &sc); @@ -34979,6 +34968,20 @@ void gc_heap::background_mark_phase () dprintf (GTC_LOG, ("FM: h%d: loh: %Id, soh: %Id, poh: %Id", heap_number, total_loh_size, total_soh_size, total_poh_size)); +#ifdef FEATURE_BASICFREEZE +#ifdef USE_REGIONS + assert (!ro_segments_in_range); +#else //USE_REGIONS + if (ro_segments_in_range) + { + dprintf (2, ("nonconcurrent marking in range ro segments")); + mark_ro_segments(); + //concurrent_print_time_delta ("nonconcurrent marking in range ro segments"); + concurrent_print_time_delta ("NRRO"); + } +#endif USE_REGIONS +#endif //FEATURE_BASICFREEZE + dprintf (2, ("nonconcurrent marking stack roots")); GCScan::GcScanRoots(background_promote, max_generation, max_generation, @@ -42328,13 +42331,8 @@ void gc_heap::background_sweep() #endif //DOUBLY_LINKED_FL #ifdef FEATURE_BASICFREEZE - generation* max_gen = generation_of (max_generation); - if ((generation_start_segment (max_gen) != ephemeral_heap_segment) && - ro_segments_in_range) - { - sweep_ro_segments (generation_start_segment (max_gen)); - } -#endif // FEATURE_BASICFREEZE + sweep_ro_segments(); +#endif //FEATURE_BASICFREEZE if (current_c_gc_state != c_gc_state_planning) { @@ -45283,7 +45281,7 @@ bool GCHeap::IsPromoted(Object* object) ((CObjectHeader*)o)->Validate(TRUE, TRUE, is_marked); } - if (!is_marked && object != nullptr && IsInFrozenSegment (object)) + if (!is_marked && (object != nullptr) && IsInFrozenSegment (object)) { // Frozen objects aren't expected to be "not promoted" here __UNREACHABLE(); diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index d46ae2cff491c1..9dbc372d81fcb7 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2868,9 +2868,9 @@ class gc_heap PER_HEAP void seg_clear_mark_bits (heap_segment* seg); PER_HEAP - void mark_ro_segments (heap_segment* start_seg); + void mark_ro_segments(); PER_HEAP - void sweep_ro_segments (heap_segment* start_seg); + void sweep_ro_segments(); #endif // FEATURE_BASICFREEZE PER_HEAP From 3447a2f7b5708ca00d423373ac4b00cbe42cd6fc Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Sun, 2 Oct 2022 22:41:46 -0700 Subject: [PATCH 13/16] Fix build breaks --- src/coreclr/gc/gc.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 1d9af15f040602..28a8506ed40400 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -26406,7 +26406,7 @@ void gc_heap::mark_phase (int condemned_gen_number, BOOL mark_only_p) mark_ro_segments(); // Should fire an ETW event here. } -#endif USE_REGIONS +#endif //USE_REGIONS #endif //FEATURE_BASICFREEZE dprintf(3,("Marking Roots")); @@ -34979,7 +34979,7 @@ void gc_heap::background_mark_phase () //concurrent_print_time_delta ("nonconcurrent marking in range ro segments"); concurrent_print_time_delta ("NRRO"); } -#endif USE_REGIONS +#endif //USE_REGIONS #endif //FEATURE_BASICFREEZE dprintf (2, ("nonconcurrent marking stack roots")); @@ -45279,12 +45279,9 @@ bool GCHeap::IsPromoted(Object* object) if (o) { ((CObjectHeader*)o)->Validate(TRUE, TRUE, is_marked); - } - - if (!is_marked && (object != nullptr) && IsInFrozenSegment (object)) - { + // Frozen objects aren't expected to be "not promoted" here - __UNREACHABLE(); + assert(is_marked || !IsInFrozenSegment(object)); } #endif //_DEBUG From a41ec8371ce58940cb67a3984a6bbb19b03d5ca8 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 3 Oct 2022 18:05:09 +0200 Subject: [PATCH 14/16] fix stupid type, although, this path is never taken (only if FOH is disabled via env.var but we'll get rid of that soon) --- src/coreclr/vm/jitinterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index ef511c1814fe08..a3e42b7ce8fc36 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11683,7 +11683,7 @@ InfoAccessType CEEJitInfo::emptyStringLiteral(void ** ppValue) } else { - *ppValue = pinnedStr; + *ppValue = pinnedStrHandlePtr; } EE_TO_JIT_TRANSITION(); From dae518f04aba69194229759555d02f89e8b3f43b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 3 Oct 2022 19:58:53 +0200 Subject: [PATCH 15/16] I should never copy-paste any code ever again --- src/coreclr/gc/gc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index b34f295e7698fc..2b5a3c1d7753c0 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -11009,7 +11009,7 @@ void gc_heap::seg_set_mark_array_bits_soh (heap_segment* seg) if (bgc_mark_array_range (seg, FALSE, &range_beg, &range_end)) { size_t beg_word = mark_word_of (align_on_mark_word (range_beg)); - size_t end_word = mark_word_of (align_on_mark_word (range_beg)); + size_t end_word = mark_word_of (align_on_mark_word (range_end)); uint8_t* op = range_beg; while (op < mark_word_address (beg_word)) From 9d4cbd27e512b6e04c1ee8079ddb97f99406f094 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 4 Oct 2022 14:47:05 +0200 Subject: [PATCH 16/16] Enable GC Regions back --- src/coreclr/gc/gcpriv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index 3bb909e4b00056..f2725a780446a7 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -52,8 +52,8 @@ inline void FATAL_GC_ERROR() // This means any empty regions can be freely used for any generation. For // Server GC we will balance regions between heaps. // For now disable regions for StandAlone GC, NativeAOT and MacOS builds -#if defined (HOST_64BIT) && defined (BUILD_AS_STANDALONE) && !defined(__APPLE__) && !defined(FEATURE_NATIVEAOT) -//#define USE_REGIONS +#if defined (HOST_64BIT) && !defined (BUILD_AS_STANDALONE) && !defined(__APPLE__) && !defined(FEATURE_NATIVEAOT) +#define USE_REGIONS #endif //HOST_64BIT && BUILD_AS_STANDALONE #ifdef USE_REGIONS