From e2eaa660a31e3ae8fca2bf2b2eb39638121a308a Mon Sep 17 00:00:00 2001 From: Eduardo Manuel Velarde Polar Date: Wed, 14 Aug 2024 14:34:05 -0700 Subject: [PATCH] Revert "Ensure that WaitForPendingFinalizers has seen the expected Full GC count (#105289)" This reverts commit 54a9efd92de0f776dcec4711b29601a1ff159223. --- .../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, 22 insertions(+), 132 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 2237b50350835..7ea73ba7c2c38 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, int observedFullGcCount); + internal static extern void RhpSignalFinalizationComplete(uint fCount); [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 80576c921f8a2..4e695601f1945 100644 --- a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs +++ b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs @@ -29,14 +29,11 @@ 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(); - // 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); + // 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); } else { diff --git a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp index b0f9eb0db5aa9..8fa6053818969 100644 --- a/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp @@ -94,22 +94,6 @@ 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 @@ -119,14 +103,6 @@ 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(); @@ -134,17 +110,6 @@ 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; - } } } @@ -211,6 +176,18 @@ 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 70b0cda4d2f6a..4751e40da3b24 100644 --- a/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -104,15 +104,5 @@ 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 97ace9a32353b..e543a3c60c346 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -404,15 +404,13 @@ 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. - // Thus we include the Full GC count that we have certaily observed. - SignalFinalizationDone(observedFullGcCount); + // consider itself satisfied by the drain that just completed. This is + // acceptable. + SignalFinalizationDone(); } if (s_InitializedFinalizerThreadForPlatform) @@ -540,13 +538,10 @@ void FinalizerThread::FinalizerThreadCreate() } } -static int g_fullGcCountSeenByFinalization; - -void FinalizerThread::SignalFinalizationDone(int observedFullGcCount) +void FinalizerThread::SignalFinalizationDone() { WRAPPER_NO_CONTRACT; - g_fullGcCountSeenByFinalization = observedFullGcCount; hEventFinalizerDone->Set(); } @@ -560,13 +555,6 @@ 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 @@ -577,8 +565,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 @@ -592,18 +580,6 @@ 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 03aae7b4e9cf6..b254773883ab8 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(int observedFullGcCount); + static void SignalFinalizationDone(); 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 ce029fc637284..137c1dc8246a1 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,7 +7,6 @@ using System.Runtime.InteropServices; using System.Diagnostics; using System.Threading; -using System.Threading.Tasks; using System.Runtime; using Microsoft.DotNet.RemoteExecutor; using Xunit; @@ -293,55 +292,6 @@ 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() {