GC finalization cleanup

src/coreclr/gc/gc.cpp

@@ -50979,19 +50979,20 @@ bool CFinalize::Initialize()
         GC_NOTRIGGER;
     } CONTRACTL_END;
 
-    m_Array = new (nothrow)(Object*[100]);
+    const int INITIAL_FINALIZER_ARRAY_SIZE = 100;
+    m_Array = new (nothrow)(Object*[INITIAL_FINALIZER_ARRAY_SIZE]);
 
     if (!m_Array)
     {
         ASSERT (m_Array);
-        STRESS_LOG_OOM_STACK(sizeof(Object*[100]));
+        STRESS_LOG_OOM_STACK(sizeof(Object*[INITIAL_FINALIZER_ARRAY_SIZE]));
         if (GCConfig::GetBreakOnOOM())
         {
             GCToOSInterface::DebugBreak();
         }
         return false;
     }
-    m_EndArray = &m_Array[100];
+    m_EndArray = &m_Array[INITIAL_FINALIZER_ARRAY_SIZE];
 
     for (int i =0; i < FreeList; i++)
     {
@@ -51120,8 +51121,8 @@ CFinalize::RegisterForFinalization (int gen, Object* obj, size_t size)
     unsigned int dest = gen_segment (gen);
 
     // Adjust boundary for segments so that GC will keep objects alive.
-    Object*** s_i = &SegQueue (FreeList);
-    if ((*s_i) == m_EndArray)
+    Object*** s_i = &SegQueue (FreeListSeg);
+    if ((*s_i) == SegQueueLimit(FreeListSeg))
     {
         if (!GrowArray())
         {
@@ -51147,7 +51148,7 @@ CFinalize::RegisterForFinalization (int gen, Object* obj, size_t size)
         //is the segment empty?
         if (!(*s_i == *(s_i-1)))
         {
-            //no, swap the end elements.
+            //no, move the first element of the segment to the (new) last location in the segment
             *(*s_i) = *(*(s_i-1));
         }
         //increment the fill pointer
@@ -51195,7 +51196,7 @@ CFinalize::GetNextFinalizableObject (BOOL only_non_critical)
 size_t
 CFinalize::GetNumberFinalizableObjects()
 {
-    return SegQueueLimit(FinalizerListSeg) - SegQueue(FinalizerListSeg);
+    return SegQueueLimit(FinalizerMaxSeg) - SegQueue(FinalizerStartSeg);
 }
 
 void
@@ -51210,11 +51211,16 @@ CFinalize::MoveItem (Object** fromIndex,
         step = -1;
     else
         step = +1;
-    // Place the element at the boundary closest to dest
+    // Each iteration places the element at the boundary closest to dest
+    // and then adjusts the boundary to move that element one segment closer
+    // to dest.
     Object** srcIndex = fromIndex;
     for (unsigned int i = fromSeg; i != toSeg; i+= step)
     {
+        // Select SegQueue[i] for step==-1, SegQueueLimit[i] for step==1
         Object**& destFill = m_FillPointers[i+(step - 1 )/2];
+        // Select SegQueue[i] for step==-1, SegQueueLimit[i]-1 for step==1
+        //   (SegQueueLimit[i]-1 is the last entry in segment i)
         Object** destIndex = destFill - (step + 1)/2;
         if (srcIndex != destIndex)
         {
@@ -51237,8 +51243,8 @@ CFinalize::GcScanRoots (promote_func* fn, int hn, ScanContext *pSC)
     pSC->thread_number = hn;
 
     //scan the finalization queue
-    Object** startIndex  = SegQueue (CriticalFinalizerListSeg);
-    Object** stopIndex  = SegQueueLimit (FinalizerListSeg);
+    Object** startIndex  = SegQueue (FinalizerStartSeg);
+    Object** stopIndex  = SegQueueLimit (FinalizerMaxSeg);
 
     for (Object** po = startIndex; po < stopIndex; po++)
     {
@@ -51252,12 +51258,20 @@ CFinalize::GcScanRoots (promote_func* fn, int hn, ScanContext *pSC)
 
 void CFinalize::WalkFReachableObjects (fq_walk_fn fn)
 {
-    Object** startIndex = SegQueue (CriticalFinalizerListSeg);
-    Object** stopCriticalIndex = SegQueueLimit (CriticalFinalizerListSeg);
-    Object** stopIndex  = SegQueueLimit (FinalizerListSeg);
+    Object** startIndex = SegQueue (FinalizerListSeg);
+    Object** stopIndex = SegQueueLimit (FinalizerListSeg);
     for (Object** po = startIndex; po < stopIndex; po++)
     {
-        fn(po < stopCriticalIndex, *po);
+        bool isCriticalFinalizer = false;
+        fn(isCriticalFinalizer, *po);
+    }
+
+    startIndex = SegQueue (CriticalFinalizerListSeg);
+    stopIndex = SegQueueLimit (CriticalFinalizerListSeg);
+    for (Object** po = startIndex; po < stopIndex; po++)
+    {
+        bool isCriticalFinalizer = true;
+        fn(isCriticalFinalizer, *po);
     }
 }
 
@@ -51295,13 +51309,13 @@ CFinalize::ScanForFinalization (promote_func* pfn, int gen, gc_heap* hp)
 
                     if (GCToEEInterface::EagerFinalized(obj))
                     {
-                        MoveItem (i, Seg, FreeList);
+                        MoveItem (i, Seg, FreeListSeg);
                     }
                     else if ((obj->GetHeader()->GetBits()) & BIT_SBLK_FINALIZER_RUN)
                     {
                         //remove the object because we don't want to
                         //run the finalizer
-                        MoveItem (i, Seg, FreeList);
+                        MoveItem (i, Seg, FreeListSeg);
 
                         //Reset the bit so it will be put back on the queue
                         //if resurrected and re-registered.
@@ -51477,14 +51491,14 @@ CFinalize::GrowArray()
 bool CFinalize::MergeFinalizationData (CFinalize* other_fq)
 {
     // compute how much space we will need for the merged data
-    size_t otherNeededArraySize = other_fq->SegQueue (FreeList) - other_fq->m_Array;
+    size_t otherNeededArraySize = other_fq->UsedCount();
     if (otherNeededArraySize == 0)
     {
         // the other queue is empty - nothing to do!
         return true;
     }
     size_t thisArraySize = (m_EndArray - m_Array);
-    size_t thisNeededArraySize = SegQueue (FreeList) - m_Array;
+    size_t thisNeededArraySize = UsedCount();
     size_t neededArraySize = thisNeededArraySize + otherNeededArraySize;
 
     Object ** newArray = m_Array;
@@ -51503,6 +51517,10 @@ bool CFinalize::MergeFinalizationData (CFinalize* other_fq)
         }
     }
 
+    // Since the target might be the original array (with the original data),
+    // the order of copying must not overwrite any data until it has been
+    // copied.
+
     // copy the finalization data from this and the other finalize queue
     for (int i = FreeList - 1; i >= 0; i--)
     {
@@ -51542,10 +51560,10 @@ bool CFinalize::MergeFinalizationData (CFinalize* other_fq)
 bool CFinalize::SplitFinalizationData (CFinalize* other_fq)
 {
     // the other finalization queue is assumed to be empty at this point
-    size_t otherCurrentArraySize = other_fq->SegQueue (FreeList) - other_fq->m_Array;
+    size_t otherCurrentArraySize = other_fq->UsedCount();
     assert (otherCurrentArraySize == 0);
 
-    size_t thisCurrentArraySize = SegQueue (FreeList) - m_Array;
+    size_t thisCurrentArraySize = UsedCount();
     if (thisCurrentArraySize == 0)
     {
         // this queue is empty - nothing to split!
@@ -51571,7 +51589,7 @@ bool CFinalize::SplitFinalizationData (CFinalize* other_fq)
     }
 
     // move half of the items in each section over to the other queue
-    PTR_PTR_Object newFillPointers[FreeList];
+    PTR_PTR_Object newFillPointers[MaxSeg];
     PTR_PTR_Object segQueue = m_Array;
     for (int i = 0; i < FreeList; i++)
     {
@@ -51587,14 +51605,15 @@ bool CFinalize::SplitFinalizationData (CFinalize* other_fq)
         memmove (&other_fq->m_Array[otherIndex], &m_Array[thisIndex + thisNewSize], sizeof(other_fq->m_Array[0])*otherSize);
         other_fq->SegQueueLimit (i) = &other_fq->m_Array[otherIndex + otherSize];
 
-        // we delete the moved half from this queue
+        // slide the unmoved half to its new position in the queue
+        // (this will delete the moved half once copies and m_FillPointers updates are completed)
         memmove (segQueue, &m_Array[thisIndex], sizeof(m_Array[0])*thisNewSize);
         segQueue += thisNewSize;
         newFillPointers[i] = segQueue;
     }
 
     // finally update the fill pointers from the new copy we generated
-    for (int i = 0; i < FreeList; i++)
+    for (int i = 0; i < MaxSeg; i++)
     {
         m_FillPointers[i] = newFillPointers[i];
     }

src/coreclr/gc/gcinterface.h

@@ -521,7 +521,7 @@ typedef bool (* walk_fn)(Object*, void*);
 typedef bool (* walk_fn2)(Object*, uint8_t**, void*);
 typedef void (* gen_walk_fn)(void* context, int generation, uint8_t* range_start, uint8_t* range_end, uint8_t* range_reserved);
 typedef void (* record_surv_fn)(uint8_t* begin, uint8_t* end, ptrdiff_t reloc, void* context, bool compacting_p, bool bgc_p);
-typedef void (* fq_walk_fn)(bool, void*);
+typedef void (* fq_walk_fn)(bool isCritical, void* pObject);
 typedef void (* fq_scan_fn)(Object** ppObject, ScanContext *pSC, uint32_t dwFlags);
 typedef void (* handle_scan_fn)(Object** pRef, Object* pSec, uint32_t flags, ScanContext* context, bool isDependent);
 typedef bool (* async_pin_enum_fn)(Object* object, void* context);

src/coreclr/gc/gcpriv.h

@@ -4682,12 +4682,27 @@ class CFinalize
 
 private:
 
-    //adjust the count and add a constant to add a segment
+    // Segments are bounded by m_Array (the overall start), each element of
+    // m_FillPointers, and then m_EndArray (the overall end). m_Array could
+    // be considered the first element of (i.e., before all of) m_FillPointers
+    // and m_EndArray the last.
+    //
+    // Therefore, the lower bound on segment X is m_FillPointers[x-1] with a
+    // special case for the first, and the upper bound on segment X is
+    // m_FillPointers[x] with special cases for the last.
+
+    // Adjust the count and add a constant to add a segment
     static const int ExtraSegCount = 2;
     static const int FinalizerListSeg = total_generation_count + 1;
     static const int CriticalFinalizerListSeg = total_generation_count;
-    //Does not correspond to a segment
-    static const int FreeList = total_generation_count + ExtraSegCount;
+    // The end of this segment is m_EndArray, not an entry in m_FillPointers.
+    static const int FreeListSeg = total_generation_count + ExtraSegCount;
+    static const int FreeList = FreeListSeg;
+
+    static const int FinalizerStartSeg = CriticalFinalizerListSeg;
+    static const int FinalizerMaxSeg = FinalizerListSeg;
+
+    static const int MaxSeg = FreeListSeg;
 
     PTR_PTR_Object m_FillPointers[total_generation_count + ExtraSegCount];
     PTR_PTR_Object m_Array;
@@ -4710,14 +4725,18 @@ class CFinalize
     }
     inline PTR_PTR_Object& SegQueueLimit (unsigned int Seg)
     {
-        return m_FillPointers [Seg];
+        return (Seg == MaxSeg ? m_EndArray : m_FillPointers[Seg]);
+    }
+
+    size_t UsedCount ()
+    {
+        return (SegQueue(FreeListSeg) - m_Array) + (m_EndArray - SegQueueLimit(FreeListSeg));
     }
 
     BOOL IsSegEmpty ( unsigned int i)
     {
-        ASSERT ( (int)i < FreeList);
+        ASSERT ((int)i <= MaxSeg);
         return (SegQueueLimit(i) == SegQueue (i));
-
     }
 
 public: