Split generic virtual method slot use and impl tracking #82222

MichalStrehovsky · 2023-02-16T08:48:35Z

I'm looking at generic virtual method again because of #80602.

The analysis of generic virtual methods within the compiler is an N * M algorithm where N is the number of unique generic virtual method instantiations called and M is the number of types that implement generic virtual methods.

We use dynamic dependencies within the dependency analysis engine to model this relationship.

It is important to try to limit the N and M. Looking at things, I realized the N we're currently operating on is bigger than it needs to be:

Foo f = new Bar();
f.Blah<int>();
f = new Baz();
f.Blah<double>();

class Foo { public virtual void Blah<T>() { } }
class Bar : Foo { public override void Blah<T> { } }
class Baz : Foo { public override void Blah<T> { } }

Previously, the analysis would see M = 3 and N = 6 because we would track each of the overrides as something that needs to be considered for each M. This changes the analysis to only look at the definition of the slot, i.e. N = 2 (one for int, other for double).

The result of the analysis will still be same, it will just take less time. The new GenericVirtualMethodImpl node responds false to HasDynamicDependencies and doesn't participate in expensive activities.

Cc @dotnet/ilc-contrib

I'm looking at generic virtual method again because of dotnet#80602. The analysis of generic virtual methods within the compiler is an N * M algorithm where N is the number of unique generic virtual method instantiations called and M is the number of types that implement generic virtual methods. We use dynamic dependencies within the dependency analysis engine to model this relationship. It is important to try to limit the N and M. Looking at things, I realized the N we're currently operating on is bigger than it needs to be: ```csharp Foo f = new Bar(); f.Blah<int>(); f = new Baz(); f.Blah<double>(); class Foo { public virtual void Blah<T>() { } } class Bar : Foo { public override void Blah<T> { } } class Baz : Foo { public override void Blah<T> { } } ``` Previously, the analysis would see M = 3 and N = 6 because we would track each of the overrides as something that needs to be considered for each M. This changes the analysis to only look at the definition of the slot, i.e. N = 2 (one for int, other for double). The result of the analysis will still be same, it will just take less time. The new GenericVirtualMethodImpl node responds false to HasDynamicDependencies and doesn't participate in expensive activities.

ghost · 2023-02-16T08:48:44Z

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

Issue Details

I'm looking at generic virtual method again because of #80602.

The analysis of generic virtual methods within the compiler is an N * M algorithm where N is the number of unique generic virtual method instantiations called and M is the number of types that implement generic virtual methods.

We use dynamic dependencies within the dependency analysis engine to model this relationship.

It is important to try to limit the N and M. Looking at things, I realized the N we're currently operating on is bigger than it needs to be:

Foo f = new Bar();
f.Blah<int>();
f = new Baz();
f.Blah<double>();

class Foo { public virtual void Blah<T>() { } }
class Bar : Foo { public override void Blah<T> { } }
class Baz : Foo { public override void Blah<T> { } }

Previously, the analysis would see M = 3 and N = 6 because we would track each of the overrides as something that needs to be considered for each M. This changes the analysis to only look at the definition of the slot, i.e. N = 2 (one for int, other for double).

The result of the analysis will still be same, it will just take less time. The new GenericVirtualMethodImpl node responds false to HasDynamicDependencies and doesn't participate in expensive activities.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

MichalStrehovsky · 2023-02-16T09:30:34Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-16T09:31:00Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2023-02-17T05:55:56Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-17T05:56:18Z

Azure Pipelines successfully started running 1 pipeline(s).

agocke

Added some questions, because I'm not quite sure what this PR is doing.

agocke · 2023-02-23T20:29:09Z

...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs

+
+            // This is either a generic virtual method or a MethodImpl for a static interface method.
+            // We can't test for static MethodImpl so at least sanity check it's static and noninterface.
+            Debug.Assert(method.IsVirtual || (method.Signature.IsStatic && !method.OwningType.IsInterface));


What about default interface implementations? In that case, isn't the method owned by an interface?

This will change with #80602 because we'll run into the assert. We don't currently track interfaces because they're assumed to be associated with a class (default implementations just show up on the owning class). This works for instance methods. It doesn't work for statics. It's a bit of a overhaul which is why I didn't want to do it in one go.

agocke · 2023-02-23T20:29:49Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs

@@ -10,6 +10,8 @@
 namespace ILCompiler.DependencyAnalysis
 {
    /// <summary>
+    /// Represents a use of a generic virtual method slot. This node only tracks


It's not quite clear to me what a "slot" means in this context.

Slot in the sense of ECMA-335 - either a newslot method, or virtual that doesn't have another virtual method with the same name/sig in the inheritance hierarchy.

agocke · 2023-02-23T20:31:26Z

...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs

+                    _method.OwningType.Instantiation.CheckValidInstantiationArguments() &&
+                    _method.CheckConstraints());
+
+            if (validInstantiation)


