Fix for solving lock contention issue in GC statics scanning. (dotnet…

…#32795)

* Fix for solving lock contention issue in GC statics scanning.

* Comment change to clarify benefit of switching order in GcScanRoots.

src/coreclr/src/vm/appdomain.cpp

@@ -255,6 +255,15 @@ OBJECTREF *LargeHeapHandleBucket::TryAllocateEmbeddedFreeHandle()
     return NULL;
 }
 
+// enumerate the handles in the bucket
+void LargeHeapHandleBucket::EnumStaticGCRefs(promote_func* fn, ScanContext* sc)
+{
+    for (int i = 0; i < m_CurrentPos; i++)
+    {
+        fn((Object**)&m_pArrayDataPtr[i], sc, 0);
+    }
+}
+
 
 // Maximum bucket size will be 64K on 32-bit and 128K on 64-bit.
 // We subtract out a small amount to leave room for the object
@@ -512,8 +521,14 @@ void LargeHeapHandleTable::ReleaseHandles(OBJECTREF *pObjRef, DWORD nReleased)
     m_cEmbeddedFree += nReleased;
 }
 
-
-
+// enumerate the handles in the handle table
+void LargeHeapHandleTable::EnumStaticGCRefs(promote_func* fn, ScanContext* sc)
+{
+    for (LargeHeapHandleBucket *pBucket = m_pHead; pBucket != nullptr; pBucket = pBucket->GetNext())
+    {
+        pBucket->EnumStaticGCRefs(fn, sc);
+    }
+}
 
 // Constructor for the ThreadStaticHandleBucket class.
 ThreadStaticHandleBucket::ThreadStaticHandleBucket(ThreadStaticHandleBucket *pNext, DWORD Size, BaseDomain *pDomain)
@@ -5954,14 +5969,12 @@ void AppDomain::EnumStaticGCRefs(promote_func* fn, ScanContext* sc)
              GCHeapUtilities::IsServerHeap()   &&
              IsGCSpecialThread());
 
-    AppDomain::AssemblyIterator asmIterator = IterateAssembliesEx((AssemblyIterationFlags)(kIncludeLoaded | kIncludeExecution));
-    CollectibleAssemblyHolder<DomainAssembly *> pDomainAssembly;
-    while (asmIterator.Next(pDomainAssembly.This()))
+#ifndef CROSSGEN_COMPILE
+    if (m_pLargeHeapHandleTable != nullptr)
     {
-        // @TODO: Review when DomainAssemblies get added.
-        _ASSERTE(pDomainAssembly != NULL);
-        pDomainAssembly->EnumStaticGCRefs(fn, sc);
+        m_pLargeHeapHandleTable->EnumStaticGCRefs(fn, sc);
     }
+#endif // CROSSGEN_COMPILE
 
     RETURN;
 }

src/coreclr/src/vm/appdomain.hpp

@@ -529,6 +529,8 @@ class LargeHeapHandleBucket
         return m_pArrayDataPtr + m_CurrentPos;
     }
 
+    void EnumStaticGCRefs(promote_func* fn, ScanContext* sc);
+
 private:
     LargeHeapHandleBucket *m_pNext;
     int m_ArraySize;
@@ -555,6 +557,8 @@ class LargeHeapHandleTable
     // Release object handles allocated using AllocateHandles().
     void ReleaseHandles(OBJECTREF *pObjRef, DWORD nReleased);
 
+    void EnumStaticGCRefs(promote_func* fn, ScanContext* sc);
+
 private:
     // The buckets of object handles.
     LargeHeapHandleBucket *m_pHead;

src/coreclr/src/vm/ceeload.cpp

@@ -2882,58 +2882,6 @@ void Module::AllocateStatics(AllocMemTracker *pamTracker)
     BuildStaticsOffsets(pamTracker);
 }
 
