Cache version-resilient hash code on MethodTableAuxiliaryData to fix superlinear behavior with polymorphic recursion

[Copilot]

GetVersionResilientTypeHashCode recomputes hash codes recursively without memoization. With polymorphic recursion, each level doubles the number of unique generic instantiations, causing exponential blowup — n=31 took ~15s before this fix.

Description

• src/coreclr/vm/methodtable.h: Added int m_cachedVersionResilientHashCode to MethodTableAuxiliaryData. The field is placed immediately after the 4-byte m_dwFlags union, filling the 4 bytes of implicit alignment padding that already exists before the 8-byte-aligned m_pLoaderModule pointer on 64-bit platforms. This means the struct size is unchanged on 64-bit (0 bytes / 0% increase). The field is lazily initialized — 0 means not yet set, and is zero-initialized for free by the loader heap so no explicit init is needed for new instantiations.

• src/coreclr/vm/versionresilienthashcode.cpp: In GetVersionResilientTypeHashCode, the separate early-return branch for array types has been removed. Since TypeHandle::IsArray() is defined as !IsTypeDesc() && AsMethodTable()->IsArray(), arrays are always MethodTable-backed and share the same MethodTableAuxiliaryData. Both array and non-array MethodTable-backed types are now handled in a single !type.IsTypeDesc() branch, with the cache check and store applied uniformly to all of them. The type-specific hash computation (ComputeArrayTypeHashCode vs. name/generic logic) is selected inside the branch after the cache check. Types whose hash code is exactly 0 are not cached (sentinel collision), but this is vanishingly rare given the non-zero initial state of the hash function (hash1 = 0x6DA3B944).

Memory Impact

On 64-bit release builds, MethodTableAuxiliaryData already had 4 bytes of implicit alignment padding between the union and m_pLoaderModule. The new field fills that slot exactly:

	Before	After
sizeof(MethodTableAuxiliaryData) (release x64)	24 bytes	24 bytes
Increase	—	0 bytes (0%)

On 32-bit, all pointers are 4 bytes so no alignment gap exists; the struct grows by 4 bytes (12 → 16 bytes, 33%).

Hash Algorithm Effectiveness

The existing ComputeGenericInstanceHashCode algorithm was evaluated against:

• All 32 unique generic types produced by the polymorphic recursion benchmark: 0 collisions, good bit distribution
• Common .NET generic instantiations (List<T>, Dictionary<K,V>, Task<T>, Nullable<T>, IEnumerable<T> with int/string/bool/double): 0 collisions

The existing algorithm produces high-quality hashes for generic instantiations; the exponential blowup was purely a memoization problem, not a hash quality issue.

Performance

Benchmark from the issue (polymorphic recursion across two type parameters):

n	Before	After
16	16 ms	4 ms
20	93 ms	4 ms
24	555 ms	4 ms
28	3716 ms	4 ms
31	~15 s	4 ms

Constant time after the first instantiation at each depth.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[davidwrighton]

@copilot This increases the memory needed on the loader heap. Please measure the memory increase for this scenario, and present the increase both in bytes and by percentage. Also, this change allows us to consider more capable hash combining algorithms. Please measure how effective the current hash combining algorithm is for generic instatiations.

[davidwrighton]

@copilot arrays are also MethodTable and can use the cache.

[Copilot]

@copilot This increases the memory needed on the loader heap. Please measure the memory increase for this scenario, and present the increase both in bytes and by percentage. Also, this change allows u...

Memory impact (commit d9e3911): MethodTableAuxiliaryData already has 4 bytes of implicit alignment padding between the 4-byte m_dwFlags union and the 8-byte-aligned m_pLoaderModule pointer on 64-bit. By placing m_cachedVersionResilientHashCode in that padding slot (immediately after the union), sizeof(MethodTableAuxiliaryData) stays at 24 bytes — 0 bytes / 0% increase on 64-bit release builds.

On 32-bit, all pointers are 4 bytes so no alignment padding exists; the struct would grow by 4 bytes (12 → 16 bytes, 33%).

Hash algorithm effectiveness: I simulated the hash algorithm on the 32 distinct generic types produced by the polymorphic recursion benchmark and found zero collisions with good bit distribution across the full 32-bit hash space. I also checked common .NET generic instantiations (List<int>, Dictionary<string,int>, Task<bool>, etc.) — again no collisions. The existing algorithm appears adequate; the caching is the key fix here.

[Copilot]

@copilot arrays are also MethodTable and can use the cache.

Done in commit f8a7695. TypeHandle::IsArray() is defined as !IsTypeDesc() && AsMethodTable()->IsArray(), so arrays are always MethodTable-backed. The separate early-return IsArray() branch has been removed; both arrays and non-array types are now handled in the single !type.IsTypeDesc() branch, sharing the same cache check/store logic around the type-specific hash computation.

[JoostK]

Why bother branching here instead of unconditionally writing the computed hash, that would just write the zero-initialized value again. The sentinel collision only means that a computed hash of 0 will have to be recomputed each time, there's no harm is storing it.

[jkotas]

there's no harm is storing it

Storing it unconditionally can introduce CPU cache contention in the rare case a type ends up with hashcode=0.

[Copilot]

Pull request overview

This PR adds memoization for GetVersionResilientTypeHashCode(TypeHandle) by caching the computed hash code on MethodTableAuxiliaryData, addressing exponential recomputation in polymorphic recursion scenarios.

