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. |
|
NOTE: this is purely optimization change, no correctness fixes here. Related improvememt (also in CastCache): #132221 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/castcache.cpp:310
s_victimCounter++performs a non-atomic read/modify/write on a shared static. In C++ this is a data race and therefore undefined behavior under concurrent writers. Since the exact count doesn't matter, consider using an atomic fetch-add (e.g.,InterlockedExchangeAdd) so the value remains best-effort without invoking UB. If you make it atomic, please also update the nearby comment in the header that says++ is not interlocked.
DWORD victimDistance = s_victimCounter++ & (BUCKET_SIZE - 1);
|
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. |
| uint version = pEntry._version; | ||
| // Volatile.Read is to ensure that the version cannot be re-fetched between here | ||
| // and the CompareExchange below, which would defeat the claim of the entry. | ||
| uint version = Volatile.Read(ref pEntry._version); |
There was a problem hiding this comment.
Volatile is unnecessary here.
- The read will happen before the CompareExchange (a full fence)
- dotnet memory model does not allow random re-reading of a field into a local.
c++ memory model allows read duplication, thus native counterpart uses VolatileLoadWithoutBarrier to defeat possible compiler optimizations, but in managed code ordinary read is sufficient.
Did it show cache misses on reading the victim counter/tableMask? Ideally the cast cache is mostly read-only. The upper bound of the cache that we selected, if I remember correctly, was "this much we can easily afford and should be enough for everybody". For the apps we tried - like self-rebuilding Roslyn it was more than enough. I wonder if we found an up that needs a larger cache. We probably need some diagnostic event to surface the cache churn (not in this change). I will log a follow up issue. |
It probably not in this case because the cache was full (4096 entries) in case of 1P as @AndyAyersMS noticed. |
|
What we see from the 1P data so far is that the cache is full. The perf issue there seems to be related to the successful lookup's volatile read ( App is running on a 32 core VM. What is odd is that this does not happen on every run of the app, just on some. Cache interactions seem similar in both fast and slow runs, at least from what we can reconstruct from dumps: 
|
| BASEARRAYREF* CastCache::s_pTableRef = NULL; | ||
| OBJECTHANDLE CastCache::s_sentinelTable = NULL; | ||
| DWORD CastCache::s_lastFlushSize = INITIAL_CACHE_SIZE; | ||
| DWORD CastCache::s_victimCounter = 0; |
There was a problem hiding this comment.
It looks like, depending on what compiler does, this can now share the cache line with s_pTableRef that every operation will read.
There was a problem hiding this comment.
Since we have a motivation to improve this pattern, I think we should do the following:
- Make a static some function like "GetNumberThatChangesFast"
- implement it in terms of
__rdtscon x64,cntvct_el0on arm64, otherwise fallback to incrementing a static counter
(optionally: align and put the fallback counter in a cache line sized struct, if not too much to bother for the fallback case) - expose the function as a fcall and use on the managed side as well.
Or just do the “update a static counter” on the managed side - if fcall costs too much, which is possible.
There was a problem hiding this comment.
implement it in terms of __rdtsc on x64, cntvct_el0 on arm64
I vaguely remember there were a few caveats with those, such as always returning 0 on certain CPUs. We were considering using them for call counts to reduce contention, but I think the JVM happily uses them without issue.
My guess would be that, since the cache is full and we have some churn, the If you have a way to experiment with the app and custom runtime builds, what happens if the cache size is 4x or 8x larger? |
|
Looks like Andy managed to detect some differences e.g. the 1P's app actually had >5600 of cast pairs vs default 4096, but this PR didn't fix the perf issue yet, so marking as draft. @VSadov if you want to take over - feel free! I think you're well more familar with these casts caches. |
|
One thing that I noticed that we nuke the entire cache for the entire process if any ALC unloads, so in theory load-unload cycle might badly impact perf, not sure it anything can be done, just a note. + This PR fixed a race condition on Grow (not a correctness issue, just redundant allocations). BTW, Grow effectively nukes the entire cache as well. |
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.