Reduce duplicity in GVM analysis #83250
Conversation
There were two very similar pieces of logic in the generic virtual method analysis. First one was deciding whether we need to emit GVM bookkeeping about the type, the second one was deciding whether the type should participate in GVM analysis (the N*M algorithm, "for each type relevant to GVM analysis" * "GVM virtual method called"). Made this into one so that it's in one spot. The interface method resolution is not cheap so this also speeds up the compilation. We should also do this for non-generic virtuals at some point. I did a test pass with an assert that the analysis came up with the same conclusions to ensure I didn't mess up.
ghost
assigned 
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThere were two very similar pieces of logic in the generic virtual method analysis. First one was deciding whether we need to emit GVM bookkeeping about the type, the second one was deciding whether the type should participate in GVM analysis (the N*M algorithm, "for each type relevant to GVM analysis" * "GVM virtual method called"). Made this into one so that it's in one spot. The interface method resolution is not cheap so this also speeds up the compilation. We should also do this for non-generic virtuals at some point. I did a test pass with an assert that the analysis came up with the same conclusions to ensure I didn't mess up. Cc @dotnet/ilc-contrib
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| [Flags] | ||
| protected enum VirtualMethodAnalysisFlags | ||
| { |
There was a problem hiding this comment.
If we're going to use the 0 value I would give it a name, like "None"
There was a problem hiding this comment.
I'll add it, but generally disagree - None is not a flag. Having a named value for it makes it non-obvious whether we're setting a flag or if this means no flag is set. We have actual non-zero flags named None even in our own CoreLib.
| // We found a GVM on one of the implemented interfaces. Find if the type implements this method. | ||
| // (Note, do this comparison against the generic definition of the method, not the specific method instantiation | ||
| MethodDesc slotDecl = method.Signature.IsStatic ? | ||
| defType.ResolveInterfaceMethodToStaticVirtualMethodOnType(method) |
There was a problem hiding this comment.
Why have two different methods here? Could we simplify to just ResolveInterfaceMethodTarget that could handle static and non-static members?
There was a problem hiding this comment.
We might be able to simplify to one method once all type system consumers can deal with static methods. crossgen2 currently still doesn't handle static virtuals and I'm not sure if changing this could break it.
(This is a copy from one of the places that I'm unifying - not new code).
| // these all will be looked at for every unique generic virtual method call in the program. | ||
| // Having a long list of interesting types affects the compilation throughput heavily. | ||
| if (slotDecl.OwningType == defType || | ||
| defType.BaseType.ResolveInterfaceMethodTarget(method) != slotDecl) |
There was a problem hiding this comment.
What's an example of when this could happen?
My understanding of the code is that it's checking to see if the impl of the interface in the base type is different, but only when the current impl isn't declared in the current type. That would seem to imply to me that the impl is somehow being pulled from above the base type, which seems impossible at first glance.
There was a problem hiding this comment.
Example is a twist on the sample 30 lines above.
// interface IFace {
// void IFaceGVMethod<U>();
// }
// class BaseClass {
// public virtual void IFaceGVMethod<U>() { ... }
// }
// public class DerivedClass : BaseClass, IFace { }
//
Move the IFace implementation down to BaseClass. Now if we try to resolve IFaceGVMethod on DerivedClass we get a method from the base, but it already implemented the interface in the base. So we don't have to think about it in the context of DerivedClass. Removing DerivedClass from the N*M algorithm is a compile throughput improvement.
(This is a copy from one of the places that I'm unifying - not new code).
There was a problem hiding this comment.
Sorry, still not getting it. If the example code is
// interface IFace {
// void IFaceGVMethod<U>();
// }
// class BaseClass : IFace {
// public virtual void IFaceGVMethod<U>() { ... }
// }
// public class DerivedClass : BaseClass { }
//I would have assumed that slotDecl.OwningType would be BaseClass, and defType would be BaseClass. So we wouldn't execute the or condition. Is slotDecl the implementing slot?
There was a problem hiding this comment.
defType would be DerivedClass.
We go over all interfaces on DerivedClass. For each interface method, we resolve to the implementation. If the implementation is something else than it was on the base, we need to track it. If the implementation is the same thing that it was on the base, we don't need to track it.
The if (slotDecl.OwningType == defType) part of the check is just a perf optimization to avoid going to the resolve step on base. If the implementation is on the type we're looking at right now, for sure the base type wouldn't have that.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There were two very similar pieces of logic in the generic virtual method analysis. First one was deciding whether we need to emit GVM bookkeeping about the type, the second one was deciding whether the type should participate in GVM analysis (the N*M algorithm, "for each type relevant to GVM analysis" * "GVM virtual method called").
Made this into one so that it's in one spot. The interface method resolution is not cheap so this also speeds up the compilation. We should also do this for non-generic virtuals at some point.
I did a test pass with an assert that the analysis came up with the same conclusions to ensure I didn't mess up.
Cc @dotnet/ilc-contrib