Changes:

• Added a lazily-initialized cached version-resilient hash code field to MethodTableAuxiliaryData.
• Updated GetVersionResilientTypeHashCode(TypeHandle) to use the cache for all MethodTable-backed types (including arrays), computing and storing the hash once per MethodTable.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/versionresilienthashcode.cpp	Uses a per-MethodTable cached hash to avoid repeated recursive recomputation; unifies array/non-array MethodTable handling under the same cache path.
src/coreclr/vm/methodtable.h	Adds m_cachedVersionResilientHashCode to MethodTableAuxiliaryData to store the memoized result (intended to fit existing x64 alignment padding).

[Copilot]

Using 0 as the "not yet cached" sentinel means types whose computed version-resilient hash code is exactly 0 will never be memoized and will still pay the full recursive recomputation cost on every call. If the goal is to fully eliminate the pathological recursion cost in all cases, consider tracking the cached/not-cached state separately (e.g., a spare bit in m_dwFlags) so that 0 can be cached as a valid value too.

[Copilot]

The placement of m_cachedVersionResilientHashCode is intended to rely on existing alignment padding so x64 size/offsets remain unchanged. To prevent accidental future layout changes (e.g., reordering fields or changing types) from silently increasing MethodTableAuxiliaryData size or shifting m_pLoaderModule, consider adding a compile-time check (e.g., static_assert(offsetof(MethodTableAuxiliaryData, m_pLoaderModule) == 8) / static_assert(sizeof(MethodTableAuxiliaryData) == 24) under HOST_64BIT + appropriate build conditions).

[github-actions]

🤖 Copilot Code Review — PR #126534

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: Well-justified. GetVersionResilientTypeHashCode is called from 10+ sites in readytoruninfo.cpp for R2R hash-table lookups. Without memoization, polymorphic recursion or deeply nested generics cause exponential re-computation — the commit message demonstrates n=31 going from ~15s to ~4ms. The problem is real and the fix addresses the root cause.

Approach: Sound and well-crafted. Caching a deterministic computation on the owning data structure (MethodTableAuxiliaryData) is the textbook approach. The placement of the field in existing alignment padding is a nice detail that avoids any memory overhead on 64-bit. The restructuring to handle arrays in the same branch is a natural extension since TypeHandle::IsArray() implies !IsTypeDesc().

Summary: ✅ LGTM. The change is correct, well-motivated, and uses appropriate patterns for the CoreCLR VM. Already approved by @jkotas. All findings below are confirmatory.

---

Detailed Findings

✅ Thread Safety — Correct benign-race caching pattern

The caching uses VolatileLoadWithoutBarrier for reads and VolatileStore for writes. This is safe because:

• int writes are atomic on all supported platforms (x64, ARM64, x86)
• The hash computation is deterministic — same MethodTable always produces the same result
• Multiple threads racing to store the same value is a benign race
• The worst case is redundant computation (not incorrect results)

This follows the same pattern used elsewhere in methodtable.h (e.g., m_iNextChainDepth at lines 3380/3562).

✅ Memory Layout — Zero overhead on 64-bit

The int m_cachedVersionResilientHashCode field fills the 4-byte alignment padding that already existed between the 4-byte m_dwFlags union and the 8-byte-aligned m_pLoaderModule pointer. sizeof(MethodTableAuxiliaryData) remains 24 bytes on 64-bit release builds. On 32-bit, the struct grows by 4 bytes (12→16), which is acceptable for the performance benefit.

The existing cDAC data descriptor offsets in datadescriptor.inc (lines 411–416) remain valid because the existing fields (LoaderModule, OffsetToNonVirtualSlots, Flags) don't shift — the new field occupies what was previously implicit padding.

✅ Code Restructuring — Logically equivalent

The original code had a separate early-return type.IsArray() branch before the !type.IsTypeDesc() check. Since TypeHandle::IsArray() is defined as !IsTypeDesc() && AsMethodTable()->IsArray(), arrays always enter the !type.IsTypeDesc() branch anyway. Moving the array handling inside that branch is correct and allows arrays to benefit from caching.

The assertion change from _ASSERTE(!pMT->IsArray()); _ASSERTE(!IsNilToken(pMT->GetCl())); to _ASSERTE(pMT->IsArray() || !IsNilToken(pMT->GetCl())); is correct: array MethodTables may have nil class tokens, but non-array types must always have valid tokens.

✅ Zero Sentinel — Acceptable tradeoff

Using 0 as "not yet cached" means types with a hash code of exactly 0 won't be cached. As discussed in the review thread, this is vanishingly rare given the hash function's non-zero initial state (0x6DA3B944), and the if (hashcode != 0) guard avoids CPU cache contention for that case (per @jkotas's comment). The behavior remains correct in all cases — only the performance characteristic differs for the edge case.

✅ Initialization — Guaranteed by loader heap

The loader heap zero-initializes allocated memory (verified in loaderheap.cpp lines 457, 763, 984–985: "Callers of loaderheap assume allocated memory is zero-inited so we must preserve this invariant"). No explicit initialization of the field is needed in AllocateAuxiliaryData.

✅ No New Public API Surface

The change is entirely internal to the CoreCLR VM. No ref/ assembly changes, no new public members. No API approval required.

Generated by Code Review for issue #126534 · ◷