-// This method will report GC static refs of the module. It doesn't have to be complete (ie, it's
-// currently used to opportunistically get more concurrency in the marking of statics), so it currently
-// ignores any statics that are not preallocated (ie: won't report statics from IsDynamicStatics() MT)
-// The reason this function is in Module and not in DomainFile (together with DomainLocalModule is because
-// for shared modules we need a very fast way of getting to the DomainLocalModule. For that we use
-// a table in DomainLocalBlock that's indexed with a module ID
-//
-// This method is a secondary way for the GC to find statics, and it is only used when we are on
-// a multiproc machine and we are using the ServerHeap. The primary way used by the GC to find
-// statics is through the handle table. Module::AllocateRegularStaticHandles() allocates a GC handle
-// from the handle table, and the GC will trace this handle and find the statics.
-
-void Module::EnumRegularStaticGCRefs(promote_func* fn, ScanContext* sc)
-{
-    CONTRACT_VOID
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-    }
-    CONTRACT_END;
-
-    _ASSERTE(GCHeapUtilities::IsGCInProgress() &&
-         GCHeapUtilities::IsServerHeap() &&
-         IsGCSpecialThread());
-
-
-    DomainLocalModule *pModuleData = GetDomainLocalModule();
-    DWORD dwHandles                = m_dwMaxGCRegularStaticHandles;
-
-    if (IsResource())
-    {
-        RETURN;
-    }
-
-    LOG((LF_GC, LL_INFO100, "Scanning statics for module %s\n", GetSimpleName()));
-
-    OBJECTREF* ppObjectRefs       = pModuleData->GetPrecomputedGCStaticsBasePointer();
-    for (DWORD i = 0 ; i < dwHandles ; i++)
-    {
-        // Handles are allocated in SetDomainFile (except for bootstrapped mscorlib). In any
-        // case, we shouldnt get called if the module hasn't had it's handles allocated (as we
-        // only get here if IsActive() is true, which only happens after SetDomainFile(), which
-        // is were we allocate handles.
-        _ASSERTE(ppObjectRefs);
-        fn((Object **)(ppObjectRefs+i), sc, 0);
-    }
-
-    LOG((LF_GC, LL_INFO100, "Done scanning statics for module %s\n", GetSimpleName()));
-
-    RETURN;
-}
-
 void Module::SetDomainFile(DomainFile *pDomainFile)
 {
     CONTRACTL

src/coreclr/src/vm/ceeload.h

@@ -3060,8 +3060,6 @@ class Module
     // Self-initializing accessor for domain-independent IJW thunk heap
     LoaderHeap              *GetDllThunkHeap();
 
-    void            EnumRegularStaticGCRefs        (promote_func* fn, ScanContext* sc);
-
 protected:
 
     void            BuildStaticsOffsets     (AllocMemTracker *pamTracker);

src/coreclr/src/vm/domainfile.cpp

@@ -2265,50 +2265,6 @@ void DomainAssembly::NotifyDebuggerUnload()
 
 }
 
-// This will enumerate for static GC refs (but not thread static GC refs)
-
-void DomainAssembly::EnumStaticGCRefs(promote_func* fn, ScanContext* sc)
-{
-    CONTRACT_VOID
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-    }
-    CONTRACT_END;
-
-    _ASSERTE(GCHeapUtilities::IsGCInProgress() &&
-         GCHeapUtilities::IsServerHeap()   &&
-         IsGCSpecialThread());
-
-    if (IsCollectible())
-    {
-        // Collectible assemblies have statics stored in managed arrays, so they don't need special handlings
-        return;
-    }
-
-    DomainModuleIterator i = IterateModules(kModIterIncludeLoaded);
-    while (i.Next())
-    {
-        DomainFile* pDomainFile = i.GetDomainFile();
-
-        if (pDomainFile->IsActive())
-        {
-            // We guarantee that at this point the module has it's DomainLocalModule set up
-            // , as we create it while we load the module
-            _ASSERTE(pDomainFile->GetLoadedModule()->GetDomainLocalModule());
-            pDomainFile->GetLoadedModule()->EnumRegularStaticGCRefs(fn, sc);
-
-            // We current to do not iterate over the ThreadLocalModules that correspond
-            // to this Module. The GC discovers thread statics through the handle table.
-        }
-    }
-
-    RETURN;
-}
-
-
-
-
 #endif // #ifndef DACCESS_COMPILE
 
 #ifdef DACCESS_COMPILE

src/coreclr/src/vm/domainfile.h

@@ -671,10 +671,6 @@ class DomainAssembly : public DomainFile
     void NotifyDebuggerUnload();
 
     inline BOOL IsCollectible();
-    //
-    //  GC API
-    //
-    void EnumStaticGCRefs(promote_func* fn, ScanContext* sc);
 
 
  private:

src/coreclr/src/vm/gcenv.ee.cpp

@@ -158,15 +158,6 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen,
 {
     STRESS_LOG1(LF_GCROOTS, LL_INFO10, "GCScan: Promotion Phase = %d\n", sc->promotion);
 
-    // In server GC, we should be competing for marking the statics
-    if (GCHeapUtilities::MarkShouldCompeteForStatics())
-    {
-        if (condemned == max_gen && sc->promotion)
-        {
-            SystemDomain::EnumAllStaticGCRefs(fn, sc);
-        }
-    }
-
     Thread* pThread = NULL;
     while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL)
     {
@@ -186,6 +177,20 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen,
         }
         STRESS_LOG2(LF_GC | LF_GCROOTS, LL_INFO100, "Ending scan of Thread %p ID = 0x%x }\n", pThread, pThread->GetThreadId());
     }
+
+    // In server GC, we should be competing for marking the statics
+    // It's better to do this *after* stack scanning, because this way
+    // we can make up for imbalances in stack scanning
+    // This would not apply to the initial mark phase in background GC,
+    // but it would apply to blocking Gen 2 collections and the final
+    // marking stage in background GC where we catch up to the user program
+    if (GCHeapUtilities::MarkShouldCompeteForStatics())
+    {
+        if (condemned == max_gen && sc->promotion)
+        {
+            SystemDomain::EnumAllStaticGCRefs(fn, sc);
+        }
+    }
 }
 
 void GCToEEInterface::GcStartWork (int condemned, int max_gen)