From c8e7d2b1f4f73138ce1564b6f75e9fd84819b98b Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 24 Feb 2026 13:03:26 -0800 Subject: [PATCH 1/5] Replace GCX_COOP with EBR for EEHashTable bucket reclamation Replace cooperative GC mode transitions (GCX_COOP_NO_THREAD_BROKEN) with Epoch-Based Reclamation for safe deferred deletion of old EEHashTable bucket arrays during resize. - Add dedicated g_EEHashEbr collector (init, cleanup, critical regions) - Add AllocateEEHashBuckets/FreeEEHashBuckets helpers in eehash.cpp - Remove SyncClean::AddEEHashTable and associated cleanup infrastructure - Remove redundant MemoryBarrier before VolatilePtr::Store (release semantics) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/ceemain.cpp | 3 + src/coreclr/vm/ebr.cpp | 6 +- src/coreclr/vm/ebr.h | 3 + src/coreclr/vm/eehash.cpp | 42 ++++++++ src/coreclr/vm/eehash.h | 8 +- src/coreclr/vm/eehash.inl | 168 +++++++++-------------------- src/coreclr/vm/finalizerthread.cpp | 9 +- src/coreclr/vm/syncclean.cpp | 46 +------- src/coreclr/vm/syncclean.hpp | 14 +-- src/coreclr/vm/threadsuspend.cpp | 9 +- 10 files changed, 123 insertions(+), 185 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index e6ce83d1cea4ef..021273e82450f0 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -795,6 +795,9 @@ void EEStartupHelper() // This must be done before any HashMap is initialized with fAsyncMode=TRUE. g_HashMapEbr.Init(); + // Initialize EBR for EEHashTable bucket reclamation. + g_EEHashEbr.Init(); + StubManager::InitializeStubManagers(); // Set up the cor handle map. This map is used to load assemblies in diff --git a/src/coreclr/vm/ebr.cpp b/src/coreclr/vm/ebr.cpp index 425d8b068556ec..87553ca40cbd55 100644 --- a/src/coreclr/vm/ebr.cpp +++ b/src/coreclr/vm/ebr.cpp @@ -67,11 +67,11 @@ struct EbrTlsDestructor final static thread_local EbrTlsDestructor t_ebrTlsDestructor; // Global EBR collector for HashMap's async mode. -// If you want to add another usage for Ebr in the future, please consider -// the tradeoffs between creating multiple collectors or treating this as -// a single shared global collector. EbrCollector g_HashMapEbr; +// Global EBR collector for EEHashTable bucket reclamation. +EbrCollector g_EEHashEbr; + // ============================================ // EbrCollector implementation // ============================================ diff --git a/src/coreclr/vm/ebr.h b/src/coreclr/vm/ebr.h index b3d7891af35cee..ed89e8bd07aff6 100644 --- a/src/coreclr/vm/ebr.h +++ b/src/coreclr/vm/ebr.h @@ -120,6 +120,9 @@ class EbrCollector final // Global EBR collector for HashMap's async mode. extern EbrCollector g_HashMapEbr; +// Global EBR collector for EEHashTable bucket reclamation. +extern EbrCollector g_EEHashEbr; + // RAII holder for EBR critical regions, analogous to GCX_COOP pattern. // When fEnable is false, the holder is a no-op. class EbrCriticalRegionHolder final diff --git a/src/coreclr/vm/eehash.cpp b/src/coreclr/vm/eehash.cpp index 4bd014e5dc3b0f..a76568903dd871 100644 --- a/src/coreclr/vm/eehash.cpp +++ b/src/coreclr/vm/eehash.cpp @@ -17,6 +17,48 @@ #ifndef DACCESS_COMPILE +// ============================================================================ +// Bucket array allocation helpers for EEHashTable. +// ============================================================================ + +// Allocate a zero-initialized bucket array with space for 'dwNumBuckets' buckets +// plus a reserved leading slot. Returns a pointer past the leading slot (matching +// the m_pBuckets convention), or NULL on failure. +EEHashEntry_t** AllocateEEHashBuckets(DWORD dwNumBuckets) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END + + DWORD dwNumBucketsPlusOne; + if (!ClrSafeInt::addition(dwNumBuckets, 1, dwNumBucketsPlusOne)) + return NULL; + + S_SIZE_T safeSize(sizeof(EEHashEntry_t*)); + safeSize *= dwNumBucketsPlusOne; + if (safeSize.IsOverflow()) + return NULL; + + SIZE_T cbAlloc = safeSize.Value(); + EEHashEntry_t** pBuckets = (EEHashEntry_t**) new (nothrow) BYTE[cbAlloc]; + if (pBuckets == NULL) + return NULL; + + memset(pBuckets, 0, cbAlloc); + + // The first slot is reserved; usable buckets start after it. + return pBuckets + 1; +} + +void FreeEEHashBuckets(EEHashEntry_t** pBuckets) +{ + LIMITED_METHOD_CONTRACT; + delete[] (BYTE*)(pBuckets - 1); +} + // ============================================================================ // Unicode string hash table helper. // ============================================================================ diff --git a/src/coreclr/vm/eehash.h b/src/coreclr/vm/eehash.h index f4e221d247faed..012944a5790dee 100644 --- a/src/coreclr/vm/eehash.h +++ b/src/coreclr/vm/eehash.h @@ -65,6 +65,10 @@ struct EEHashTableIteration; class GCHeap; +// Bucket array allocation helpers for EEHashTable. +EEHashEntry_t** AllocateEEHashBuckets(DWORD dwNumBuckets); +void FreeEEHashBuckets(EEHashEntry_t** pBuckets); + // Generic hash table. template @@ -94,7 +98,7 @@ class EEHashTableBase // A fast inlinable flavor of GetValue that can return false instead of the actual item // if there is race with updating of the hashtable. Callers of GetValueSpeculative // should fall back to the slow GetValue if GetValueSpeculative returns false. - // Assumes that we are in cooperative mode already. For performance-sensitive codepaths. + // Uses EBR internally for safe concurrent access. For performance-sensitive codepaths. BOOL GetValueSpeculative(KeyType pKey, HashDatum *pData); BOOL GetValueSpeculative(KeyType pKey, HashDatum *pData, DWORD hashValue); @@ -140,7 +144,7 @@ class EEHashTableBase // A fast inlinable flavor of FindItem that can return null instead of the actual item // if there is race with updating of the hashtable. Callers of FindItemSpeculative // should fall back to the slow FindItem if FindItemSpeculative returns null. - // Assumes that we are in cooperative mode already. For performance-sensitive codepaths. + // Uses EBR internally for safe concurrent access. For performance-sensitive codepaths. EEHashEntry_t * FindItemSpeculative(KeyType pKey, DWORD hashValue); // Double buffer to fix the race condition of growhashtable (the update diff --git a/src/coreclr/vm/eehash.inl b/src/coreclr/vm/eehash.inl index f8e4ecb6064a1d..a3ca53a8d67569 100644 --- a/src/coreclr/vm/eehash.inl +++ b/src/coreclr/vm/eehash.inl @@ -6,6 +6,17 @@ #ifndef _EE_HASH_INL #define _EE_HASH_INL +#include "ebr.h" + +#ifndef DACCESS_COMPILE +// Static helper for EBR deferred deletion of obsolete EEHash bucket arrays. +static void DeleteObsoleteEEHashBuckets(void* p) +{ + LIMITED_METHOD_CONTRACT; + FreeEEHashBuckets((EEHashEntry_t**)p); +} +#endif // DACCESS_COMPILE + #ifdef _DEBUG_IMPL template BOOL EEHashTableBase::OwnLock() @@ -56,7 +67,7 @@ void EEHashTableBase::Destroy() } } - delete[] (m_pVolatileBucketTable->m_pBuckets-1); + FreeEEHashBuckets(m_pVolatileBucketTable->m_pBuckets); m_pVolatileBucketTable = NULL; } @@ -76,15 +87,7 @@ void EEHashTableBase::ClearHashTable() //_ASSERTE (OwnLock()); - // Transition to COOP mode. This is need because EEHashTable is lock free and it can be read - // from multiple threads without taking locks. On rehash, you want to get rid of the old copy - // of table. You can only get rid of it once nobody is using it. That's a problem because - // there is no lock to tell when the last reader stopped using the old copy of the table. - // The solution to this problem is to access the table in cooperative mode, and to get rid of - // the old copy of the table when we are suspended for GC. When we are suspended for GC, - // we know that nobody is using the old copy of the table anymore. - // BROKEN: This is called sometimes from the CorMap hash before the EE is started up - GCX_COOP_NO_THREAD_BROKEN(); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -101,7 +104,7 @@ void EEHashTableBase::ClearHashTable() } } - delete[] (m_pVolatileBucketTable->m_pBuckets-1); + FreeEEHashBuckets(m_pVolatileBucketTable->m_pBuckets); m_pVolatileBucketTable->m_pBuckets = NULL; } @@ -125,15 +128,7 @@ void EEHashTableBase::EmptyHashTable() _ASSERTE (OwnLock()); - // Transition to COOP mode. This is need because EEHashTable is lock free and it can be read - // from multiple threads without taking locks. On rehash, you want to get rid of the old copy - // of table. You can only get rid of it once nobody is using it. That's a problem because - // there is no lock to tell when the last reader stopped using the old copy of the table. - // The solution to this problem is to access the table in cooperative mode, and to get rid of - // the old copy of the table when we are suspended for GC. When we are suspended for GC, - // we know that nobody is using the old copy of the table anymore. - // BROKEN: This is called sometimes from the CorMap hash before the EE is started up - GCX_COOP_NO_THREAD_BROKEN(); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -175,27 +170,11 @@ BOOL EEHashTableBase::Init(DWORD dwNumBucke m_pVolatileBucketTable = &m_BucketTable[0]; - DWORD dwNumBucketsPlusOne; - - // Prefast overflow sanity check the addition - if (!ClrSafeInt::addition(dwNumBuckets, 1, dwNumBucketsPlusOne)) - return FALSE; - - S_SIZE_T safeSize(sizeof(EEHashEntry_t *)); - safeSize *= dwNumBucketsPlusOne; - if (safeSize.IsOverflow()) - ThrowHR(COR_E_OVERFLOW); - SIZE_T cbAlloc = safeSize.Value(); - - m_pVolatileBucketTable->m_pBuckets = (EEHashEntry_t **) new (nothrow) BYTE[cbAlloc]; + m_pVolatileBucketTable->m_pBuckets = AllocateEEHashBuckets(dwNumBuckets); if (m_pVolatileBucketTable->m_pBuckets == NULL) return FALSE; - memset(m_pVolatileBucketTable->m_pBuckets, 0, cbAlloc); - - // The first slot links to the next list. - m_pVolatileBucketTable->m_pBuckets++; m_pVolatileBucketTable->m_dwNumBuckets = dwNumBuckets; #ifdef TARGET_64BIT m_pVolatileBucketTable->m_dwNumBucketsMul = dwNumBuckets == 0 ? 0 : GetFastModMultiplier(dwNumBuckets); @@ -238,15 +217,7 @@ void EEHashTableBase::InsertValue(KeyType p _ASSERTE (OwnLock()); - // Transition to COOP mode. This is need because EEHashTable is lock free and it can be read - // from multiple threads without taking locks. On rehash, you want to get rid of the old copy - // of table. You can only get rid of it once nobody is using it. That's a problem because - // there is no lock to tell when the last reader stopped using the old copy of the table. - // The solution to this problem is to access the table in cooperative mode, and to get rid of - // the old copy of the table when we are suspended for GC. When we are suspended for GC, - // we know that nobody is using the old copy of the table anymore. - // BROKEN: This is called sometimes from the CorMap hash before the EE is started up - GCX_COOP_NO_THREAD_BROKEN(); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -292,15 +263,7 @@ void EEHashTableBase::InsertKeyAsValue(KeyT _ASSERTE (OwnLock()); - // Transition to COOP mode. This is need because EEHashTable is lock free and it can be read - // from multiple threads without taking locks. On rehash, you want to get rid of the old copy - // of table. You can only get rid of it once nobody is using it. That's a problem because - // there is no lock to tell when the last reader stopped using the old copy of the table. - // The solution to this problem is to access the table in cooperative mode, and to get rid of - // the old copy of the table when we are suspended for GC. When we are suspended for GC, - // we know that nobody is using the old copy of the table anymore. - // BROKEN: This is called sometimes from the CorMap hash before the EE is started up - GCX_COOP_NO_THREAD_BROKEN(); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -345,8 +308,7 @@ BOOL EEHashTableBase::DeleteValue(KeyType p _ASSERTE (OwnLock()); - Thread *pThread = GetThreadNULLOk(); - GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread != NULL); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -494,9 +456,6 @@ FORCEINLINE BOOL EEHashTableBase::GetValueS { WRAPPER(THROWS); WRAPPER(GC_NOTRIGGER); -#ifdef MODE_COOPERATIVE // This header file sees contract.h, not eecontract.h - what a kludge! - MODE_COOPERATIVE; -#endif } CONTRACTL_END @@ -520,9 +479,6 @@ FORCEINLINE BOOL EEHashTableBase::GetValueS { WRAPPER(THROWS); WRAPPER(GC_NOTRIGGER); -#ifdef MODE_COOPERATIVE // This header file sees contract.h, not eecontract.h - what a kludge! - MODE_COOPERATIVE; -#endif } CONTRACTL_END @@ -566,16 +522,11 @@ EEHashEntry_t *EEHashTableBase::FindItem(Ke } CONTRACTL_END - // Transition to COOP mode. This is need because EEHashTable is lock free and it can be read - // from multiple threads without taking locks. On rehash, you want to get rid of the old copy - // of table. You can only get rid of it once nobody is using it. That's a problem because - // there is no lock to tell when the last reader stopped using the old copy of the table. - // The solution to this problem is to access the table in cooperative mode, and to get rid of - // the old copy of the table when we are suspended for GC. When we are suspended for GC, - // we know that nobody is using the old copy of the table anymore. + // EBR protects against use-after-free of old bucket arrays during GrowHashTable. + // Before EE starts there is only one thread, so EBR is not needed. // #ifndef DACCESS_COMPILE - GCX_COOP_NO_THREAD_BROKEN(); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); #endif // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read @@ -627,12 +578,11 @@ FORCEINLINE EEHashEntry_t *EEHashTableBase: { WRAPPER(THROWS); WRAPPER(GC_NOTRIGGER); -#ifdef MODE_COOPERATIVE // This header file sees contract.h, not eecontract.h - what a kludge! - MODE_COOPERATIVE; -#endif } CONTRACTL_END + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read // from m_pVolatileBucketTable!!!!!!! A racing condition would occur. DWORD dwOldNumBuckets; @@ -688,14 +638,12 @@ BOOL EEHashTableBase::GrowHashTable() CONTRACTL_END #if defined(_DEBUG) - Thread * pThread = GetThreadNULLOk(); - _ASSERTE(!g_fEEStarted || (pThread == NULL) || (pThread->PreemptiveGCDisabled())); + _ASSERTE(!g_fEEStarted || g_EEHashEbr.InCriticalRegion()); #endif // Make the new bucket table 4 times bigger // DWORD dwNewNumBuckets; - DWORD dwNewNumBucketsPlusOne; { S_UINT32 safeSize(m_pVolatileBucketTable->m_dwNumBuckets); @@ -705,43 +653,35 @@ BOOL EEHashTableBase::GrowHashTable() return FALSE; dwNewNumBuckets = safeSize.Value(); - - safeSize += 1; // Allocate one extra - - if (safeSize.IsOverflow()) - return FALSE; - - dwNewNumBucketsPlusOne = safeSize.Value(); } // On resizes, we still have an array of old pointers we need to worry about. // We can't free these old pointers, for we may hit a race condition where we're - // resizing and reading from the array at the same time. We need to keep track of these - // old arrays of pointers, so we're going to use the last item in the array to "link" - // to previous arrays, so that they may be freed at the end. + // resizing and reading from the array at the same time. Old bucket arrays are + // deferred for deletion via EBR (or deleted immediately before EE starts). // - SIZE_T cbAlloc; - { - S_SIZE_T safeSize(sizeof(EEHashEntry_t *)); - - safeSize *= dwNewNumBucketsPlusOne; - - if (safeSize.IsOverflow()) - return FALSE; - - cbAlloc = safeSize.Value(); - } - - EEHashEntry_t **pNewBuckets = (EEHashEntry_t **) new (nothrow) BYTE[cbAlloc]; + EEHashEntry_t **pNewBuckets = AllocateEEHashBuckets(dwNewNumBuckets); if (pNewBuckets == NULL) return FALSE; - memset(pNewBuckets, 0, cbAlloc); + EEHashEntry_t **pOldBuckets = m_pVolatileBucketTable->m_pBuckets; - // The first slot is linked to next list. - pNewBuckets++; + // Queue old bucket array for EBR deferred deletion before moving entries. + // We are in an EBR critical region (entered by caller), so the old array + // won't be freed until we (and all other readers) exit. + if (g_fEEStarted) + { + if (!g_EEHashEbr.QueueForDeletion( + pOldBuckets, + DeleteObsoleteEEHashBuckets, + (m_pVolatileBucketTable->m_dwNumBuckets + 1) * sizeof(EEHashEntry_t*))) + { + FreeEEHashBuckets(pNewBuckets); + return FALSE; + } + } // Run through the old table and transfer all the entries @@ -789,19 +729,18 @@ BOOL EEHashTableBase::GrowHashTable() pNewBucketTable->m_dwNumBucketsMul = dwNewNumBuckets == 0 ? 0 : GetFastModMultiplier(dwNewNumBuckets); #endif - // Add old table to the to free list. Note that the SyncClean thing will only - // delete the buckets at a safe point - // - SyncClean::AddEEHashTable (m_pVolatileBucketTable->m_pBuckets); - - // Note that the SyncClean:AddEEHashTable performs at least one Interlock operation - // So we do not need to use an Interlocked operation to write m_pVolatileBucketTable - // Swap the double buffer, this is an atomic operation (the assignment) - // - m_pVolatileBucketTable = pNewBucketTable; + // Publish the new bucket table to readers. The release semantics of + // VolatileStore ensure all prior writes are visible before this store. + m_pVolatileBucketTable.Store(pNewBucketTable); InterlockedExchange( (LONG *) &m_bGrowing, 0); + // Before EE starts we're single-threaded; delete old buckets immediately. + if (!g_fEEStarted) + { + FreeEEHashBuckets(pOldBuckets); + } + return TRUE; } @@ -849,8 +788,7 @@ BOOL EEHashTableBase:: _ASSERTE_IMPL(OwnLock()); - Thread *pThread = GetThreadNULLOk(); - GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread != NULL); + EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); _ASSERTE(pIter->m_pTable == (void *) this); diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index e9f701b169316c..df3ba96ffaf686 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -153,7 +153,8 @@ bool FinalizerThread::HaveExtraWorkForFinalizer() || YieldProcessorNormalization::IsMeasurementScheduled() || HasDelayedDynamicMethod() || ThreadStore::s_pThreadStore->ShouldTriggerGCForDeadThreads() - || g_HashMapEbr.CleanUpRequested(); + || g_HashMapEbr.CleanUpRequested() + || g_EEHashEbr.CleanUpRequested(); #endif // TARGET_WASM } @@ -209,6 +210,12 @@ static void DoExtraWorkForFinalizer(Thread* finalizerThread) GCX_PREEMP(); g_HashMapEbr.CleanUpPending(); } + + if (g_EEHashEbr.CleanUpRequested()) + { + GCX_PREEMP(); + g_EEHashEbr.CleanUpPending(); + } } OBJECTREF FinalizerThread::GetNextFinalizableObject() diff --git a/src/coreclr/vm/syncclean.cpp b/src/coreclr/vm/syncclean.cpp index f0d3bdbd59c165..1fa97cd589f745 100644 --- a/src/coreclr/vm/syncclean.cpp +++ b/src/coreclr/vm/syncclean.cpp @@ -12,39 +12,7 @@ #include "interpexec.h" #endif -VolatilePtr SyncClean::m_EEHashTable; - -void SyncClean::Terminate() -{ - CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - } CONTRACTL_END; - - CleanUp(); -} - -void SyncClean::AddEEHashTable (EEHashEntry** entry) -{ - WRAPPER_NO_CONTRACT; - - if (!g_fEEStarted) { - delete [] (entry-1); - return; - } - - _ASSERTE (GetThreadNULLOk() == NULL || GetThread()->PreemptiveGCDisabled()); - - EEHashEntry ** pTempHashEntry = NULL; - do - { - pTempHashEntry = (EEHashEntry**)m_EEHashTable; - entry[-1] = (EEHashEntry *)pTempHashEntry; - } - while (InterlockedCompareExchangeT(m_EEHashTable.GetPointer(), entry, pTempHashEntry) != pTempHashEntry); -} - -void SyncClean::CleanUp () +void SyncClean::CleanUp() { LIMITED_METHOD_CONTRACT; @@ -53,18 +21,6 @@ void SyncClean::CleanUp () IsGCSpecialThread() || (GCHeapUtilities::IsGCInProgress() && GetThreadNULLOk() == ThreadSuspend::GetSuspensionThread())); - if (m_EEHashTable) - { - EEHashEntry ** pTempHashEntry = InterlockedExchangeT(m_EEHashTable.GetPointer(), NULL); - - while (pTempHashEntry) { - EEHashEntry **pNextHashEntry = (EEHashEntry **)pTempHashEntry[-1]; - pTempHashEntry --; - delete [] pTempHashEntry; - pTempHashEntry = pNextHashEntry; - } - } - // Give others we want to reclaim during the GC sync point a chance to do it VirtualCallStubManager::ReclaimAll(); diff --git a/src/coreclr/vm/syncclean.hpp b/src/coreclr/vm/syncclean.hpp index 7b8761c76c765b..d0cc575938b3cf 100644 --- a/src/coreclr/vm/syncclean.hpp +++ b/src/coreclr/vm/syncclean.hpp @@ -5,21 +5,9 @@ #ifndef _SYNCCLEAN_HPP_ #define _SYNCCLEAN_HPP_ -// We keep a list of memory blocks to be freed at the end of GC, but before we resume EE. -// To make this work, we need to make sure that these data are accessed in cooperative GC -// mode. - -struct EEHashEntry; - class SyncClean final { public: - static void Terminate (); - - static void AddEEHashTable (EEHashEntry** entry); - static void CleanUp (); - -private: - static VolatilePtr m_EEHashTable; // Cleanup list for EEHashTable + static void CleanUp(); }; #endif diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index dafb078348c34a..8923aa33023c5a 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5339,12 +5339,9 @@ void ThreadSuspend::RestartEE(BOOL bFinishedGC, BOOL SuspendSucceeded) #endif //TARGET_ARM || TARGET_ARM64 // - // SyncClean holds a list of things to be cleaned up when it's possible. - // SyncClean uses the GC mode to synchronize access to this list. Threads must be - // in COOP mode to add things to the list, and the list can only be cleaned up - // while no threads are adding things. - // Since we know that no threads are in COOP mode at this point (because the EE is - // suspended), we clean up the list here. + // SyncClean::CleanUp reclaims resources that are safe to free only + // when no threads are running managed code. Since the EE is + // suspended at this point, we know it's safe to clean up here. // SyncClean::CleanUp(); From c6e39a834ec830043a5a8f6748382e4340d2b0ad Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 25 Feb 2026 11:54:29 -0800 Subject: [PATCH 2/5] Consolidate g_HashMapEbr and g_EEHashEbr into single g_EbrCollector The per-thread EbrThreadData is a single thread_local with one m_pCollector field, so it can only be associated with one collector at a time. Having two collectors caused the same TLS node to be linked into both thread lists, corrupting epoch tracking and thread detach cleanup. Both HashMap and EEHashTable use generic byte-array reclamation, so a single shared collector is sufficient. Added an assertion in GetOrCreateThreadData to guard against future reintroduction of multiple collectors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/ceemain.cpp | 7 ++----- src/coreclr/vm/ebr.cpp | 12 +++++++----- src/coreclr/vm/ebr.h | 15 +++++++-------- src/coreclr/vm/eehash.inl | 20 ++++++++++---------- src/coreclr/vm/finalizerthread.cpp | 13 +++---------- src/coreclr/vm/hash.cpp | 18 +++++++++--------- 6 files changed, 38 insertions(+), 47 deletions(-) diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 021273e82450f0..086e8a5e14e57d 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -791,12 +791,9 @@ void EEStartupHelper() // Cache the (potentially user-overridden) values now so they are accessible from asm routines InitializeSpinConstants(); - // Initialize EBR (Epoch-Based Reclamation) for HashMap's async mode. + // Initialize EBR (Epoch-Based Reclamation) for safe deferred deletion. // This must be done before any HashMap is initialized with fAsyncMode=TRUE. - g_HashMapEbr.Init(); - - // Initialize EBR for EEHashTable bucket reclamation. - g_EEHashEbr.Init(); + g_EbrCollector.Init(); StubManager::InitializeStubManagers(); diff --git a/src/coreclr/vm/ebr.cpp b/src/coreclr/vm/ebr.cpp index 87553ca40cbd55..9d4ea4976b908f 100644 --- a/src/coreclr/vm/ebr.cpp +++ b/src/coreclr/vm/ebr.cpp @@ -66,11 +66,12 @@ struct EbrTlsDestructor final }; static thread_local EbrTlsDestructor t_ebrTlsDestructor; -// Global EBR collector for HashMap's async mode. -EbrCollector g_HashMapEbr; - -// Global EBR collector for EEHashTable bucket reclamation. -EbrCollector g_EEHashEbr; +// Global EBR collector for safe deferred deletion of memory that may be +// concurrently accessed by reader threads. +// +// A single collector is used because each thread has a single thread-local +// EbrThreadData that can only be associated with one collector at a time. +EbrCollector g_EbrCollector; // ============================================ // EbrCollector implementation @@ -141,6 +142,7 @@ EbrCollector::GetOrCreateThreadData() return pData; _ASSERTE_ALL_BUILDS(pData->m_pCollector != DetachedCollector && "Attempt to reattach detached thread."); + _ASSERTE_ALL_BUILDS(pData->m_pCollector == nullptr && "Thread already registered with a different collector."); pData->m_pCollector = this; diff --git a/src/coreclr/vm/ebr.h b/src/coreclr/vm/ebr.h index ed89e8bd07aff6..840833924e2cbd 100644 --- a/src/coreclr/vm/ebr.h +++ b/src/coreclr/vm/ebr.h @@ -10,18 +10,18 @@ // // Usage: // // Startup: -// g_HashMapEbr.Init(); +// g_EbrCollector.Init(); // // // Reader/Writer thread: // { -// EbrCriticalRegionHolder ebr(&g_HashMapEbr, m_fAsyncMode); +// EbrCriticalRegionHolder ebr(&g_EbrCollector, fAsyncMode); // // ... access shared data safely ... // // Objects queued for deletion will not be freed while any thread // // is in a critical region that observed a prior epoch. // } // // // Writer thread (inside critical region), after replacing shared pointer: -// g_HashMapEbr.QueueForDeletion(pOldData, deleteFn, sizeEstimate); +// g_EbrCollector.QueueForDeletion(pOldData, deleteFn, sizeEstimate); // // // Shutdown: // The EBR collector doesn't support a shutdown feature. CoreCLR doesn't support @@ -117,11 +117,10 @@ class EbrCollector final Volatile m_pendingSizeInBytes; }; -// Global EBR collector for HashMap's async mode. -extern EbrCollector g_HashMapEbr; - -// Global EBR collector for EEHashTable bucket reclamation. -extern EbrCollector g_EEHashEbr; +// Global EBR collector for safe deferred deletion of memory that may be +// concurrently accessed by reader threads (e.g. HashMap and EEHashTable +// bucket arrays during resize). +extern EbrCollector g_EbrCollector; // RAII holder for EBR critical regions, analogous to GCX_COOP pattern. // When fEnable is false, the holder is a no-op. diff --git a/src/coreclr/vm/eehash.inl b/src/coreclr/vm/eehash.inl index a3ca53a8d67569..6c51880c8ceadc 100644 --- a/src/coreclr/vm/eehash.inl +++ b/src/coreclr/vm/eehash.inl @@ -87,7 +87,7 @@ void EEHashTableBase::ClearHashTable() //_ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -128,7 +128,7 @@ void EEHashTableBase::EmptyHashTable() _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -217,7 +217,7 @@ void EEHashTableBase::InsertValue(KeyType p _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -263,7 +263,7 @@ void EEHashTableBase::InsertKeyAsValue(KeyT _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -308,7 +308,7 @@ BOOL EEHashTableBase::DeleteValue(KeyType p _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -526,7 +526,7 @@ EEHashEntry_t *EEHashTableBase::FindItem(Ke // Before EE starts there is only one thread, so EBR is not needed. // #ifndef DACCESS_COMPILE - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); #endif // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read @@ -581,7 +581,7 @@ FORCEINLINE EEHashEntry_t *EEHashTableBase: } CONTRACTL_END - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read // from m_pVolatileBucketTable!!!!!!! A racing condition would occur. @@ -638,7 +638,7 @@ BOOL EEHashTableBase::GrowHashTable() CONTRACTL_END #if defined(_DEBUG) - _ASSERTE(!g_fEEStarted || g_EEHashEbr.InCriticalRegion()); + _ASSERTE(!g_fEEStarted || g_EbrCollector.InCriticalRegion()); #endif // Make the new bucket table 4 times bigger @@ -673,7 +673,7 @@ BOOL EEHashTableBase::GrowHashTable() // won't be freed until we (and all other readers) exit. if (g_fEEStarted) { - if (!g_EEHashEbr.QueueForDeletion( + if (!g_EbrCollector.QueueForDeletion( pOldBuckets, DeleteObsoleteEEHashBuckets, (m_pVolatileBucketTable->m_dwNumBuckets + 1) * sizeof(EEHashEntry_t*))) @@ -788,7 +788,7 @@ BOOL EEHashTableBase:: _ASSERTE_IMPL(OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EEHashEbr, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); _ASSERTE(pIter->m_pTable == (void *) this); diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index df3ba96ffaf686..64c7220d15ed56 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -153,8 +153,7 @@ bool FinalizerThread::HaveExtraWorkForFinalizer() || YieldProcessorNormalization::IsMeasurementScheduled() || HasDelayedDynamicMethod() || ThreadStore::s_pThreadStore->ShouldTriggerGCForDeadThreads() - || g_HashMapEbr.CleanUpRequested() - || g_EEHashEbr.CleanUpRequested(); + || g_EbrCollector.CleanUpRequested(); #endif // TARGET_WASM } @@ -205,16 +204,10 @@ static void DoExtraWorkForFinalizer(Thread* finalizerThread) CleanupDelayedDynamicMethods(); } - if (g_HashMapEbr.CleanUpRequested()) + if (g_EbrCollector.CleanUpRequested()) { GCX_PREEMP(); - g_HashMapEbr.CleanUpPending(); - } - - if (g_EEHashEbr.CleanUpRequested()) - { - GCX_PREEMP(); - g_EEHashEbr.CleanUpPending(); + g_EbrCollector.CleanUpPending(); } } diff --git a/src/coreclr/vm/hash.cpp b/src/coreclr/vm/hash.cpp index 4c565da2b6f78a..87015efd33275a 100644 --- a/src/coreclr/vm/hash.cpp +++ b/src/coreclr/vm/hash.cpp @@ -153,7 +153,7 @@ PTR_Bucket HashMap::Buckets() LIMITED_METHOD_DAC_CONTRACT; #if !defined(DACCESS_COMPILE) - _ASSERTE (!m_fAsyncMode || g_HashMapEbr.InCriticalRegion()); + _ASSERTE (!m_fAsyncMode || g_EbrCollector.InCriticalRegion()); #endif return GetBucketPointer(m_rgBuckets); } @@ -503,7 +503,7 @@ void HashMap::InsertValue (UPTR key, UPTR value) // Enter EBR critical region to protect against concurrent bucket array // deletion during async mode. - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); ASSERT(m_rgBuckets != NULL); @@ -567,7 +567,7 @@ UPTR HashMap::LookupValue(UPTR key, UPTR value) #ifndef DACCESS_COMPILE _ASSERTE (m_fAsyncMode || OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); ASSERT(m_rgBuckets != NULL); // This is necessary in case some other thread @@ -639,7 +639,7 @@ UPTR HashMap::ReplaceValue(UPTR key, UPTR value) _ASSERTE(OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); ASSERT(m_rgBuckets != NULL); // This is necessary in case some other thread @@ -712,7 +712,7 @@ UPTR HashMap::DeleteValue (UPTR key, UPTR value) _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); // check proper use in synchronous mode SyncAccessHolder holoder(this); //no-op in non DEBUG code @@ -883,9 +883,9 @@ void HashMap::Rehash() STATIC_CONTRACT_GC_NOTRIGGER; STATIC_CONTRACT_FAULT; - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); - _ASSERTE (!m_fAsyncMode || g_HashMapEbr.InCriticalRegion()); + _ASSERTE (!m_fAsyncMode || g_EbrCollector.InCriticalRegion()); _ASSERTE (OwnLock()); UPTR newPrimeIndex = NewSize(); @@ -943,7 +943,7 @@ void HashMap::Rehash() // all threads have exited their critical regions or later. // If we fail to queue for deletion, throw an OOM. size_t obsoleteSize = currentBucketsSize; - if (!g_HashMapEbr.QueueForDeletion( + if (!g_EbrCollector.QueueForDeletion( pObsoleteBucketsAlloc, DeleteObsoleteBuckets, (obsoleteSize + 1) * sizeof(Bucket))) // See AllocateBuckets for +1 @@ -1036,7 +1036,7 @@ void HashMap::Compact() _ASSERTE (OwnLock()); // - EbrCriticalRegionHolder ebrHolder(&g_HashMapEbr, m_fAsyncMode); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, m_fAsyncMode); ASSERT(m_rgBuckets != NULL); // Try to resize if that makes sense (reduce the size of the table), but From a0a3d70de6f9e0f13868255dd26cd961d0a49cee Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 25 Feb 2026 18:07:05 -0800 Subject: [PATCH 3/5] Add dedicated CrstAsyncContinuations lock type Replace CrstLeafLock usage in AsyncContinuationsManager with a dedicated CrstAsyncContinuations crst type that has proper lock ordering (AcquiredBefore LeafLock). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/inc/CrstTypes.def | 4 + src/coreclr/inc/crsttypes_generated.h | 205 +++++++++++++------------- src/coreclr/vm/asynccontinuations.cpp | 2 +- 3 files changed, 109 insertions(+), 102 deletions(-) diff --git a/src/coreclr/inc/CrstTypes.def b/src/coreclr/inc/CrstTypes.def index e3ae5275951f87..c9798ea5ebe084 100644 --- a/src/coreclr/inc/CrstTypes.def +++ b/src/coreclr/inc/CrstTypes.def @@ -86,6 +86,10 @@ Crst AssemblyLoader AcquiredBefore DeadlockDetection UniqueStack DebuggerMutex End +Crst AsyncContinuations + AcquiredBefore LeafLock +End + Crst AvailableClass AcquiredBefore LoaderHeap End diff --git a/src/coreclr/inc/crsttypes_generated.h b/src/coreclr/inc/crsttypes_generated.h index 28eb23c9e267d4..afab5353f7d97a 100644 --- a/src/coreclr/inc/crsttypes_generated.h +++ b/src/coreclr/inc/crsttypes_generated.h @@ -18,107 +18,108 @@ enum CrstType CrstAppDomainCache = 0, CrstAssemblyList = 1, CrstAssemblyLoader = 2, - CrstAvailableClass = 3, - CrstAvailableParamTypes = 4, - CrstCallStubCache = 5, - CrstCCompRC = 6, - CrstClassFactInfoHash = 7, - CrstClassInit = 8, - CrstClrNotification = 9, - CrstCodeFragmentHeap = 10, - CrstCodeVersioning = 11, - CrstCOMCallWrapper = 12, - CrstCOMWrapperCache = 13, - CrstDataTest1 = 14, - CrstDataTest2 = 15, - CrstDbgTransport = 16, - CrstDeadlockDetection = 17, - CrstDebuggerController = 18, - CrstDebuggerFavorLock = 19, - CrstDebuggerHeapExecMemLock = 20, - CrstDebuggerHeapLock = 21, - CrstDebuggerJitInfo = 22, - CrstDebuggerMutex = 23, - CrstDynamicIL = 24, - CrstDynamicMT = 25, - CrstEtwTypeLogHash = 26, - CrstEventPipe = 27, - CrstEventStore = 28, - CrstException = 29, - CrstExecutableAllocatorLock = 30, - CrstFCall = 31, - CrstFrozenObjectHeap = 32, - CrstFuncPtrStubs = 33, - CrstFusionAppCtx = 34, - CrstGCCover = 35, - CrstGenericDictionaryExpansion = 36, - CrstGlobalStrLiteralMap = 37, - CrstHandleTable = 38, - CrstIJWFixupData = 39, - CrstIJWHash = 40, - CrstILStubGen = 41, - CrstInlineTrackingMap = 42, - CrstInstMethodHashTable = 43, - CrstInterfaceDispatchGlobalLists = 44, - CrstInterop = 45, - CrstInteropData = 46, - CrstIsJMCMethod = 47, - CrstISymUnmanagedReader = 48, - CrstJit = 49, - CrstJitInlineTrackingMap = 50, - CrstJitPatchpoint = 51, - CrstJumpStubCache = 52, - CrstLeafLock = 53, - CrstListLock = 54, - CrstLoaderAllocator = 55, - CrstLoaderAllocatorReferences = 56, - CrstLoaderHeap = 57, - CrstManagedObjectWrapperMap = 58, - CrstMethodDescBackpatchInfoTracker = 59, - CrstMethodTableExposedObject = 60, - CrstModule = 61, - CrstModuleLookupTable = 62, - CrstMulticoreJitHash = 63, - CrstMulticoreJitManager = 64, - CrstNativeImageEagerFixups = 65, - CrstNativeImageLoad = 66, - CrstNotifyGdb = 67, - CrstPEImage = 68, - CrstPendingTypeLoadEntry = 69, - CrstPerfMap = 70, - CrstPgoData = 71, - CrstPinnedByrefValidation = 72, - CrstPinnedHeapHandleTable = 73, - CrstProfilerGCRefDataFreeList = 74, - CrstProfilingAPIStatus = 75, - CrstRCWCache = 76, - CrstRCWCleanupList = 77, - CrstReadyToRunEntryPointToMethodDescMap = 78, - CrstReflection = 79, - CrstReJITGlobalRequest = 80, - CrstSigConvert = 81, - CrstSingleUseLock = 82, - CrstStressLog = 83, - CrstStubCache = 84, - CrstStubDispatchCache = 85, - CrstSyncBlockCache = 86, - CrstSyncHashLock = 87, - CrstSystemDomain = 88, - CrstSystemDomainDelayedUnloadList = 89, - CrstThreadIdDispenser = 90, - CrstThreadLocalStorageLock = 91, - CrstThreadStore = 92, - CrstTieredCompilation = 93, - CrstTypeEquivalenceMap = 94, - CrstTypeIDMap = 95, - CrstUMEntryThunkCache = 96, - CrstUMEntryThunkFreeListLock = 97, - CrstUniqueStack = 98, - CrstUnresolvedClassLock = 99, - CrstUnwindInfoTableLock = 100, - CrstVSDIndirectionCellLock = 101, - CrstWrapperTemplate = 102, - kNumberOfCrstTypes = 103 + CrstAsyncContinuations = 3, + CrstAvailableClass = 4, + CrstAvailableParamTypes = 5, + CrstCallStubCache = 6, + CrstCCompRC = 7, + CrstClassFactInfoHash = 8, + CrstClassInit = 9, + CrstClrNotification = 10, + CrstCodeFragmentHeap = 11, + CrstCodeVersioning = 12, + CrstCOMCallWrapper = 13, + CrstCOMWrapperCache = 14, + CrstDataTest1 = 15, + CrstDataTest2 = 16, + CrstDbgTransport = 17, + CrstDeadlockDetection = 18, + CrstDebuggerController = 19, + CrstDebuggerFavorLock = 20, + CrstDebuggerHeapExecMemLock = 21, + CrstDebuggerHeapLock = 22, + CrstDebuggerJitInfo = 23, + CrstDebuggerMutex = 24, + CrstDynamicIL = 25, + CrstDynamicMT = 26, + CrstEtwTypeLogHash = 27, + CrstEventPipe = 28, + CrstEventStore = 29, + CrstException = 30, + CrstExecutableAllocatorLock = 31, + CrstFCall = 32, + CrstFrozenObjectHeap = 33, + CrstFuncPtrStubs = 34, + CrstFusionAppCtx = 35, + CrstGCCover = 36, + CrstGenericDictionaryExpansion = 37, + CrstGlobalStrLiteralMap = 38, + CrstHandleTable = 39, + CrstIJWFixupData = 40, + CrstIJWHash = 41, + CrstILStubGen = 42, + CrstInlineTrackingMap = 43, + CrstInstMethodHashTable = 44, + CrstInterfaceDispatchGlobalLists = 45, + CrstInterop = 46, + CrstInteropData = 47, + CrstIsJMCMethod = 48, + CrstISymUnmanagedReader = 49, + CrstJit = 50, + CrstJitInlineTrackingMap = 51, + CrstJitPatchpoint = 52, + CrstJumpStubCache = 53, + CrstLeafLock = 54, + CrstListLock = 55, + CrstLoaderAllocator = 56, + CrstLoaderAllocatorReferences = 57, + CrstLoaderHeap = 58, + CrstManagedObjectWrapperMap = 59, + CrstMethodDescBackpatchInfoTracker = 60, + CrstMethodTableExposedObject = 61, + CrstModule = 62, + CrstModuleLookupTable = 63, + CrstMulticoreJitHash = 64, + CrstMulticoreJitManager = 65, + CrstNativeImageEagerFixups = 66, + CrstNativeImageLoad = 67, + CrstNotifyGdb = 68, + CrstPEImage = 69, + CrstPendingTypeLoadEntry = 70, + CrstPerfMap = 71, + CrstPgoData = 72, + CrstPinnedByrefValidation = 73, + CrstPinnedHeapHandleTable = 74, + CrstProfilerGCRefDataFreeList = 75, + CrstProfilingAPIStatus = 76, + CrstRCWCache = 77, + CrstRCWCleanupList = 78, + CrstReadyToRunEntryPointToMethodDescMap = 79, + CrstReflection = 80, + CrstReJITGlobalRequest = 81, + CrstSigConvert = 82, + CrstSingleUseLock = 83, + CrstStressLog = 84, + CrstStubCache = 85, + CrstStubDispatchCache = 86, + CrstSyncBlockCache = 87, + CrstSyncHashLock = 88, + CrstSystemDomain = 89, + CrstSystemDomainDelayedUnloadList = 90, + CrstThreadIdDispenser = 91, + CrstThreadLocalStorageLock = 92, + CrstThreadStore = 93, + CrstTieredCompilation = 94, + CrstTypeEquivalenceMap = 95, + CrstTypeIDMap = 96, + CrstUMEntryThunkCache = 97, + CrstUMEntryThunkFreeListLock = 98, + CrstUniqueStack = 99, + CrstUnresolvedClassLock = 100, + CrstUnwindInfoTableLock = 101, + CrstVSDIndirectionCellLock = 102, + CrstWrapperTemplate = 103, + kNumberOfCrstTypes = 104 }; #endif // __CRST_TYPES_INCLUDED @@ -132,6 +133,7 @@ int g_rgCrstLevelMap[] = 9, // CrstAppDomainCache 2, // CrstAssemblyList 13, // CrstAssemblyLoader + 2, // CrstAsyncContinuations 3, // CrstAvailableClass 4, // CrstAvailableParamTypes 3, // CrstCallStubCache @@ -240,6 +242,7 @@ LPCSTR g_rgCrstNameMap[] = "CrstAppDomainCache", "CrstAssemblyList", "CrstAssemblyLoader", + "CrstAsyncContinuations", "CrstAvailableClass", "CrstAvailableParamTypes", "CrstCallStubCache", diff --git a/src/coreclr/vm/asynccontinuations.cpp b/src/coreclr/vm/asynccontinuations.cpp index f55e276c402709..ed5a7a51651fa7 100644 --- a/src/coreclr/vm/asynccontinuations.cpp +++ b/src/coreclr/vm/asynccontinuations.cpp @@ -19,7 +19,7 @@ AsyncContinuationsManager::AsyncContinuationsManager(LoaderAllocator* allocator) { LIMITED_METHOD_CONTRACT; - m_layoutsLock.Init(CrstLeafLock); + m_layoutsLock.Init(CrstAsyncContinuations); LockOwner lock = {&m_layoutsLock, IsOwnerOfCrst}; m_layouts.Init(16, &lock, m_allocator->GetLowFrequencyHeap()); } From e8aca6d71672b6073ec36956b3d218f5bba064a9 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 26 Feb 2026 12:25:44 -0800 Subject: [PATCH 4/5] Always use EBR for EEHashTable bucket reclamation Remove the g_fEEStarted conditional that immediately freed old bucket arrays during startup. Other threads (finalizer, debugger, tracing) are created before g_fEEStarted is set, so the single-threaded assumption was incorrect. Now always queue old buckets via EBR deferred deletion and always enter EBR critical regions, matching the safety guarantees that HashMap provides for async mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/eehash.inl | 43 +++++++++++++++------------------------ 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/src/coreclr/vm/eehash.inl b/src/coreclr/vm/eehash.inl index 6c51880c8ceadc..f5c5fa424eec2f 100644 --- a/src/coreclr/vm/eehash.inl +++ b/src/coreclr/vm/eehash.inl @@ -87,7 +87,7 @@ void EEHashTableBase::ClearHashTable() //_ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -128,7 +128,7 @@ void EEHashTableBase::EmptyHashTable() _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); if (m_pVolatileBucketTable->m_pBuckets != NULL) { @@ -217,7 +217,7 @@ void EEHashTableBase::InsertValue(KeyType p _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -263,7 +263,7 @@ void EEHashTableBase::InsertKeyAsValue(KeyT _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -308,7 +308,7 @@ BOOL EEHashTableBase::DeleteValue(KeyType p _ASSERTE (OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); _ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0); @@ -523,10 +523,8 @@ EEHashEntry_t *EEHashTableBase::FindItem(Ke CONTRACTL_END // EBR protects against use-after-free of old bucket arrays during GrowHashTable. - // Before EE starts there is only one thread, so EBR is not needed. - // #ifndef DACCESS_COMPILE - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); #endif // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read @@ -581,7 +579,7 @@ FORCEINLINE EEHashEntry_t *EEHashTableBase: } CONTRACTL_END - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read // from m_pVolatileBucketTable!!!!!!! A racing condition would occur. @@ -638,7 +636,7 @@ BOOL EEHashTableBase::GrowHashTable() CONTRACTL_END #if defined(_DEBUG) - _ASSERTE(!g_fEEStarted || g_EbrCollector.InCriticalRegion()); + _ASSERTE(g_EbrCollector.InCriticalRegion()); #endif // Make the new bucket table 4 times bigger @@ -658,7 +656,7 @@ BOOL EEHashTableBase::GrowHashTable() // On resizes, we still have an array of old pointers we need to worry about. // We can't free these old pointers, for we may hit a race condition where we're // resizing and reading from the array at the same time. Old bucket arrays are - // deferred for deletion via EBR (or deleted immediately before EE starts). + // deferred for deletion via EBR. // EEHashEntry_t **pNewBuckets = AllocateEEHashBuckets(dwNewNumBuckets); @@ -671,16 +669,13 @@ BOOL EEHashTableBase::GrowHashTable() // Queue old bucket array for EBR deferred deletion before moving entries. // We are in an EBR critical region (entered by caller), so the old array // won't be freed until we (and all other readers) exit. - if (g_fEEStarted) + if (!g_EbrCollector.QueueForDeletion( + pOldBuckets, + DeleteObsoleteEEHashBuckets, + (m_pVolatileBucketTable->m_dwNumBuckets + 1) * sizeof(EEHashEntry_t*))) { - if (!g_EbrCollector.QueueForDeletion( - pOldBuckets, - DeleteObsoleteEEHashBuckets, - (m_pVolatileBucketTable->m_dwNumBuckets + 1) * sizeof(EEHashEntry_t*))) - { - FreeEEHashBuckets(pNewBuckets); - return FALSE; - } + FreeEEHashBuckets(pNewBuckets); + return FALSE; } // Run through the old table and transfer all the entries @@ -735,12 +730,6 @@ BOOL EEHashTableBase::GrowHashTable() InterlockedExchange( (LONG *) &m_bGrowing, 0); - // Before EE starts we're single-threaded; delete old buckets immediately. - if (!g_fEEStarted) - { - FreeEEHashBuckets(pOldBuckets); - } - return TRUE; } @@ -788,7 +777,7 @@ BOOL EEHashTableBase:: _ASSERTE_IMPL(OwnLock()); - EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, g_fEEStarted); + EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); _ASSERTE(pIter->m_pTable == (void *) this); From 02601cb1dc08852afa97758fc410d5eece253127 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 26 Feb 2026 13:35:11 -0800 Subject: [PATCH 5/5] Remove exposure of the EBR holder from dac builds. --- src/coreclr/vm/ebr.h | 2 ++ src/coreclr/vm/eehash.inl | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/ebr.h b/src/coreclr/vm/ebr.h index 840833924e2cbd..0ec2677ff05b1f 100644 --- a/src/coreclr/vm/ebr.h +++ b/src/coreclr/vm/ebr.h @@ -122,6 +122,7 @@ class EbrCollector final // bucket arrays during resize). extern EbrCollector g_EbrCollector; +#ifndef DACCESS_COMPILE // RAII holder for EBR critical regions, analogous to GCX_COOP pattern. // When fEnable is false, the holder is a no-op. class EbrCriticalRegionHolder final @@ -150,5 +151,6 @@ class EbrCriticalRegionHolder final private: EbrCollector* m_pCollector; }; +#endif // !DACCESS_COMPILE #endif // __EBR_H__ diff --git a/src/coreclr/vm/eehash.inl b/src/coreclr/vm/eehash.inl index f5c5fa424eec2f..24cf79cede12d5 100644 --- a/src/coreclr/vm/eehash.inl +++ b/src/coreclr/vm/eehash.inl @@ -525,7 +525,7 @@ EEHashEntry_t *EEHashTableBase::FindItem(Ke // EBR protects against use-after-free of old bucket arrays during GrowHashTable. #ifndef DACCESS_COMPILE EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); -#endif +#endif // !DACCESS_COMPILE // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read // from m_pVolatileBucketTable!!!!!!! A racing condition would occur. @@ -579,7 +579,9 @@ FORCEINLINE EEHashEntry_t *EEHashTableBase: } CONTRACTL_END +#ifndef DACCESS_COMPILE EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); +#endif // !DACCESS_COMPILE // Atomic transaction. In any other point of this method or ANY of the callees of this function you can not read // from m_pVolatileBucketTable!!!!!!! A racing condition would occur. @@ -777,7 +779,9 @@ BOOL EEHashTableBase:: _ASSERTE_IMPL(OwnLock()); +#ifndef DACCESS_COMPILE EbrCriticalRegionHolder ebrHolder(&g_EbrCollector, /* fEnable */ true); +#endif // !DACCESS_COMPILE _ASSERTE(pIter->m_pTable == (void *) this);