diff --git a/src/coreclr/vm/castcache.cpp b/src/coreclr/vm/castcache.cpp index a1cf95999327b3..992c955296f110 100644 --- a/src/coreclr/vm/castcache.cpp +++ b/src/coreclr/vm/castcache.cpp @@ -12,6 +12,7 @@ BASEARRAYREF* CastCache::s_pTableRef = NULL; OBJECTHANDLE CastCache::s_sentinelTable = NULL; DWORD CastCache::s_lastFlushSize = INITIAL_CACHE_SIZE; +DWORD CastCache::s_victimCounter = 0; const DWORD CastCache::INITIAL_CACHE_SIZE; BASEARRAYREF CastCache::CreateCastCache(DWORD size) @@ -83,6 +84,15 @@ BOOL CastCache::MaybeReplaceCacheWithLarger(DWORD size) } CONTRACTL_END; + // Another thread may have already grown the table while we were probing a stale one. + // Do not shrink it back and do not waste an allocation - just retry against what is there. + // This matters under load: without it every thread that finds a full bucket allocates + // its own table, and all but the last one are thrown away along with their entries. + if (CacheElementCount(TableData(*s_pTableRef)) >= size) + { + return TRUE; + } + BASEARRAYREF newTable = CreateCastCache(size); if (!newTable) { @@ -297,8 +307,7 @@ void CastCache::TrySet(TADDR source, TADDR target, BOOL result) } // 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); // position the victim in a quadratic reprobe bucket DWORD victim = (victimDistance * victimDistance + victimDistance) / 2; diff --git a/src/coreclr/vm/castcache.h b/src/coreclr/vm/castcache.h index e03e32efd53113..81f540d8b4f365 100644 --- a/src/coreclr/vm/castcache.h +++ b/src/coreclr/vm/castcache.h @@ -206,6 +206,12 @@ class CastCache static DWORD s_lastFlushSize; + // Rotating victim index used when a bucket is full. It is deliberately not stored + // in the table's aux data: every lookup reads hashShift/tableMask from that cache line, + // so an ordinary RMW from an inserting thread would false-share with all readers. + // NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes. + static DWORD s_victimCounter; + FORCEINLINE static TypeHandle::CastResult TryGetFromCache(TADDR source, TADDR target) { CONTRACTL @@ -280,6 +286,8 @@ class CastCache LIMITED_METHOD_CONTRACT; // element 0 is used for embedded aux data + // + // AuxData: { hashShift, tableMask } return (DWORD*)((BYTE*)OBJECTREFToObject(table) + ARRAYBASE_SIZE); } @@ -304,12 +312,6 @@ class CastCache return *(tableData + 1); } - FORCEINLINE static DWORD& VictimCounter(DWORD* tableData) - { - LIMITED_METHOD_CONTRACT; - return *(tableData + 2); - } - FORCEINLINE static DWORD CacheElementCount(DWORD* tableData) { LIMITED_METHOD_CONTRACT; diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs index 41effcee98dbef..cb2466f80a64d3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs @@ -25,6 +25,12 @@ internal unsafe struct CastCache // nothing is ever stored into this, so we can use a static instance. private static int[]? s_sentinelTable; + // Rotating victim index used when a bucket is full. It is deliberately not stored + // in the table's aux data: every lookup reads hashShift/tableMask from that cache line, + // so an ordinary RMW from an inserting thread would false-share with all readers. + // NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes. + private static uint s_victimCounter; + // The actual storage. private int[] _table; @@ -105,7 +111,7 @@ private static ref int TableData(int[] table) { // element 0 is used for embedded aux data // - // AuxData: { hashShift, tableMask, victimCounter } + // AuxData: { hashShift, tableMask } return ref Unsafe.As(ref Unsafe.AddByteOffset(ref table.GetRawData(), (nint)sizeof(nint))); } @@ -123,12 +129,6 @@ private static ref int TableMask(ref int tableData) return ref Unsafe.Add(ref tableData, 1); } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static ref uint VictimCounter(ref int tableData) - { - return ref Unsafe.As(ref Unsafe.Add(ref tableData, 2)); - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private static ref CastCacheEntry Element(ref int tableData, int index) { @@ -289,7 +289,9 @@ internal void TrySet(nuint source, nuint target, bool result) // If we used Robin Hood strategy we could eventually end up with all // entries in the table being maximally "poor". - 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); // mask the lower version bit to make it even. // This way we will detect both if version is changing (odd) or has changed (even, but different). @@ -346,15 +348,16 @@ internal void TrySet(nuint source, nuint target, bool result) } // 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. - uint victimDistance = VictimCounter(ref tableData)++ & (BUCKET_SIZE - 1); + uint victimDistance = s_victimCounter++ & (BUCKET_SIZE - 1); // position the victim in a quadratic reprobe bucket uint victim = (victimDistance * victimDistance + victimDistance) / 2; { ref CastCacheEntry pEntry = ref Element(ref tableData, (bucket + (int)victim) & TableMask(ref tableData)); - 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); // mask the lower version bit to make it even. // This way we will detect both if version is changing (odd) or has changed (even, but different). @@ -402,6 +405,15 @@ private void FlushCurrentCache() private bool MaybeReplaceCacheWithLarger(int size) { + // Another thread may have already grown the table while we were probing a stale one. + // Do not shrink it back and do not waste an allocation - just retry against what is there. + // This matters under load: without it every thread that finds a full bucket allocates + // its own table, and all but the last one are thrown away along with their entries. + if (CacheElementCount(ref TableData(_table)) >= size) + { + return true; + } + int[]? newTable = CreateCastCache(size); if (newTable == null) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericCache.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericCache.cs index 94d9ecdd73bf3f..d7923d600d43ad 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericCache.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericCache.cs @@ -34,8 +34,6 @@ internal struct EntryInfo // AuxData (to store some data specific to the table in the element #0 ) [FieldOffset(0)] internal byte hashShift; - [FieldOffset(1)] - internal byte victimCounter; } // NOTE: It is ok if TKey contains references, but we want it to be a struct, @@ -57,6 +55,12 @@ private struct Entry private const uint VERSION_NUM_MASK = (1 << VERSION_NUM_SIZE) - 1; private const int BUCKET_SIZE = 8; + // Rotating victim index used when a bucket is full. It is deliberately not stored + // in the table's aux data: every lookup reads hashShift from that cache line, so an + // ordinary RMW from an inserting thread would false-share with all readers. + // NB: ++ is not interlocked. We are ok if we lose counts here. It is just a number that changes. + private static byte s_victimCounter; + // The fields of this structure are known to coreclr, so if they are updated, you must also update object.h // The actual storage. @@ -118,12 +122,6 @@ private static ref byte HashShift(Entry[] table) return ref TableData(table)._info.hashShift; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static ref byte VictimCounter(Entry[] table) - { - return ref TableData(table)._info.victimCounter; - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private static int TableMask(Entry[] table) { @@ -271,7 +269,9 @@ internal void TrySet(TKey key, TValue value) // If we used Robin Hood strategy we could eventually end up with all // entries in the table being maximally "poor". - 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); // mask the lower version bit to make it even. // This way we will detect both if version is changing (odd) or has changed (even, but different). @@ -329,15 +329,16 @@ internal void TrySet(TKey key, TValue value) } // 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. - byte victimDistance = (byte)(VictimCounter(table)++ & (BUCKET_SIZE - 1)); + byte victimDistance = (byte)(s_victimCounter++ & (BUCKET_SIZE - 1)); // position the victim in a quadratic reprobe bucket int victim = (victimDistance * victimDistance + victimDistance) / 2; { ref Entry pEntry = ref Element(table, (bucket + victim) & TableMask(table)); - 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); // mask the lower version bit to make it even. // This way we will detect both if version is changing (odd) or has changed (even, but different). @@ -388,6 +389,15 @@ private void FlushCurrentCache() private bool MaybeReplaceCacheWithLarger(int size) { + // Another thread may have already grown the table while we were probing a stale one. + // Do not shrink it back and do not waste an allocation - just retry against what is there. + // This matters under load: without it every thread that finds a full bucket allocates + // its own table, and all but the last one are thrown away along with their entries. + if (CacheElementCount(_table) >= size) + { + return true; + } + Entry[]? newTable = CreateCacheTable(size); if (newTable == null) {