From ea1e6f3b669f0288386946578462b1e53c814e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 16 Feb 2023 17:47:50 +0900 Subject: [PATCH 1/3] Split generic virtual method slot use and impl tracking 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: ```csharp Foo f = new Bar(); f.Blah(); f = new Baz(); f.Blah(); class Foo { public virtual void Blah() { } } class Bar : Foo { public override void Blah { } } class Baz : Foo { public override void Blah { } } ``` 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. --- .../DependencyAnalysis/GVMDependenciesNode.cs | 53 ++++--------- .../GenericVirtualMethodImplNode.cs | 78 +++++++++++++++++++ .../DependencyAnalysis/NodeFactory.cs | 11 +++ .../ILCompiler.Compiler.csproj | 1 + 4 files changed, 103 insertions(+), 40 deletions(-) create mode 100644 src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs index f7b1bd694b8a09..46e6189b10a419 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs @@ -10,6 +10,8 @@ namespace ILCompiler.DependencyAnalysis { /// + /// Represents a use of a generic virtual method slot. This node only tracks + /// the use of the slot definition. /// This analysis node is used for computing GVM dependencies for the following cases: /// 1) Derived types where the GVM is overridden /// 2) Variant-interfaces GVMs @@ -19,17 +21,14 @@ namespace ILCompiler.DependencyAnalysis /// public class GVMDependenciesNode : DependencyNodeCore { - private const int UniversalCanonGVMDepthHeuristic_CanonDepth = 2; private readonly MethodDesc _method; public GVMDependenciesNode(MethodDesc method) { Debug.Assert(method.GetCanonMethodTarget(CanonicalFormKind.Specific) == method); Debug.Assert(method.HasInstantiation); - - // 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)); + Debug.Assert(method.IsVirtual); + Debug.Assert(MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method) == method); _method = method; } @@ -39,41 +38,10 @@ public GVMDependenciesNode(MethodDesc method) public override bool StaticDependenciesAreComputed => true; protected override string GetName(NodeFactory factory) => "__GVMDependenciesNode_" + factory.NameMangler.GetMangledMethodName(_method); - public override IEnumerable GetStaticDependencies(NodeFactory context) + public override IEnumerable GetStaticDependencies(NodeFactory factory) { - DependencyList dependencies = null; - - context.MetadataManager.GetDependenciesDueToVirtualMethodReflectability(ref dependencies, context, _method); - if (!_method.IsAbstract) - { - bool validInstantiation = - _method.IsSharedByGenericInstantiations || ( // Non-exact methods are always valid instantiations (always pass constraints check) - _method.Instantiation.CheckValidInstantiationArguments() && - _method.OwningType.Instantiation.CheckValidInstantiationArguments() && - _method.CheckConstraints()); - - if (validInstantiation) - { - if (context.TypeSystemContext.SupportsUniversalCanon && _method.IsGenericDepthGreaterThan(UniversalCanonGVMDepthHeuristic_CanonDepth)) - { - // fall back to using the universal generic variant of the generic method - return dependencies; - } - - bool getUnboxingStub = _method.OwningType.IsValueType && !_method.Signature.IsStatic; - dependencies ??= new DependencyList(); - dependencies.Add(context.MethodEntrypoint(_method, getUnboxingStub), "GVM Dependency - Canon method"); - - if (_method.IsSharedByGenericInstantiations) - { - dependencies.Add(context.NativeLayout.TemplateMethodEntry(_method), "GVM Dependency - Template entry"); - dependencies.Add(context.NativeLayout.TemplateMethodLayout(_method), "GVM Dependency - Template"); - } - } - } - - return dependencies; + yield return new DependencyListEntry(factory.GenericVirtualMethodImpl(_method), "Implementation of the generic virtual method"); } public override IEnumerable GetConditionalStaticDependencies(NodeFactory context) => null; @@ -173,7 +141,12 @@ public override IEnumerable SearchDynamicDependenci for (int instArg = 0; instArg < openInstantiation.Length; instArg++) openInstantiation[instArg] = context.GetSignatureVariable(instArg, method: true); MethodDesc implementingMethodInstantiation = slotDecl.MakeInstantiatedMethod(openInstantiation).InstantiateSignature(potentialOverrideType.Instantiation, _method.Instantiation); - dynamicDependencies.Add(new CombinedDependencyListEntry(factory.GVMDependencies(implementingMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Specific)), null, "ImplementingMethodInstantiation")); + + // Static virtuals cannot be further overriden so this is an impl use. Otherwise it's a virtual slot use. + if (implementingMethodInstantiation.Signature.IsStatic) + dynamicDependencies.Add(new CombinedDependencyListEntry(factory.GenericVirtualMethodImpl(implementingMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Specific)), null, "ImplementingMethodInstantiation")); + else + dynamicDependencies.Add(new CombinedDependencyListEntry(factory.GVMDependencies(implementingMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Specific)), null, "ImplementingMethodInstantiation")); factory.MetadataManager.NoteOverridingMethod(_method, implementingMethodInstantiation); } @@ -225,7 +198,7 @@ public override IEnumerable SearchDynamicDependenci if (instantiatedTargetMethod != _method) { dynamicDependencies.Add(new CombinedDependencyListEntry( - factory.GVMDependencies(instantiatedTargetMethod), null, "DerivedMethodInstantiation")); + factory.GenericVirtualMethodImpl(instantiatedTargetMethod), null, "DerivedMethodInstantiation")); factory.MetadataManager.NoteOverridingMethod(_method, instantiatedTargetMethod); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs new file mode 100644 index 00000000000000..b7be9dd525f5c8 --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs @@ -0,0 +1,78 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Collections.Generic; + +using ILCompiler.DependencyAnalysisFramework; + +using Internal.TypeSystem; + +namespace ILCompiler.DependencyAnalysis +{ + /// + /// Represents a use of a generic virtual method body implementation. + /// + public class GenericVirtualMethodImplNode : DependencyNodeCore + { + private const int UniversalCanonGVMDepthHeuristic_CanonDepth = 2; + private readonly MethodDesc _method; + + public GenericVirtualMethodImplNode(MethodDesc method) + { + Debug.Assert(method.GetCanonMethodTarget(CanonicalFormKind.Specific) == method); + Debug.Assert(method.HasInstantiation); + + // 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)); + + _method = method; + } + + public override bool HasConditionalStaticDependencies => false; + public override bool InterestingForDynamicDependencyAnalysis => false; + public override bool StaticDependenciesAreComputed => true; + protected override string GetName(NodeFactory factory) => "__GVMImplNode_" + factory.NameMangler.GetMangledMethodName(_method); + + public override IEnumerable GetStaticDependencies(NodeFactory factory) + { + DependencyList dependencies = null; + + factory.MetadataManager.GetDependenciesDueToVirtualMethodReflectability(ref dependencies, factory, _method); + + bool validInstantiation = + _method.IsSharedByGenericInstantiations || ( // Non-exact methods are always valid instantiations (always pass constraints check) + _method.Instantiation.CheckValidInstantiationArguments() && + _method.OwningType.Instantiation.CheckValidInstantiationArguments() && + _method.CheckConstraints()); + + if (validInstantiation) + { + if (factory.TypeSystemContext.SupportsUniversalCanon && _method.IsGenericDepthGreaterThan(UniversalCanonGVMDepthHeuristic_CanonDepth)) + { + // fall back to using the universal generic variant of the generic method + return dependencies; + } + + bool getUnboxingStub = _method.OwningType.IsValueType && !_method.Signature.IsStatic; + dependencies ??= new DependencyList(); + dependencies.Add(factory.MethodEntrypoint(_method, getUnboxingStub), "GVM Dependency - Canon method"); + + if (_method.IsSharedByGenericInstantiations) + { + dependencies.Add(factory.NativeLayout.TemplateMethodEntry(_method), "GVM Dependency - Template entry"); + dependencies.Add(factory.NativeLayout.TemplateMethodLayout(_method), "GVM Dependency - Template"); + } + } + + return dependencies; + } + + public override IEnumerable GetConditionalStaticDependencies(NodeFactory context) => null; + + public override bool HasDynamicDependencies => false; + + public override IEnumerable SearchDynamicDependencies(List> markedNodes, int firstNode, NodeFactory factory) => null; + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index ba0f5be86ea0f6..7a22f2bbc7b7ec 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -284,6 +284,11 @@ private void CreateNodeCaches() return new GVMDependenciesNode(method); }); + _gvmImpls = new NodeCache(method => + { + return new GenericVirtualMethodImplNode(method); + }); + _gvmTableEntries = new NodeCache(type => { return new TypeGVMEntriesNode(type); @@ -931,6 +936,12 @@ public GVMDependenciesNode GVMDependencies(MethodDesc method) return _gvmDependenciesNode.GetOrAdd(method); } + private NodeCache _gvmImpls; + public GenericVirtualMethodImplNode GenericVirtualMethodImpl(MethodDesc method) + { + return _gvmImpls.GetOrAdd(method); + } + private NodeCache _gvmTableEntries; internal TypeGVMEntriesNode TypeGVMEntries(TypeDesc type) { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj b/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj index 0cc8c90ca01446..ea72bb4d6fe76f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj @@ -393,6 +393,7 @@ + From c4e7dfebdd9b292485007a84a12e4164f3be6f1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 17 Feb 2023 11:04:07 +0900 Subject: [PATCH 2/3] Fix CHK failures --- .../Compiler/DependencyAnalysis/ReflectedMethodNode.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs index 0a4a1ef59ed815..729bf520192779 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs @@ -64,14 +64,17 @@ public override IEnumerable GetStaticDependencies(NodeFacto { if (_method.IsVirtual) { + // Virtual method use is tracked on the slot defining method only. + MethodDesc slotDefiningMethod = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(_method); 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) + slotDefiningMethod = factory.TypeSystemContext.GetInstantiatedMethod(slotDefiningMethod, _method.Instantiation); + dependencies.Add(factory.GVMDependencies(slotDefiningMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)), "GVM callable reflectable method"); } else { - // Virtual method use is tracked on the slot defining method only. - MethodDesc slotDefiningMethod = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(_method); if (!factory.VTable(slotDefiningMethod.OwningType).HasFixedSlots) dependencies.Add(factory.VirtualMethodUse(slotDefiningMethod), "Virtually callable reflectable method"); } From a63df6d7e2e0ae70b6998f390cd454f9eee88e3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 17 Feb 2023 11:52:04 +0900 Subject: [PATCH 3/3] GVMDependencies must be virtual - this is now unreachable --- .../Compiler/DependencyAnalysis/GVMDependenciesNode.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs index 46e6189b10a419..28b2a79c93d4d5 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs @@ -57,14 +57,6 @@ public override bool HasDynamicDependencies (methodOwningType.IsSealed() || _method.IsFinal)) return false; - // We model static (non-virtual) methods that are MethodImpls for a static interface method. - // But those cannot be overriden (they're not virtual to begin with). - if (!methodOwningType.IsInterface && _method.Signature.IsStatic) - { - Debug.Assert(!_method.IsVirtual); - return false; - } - return true; } }