Generic Virtual Methods Hashtables by fadimounir · Pull Request #2521 · dotnet/corert

fadimounir · 2017-01-17T20:27:51Z

GVM dependency tracking analysis and GVM table building logic:
1) GenericVirtualMethodTableNode: Node that builds the GVM hashtable blob for GVMs on classes
2) InterfaceGenericVirtualMethodTableNode: Node that builds the GVM hashtable blob for GVMs on interfaces
3) MethodLdTokenNode: Analysis node that tracks dependencies of GVMs for derived types and variant interfaces
4) TypeGVMEntriesNode: Analysis node that scans an input type and computes the list of GVM entries that would be added to the hashtables. Also used to track dependencies of GVM table entries (ex: native layout signatures of methods).

Couple of additions to the typesystem:
1) New helper method to compute whether a type has generic virtual methods
2) Concrete constraints validation for types and methods.

fadimounir · 2017-01-17T20:30:45Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

+            get
+            {
+                if(_type.IsInterface || _type.IsValueType || _type.Category == TypeFlags.Class)
+                    return _type.HasGenericVirtualMethod();


Is this check really necessary? I guessed that we probably use the EETypeNode for array EETypes too, that's why I added the check for interface/class/valuetype #Closed

The if check looks like a perf optimization and not much else. I don't mind either way, but consider making the check just _type.IsDefType if you want to keep it. I don't think there's value in doing 3 checks just to avoid nullables and enums. #Resolved

fadimounir · 2017-01-17T20:33:11Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodLdtokenNode.cs

+    /// This method node is used for dependency tracking, like in the case of GVMs for example.
+    /// </summary>
+    internal class MethodLdtokenNode : DependencyNodeCore<NodeFactory>
+    {


The naming "MethodLdTokenNode" is just to match NUTC's nodes in PN. I don't know if this node will be used for other non-GVM dependency tracking in the future, but if not, i'm leaning towards renaming this to something a bit different to reflect what this node does. Opinions?

My vote is to rename it. Even if we add more stuff to it, this will probably not have much in common with LDTOKEN.

fadimounir · 2017-01-17T20:34:55Z

@MichalStrehovsky @davidwrighton PTAL

MichalStrehovsky · 2017-01-17T20:48:56Z

Constraint validation should at minimum be a separate commit, but I would prefer a separate pull request. I can see a number of issues with it and I would prefer we don't lose focus due to the GVM changes:

ComputeTypeFlags on SignatureVariable doesn't look right. We need a better understanding about what's going on. ContainsGenericVariables is not used for anything right now and I've been meaning to delete it for a long time.
Special constraints are flags: you can have ReferenceTypeConstraint and DefaultConstructorConstraint at the same time. The code as proposed checks nothing in that case.
Nullable does not meet reference type constraints
We shouldn't intertwine the concept of constraint validation and general validation. E.g. List<void> is an invalid instantiation - not a failure to meet constraints of generic parameter 0.
Non-trivial typesystem code needs type system unit tests. At minimum, the constraint tests from the DynamicGenerics test should be ported to the type system unit test suite.
General purpose typesystem code should not have references to Canonical or RuntimeDetermined stuff. I would want to delete the constraint checking in src\System.Private.Reflection.Execution\src\Internal\Reflection\Execution\TypeLoader\ConstraintValidator.cs when this is checked in (and validate in the type loader), but the runtime type system doesn't include any of the .RuntimeDetermined.cs partial class files (because we don't need them). Constraint validation shouldn't rely on anything that's not in the Common directory of the TypeSystem.

Was this code ported from NUTC/reflection (so that we can compare it against that), or written from scratch? #Resolved

fadimounir · 2017-01-17T20:55:44Z

I'll separate the constraints validation to a separate PR. You can ignore it from here.
This code is ported from NUTC.

In reply to: 273295123 [](ancestors = 273295123)

MichalStrehovsky

I didn't review the actual dependency tracking part because it's confusing and I'll need a few rounds of meditation before I have anything constructive.

MichalStrehovsky · 2017-01-17T23:28:38Z

src/Common/src/TypeSystem/Common/TypeSystemHelpers.cs

+        /// <summary>
+        /// Gets a value indicating whether this type has any generic virtual methods.
+        /// </summary>
+        public static bool HasGenericVirtualMethod(this TypeDesc type)


This file has extension methods that the type system itself uses.

Since the type system doesn't use this one, it would be more fitting to put it in src\ILCompiler.Compiler\src\Compiler\TypeExtensions.cs (that has extension methods related to codegen and such). #Resolved

MichalStrehovsky · 2017-01-17T23:30:02Z

src/Common/src/TypeSystem/Common/TypeSystemHelpers.cs

+        /// </summary>
+        public static bool HasGenericVirtualMethod(this TypeDesc type)
+        {
+            foreach (var method in type.GetMethods())


This should be GetAllMethods().

(If we ever have typesystem context-injected generic virtual methods, this method should still report true.) #Resolved

MichalStrehovsky · 2017-01-17T23:40:45Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

+            get
+            {
+                if(_type.IsInterface || _type.IsValueType || _type.Category == TypeFlags.Class)
+                    return _type.HasGenericVirtualMethod();


The if check looks like a perf optimization and not much else. I don't mind either way, but consider making the check just _type.IsDefType if you want to keep it. I don't think there's value in doing 3 checks just to avoid nullables and enums. #Resolved

MichalStrehovsky · 2017-01-17T23:41:35Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

            return null;
        }

+        public override bool InterestingForDynamicDependencyAnalysis


Should this be on the ConstructedEETypeNode? If a type wasn't allocated with newobj, we don't care about any of it's virtual methods because they can't be called. #Resolved

Based on your offline explanation to me of what a ConstructedEETypeNode is used for, yes this should go there :)

In reply to: 96538247 [](ancestors = 96538247)

MichalStrehovsky · 2017-01-17T23:47:24Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs

+            // Generated type contains generic virtual methods that will get added to the GVM tables.
+            // The entries in the GVM tables are expressed in open generic definitions, so any generic 
+            // definition type that has generic virtual methods will need some dependency tracking.
+            if (TypeGVMEntriesNode.TypeNeedsGVMTableEntries(_type))


There's no guarantee that a GenericDefinitionEETypeNode will be generated in the current compilation:

Consider multifile mode where assembly A contains the type definition for Foo<T> (but doesn't use it in any way). The EEType node that represents the generic definition is generated into A.obj though, because that's the home module of the definition. Now assembly B allocates Foo<object> and calls a GVM method on it. EETypeNode for Foo<object> is generated in B.obj, but not the generic definition. The generic definition will be just an external symbol reference.

If analysis assumes we will be generating the generic definition EEType, these dependencies have to come in different way. #Resolved

I see. I'll remove this from here. This will definitely need testing when multimodule mode is enabled

In reply to: 96538983 [](ancestors = 96538983)

MichalStrehovsky · 2017-01-18T00:27:58Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodLdtokenNode.cs

+    /// This method node is used for dependency tracking, like in the case of GVMs for example.
+    /// </summary>
+    internal class MethodLdtokenNode : DependencyNodeCore<NodeFactory>
+    {


My vote is to rename it. Even if we add more stuff to it, this will probably not have much in common with LDTOKEN.

MichalStrehovsky · 2017-01-18T00:56:23Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodLdtokenNode.cs

+                }
+
+                // Open generic types are not interesting.
+                if (potentialOverrideType.ContainsGenericVariables)


This should be IsGenericDefinition. Checking ContainsGenericVariables requires a walk of the composition to compute. I'm always suspicious when I see ContainsGenericVariables getting used. In fact, the existing code doesn't use it for anything, ever.

We never have EEType nodes for things where IsGenericDefinition and ContainsGenericVariables would give different answers. #Resolved

There is no need to check for this... There should never be any open generics at this level

MichalStrehovsky · 2017-01-18T00:57:42Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

+        public override bool HasDynamicDependencies => false;
+        public override bool InterestingForDynamicDependencyAnalysis => false;
+        public override bool StaticDependenciesAreComputed => true;
+        protected override string GetName() => null;


This needs a name for the log. #Resolved

MichalStrehovsky · 2017-01-18T01:20:35Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

+
+        public static bool TypeNeedsGVMTableEntries(TypeDesc type)
+        {
+            if (type.Category != TypeFlags.Class && !type.IsValueType)


This is rather hard to read. What is the intent? Is it:

if (!type.IsDefType || type.IsInterface) return false;

Or something else? #Resolved

Using .IsDefType is more readable I agree :). I just didn't know about it before

In reply to: 96550314 [](ancestors = 96550314)

MichalStrehovsky · 2017-01-18T01:28:52Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodLdtokenNode.cs

+                    if (_method.IsCanonicalMethod(CanonicalFormKind.Specific))
+                        return false;
+
+                    if (_method.OwningType.ContainsGenericVariables || _method.ContainsGenericVariables)


Can we make this a check for generic definition instead? Or does the code in this pull request actually generate things such as a Foo instantiated over IFoo's T and the like that would warrant this expensive check?

MichalStrehovsky · 2017-01-18T19:54:30Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

+            {
+                foreach (var method in iface.GetMethods())
+                {
+                    if (!method.HasInstantiation)


This should also filter out static methods on interfaces. #Resolved

Good point! I keep forgetting about static methods on interfaces

In reply to: 96720291 [](ancestors = 96720291)

MichalStrehovsky · 2017-01-18T20:03:43Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunHelperNode.cs

+
+                // If the target method is an exact (non-canonical) instantiation, we'll need an entry for it in the exact method instantiations 
+                // hashtable, so we append the entry's dependencies to the method here.
+                var exactMethodInstantiationDependencies = ExactMethodInstantiationsNode.GetExactMethodInstantiationDependenciesForMethod(factory, method);


How does an entry for this end up in the ExactMethodInstantiations hashtable though? It seems like we generate dependencies for a thing that never actually lands in the hashtable. #Resolved

It should have been the other way around (this was some very old code I added to my branch). We should track the method entry point here, not the hashtable dependencies.
Hashtable entries will be created from the MethodCodeNodes that we would create from here

In reply to: 96722057 [](ancestors = 96722057)

MichalStrehovsky · 2017-01-18T22:52:07Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

+            return type.HasGenericVirtualMethod();
+        }
+
+        public void ScanForGenericVirtualMethodEntries(NodeFactory factory, Action<NodeFactory, MethodDesc, MethodDesc> gvmEntryFoundCallback)


Did you consider making this method return an IEnumerable (of a custom helper struct that has two fields for the declaring and implementing method in it)?

The callback delegate makes the code rather hard to follow because it requires the reader to inspect bodies of 3 methods to find out what's going on. #Resolved

I hadn't considered making it return an IEnumerable. I like that better than the callback :)

In reply to: 96753917 [](ancestors = 96753917)

fadimounir · 2017-01-19T01:19:37Z

cc @yizhang82

davidwrighton · 2017-01-23T19:30:07Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs

+        {
+            get
+            {
+                return _type.IsDefType && _type.HasGenericVirtualMethod();


Do we need the check for if this type is a RuntimeDeterminedSubtype here? I'm not familiar if RuntimeDetermined things can appear in ConstructedEETypeNodes #Closed

ConstructedEETypeNode is an ObjectNode and represents an EEType. It needs to be concrete, and we assert that. EEType for a non-exact type wouldn't make sense. #Closed

The base type, EETypeNode, explicitly has Debug.Assert(!type.IsRuntimeDeterminedSubtype); #Closed

davidwrighton · 2017-01-23T19:39:04Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GVMDependenciesNode.cs

+                derivedMethodInstantiation = derivedMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Universal);
+            }
+
+            // TODO: verify for invalid instantiations, like List<void>?


// TODO: verify for invalid instantiations, like List? [](start = 12, length = 60)

Is this TODO not handled by the constraints check below? #WontFix

No... Michal was suggesting we have the constraints validation just do constraints validations, and not check for invalid instantiations. If we need invalid instantiations validation, let it be separate from constraints validation. #WontFix

davidwrighton · 2017-01-23T19:43:07Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GVMDependenciesNode.cs

+                                slotDecl = currentType.ResolveInterfaceMethodToVirtualMethodOnType(genericDefinition);
+                                currentType = currentType.BaseType;
+                            }
+                            while (slotDecl == null && currentType != null);


This is a very odd algorithm. As such we should make it testable. Could you put it into the typesystem dll as an extension method or something, and then write some tests? #Resolved

Ok. I'll do it as a cleanup thing before merging the PR #Resolved

I'd like to see the tests before you checkin. Please ping me again when you're done writing up tests.

In reply to: 97399311 [](ancestors = 97399311)

fadimounir · 2017-01-23T20:00:56Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

+                    }
+                    while (slotDecl == null && currentType != null);
+
+                    Debug.Assert(slotDecl != null);


I will remove this assert. I think it is wrong (ex: abstract type that doesn't implement all interface methods of all implemented interfaces). I'll also add a test for it in my GVM unit test (which i'll send later in another PR) #Resolved

davidwrighton · 2017-01-23T21:58:20Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericVirtualMethodTableNode.cs

+        protected override string GetName() => this.GetMangledName();
+
+        public static DependencyList GetGenericVirtualMethodImplementationDependencies(NodeFactory factory, MethodDesc callingMethod, MethodDesc implementationMethod)
+        {


Please comment as to when and why this method is to be used. #Resolved

davidwrighton · 2017-01-23T23:03:50Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericVirtualMethodTableNode.cs

+            var openImplementationMethodNameAndSig = factory.NativeLayout.MethodNameAndSignatureVertex(openImplementationMethod);
+
+            dependencyNodes.Add(new DependencyListEntry(factory.NativeLayout.PlacedSignatureVertex(openCallingMethodNameAndSig), "gvm table needed signature"));
+            dependencyNodes.Add(new DependencyListEntry(factory.NativeLayout.PlacedSignatureVertex(openImplementationMethodNameAndSig), "gvm table needed signature"));


gvm table needed signature [](start = 137, length = 26)

Please use different strings for dependency node additions wherever possible. That assists with debugging this later. #Resolved

davidwrighton · 2017-01-23T23:04:43Z

...LCompiler.Compiler/src/Compiler/DependencyAnalysis/InterfaceGenericVirtualMethodTableNode.cs

+            var openCallingMethodNameAndSig = factory.NativeLayout.MethodNameAndSignatureVertex(openCallingMethod);
+            var openImplementationMethodNameAndSig = factory.NativeLayout.MethodNameAndSignatureVertex(openImplementationMethod);
+
+            dependencyNodes.Add(new DependencyListEntry(factory.NativeLayout.PlacedSignatureVertex(openCallingMethodNameAndSig), "gvm table needed signature"));


gvm table needed signature [](start = 130, length = 26)

Please make all of these strings unique #Resolved

davidwrighton · 2017-01-23T23:05:45Z

1) GenericVirtualMethodTableNode: Node that builds the GVM hashtable blob for GVMs on classes 2) InterfaceGenericVirtualMethodTableNode: Node that builds the GVM hashtable blob for GVMs on interfaces 3) MethodLdTokenNode: Analysis node that tracks dependencies of GVMs for derived types and variant interfaces 4) TypeGVMEntriesNode: Analysis node that scans an input type and computes the list of GVM entries that would be added to the hashtables. Also used to track dependencies of GVM table entries (ex: native layout signatures of methods). Other 1) New helper method to compute whether a type has generic virtual methods

