Uh oh!
There was an error while loading. Please reload this page.
Generate less metadata for generic compositions - #130842
Generate less metadata for generic compositions#130842MichalStrehovsky wants to merge 2 commits into
Conversation
Something I noticed as I was working on dotnet#129609. I don't think we need fully constructed `MethodTable` for composition information (neither for types, nor for generic virtual methods).
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 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 adjusts NativeAOT (ILCompiler) dependency analysis so “generic composition” data can be emitted without forcing fully constructed EEType / MethodTable nodes, by introducing a “metadata-enabled” path for generic compositions and type arguments.
Changes:
- Renamed the “constructed” generic composition cache/API to “metadata-enabled” and routed relevant emit sites to use it.
- Added
MaximallyMetadataEnabledTypeto pick metadata-capableEETypesymbols for generic composition arguments without requiring fully constructed types. - Updated EEType instantiation detail emission and GVM dispatch cell info emission to use metadata-enabled compositions/types.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs | Adds MaximallyMetadataEnabledType, renames/rewires generic composition caches to “metadata-enabled”. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GvmDispatchCellInfoSectionNode.cs | Switches GVM instantiation info emission/deps to metadata-enabled generic composition. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericCompositionNode.cs | Renames flag to metadataEnabled and uses MaximallyMetadataEnabledType for arg EEType selection. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs | Uses metadata-enabled generic composition/types when emitting reflection-visible instantiation details. |
Uh oh!
There was an error while loading. Please reload this page.
MichalStrehovsky
commented
Jul 16, 2026
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a08f92830065e3817b4a0837cb0f4093ea235efc",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
"last_reviewed_commit": "a08f92830065e3817b4a0837cb0f4093ea235efc",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
"last_recorded_worker_run_id": "29684896025",
"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": "a08f92830065e3817b4a0837cb0f4093ea235efc",
"review_id": 4730669188
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: MethodTables referenced by generic composition information (for generic types and generic virtual methods) were being emitted as fully constructed types, which pulls in more metadata than is actually needed. Composition info only needs enough of a MethodTable to be metadata-enabled, not fully constructed. The author noticed this while working on #129609, so the goal is a straightforward size/metadata optimization for NativeAOT output.
Approach: The PR renames the constructed concept in generic composition to metadataEnabled and introduces two new NodeFactory helpers: MaximallyMetadataEnabledType(TypeDesc) (returns NecessaryTypeSymbol for canonical-definition types, otherwise MetadataTypeSymbol) and MetadataEnabledGenericComposition(Instantiation). Call sites in EETypeNode, GvmDispatchCellInfoSectionNode, and GenericCompositionNode are switched from the Constructed* variants to the new MetadataEnabled* variants, and the backing node cache/field (_constructedGenericCompositions → _metadataEnabledGenericCompositions) plus the GenericCompositionNode._constructed flag are renamed consistently.
Summary: This is a focused, low-risk change confined to the NativeAOT (ILCompiler) dependency-analysis layer. The renames are applied consistently across the node, factory, cache, and comparison (CompareToImpl) paths, so node identity/ordering semantics are preserved. The new MaximallyMetadataEnabledType correctly mirrors MaximallyConstructableType's canonical-type fallback via IsCanonicalDefinitionType, and generic-composition type arguments are concrete or canonical instantiation types — not generic definitions or synthetic AsyncContinuationTypes — so the tightened Debug.Asserts inside MetadataTypeSymbol are not expected to trip. The author has run runtime-nativeaot-outerloop to exercise the affected code paths, which is the appropriate validation for a metadata-reduction change of this kind. No correctness or maintainability concerns were found. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 54.3 AIC · ⌖ 10.3 AIC · ⊞ 10K
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Review details
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs:831
- MaximallyMetadataEnabledType unconditionally calls MetadataTypeSymbol for non-canonical types, but MetadataTypeSymbol asserts that AsyncContinuationType is not passed (see Debug.Assert in MetadataTypeSymbol). Prior code paths using MaximallyConstructableType would route AsyncContinuationType to ConstructedTypeSymbol, so this introduces a potential debug-build assert/regression if an instantiation/composition contains AsyncContinuationType. Consider mirroring the existing special-case handling to keep behavior consistent and avoid the assert.
public IEETypeNode MaximallyMetadataEnabledType(TypeDesc type)
{
if (type.IsCanonicalDefinitionType(CanonicalFormKind.Any))
return NecessaryTypeSymbol(type);
else
return MetadataTypeSymbol(type);
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Something I noticed as I was working on #129609. I don't think we need fully constructed
MethodTablefor composition information (neither for types, nor for generic virtual methods).