Reuse typical instantiation DispatchMap for generic instantiations#130530
Reuse typical instantiation DispatchMap for generic instantiations#130530davidwrighton wants to merge 4 commits into
Conversation
When loading a non-typical instantiation of a generic type that undergoes a full MethodTableBuilder run (the __Canon canonical form and value-type instantiations such as List<int>), the interface DispatchMap was rebuilt from scratch via PlaceInterfaceMethods for every instantiation. The encoded DispatchMap is instantiation-independent (it stores type IDs and slot numbers), so it can instead be built once while constructing the type's typical instantiation and reused for all of its non-typical instantiations. In release builds, PlaceInterfaceMethods is skipped for a non-typical instantiation when the typical instantiation's DispatchMap can be reused, and the typical instantiation's encoded map bytes are copied into the new MethodTable's inline DispatchMap. In debug/checked builds the specific instantiation's DispatchMap is still built and asserted to be byte-for-byte identical to the typical instantiation's map, guarding the instantiation-independence invariant. This is safe because PlaceInterfaceMethods only produces DispatchMap interface entries (it does not mutate the vtable; PlaceMethodImpls still always runs), and the two consumers that read the half-built dispatch map after PlaceInterfaceMethods (ValidateInterfaceMethodConstraints and VerifyVirtualMethodsImplemented) are already skipped for non-typical instantiations because fNoSanityChecks is TRUE for them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3bea685d-50eb-4245-b1ea-4310df00c7d3
bmtInterfaceEntry::CreateSlotTable walked every method of an interface that has virtual static methods, counting the static+virtual ones solely to size the bmtInterfaceSlotImpl array. The subsequent loop already recomputes the exact per-method placement, so the array can simply be over-allocated to the interface's method count, eliminating the extra MethodIterator walk. The reused pDeclMD from the placement loop replaces the redundant it.GetDeclMethodDesc() call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3bea685d-50eb-4245-b1ea-4310df00c7d3
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR optimizes CoreCLR generic type loading by avoiding redundant work when building interface dispatch maps for non-typical generic instantiations, and by reducing per-interface iteration overhead when sizing virtual-static slot tables.
Changes:
- Reuse the typical instantiation’s encoded
DispatchMapfor eligible non-typical generic instantiations (release), with byte-for-byte validation against a rebuilt map in_DEBUG/Checked builds. - Reduce work in
bmtInterfaceEntry::CreateSlotTableby avoiding an extraMethodIteratorpass when sizing storage for interfaces with virtual static methods. - Add a
DispatchMaphelper to expose the encoded map bytes for reuse/copying.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/methodtablebuilder.h | Tracks typical instantiation MethodTable on all builds and adds helper declaration for DispatchMap reuse. |
| src/coreclr/vm/methodtablebuilder.cpp | Implements DispatchMap reuse (skip PlaceInterfaceMethods in release when safe) and adjusts interface slot table sizing for virtual static methods. |
| src/coreclr/vm/contractimpl.h | Adds DispatchMap::GetEncodedMapData() to access encoded map bytes for reuse/copying. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "e7f56e871d121042f4ece28f0b034b08acf72020",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "2f3204f6e8df27046832bb56afa980a923deaf91",
"last_reviewed_commit": "e7f56e871d121042f4ece28f0b034b08acf72020",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "2f3204f6e8df27046832bb56afa980a923deaf91",
"last_recorded_worker_run_id": "29681926970",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e7f56e871d121042f4ece28f0b034b08acf72020",
"review_id": 4730545488
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Two related type-loader optimizations in CoreCLR's MethodTableBuilder. (1) The encoded interface DispatchMap is instantiation-independent (type IDs + slot numbers), yet it was rebuilt from scratch via PlaceInterfaceMethods for every non-typical instantiation of a generic type that undergoes a full builder run. (2) bmtInterfaceEntry::CreateSlotTable walked every method of an interface with virtual-static methods purely to size a temporary array. Both add measurable overhead on type-loading-bound workloads.
Approach: For (1), a new GetTypicalMethodTableForDispatchMapReuse helper identifies non-typical generic instantiations whose typical instantiation already has a DispatchMap. In release builds, PlaceInterfaceMethods is skipped for those and AllocateNewMT copies the typical instantiation's encoded map bytes (via the new DispatchMap::GetEncodedMapData) directly into the new MethodTable's inline map. In debug/checked builds, PlaceInterfaceMethods still always runs and the freshly built map is asserted byte-for-byte identical to the typical map, guarding the instantiation-independence invariant. The previously debug-only dbg_pTypicalInstantiationMT field is promoted to a real field (pTypicalInstantiationMT) since it is now consumed in release. For (2), the counting MethodIterator walk is removed and the array is over-allocated to the interface's method count.
Summary: The changes are correct and carefully guarded. Safety reasoning holds: PlaceInterfaceMethods only produces DispatchMap interface entries and does not mutate the vtable (PlaceMethodImpls/ProcessMethodImpls still always run), and the two consumers that read the half-built map (ValidateInterfaceMethodConstraints, VerifyVirtualMethodsImplemented) are already skipped for non-typical instantiations via fNoSanityChecks. The over-allocation to GetNumMethods() is safe because instance-virtual and static-virtual methods are disjoint subsets of the interface's method set, so cSlots + numStaticVirtual <= GetNumMethods(). DispatchMap's constructor copies the source bytes (memcpyNoGCRefs), so aliasing the typical instantiation's immutable buffer as the source is fine. The debug byte-equality assertion (exercised 141 times per the PR notes) provides strong ongoing protection against a future change that would break instantiation-independence. The gating on dispatchMapAllocationSize > 0 (replacing the builder-count check) is consistent across both the reuse and normal paths. No correctness, memory-safety, or lifetime issues found; DAC contract on the new accessor is appropriate. Verdict: LGTM.
Detailed Findings
No blocking issues.
Non-actionable observations:
- The debug/release divergence (debug always builds + validates; release reuses) is intentional and clearly documented, but it does mean the release-only fast path (skipping
PlaceInterfaceMethodsand encoding) receives no direct in-CI functional coverage beyond the equality invariant validated in checked builds. This is an acceptable and common pattern here. GetEncodedMapDatareturns a pointer into the typical instantiation's liveDispatchMap; correctness relies on thatMethodTableremaining loaded for the duration ofAllocateNewMT. Since the typical instantiation is a load-time dependency of the non-typical one, this holds.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 68.4 AIC · ⌖ 10.4 AIC · ⊞ 10K
…ric instantiations GetTypicalMethodTableForDispatchMapReuse previously returned NULL both when there was no typical MethodTable to reuse and when the typical MethodTable had no DispatchMap slot. In the latter case the resulting DispatchMap is known to be empty, so PlaceInterfaceMethods was run needlessly. Return a DispatchMapReuseKind enum so callers can skip PlaceInterfaceMethods for the known-empty case and produce an empty map directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 11ab8b48-7c38-47af-a41b-d1a9637c5f96
| cSlotsTotal++; | ||
| } | ||
| } | ||
| cSlotsTotal = GetInterfaceType()->GetMethodTable()->GetNumMethods(); |
There was a problem hiding this comment.
The previous logic seems substantially different from calling the non-descripe GetNumMethods() on a MethodTable instance. Even if it is the one that represents the interface type, it isn't clear to me they are both marked static and virtual. If the previously loop only look at IsVirtual() I could see it being the same, but the IsStatic() seems to indicate another attribute of the methods to consider.
| // typical instantiation's DispatchMap can be reused, or when the resulting DispatchMap is | ||
| // already known to be empty (see GetTypicalMethodTableForDispatchMapReuse). | ||
| MethodTable *pUnusedTypicalMTForDispatchMap = NULL; | ||
| if (GetTypicalMethodTableForDispatchMapReuse(&pUnusedTypicalMTForDispatchMap) == DispatchMapReuseKind::BuildNormally) |
There was a problem hiding this comment.
This doesn't seem right. Yes, we always want to run PlaceInterfaceMethods() as a validation state on a non-Release build, but we also should want to run GetTypicalMethodTableForDispatchMapReuse() on non-Release builds too.
| #ifdef _DEBUG | ||
| // Validate that the DispatchMap we just built for this specific instantiation is | ||
| // byte-for-byte identical to the typical instantiation's DispatchMap. If this fires, | ||
| // the DispatchMap is not actually instantiation-independent and cannot be reused. | ||
| _ASSERTE_MSG(cbTypicalMap == cbDispatchMapTemp, | ||
| "Typical instantiation DispatchMap size differs from the specific instantiation's DispatchMap"); | ||
| _ASSERTE_MSG((cbTypicalMap == 0) || (memcmp(pbTypicalMap, pbDispatchMapTemp, cbTypicalMap) == 0), | ||
| "Typical instantiation DispatchMap contents differ from the specific instantiation's DispatchMap"); | ||
| #else |
There was a problem hiding this comment.
| #ifdef _DEBUG | |
| // Validate that the DispatchMap we just built for this specific instantiation is | |
| // byte-for-byte identical to the typical instantiation's DispatchMap. If this fires, | |
| // the DispatchMap is not actually instantiation-independent and cannot be reused. | |
| _ASSERTE_MSG(cbTypicalMap == cbDispatchMapTemp, | |
| "Typical instantiation DispatchMap size differs from the specific instantiation's DispatchMap"); | |
| _ASSERTE_MSG((cbTypicalMap == 0) || (memcmp(pbTypicalMap, pbDispatchMapTemp, cbTypicalMap) == 0), | |
| "Typical instantiation DispatchMap contents differ from the specific instantiation's DispatchMap"); | |
| #else | |
| // Validate that the DispatchMap we just built for this specific instantiation is | |
| // byte-for-byte identical to the typical instantiation's DispatchMap. If this fires, | |
| // the DispatchMap is not actually instantiation-independent and cannot be reused. | |
| _ASSERTE_MSG(cbTypicalMap == cbDispatchMapTemp, | |
| "Typical instantiation DispatchMap size differs from the specific instantiation's DispatchMap"); | |
| _ASSERTE_MSG((cbTypicalMap == 0) || (memcmp(pbTypicalMap, pbDispatchMapTemp, cbTypicalMap) == 0), | |
| "Typical instantiation DispatchMap contents differ from the specific instantiation's DispatchMap"); | |
| #ifndef _DEBUG |
The _ASSERT_MSG are only set in non-Debug, no need to wrap them in an ifdef.
| #ifdef _DEBUG | ||
| else if (dispatchMapReuseKind == DispatchMapReuseKind::KnownEmpty) | ||
| { | ||
| // The typical instantiation has no DispatchMap, so this instantiation's DispatchMap must be | ||
| // empty too. In debug builds PlaceInterfaceMethods always runs, so validate that it indeed | ||
| // produced nothing. | ||
| _ASSERTE_MSG(bmtVT->pDispatchMapBuilder->Count() == 0, | ||
| "Non-typical instantiation produced DispatchMap entries even though its typical instantiation has no DispatchMap"); | ||
| } | ||
| #endif // _DEBUG |
There was a problem hiding this comment.
| #ifdef _DEBUG | |
| else if (dispatchMapReuseKind == DispatchMapReuseKind::KnownEmpty) | |
| { | |
| // The typical instantiation has no DispatchMap, so this instantiation's DispatchMap must be | |
| // empty too. In debug builds PlaceInterfaceMethods always runs, so validate that it indeed | |
| // produced nothing. | |
| _ASSERTE_MSG(bmtVT->pDispatchMapBuilder->Count() == 0, | |
| "Non-typical instantiation produced DispatchMap entries even though its typical instantiation has no DispatchMap"); | |
| } | |
| #endif // _DEBUG | |
| else if (dispatchMapReuseKind == DispatchMapReuseKind::KnownEmpty) | |
| { | |
| // The typical instantiation has no DispatchMap, so this instantiation's DispatchMap must be | |
| // empty too. In debug builds PlaceInterfaceMethods always runs, so validate that it indeed | |
| // produced nothing. | |
| _ASSERTE_MSG(bmtVT->pDispatchMapBuilder->Count() == 0, | |
| "Non-typical instantiation produced DispatchMap entries even though its typical instantiation has no DispatchMap"); | |
| } |
Dead branches will be elided. There is no need to put an ifdef around, unless an "else" clause exists.
This PR contains two related optimizations to generic type loading / interface slot handling in the CoreCLR type loader.
1. Reuse typical instantiation DispatchMap for generic instantiations
When loading a non-typical instantiation of a generic type that undergoes a full
MethodTableBuilderrun (the__Canoncanonical form and value-type instantiations such asList<int>), the interfaceDispatchMapwas rebuilt from scratch viaPlaceInterfaceMethodsfor every instantiation. The encodedDispatchMapis instantiation-independent (it stores type IDs and slot numbers), so it can instead be built once while constructing the type's typical instantiation and reused for all of its non-typical instantiations.PlaceInterfaceMethodsis skipped for a non-typical instantiation when the typical instantiation'sDispatchMapcan be reused, and the typical instantiation's encoded map bytes are copied into the newMethodTable's inlineDispatchMap.DispatchMapis still built and asserted to be byte-for-byte identical to the typical instantiation's map, guarding the instantiation-independence invariant.This is safe because
PlaceInterfaceMethodsonly producesDispatchMapinterface entries (it does not mutate the vtable;PlaceMethodImplsstill always runs), and the two consumers that read the half-built dispatch map afterPlaceInterfaceMethods(ValidateInterfaceMethodConstraintsandVerifyVirtualMethodsImplemented) are already skipped for non-typical instantiations becausefNoSanityChecksisTRUEfor them.2. Avoid iterating interface methods to size virtual-static slot table
bmtInterfaceEntry::CreateSlotTablewalked every method of an interface that has virtual static methods, counting the static+virtual ones solely to size thebmtInterfaceSlotImplarray. The subsequent loop already recomputes the exact per-method placement, so the array can simply be over-allocated to the interface's method count, eliminating the extraMethodIteratorwalk.Validation
clr.runtimebuilds succeed with 0 warnings/errors.Performance
Measured on an Rx cold-start micro-benchmark that isolates the
System.Reactiveconstruct+initialize type-loading path (interleaved A/B, 60 iterations each, releasecoreclr.dllswapped, baseline = before both changes):Standard deviation was ~1.8 ms, so the ~1.4% improvement is a clear signal on type-loading-bound workloads. On WPF R2R startup (where type loading is a much smaller fraction of total startup) the effect is smaller and within run-to-run noise.