Make CastCache more multi-thread friendly - #132250
Conversation
The rotating victim counter used when a bucket is full lives in the table's aux data (element 0), in the same cache line that every lookup reads hashShift/tableMask from. An inserting thread does an ordinary RMW on it, so it invalidates that line on every core doing casts. Move it to a static, which makes the aux data read-only and leaves the line Shared in every reader's cache. Also: - MaybeReplaceCacheWithLarger: bail out if another thread already grew the table. Without this, every thread that finds a full bucket allocates its own table (up to 98KB, so LOH) and all but the last are discarded along with their entries, and a thread working off a stale table can publish one smaller than the current. - TrySet: read the version with Volatile.Read before the CompareExchange, matching what the native writer already does deliberately. Same fixes in GenericCache, which has the same layout (there hashShift and victimCounter share a 4-byte word). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f97a178a-0878-4c31-b85e-39320f4a8a0c
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the runtime’s cast cache (CastCache) and generic virtual dispatch cache (GenericCache) to reduce cross-thread cache-line contention during concurrent reads/writes, primarily by moving the “victim counter” out of per-table aux data and tightening a couple of multi-threading behaviors in the grow/claim paths.
Changes:
- Move the rotating victim counter from table aux data into a static to avoid false sharing with frequently-read table metadata.
- Add an early-out in
MaybeReplaceCacheWithLargerto avoid allocating/replacing when another thread already grew the table. - Use
Volatile.Readfor version reads immediately precedingCompareExchangein managed writers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericCache.cs | Moves victim counter out of aux data; adds grow early-out; uses Volatile.Read for version claim reads. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs | Moves victim counter out of aux data; adds grow early-out; uses Volatile.Read for version claim reads. |
| src/coreclr/vm/castcache.h | Removes victim counter from aux data; introduces static victim counter. |
| src/coreclr/vm/castcache.cpp | Initializes static victim counter; adds grow early-out; uses static victim counter for victim selection. |
| // pick a victim somewhat randomly within a bucket | ||
| // NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes. | ||
| DWORD victimDistance = VictimCounter(tableData)++ & (BUCKET_SIZE - 1); | ||
| DWORD victimDistance = s_victimCounter++ & (BUCKET_SIZE - 1); |
There was a problem hiding this comment.
The previous code intentionally didn't use Interlocked here, because it's not a correctness issue: // NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes.
There was a problem hiding this comment.
The previous code intentionally didn't use Interlocked here, because it's not a correctness issue:
// NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes.
Copilots point here is that multithreaded access is UB in C++ without std::atomic and such.
There was a problem hiding this comment.
The previous code intentionally didn't use Interlocked here, because it's not a correctness issue:
// NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes.Copilots point here is that multithreaded access is UB in C++ without
std::atomicand such.
Yep, I can read and see "(undefined behavior)" in its reply. My PR didn't change existing logic here.
|
NOTE: this is purely optimization change, no correctness fixes here. Related improvememt (also in CastCache): #132221 |
The rotating victim counter (used when a bucket is full) lives in the table's aux data, element 0 — the same cache line every lookup reads
hashShift/tableMaskfrom. An inserting thread does a plain RMW on it and invalidates that line on every core doing casts. Moving it to a static keeps the aux data read-only.Readers cast a resident 16-pair set; writers cast an 8192-pair set (2x
MAXIMUM_CACHE_SIZE) so every cast misses and inserts via the victim path. Read Mops/s, 7950X:Also:
MaybeReplaceCacheWithLargernow bails if another thread already grew the table (otherwise each thread hitting a full bucket allocates its own, up to 98KB/LOH, and a stale one can publish a smaller table), and managedTrySetreads the version withVolatile.Readbefore the CAS, like the native writer already does. Same three fixes inGenericCache.