By "valid" here, do we mean that the code is not broken? That the compiler hasn't written incorrect IL?

This is a cargo cult I brought over from the previous spot. I don't know when it kicks in. It goes back all the way to dotnet/corert#2521.

agocke · 2023-02-23T20:32:13Z

...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs

+
+            if (validInstantiation)
+            {
+                if (factory.TypeSystemContext.SupportsUniversalCanon && _method.IsGenericDepthGreaterThan(UniversalCanonGVMDepthHeuristic_CanonDepth))


I don't think I understand the "generic depth" thing here.

Copy paste from the previous spot - if universal shared generics are supported, we cut off analysis early if it gets too deep and leave universal shared code to handle it. SupportsUniversalCanon is always false in NativeAOT.

agocke · 2023-02-23T20:33:11Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs

                    if (_method.HasInstantiation)
                    {
-                        dependencies.Add(factory.GVMDependencies(_method.GetCanonMethodTarget(CanonicalFormKind.Specific)), "GVM callable reflectable method");
+                        // FindSlotDefiningMethod might uninstantiate. We might want to fix the method not to do that.
+                        if (slotDefiningMethod.IsMethodDefinition)


I definitely don't understand what "uninstantiate" means here.

Might remove the instantiation of the generic method. Sometimes. I think we just don't call this method with generic methods. I didn't want to fix this here because we call this method in two dozen places and it would require carefully reviewing each. So we just instantiate the method back if it became uninstantiated.

agocke · 2023-02-23T20:35:28Z

...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs

+
+        public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory context) => null;
+
+        public override bool HasDynamicDependencies => false;


It seems like this is important, but it's not clear to me why.

All nodes have HasDynamicDependencies false, except for GVMDependencies node.

Returning true means the dependency analysis will call into SearchDynamicDependencies after every iteration of the graph expansion with a list of all nodes we've seen so far.

This node not reporting it is the optimization.

And why is this a valid optimization? Or, why is it necessary for GVMDependencies to always return true?

GVMDependencies needs to look at each type in the graph that has something to do with generic virtual methods to check whether the type provides an override of the slot. If so, it needs to instantiate the override with whatever generic method instantiation this GVMDependencies has.

But we don't need to do this for generic virtual method implementations that don't define a new slot (i.e. they're just overrides) - this is what the optimization tries to split - a slot use + an override ("implementation", since we also introduce this node for non abstract new slots) of the slot.

agocke

LGTM, I think I understand this code more now. Just a few follow-up questions

agocke · 2023-02-27T22:33:27Z

...lr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs

+
+        public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory context) => null;
+
+        public override bool HasDynamicDependencies => false;


And why is this a valid optimization? Or, why is it necessary for GVMDependencies to always return true?

MichalStrehovsky added the area-NativeAOT-coreclr label Feb 16, 2023

ghost assigned MichalStrehovsky Feb 16, 2023

build-analysis bot mentioned this pull request Feb 16, 2023

Tracking issue for CI build timeouts #76454

Closed

MichalStrehovsky added 2 commits February 17, 2023 11:04

Fix CHK failures

c4e7dfe

GVMDependencies must be virtual - this is now unreachable

a63df6d

build-analysis bot mentioned this pull request Feb 17, 2023

OSX infra issue - prereq check for 'pkg-config' missing #82240

Closed

build-analysis bot mentioned this pull request Feb 17, 2023

SupportedLinuxPlatforms_IsSupportedIsTrue fails on distros without quic #82077

Closed

jakobbotsch mentioned this pull request Feb 17, 2023

[QUIC] Missing libmsquic on tested platforms #81901

Closed

4 tasks

agocke reviewed Feb 23, 2023

View reviewed changes

agocke approved these changes Feb 27, 2023

View reviewed changes

MichalStrehovsky merged commit 443c42b into dotnet:main Feb 27, 2023

MichalStrehovsky deleted the splitgvm branch February 27, 2023 23:49

ghost locked as resolved and limited conversation to collaborators Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split generic virtual method slot use and impl tracking #82222

Split generic virtual method slot use and impl tracking #82222

MichalStrehovsky commented Feb 16, 2023

ghost commented Feb 16, 2023

MichalStrehovsky commented Feb 16, 2023

azure-pipelines bot commented Feb 16, 2023

MichalStrehovsky commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

agocke left a comment

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 23, 2023

MichalStrehovsky Feb 24, 2023

agocke Feb 27, 2023

MichalStrehovsky Feb 27, 2023

agocke left a comment

agocke Feb 27, 2023


		public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory context) => null;

		public override bool HasDynamicDependencies => false;

Split generic virtual method slot use and impl tracking #82222

Split generic virtual method slot use and impl tracking #82222

Conversation

MichalStrehovsky commented Feb 16, 2023

ghost commented Feb 16, 2023

MichalStrehovsky commented Feb 16, 2023

azure-pipelines bot commented Feb 16, 2023

MichalStrehovsky commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment