Skip to content

Conversation

MichalStrehovsky
Copy link
Member

Ever since GVM support was added to native AOT, we were generating the GVM resolution metadata for every type considered allocated. This included GVMs that were never even called: see TypeGVMEntriesNode that simply goes over everything on the type - we were adding a TypeGVMEntriesNode for all allocated types that have something to do with GVMs.

This PR introduces tracking with method level granularity. I ran into this in a different PR where this was dragging double/float into compilation just because int implements generic math.

After this PR, we'll have a GVMMetadataNode for every generic virtual method slot definition or slot override (uninstantiated) that is used in the program. We'll also have a InterfaceGVMMetadataNode that is the same but for interface methods. The generation logic in InterfaceGenericVirtualMethodTableNode and GenericVirtualMethodTableNode was updated to look at these new nodes instead of walking everything on the type.

Cc @dotnet/ilc-contrib

Ever since GVM support was added to native AOT, we were generating the GVM resolution metadata for every type considered allocated. This included GVMs that were never even called (see `TypeGVMEntriesNode` that simply goes over everything on the type). This PR introduces tracking with method level granularity. I ran into this in a different PR where this was dragging `double`/`float` into compilation just because `int` implements generic math.
@Copilot Copilot AI review requested due to automatic review settings August 12, 2025 09:48
Copy link
Contributor

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

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes GVM (Generic Virtual Method) resolution metadata generation in native AOT by implementing method-level granularity tracking instead of type-level tracking. Previously, GVM metadata was generated for all allocated types that had any relationship to GVMs, even if specific GVMs were never called. Now, metadata is only generated for GVMs that are actually used in the program.

Key changes include:

  • Replace type-based TypeGVMEntriesNode with method-specific GVMMetadataNode and InterfaceGVMMetadataNode
  • Update dependency tracking to register metadata nodes only when GVMs are actually needed
  • Modify table generation logic to use the new granular metadata nodes

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ILCompiler.Compiler.csproj Updates project file to remove TypeGVMEntriesNode and add new metadata node files
MetadataManager.cs Replaces type-based GVM tracking with method-specific collections and accessors
TypeGVMEntriesNode.cs Removes the old type-level GVM metadata tracking implementation
NodeFactory.cs Adds factory methods and cache structures for the new method-specific metadata nodes
InterfaceGenericVirtualMethodTableNode.cs Updates table generation to use new granular interface GVM metadata
InterfaceGVMMetadataNode.cs New node for tracking interface GVM metadata at method level
GenericVirtualMethodTableNode.cs Updates table generation to use new granular GVM metadata
GVMMetadataNode.cs New node for tracking GVM metadata at method level
GVMDependenciesNode.cs Updates dependency registration to create metadata nodes when GVMs are used
EETypeNode.cs Removes automatic creation of type-level GVM entries
ConstructedEETypeNode.cs Removes type-level GVM dependency tracking for canonical types


public override bool Equals(object obj) => obj is InterfaceGVMMetadataKey other && Equals(other);
public bool Equals(InterfaceGVMMetadataKey other) => CallingMethod == other.CallingMethod && ImplementationType == other.ImplementationType;
public override int GetHashCode() => HashCode.Combine(CallingMethod, ImplementationType);
Copy link
Preview

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Equals method for InterfaceGVMMetadataKey is incomplete. It only compares CallingMethod and ImplementationType, but ignores ImplementationMethod and DefaultResolution fields. This could lead to incorrect equality comparisons and potential cache key collisions.

Suggested change
public override int GetHashCode() => HashCode.Combine(CallingMethod, ImplementationType);
public bool Equals(InterfaceGVMMetadataKey other) =>
CallingMethod == other.CallingMethod &&
ImplementationMethod == other.ImplementationMethod &&
ImplementationType == other.ImplementationType &&
DefaultResolution == other.DefaultResolution;
public override int GetHashCode() => HashCode.Combine(CallingMethod, ImplementationMethod, ImplementationType, DefaultResolution);

Copilot uses AI. Check for mistakes.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Aug 13, 2025
Ever since GVM support was added to native AOT, we were generating the GVM resolution metadata for every type considered allocated. This included GVMs that were never even called: see `TypeGVMEntriesNode` that simply goes over everything on the type - we were adding a `TypeGVMEntriesNode` for all allocated types that have something to do with GVMs.

This PR changes it so that we only generate `TypeGVMEntries` for types that have at least one used GVM. This is a scoped down version of dotnet#118632 (that tries to track things per-method) with a lot less risk.
@MichalStrehovsky
Copy link
Member Author

This is problematic because we don't keep track of all MethodImpls that were relevant to resolve a virtual method. E.g. covariant returns get us into a situation where "to resolve method A on type B we need info about override C" doesn't hold up (there's another override in the inheritance hierarchy we need).

Scoped down version of this at #118704.

@MichalStrehovsky MichalStrehovsky deleted the gvmmdopt branch August 13, 2025 22:05
MichalStrehovsky added a commit that referenced this pull request Aug 14, 2025
Ever since GVM support was added to native AOT, we were generating the GVM resolution metadata for every type considered allocated. This included GVMs that were never even called: see `TypeGVMEntriesNode` that simply goes over everything on the type - we were adding a `TypeGVMEntriesNode` for all allocated types that have something to do with GVMs.

This PR changes it so that we only generate `TypeGVMEntries` for types that have at least one used GVM. This is a scoped down version of #118632 (that tries to track things per-method) with a lot less risk.
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant