From 54a9efd92de0f776dcec4711b29601a1ff159223 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Wed, 24 Jul 2024 21:10:31 -0700 Subject: [PATCH] Ensure that WaitForPendingFinalizers has seen the expected Full GC count (#105289) * Ensure that WaitForPendingFinalizers has seen the expected Full GC * NativeAOT and some renames * a testcase * make the test not unsafe and make OuterLoop * Use unsigned math when comparing collection ticks * cast the diff to int when comparing gc ticks --- .../src/System/Runtime/InternalCalls.cs | 2 +- .../src/System/Runtime/__Finalizer.cs | 9 ++-- .../nativeaot/Runtime/FinalizerHelpers.cpp | 47 ++++++++++++----- .../src/System/Runtime/RuntimeImports.cs | 10 ++++ src/coreclr/vm/finalizerthread.cpp | 34 +++++++++++-- src/coreclr/vm/finalizerthread.h | 2 +- .../System.Runtime.Tests/System/GCTests.cs | 50 +++++++++++++++++++ 7 files changed, 132 insertions(+), 22 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..b0f9eb0db5aa9 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,17 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai // Wait for the finalizer thread to get back to us. g_FinalizerDoneEvent.Wait(INFINITE, false, allowReentrantWait); + + // 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 ((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, + // so we try one more time. + goto tryAgain; + } } } @@ -176,18 +211,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 e543a3c60c346..97ace9a32353b 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 g_fullGcCountSeenByFinalization; + +void FinalizerThread::SignalFinalizationDone(int observedFullGcCount) { WRAPPER_NO_CONTRACT; + g_fullGcCountSeenByFinalization = observedFullGcCount; hEventFinalizerDone->Set(); } @@ -555,6 +560,13 @@ void FinalizerThread::FinalizerThreadWait() // 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 @@ -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,18 @@ void FinalizerThread::FinalizerThreadWait() DWORD status; status = hEventFinalizerDone->Wait(INFINITE,TRUE); + + // 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 ((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, + // 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); 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..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 @@ -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,55 @@ private class TestObject } } + [OuterLoop] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public 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 < 20000; i++) + { + BoxedFinalized flag = new BoxedFinalized(); + MakeAndNull(flag); + GC.Collect(); + GC.WaitForPendingFinalizers(); + Assert.True(flag.finalized); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void MakeAndNull(BoxedFinalized flag) + { + var deadObj = new TestObjectWithFinalizer(flag); + // it's dead here + }; + } + + class BoxedFinalized + { + public bool finalized; + } + + class TestObjectWithFinalizer + { + BoxedFinalized _flag; + + public TestObjectWithFinalizer(BoxedFinalized flag) + { + _flag = flag; + } + + ~TestObjectWithFinalizer() => _flag.finalized = true; + } + [Fact] public static void SuppressFinalizer_NullObject_ThrowsArgumentNullException() {