Replace GCX_COOP with EBR for EEHashTable bucket reclamation#124822
Replace GCX_COOP with EBR for EEHashTable bucket reclamation#124822AaronRobinsonMSFT wants to merge 3 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR replaces the prior GC cooperative-mode based scheme for safely reclaiming obsolete EEHashTable bucket arrays with Epoch-Based Reclamation (EBR), aligning EEHashTable resizing behavior with the earlier HashMap EBR work.
Changes:
- Add a dedicated global EBR collector (
g_EEHashEbr) and initialize it during EE startup; trigger cleanup from the finalizer thread’s “extra work” loop. - Refactor EEHashTable bucket allocation/freeing into helpers and use EBR critical regions + deferred deletion during table growth instead of
GCX_COOP_NO_THREAD_BROKEN+SyncClean. - Simplify
SyncCleanby removing the EEHashTable-specific cleanup list and related APIs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/syncclean.hpp | Removes EEHashTable cleanup list API; leaves SyncClean::CleanUp() only. |
| src/coreclr/vm/syncclean.cpp | Removes EEHashTable cleanup list storage and freeing logic. |
| src/coreclr/vm/finalizerthread.cpp | Adds g_EEHashEbr cleanup request detection and processing in finalizer extra-work path. |
| src/coreclr/vm/eehash.inl | Replaces COOP transitions with EBR critical regions; defers old bucket array deletion via EBR during growth. |
| src/coreclr/vm/eehash.h | Declares EEHashTable bucket allocation/free helpers. |
| src/coreclr/vm/eehash.cpp | Implements AllocateEEHashBuckets / FreeEEHashBuckets. |
| src/coreclr/vm/ebr.h | Declares extern EbrCollector g_EEHashEbr. |
| src/coreclr/vm/ebr.cpp | Defines global g_EEHashEbr. |
| src/coreclr/vm/ceemain.cpp | Initializes g_EEHashEbr during EE startup. |
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 <[email protected]>
AaronRobinsonMSFT
force-pushed
the
ebr-eehash
branch
from
b4f6cb0 to
c8e7d2b
Compare
|
|
||
| InterlockedExchange( (LONG *) &m_bGrowing, 0); | ||
|
|
||
| // Before EE starts we're single-threaded; delete old buckets immediately. |
There was a problem hiding this comment.
Is this a correct statement? We do create finalizer thread, debugger threads, tracing threads, ... before g_fEEStarted is set to true. Is it guaranteed that none of those threads cross the path with this?
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 <[email protected]>
Replace CrstLeafLock usage in AsyncContinuationsManager with a dedicated CrstAsyncContinuations crst type that has proper lock ordering (AcquiredBefore LeafLock). Co-authored-by: Copilot <[email protected]>
| // Before EE starts we're single-threaded; delete old buckets immediately. | ||
| if (!g_fEEStarted) | ||
| { | ||
| FreeEEHashBuckets(pOldBuckets); | ||
| } |
There was a problem hiding this comment.
The comment and logic assume single-threaded execution before g_fEEStarted, but the finalizer thread, debugger threads, and other system threads are created before g_fEEStarted is set to TRUE (see ceemain.cpp lines 871, 940, 854). If these threads could access EEHashTable instances during this period, the immediate deletion of old buckets here would be unsafe.
This pattern was inherited from the old SyncClean::AddEEHashTable implementation, but given that EEHashTable always allows lock-free reads (unlike HashMap which has an m_fAsyncMode flag), it may be safer to always use EBR for deferred deletion rather than conditional logic based on g_fEEStarted.
Consider either: (1) always queueing for EBR deletion regardless of g_fEEStarted, or (2) adding documentation/assertions that EEHashTable operations cannot occur from multiple threads before g_fEEStarted.
| // Before EE starts we're single-threaded; delete old buckets immediately. | |
| if (!g_fEEStarted) | |
| { | |
| FreeEEHashBuckets(pOldBuckets); | |
| } | |
| // Always retire old buckets via EBR to ensure safety for lock-free readers. | |
| EBR::DeleteLater(DeleteObsoleteEEHashBuckets, pOldBuckets); |
Replace cooperative GC mode transitions (
GCX_COOP_NO_THREAD_BROKEN) with Epoch-Based Reclamation for safe deferred deletion of oldEEHashTablebucket arrays during resize.Changes
g_EEHashEbrcollector (separate fromHashMap'sg_HashMapEbr), with initialization inceemain.cppand cleanup inf inalizerthread.cppAllocateEEHashBucketsFreeEEHashBucketshelpers ineehash.cpp, following the same pattern asHashMap'sAllocateBucketsFreeBucketsSyncClean::AddEEHashTableand associated cleanup infrastructure fromsyncclean.cpp/.hppMemoryBarrier()beforeVolatilePtr::Store()— release semantics already provide the ordering guaranteeFollow-up to #124307 (Replace
HashMapCOOP transitions with EBR). Specifically see #124307 (comment).