fadimounir requested review from MichalStrehovsky and davidwrighton January 17, 2017 20:27

dnfclas added the cla-already-signed label Jan 17, 2017

fadimounir commented Jan 17, 2017

View reviewed changes

fadimounir force-pushed the gvm_tables branch from 47c4488 to 90cbcc4 Compare January 18, 2017 01:28

MichalStrehovsky reviewed Jan 18, 2017

View reviewed changes

fadimounir force-pushed the gvm_tables branch 4 times, most recently from 4eee69e to 362bf7c Compare January 19, 2017 01:09

davidwrighton reviewed Jan 23, 2017

View reviewed changes

fadimounir commented Jan 23, 2017

View reviewed changes

davidwrighton reviewed Jan 23, 2017

View reviewed changes

fadimounir force-pushed the gvm_tables branch from 11d175d to 3d79516 Compare January 24, 2017 00:28

fadimounir merged commit e7d315e into dotnet:master Jan 24, 2017

fadimounir deleted the gvm_tables branch February 1, 2017 22:00

MichalStrehovsky mentioned this pull request Feb 24, 2023

Split generic virtual method slot use and impl tracking dotnet/runtime#82222

Merged

Conversation

fadimounir commented Jan 17, 2017

Uh oh!

fadimounir Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fadimounir commented Jan 17, 2017

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 17, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 18, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 18, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 18, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 18, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Jan 18, 2017 • edited by fadimounir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir Jan 17, 2017 •

edited

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

fadimounir commented Jan 17, 2017 •

edited

Loading

MichalStrehovsky commented Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 17, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 18, 2017 •

edited by fadimounir

Loading

davidwrighton Jan 23, 2017 •

edited by fadimounir

Loading

MichalStrehovsky Jan 23, 2017 •

edited by fadimounir

Loading

fadimounir Jan 23, 2017 •

edited

Loading

davidwrighton Jan 23, 2017 •

edited by fadimounir

Loading

fadimounir Jan 23, 2017 •

edited

Loading

davidwrighton Jan 23, 2017 •

edited by fadimounir

Loading

fadimounir Jan 23, 2017 •

edited

Loading

fadimounir Jan 23, 2017 •

edited

Loading

davidwrighton Jan 23, 2017 •

edited by fadimounir

Loading

davidwrighton Jan 23, 2017 •

edited by fadimounir

Loading