[r2r] Include generic type instantiations from GenericLookupSignature for discovery of virtual methods

[BrzVlad]

Normally, we detect the usage of a type in the application via the TypeFixupSignature and then we use this type to determine which virtual methods on the type need compilation (via supporting GVMDependenciesNode and VirtualMethodUseNode).

Consider the following scenario:

public class TestA<T, U>
{
    public virtual void TestMethod(T item) => Console.WriteLine($"{item} / {typeof(U)}");
}

public class TestB<T, U>
{
    public TestA<T, U> TestCreate() => new TestA<T, U>();
}

TestB<string, int> obj = new TestB<string, int>();
obj.TestCreate().TestMethod("hello");

When compiling TestB<Canon,int32>.TestCreate we need to create an instance of TestA<T,int32>. Because we are compiling a shared version we need to embed a call to a generic lookup helper in order to obtain the actual T. This is done with a GenericLookupSignature fixup, which contains the type TestA<Canon,int32> that needs resolving. When creating this node, we add the InheritedVirtualMethodsNode as a dependency which facilitates discovery of virtual methods on this type.

Fixes perf on linq sorting, ex: LinqBenchmarks.Order00LinqMethodX (~20x improvement)

[Copilot]

Pull request overview

This PR updates ReadyToRun dependency analysis so that when a type handle is obtained via a GenericLookupSignature (generic dictionary lookup), the compiler still records the type usage needed to discover and compile inherited virtual methods for that instantiated type. It also adds a focused regression test covering the scenario.

Changes:

• Add InheritedVirtualMethods dependency tracking for GenericLookupSignature when _fixupKind == ReadyToRunFixupKind.TypeHandle.
• Add a new VirtualMethodGenerics test case that reaches a generic type instantiation only via a GenericLookupSignature fixup.
• Extend R2RTestSuites with a new [Fact] that validates the relevant virtual method is compiled.

Reviewed changes

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

File	Description
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs	Adds dependency analysis to record generic-lookup type usage for virtual dispatch discovery (mirrors TypeFixupSignature’s behavior for this scenario).
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/VirtualMethodGenerics/GenericLookup.cs	New embedded test-case source driving a generic instantiation through a GenericLookupSignature-only path.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs	Adds an xUnit test validating TestA\2<__Canon,int>.TestMethod` is compiled as expected.

[BrzVlad]

@davidwrighton @MichalStrehovsky Did some measurements for this change. Increases the size by about 4% on composite framework compilation. Wanted to validate this change on a real mobile app, tested on the biggest app I've encountered so far. The size difference was still about the same 4%. Compared to mono, the coreclr size was still 70% of the mono size. Other measurements we did in the past showed that most of the time coreclr beats mono on size so I believe we have some flexibility in using more r2r to avoid unreasonable perf hits at runtime.

Having said this, I'm open to the option of having some extra argument to r2r, which would allow to disable all these features which can lead in theory to excessive generic compilation. In case some users do hit excessive size we could have this easy workaround/diagnosis. Any thoughts on this ?

[MichalStrehovsky]

Maybe a switch wouldn't hurt? I don't think we'd want to expose these switches to users directly, but on native AOT front we have a global OptimizationPreference switch that changes some of the policies.

This/these are the things we might want to tune for OptimizationPreference=Size. It will probably depend on real-world measurements. For example trading off 1% of startup for 5% of size might be something that is acceptable/expected for someone who sets OptimizationPreference=Size (it's still way better than R2R off completely). We don't document what exactly these switches do and it's deliberately, only tell people to measure perf and if they're happy with the tradeoff, they can use it...

OptimizationPreference is not currently looked at by crossgen. It potentially could.

[BrzVlad]

My concern with adding this under general Size/Speed optimization is that, as far as I'm aware, these knobs are doing less drastic adjustements, as compared to falling back to interpreter. As you said, squeeze some percentages from here to gain some percentages somewhere else. The problem with interpreter fallback is that it can produce regressions of 10-100x which can potentially make the app unusable, so I'm viewing this problem also a bit from the perspective of correctness.

Also, mobile was always size optimized (for example macios sets UseSizeOptimizedLinq=true by default). Looking into the OptimizationPreference, I've noticed that crossgen2 also has a --Os option which macios doesn't currently pass, but I believe it should by default (seems to reduce size by around 1.5%). So, while compiling these generic methods could be seen as a speed preference on iOS, I still believe the other parts should be kept as size optimized for now, since there is less justification for them to needlessly increase the size of the app.

I believe that, once we have an optimizing interpreter which reduces enough the gap between R2R and interpreter code, it would make more sense to guard r2r generic code expansion by this optimization preference. I know a customer at some point used the mono interpreter for the entire app and they didn't realize, since the app still had acceptable perf.

In conclusion, I would lean towards having an internal crossgen2 switch, just as a temporary fallback for now in case something goes wrong size-wise somewhere (ex --limit-generic-expansion). A longer term Size/Opt configuration could look something like this on crossgen2 side: Default = --optimize-size; SpeedOpt = --optimize-time; SizeOpt = --optimize-space --limit-generic-expansion. Does this sound reasonable ? cc also @vitek-karas @jkotas

[BrzVlad]

Making r2r use --Os by default on ios in #130128

[vitek-karas]

My main worry is predictability or you could call it consistency. As noted by Vlad the perf difference between AOT and Interpreter is very big right now and the compiler behavior is effectively unpredictable (or at least that's how it looks from the developer's perspective). With interpreter we have this problem regardless, but if we can I would prefer we don't make it worse.