From 12aeee5b52833540c253f48cba462fe4ee4fc184 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 22 Jul 2024 17:28:36 -0700 Subject: [PATCH 1/6] Ensure that WaitForPendingFinalizers has seen the expected Full GC --- src/coreclr/vm/finalizerthread.cpp | 31 +++++++++++++++++++++++++----- src/coreclr/vm/finalizerthread.h | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index e543a3c60c346..2586deeaef97d 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -404,13 +404,15 @@ VOID FinalizerThread::FinalizerThreadWorker(void *args) } LOG((LF_GC, LL_INFO100, "***** Calling Finalizers\n")); + int observedFullGcCount = + GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); FinalizeAllObjects(); // Anyone waiting to drain the Q can now wake up. Note that there is a // race in that another thread starting a drain, as we leave a drain, may - // consider itself satisfied by the drain that just completed. This is - // acceptable. - SignalFinalizationDone(); + // consider itself satisfied by the drain that just completed. + // Thus we include the Full GC count that we have certaily observed. + SignalFinalizationDone(observedFullGcCount); } if (s_InitializedFinalizerThreadForPlatform) @@ -538,10 +540,13 @@ void FinalizerThread::FinalizerThreadCreate() } } -void FinalizerThread::SignalFinalizationDone() +static int fullGcCountSeenByFinalization; + +void FinalizerThread::SignalFinalizationDone(int observedFullGcCount) { WRAPPER_NO_CONTRACT; + fullGcCountSeenByFinalization = observedFullGcCount; hEventFinalizerDone->Set(); } @@ -552,6 +557,13 @@ void FinalizerThread::FinalizerThreadWait() ASSERT(hEventFinalizer->IsValid()); ASSERT(GetFinalizerThread()); + // We may see a completion of finalization cycle that might not see objects that became + // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle. + // However, since an object cannot be prevented from promoting, one can only rely on Full GCs + // to collect unreferenced objects deterministically. Thus we only care about Full GCs here. + int desiredFullGcCount = + GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); + // Can't call this from within a finalized method. if (!IsCurrentThreadFinalizer()) { @@ -565,8 +577,8 @@ void FinalizerThread::FinalizerThreadWait() g_pRCWCleanupList->CleanupWrappersInCurrentCtxThread(); #endif // FEATURE_COMINTEROP + tryAgain: hEventFinalizerDone->Reset(); - EnableFinalization(); // Under GC stress the finalizer queue may never go empty as frequent @@ -580,6 +592,15 @@ void FinalizerThread::FinalizerThreadWait() DWORD status; status = hEventFinalizerDone->Wait(INFINITE,TRUE); + + if (desiredFullGcCount - fullGcCountSeenByFinalization > 0) + { + // There were some Full GCs happened before we started waiting and possibly not seen by the + // last finalization cycle. This is rare, but we need to be sure we have seen those, + // so we try one more time. + goto tryAgain; + } + _ASSERTE(status == WAIT_OBJECT_0); } } diff --git a/src/coreclr/vm/finalizerthread.h b/src/coreclr/vm/finalizerthread.h index b254773883ab8..03aae7b4e9cf6 100644 --- a/src/coreclr/vm/finalizerthread.h +++ b/src/coreclr/vm/finalizerthread.h @@ -67,7 +67,7 @@ class FinalizerThread static void FinalizerThreadWait(); - static void SignalFinalizationDone(); + static void SignalFinalizationDone(int observedFullGcCount); static VOID FinalizerThreadWorker(void *args); static DWORD WINAPI FinalizerThreadStart(void *args); From 334f1c7864edf718d17aa451ffa770685e236cfc Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 22 Jul 2024 23:01:25 -0700 Subject: [PATCH 2/6] NativeAOT and some renames --- .../src/System/Runtime/InternalCalls.cs | 2 +- .../src/System/Runtime/__Finalizer.cs | 9 ++-- .../nativeaot/Runtime/FinalizerHelpers.cpp | 44 ++++++++++++++----- .../src/System/Runtime/RuntimeImports.cs | 10 +++++ src/coreclr/vm/finalizerthread.cpp | 22 +++++----- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs index 7ea73ba7c2c38..2237b50350835 100644 --- a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs +++ b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs @@ -276,7 +276,7 @@ internal static extern unsafe IntPtr RhpCallPropagateExceptionCallback( // Indicate that the current round of finalizations is complete. [DllImport(Redhawk.BaseName)] - internal static extern void RhpSignalFinalizationComplete(uint fCount); + internal static extern void RhpSignalFinalizationComplete(uint fCount, int observedFullGcCount); [DllImport(Redhawk.BaseName)] internal static extern ulong RhpGetTickCount64(); diff --git a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs index 4e695601f1945..80576c921f8a2 100644 --- a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs +++ b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs @@ -29,11 +29,14 @@ public static void ProcessFinalizers() // otherwise memory is low and we should initiate a collection. if (InternalCalls.RhpWaitForFinalizerRequest() != 0) { + int observedFullGcCount = RuntimeImports.RhGetGcCollectionCount(RuntimeImports.RhGetMaxGcGeneration(), false); uint finalizerCount = DrainQueue(); - // Tell anybody that's interested that the finalization pass is complete (there is a race condition here - // where we might immediately signal a new request as complete, but this is acceptable). - InternalCalls.RhpSignalFinalizationComplete(finalizerCount); + // Anyone waiting to drain the Q can now wake up. Note that there is a + // race in that another thread starting a drain, as we leave a drain, may + // consider itself satisfied by the drain that just completed. + // Thus we include the Full GC count that we have certaily observed. + InternalCalls.RhpSignalFinalizationComplete(finalizerCount, observedFullGcCount); } else { diff --git a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp index 8fa6053818969..67b718bdc5bb7 100644 --- a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp @@ -94,6 +94,22 @@ EXTERN_C void QCALLTYPE RhInitializeFinalizerThread() g_FinalizerEvent.Set(); } +static int32_t g_fullGcCountSeenByFinalization; + +// Indicate that the current round of finalizations is complete. +EXTERN_C void QCALLTYPE RhpSignalFinalizationComplete(uint32_t fcount, int32_t observedFullGcCount) +{ + FireEtwGCFinalizersEnd_V1(fcount, GetClrInstanceId()); + + g_fullGcCountSeenByFinalization = observedFullGcCount; + g_FinalizerDoneEvent.Set(); + + if (YieldProcessorNormalization::IsMeasurementScheduled()) + { + YieldProcessorNormalization::PerformMeasurement(); + } +} + EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWait) { // This must be called via p/invoke rather than RuntimeImport since it blocks and could starve the GC if @@ -103,6 +119,14 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai // Can't call this from the finalizer thread itself. if (ThreadStore::GetCurrentThread() != g_pFinalizerThread) { + // We may see a completion of finalization cycle that might not see objects that became + // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle. + // However, since an object cannot be prevented from promoting, one can only rely on Full GCs + // to collect unreferenced objects deterministically. Thus we only care about Full GCs here. + int desiredFullGcCount = + GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); + + tryAgain: // Clear any current indication that a finalization pass is finished and wake the finalizer thread up // (if there's no work to do it'll set the done event immediately). g_FinalizerDoneEvent.Reset(); @@ -110,6 +134,14 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai // Wait for the finalizer thread to get back to us. g_FinalizerDoneEvent.Wait(INFINITE, false, allowReentrantWait); + + if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0) + { + // There were some Full GCs happening before we started waiting and possibly not seen by the + // last finalization cycle. This is rare, but we need to be sure we have seen those, + // so we try one more time. + goto tryAgain; + } } } @@ -176,18 +208,6 @@ EXTERN_C UInt32_BOOL QCALLTYPE RhpWaitForFinalizerRequest() } while (true); } -// Indicate that the current round of finalizations is complete. -EXTERN_C void QCALLTYPE RhpSignalFinalizationComplete(uint32_t fcount) -{ - FireEtwGCFinalizersEnd_V1(fcount, GetClrInstanceId()); - g_FinalizerDoneEvent.Set(); - - if (YieldProcessorNormalization::IsMeasurementScheduled()) - { - YieldProcessorNormalization::PerformMeasurement(); - } -} - // // The following helpers are special in that they interact with internal GC state or directly manipulate // managed references so they're called with a special co-operative p/invoke. diff --git a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs index 4751e40da3b24..70b0cda4d2f6a 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -104,5 +104,15 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhBulkMoveWithWriteBarrier")] internal static extern unsafe void RhBulkMoveWithWriteBarrier(ref byte dmem, ref byte smem, nuint size); + + // Get maximum GC generation number. + [MethodImplAttribute(MethodImplOptions.InternalCall)] + [RuntimeImport(RuntimeLibrary, "RhGetMaxGcGeneration")] + internal static extern int RhGetMaxGcGeneration(); + + // Get count of collections so far. + [MethodImplAttribute(MethodImplOptions.InternalCall)] + [RuntimeImport(RuntimeLibrary, "RhGetGcCollectionCount")] + internal static extern int RhGetGcCollectionCount(int generation, bool getSpecialGCCount); } } diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 2586deeaef97d..0fdc747f74405 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -540,13 +540,13 @@ void FinalizerThread::FinalizerThreadCreate() } } -static int fullGcCountSeenByFinalization; +static int g_fullGcCountSeenByFinalization; void FinalizerThread::SignalFinalizationDone(int observedFullGcCount) { WRAPPER_NO_CONTRACT; - fullGcCountSeenByFinalization = observedFullGcCount; + g_fullGcCountSeenByFinalization = observedFullGcCount; hEventFinalizerDone->Set(); } @@ -557,16 +557,16 @@ void FinalizerThread::FinalizerThreadWait() ASSERT(hEventFinalizer->IsValid()); ASSERT(GetFinalizerThread()); - // We may see a completion of finalization cycle that might not see objects that became - // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle. - // However, since an object cannot be prevented from promoting, one can only rely on Full GCs - // to collect unreferenced objects deterministically. Thus we only care about Full GCs here. - int desiredFullGcCount = - GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); - // Can't call this from within a finalized method. if (!IsCurrentThreadFinalizer()) { + // We may see a completion of finalization cycle that might not see objects that became + // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle. + // However, since an object cannot be prevented from promoting, one can only rely on Full GCs + // to collect unreferenced objects deterministically. Thus we only care about Full GCs here. + int desiredFullGcCount = + GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); + GCX_PREEMP(); #ifdef FEATURE_COMINTEROP @@ -593,9 +593,9 @@ void FinalizerThread::FinalizerThreadWait() DWORD status; status = hEventFinalizerDone->Wait(INFINITE,TRUE); - if (desiredFullGcCount - fullGcCountSeenByFinalization > 0) + if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0) { - // There were some Full GCs happened before we started waiting and possibly not seen by the + // There were some Full GCs happening before we started waiting and possibly not seen by the // last finalization cycle. This is rare, but we need to be sure we have seen those, // so we try one more time. goto tryAgain; From f6b5acfe3d32e086c50e5ebe575d7e3086dc8fc4 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 22 Jul 2024 23:50:19 -0700 Subject: [PATCH 3/6] a testcase --- .../System.Runtime.Tests/System/GCTests.cs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs index 137c1dc8246a1..2b0ae07e71773 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs @@ -7,6 +7,7 @@ using System.Runtime.InteropServices; using System.Diagnostics; using System.Threading; +using System.Threading.Tasks; using System.Runtime; using Microsoft.DotNet.RemoteExecutor; using Xunit; @@ -292,6 +293,50 @@ private class TestObject } } + // [OuterLoop] // TODO: VS uncomment before merging + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public unsafe static void WaitForPendingFinalizersRaces() + { + Task.Run(Test); + Task.Run(Test); + Task.Run(Test); + Task.Run(Test); + Task.Run(Test); + Task.Run(Test); + Test(); + + static void Test() + { + for (int i = 0; i < 2000; i++) + { + bool finalized = false; + MakeAndNull(&finalized); + GC.Collect(); + GC.WaitForPendingFinalizers(); + Assert.True(finalized); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void MakeAndNull(bool* flag) + { + var deadObj = new TestObjectWithFinalizer(flag); + // it's dead here + }; + } + + unsafe class TestObjectWithFinalizer + { + bool* _flag; + + public TestObjectWithFinalizer(bool* flag) + { + _flag = flag; + } + + ~TestObjectWithFinalizer() => *_flag = true; + } + [Fact] public static void SuppressFinalizer_NullObject_ThrowsArgumentNullException() { From 55830b1c223741f4338c468631f8f57d149fdbbf Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 24 Jul 2024 10:55:43 -0700 Subject: [PATCH 4/6] make the test not unsafe and make OuterLoop --- .../System.Runtime.Tests/System/GCTests.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs index 2b0ae07e71773..ce029fc637284 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs @@ -293,9 +293,9 @@ private class TestObject } } - // [OuterLoop] // TODO: VS uncomment before merging + [OuterLoop] [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] - public unsafe static void WaitForPendingFinalizersRaces() + public static void WaitForPendingFinalizersRaces() { Task.Run(Test); Task.Run(Test); @@ -307,34 +307,39 @@ public unsafe static void WaitForPendingFinalizersRaces() static void Test() { - for (int i = 0; i < 2000; i++) + for (int i = 0; i < 20000; i++) { - bool finalized = false; - MakeAndNull(&finalized); + BoxedFinalized flag = new BoxedFinalized(); + MakeAndNull(flag); GC.Collect(); GC.WaitForPendingFinalizers(); - Assert.True(finalized); + Assert.True(flag.finalized); } } [MethodImpl(MethodImplOptions.NoInlining)] - static void MakeAndNull(bool* flag) + static void MakeAndNull(BoxedFinalized flag) { var deadObj = new TestObjectWithFinalizer(flag); // it's dead here }; } - unsafe class TestObjectWithFinalizer + class BoxedFinalized + { + public bool finalized; + } + + class TestObjectWithFinalizer { - bool* _flag; + BoxedFinalized _flag; - public TestObjectWithFinalizer(bool* flag) + public TestObjectWithFinalizer(BoxedFinalized flag) { _flag = flag; } - ~TestObjectWithFinalizer() => *_flag = true; + ~TestObjectWithFinalizer() => _flag.finalized = true; } [Fact] From d1d2b7e79f9f34b1503748c8d8c1b4340f33e09d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 24 Jul 2024 11:31:43 -0700 Subject: [PATCH 5/6] Use unsigned math when comparing collection ticks --- src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp | 5 ++++- src/coreclr/vm/finalizerthread.cpp | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp index 67b718bdc5bb7..3a866b7c42b9d 100644 --- a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp @@ -135,7 +135,10 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai // Wait for the finalizer thread to get back to us. g_FinalizerDoneEvent.Wait(INFINITE, false, allowReentrantWait); - if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0) + // we use unsigned math here as the collection counts, which are size_t internally, + // can in theory overflow an int and wrap around. + // unsigned math would have more defined/portable behavior in such case + if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0) { // There were some Full GCs happening before we started waiting and possibly not seen by the // last finalization cycle. This is rare, but we need to be sure we have seen those, diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 0fdc747f74405..07a7b7cc60578 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -593,7 +593,10 @@ void FinalizerThread::FinalizerThreadWait() DWORD status; status = hEventFinalizerDone->Wait(INFINITE,TRUE); - if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0) + // we use unsigned math here as the collection counts, which are size_t internally, + // can in theory overflow an int and wrap around. + // unsigned math would have more defined/portable behavior in such case + if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0) { // There were some Full GCs happening before we started waiting and possibly not seen by the // last finalization cycle. This is rare, but we need to be sure we have seen those, From c2b8cf98627acacde825a02afae0c4a6c6e57907 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 24 Jul 2024 15:56:30 -0700 Subject: [PATCH 6/6] cast the diff to int when comparing gc ticks --- src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp | 2 +- src/coreclr/vm/finalizerthread.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp index 3a866b7c42b9d..b0f9eb0db5aa9 100644 --- a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp @@ -138,7 +138,7 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai // we use unsigned math here as the collection counts, which are size_t internally, // can in theory overflow an int and wrap around. // unsigned math would have more defined/portable behavior in such case - if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0) + if ((int)((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization) > 0) { // There were some Full GCs happening before we started waiting and possibly not seen by the // last finalization cycle. This is rare, but we need to be sure we have seen those, diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 07a7b7cc60578..97ace9a32353b 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -596,7 +596,7 @@ void FinalizerThread::FinalizerThreadWait() // we use unsigned math here as the collection counts, which are size_t internally, // can in theory overflow an int and wrap around. // unsigned math would have more defined/portable behavior in such case - if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0) + if ((int)((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization) > 0) { // There were some Full GCs happening before we started waiting and possibly not seen by the // last finalization cycle. This is rare, but we need to be sure we have seen those,