From d95832f5c705adc55d693cb5a703984e0dc4d683 Mon Sep 17 00:00:00 2001 From: Peter Sollich Date: Tue, 25 Feb 2020 15:52:25 +0100 Subject: [PATCH 1/2] Fix for solving lock contention issue in GC statics scanning. --- src/coreclr/src/vm/appdomain.cpp | 29 ++++++++++++----- src/coreclr/src/vm/appdomain.hpp | 4 +++ src/coreclr/src/vm/ceeload.cpp | 52 ------------------------------- src/coreclr/src/vm/ceeload.h | 2 -- src/coreclr/src/vm/domainfile.cpp | 44 -------------------------- src/coreclr/src/vm/domainfile.h | 4 --- src/coreclr/src/vm/gcenv.ee.cpp | 20 ++++++------ 7 files changed, 36 insertions(+), 119 deletions(-) diff --git a/src/coreclr/src/vm/appdomain.cpp b/src/coreclr/src/vm/appdomain.cpp index 44aa02dad8cfa9..4eff9577759cf7 100644 --- a/src/coreclr/src/vm/appdomain.cpp +++ b/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) @@ -5956,14 +5971,12 @@ void AppDomain::EnumStaticGCRefs(promote_func* fn, ScanContext* sc) GCHeapUtilities::IsServerHeap() && IsGCSpecialThread()); - AppDomain::AssemblyIterator asmIterator = IterateAssembliesEx((AssemblyIterationFlags)(kIncludeLoaded | kIncludeExecution)); - CollectibleAssemblyHolder 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; } diff --git a/src/coreclr/src/vm/appdomain.hpp b/src/coreclr/src/vm/appdomain.hpp index 8dd474184c1ecb..fc8b48fdc2ad82 100644 --- a/src/coreclr/src/vm/appdomain.hpp +++ b/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; diff --git a/src/coreclr/src/vm/ceeload.cpp b/src/coreclr/src/vm/ceeload.cpp index 3ff65d67432e50..7087d12440a80b 100644 --- a/src/coreclr/src/vm/ceeload.cpp +++ b/src/coreclr/src/vm/ceeload.cpp @@ -2883,58 +2883,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 diff --git a/src/coreclr/src/vm/ceeload.h b/src/coreclr/src/vm/ceeload.h index f4a0b05120dd9c..7647ed660f77fd 100644 --- a/src/coreclr/src/vm/ceeload.h +++ b/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); diff --git a/src/coreclr/src/vm/domainfile.cpp b/src/coreclr/src/vm/domainfile.cpp index 2adb813cf16ef0..02cefea5336da9 100644 --- a/src/coreclr/src/vm/domainfile.cpp +++ b/src/coreclr/src/vm/domainfile.cpp @@ -2273,50 +2273,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 diff --git a/src/coreclr/src/vm/domainfile.h b/src/coreclr/src/vm/domainfile.h index f01988721c0b35..c0f7e15fd10570 100644 --- a/src/coreclr/src/vm/domainfile.h +++ b/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: diff --git a/src/coreclr/src/vm/gcenv.ee.cpp b/src/coreclr/src/vm/gcenv.ee.cpp index 72cac07c9f6ef5..17697e0d352b5a 100644 --- a/src/coreclr/src/vm/gcenv.ee.cpp +++ b/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,17 @@ 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 + if (GCHeapUtilities::MarkShouldCompeteForStatics()) + { + if (condemned == max_gen && sc->promotion) + { + SystemDomain::EnumAllStaticGCRefs(fn, sc); + } + } } void GCToEEInterface::GcStartWork (int condemned, int max_gen) From 8918e87dfd75e809112e8dc9f0905c821d857ffe Mon Sep 17 00:00:00 2001 From: Peter Sollich Date: Mon, 2 Mar 2020 12:54:03 -0800 Subject: [PATCH 2/2] Comment change to clarify benefit of switching order in GcScanRoots. --- src/coreclr/src/vm/gcenv.ee.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/src/vm/gcenv.ee.cpp b/src/coreclr/src/vm/gcenv.ee.cpp index 17697e0d352b5a..fe0fc30575d236 100644 --- a/src/coreclr/src/vm/gcenv.ee.cpp +++ b/src/coreclr/src/vm/gcenv.ee.cpp @@ -181,6 +181,9 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen, // 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)