Revert "Ensure that WaitForPendingFinalizers has seen the expected Full GC count (dotnet#105289)"

This reverts commit 54a9efd.

Author: MichalStrehovsky

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();

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
                 {

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,32 +103,13 @@ 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();
         g_FinalizerEvent.Set();
 
         // 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.

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);
     }
 }

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);
     }
 }

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);

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()
         {