Reduce the number of forced MethodTables

[MichalStrehovsky]

When we're doing an optimized build, we go over the whole program twice:

• First time to build a whole program view: at this point we e.g. scan the IL method bodies to see what they depend on, or analyze reflection use within the program
• The second time to do actual codegen: this time we no longer analyze reflection use and expect that the first phase would have rooted everything that's necessary from reflection perspective.

In both phases, we assume any type with a "constructed" MethodTable is visible to reflection because one can just call object.GetType() and reflect on stuff. We need to pass a list of constructed MethodTables from the first phase to the second phase because some MethodTables could be the result of reflection analysis and we need to make sure they're compiled.

But crucially, up until now we didn't really track which MethodTables are actual reflection roots and which ones just showed up in the dependency graph because the analyzed program happened to use it. We don't actually need to pass the latter ones as roots to compilation because the compilation phase is going to figure them out if they're needed anyway and if the compilation doesn't come up with some, that's fine because one wouldn't be able to call object.GetType on those anyway, because they're not actually part of the program.

Passing all of the MethodTables we saw from scanning to compilation is actually size bloat because scanning overapproximates things (by necessity, since it doesn't have a whole program view).

In this pull request I'm introducing ReflectedTypeNode to model MethodTables that are actual targets of reflection. Only those will get passed as roots to the compilation phase. From now on we need to be mindful of how we refer to types. If a reference to a type is a result of non-code dependency, we should use ReflectedType to model it.

Saves about 1.2% (32 kB) in size on a Hello World.

I'm seeing it helps in two patterns:

runtime/src/libraries/System.Private.CoreLib/src/System/Byte.cs

Lines 1088 to 1093 in b1a2080

	else if (typeof(TOther) == typeof(Half))
	{
	Half actualResult = value;
	result = (TOther)(object)actualResult;
	return true;
	}

RyuJIT is able to eliminate the dead code in the if branch, but we were still rooting the type within the branch from the box.

The second pattern seems to be around RyuJIT devirtualizing things and preventing a box, which now eliminates the MethodTable.

Cc @dotnet/ilc-contrib

[MichalStrehovsky]

We still keep the metadata around because metadata got figured out during scanning, but we can check for the presence of the MethodTable by looking at the TypeHandle property.

The Type is damaged like this because we obtained it through unsafe reflection code. Legit code should not be able to see damaged types like this.

[vitek-karas]

I'm also struggling with the relationship between the new node and MetadataManager.GetDependenciesDueToReflectability.
Naively, with the new node I would expect that this method is called from the new node and not from various other places.

[vitek-karas]

Could you please add a comment about the difference between ReflectableTypeNode and TypeMetadataNode?
Naively I would expect that if type is reflectable it needs metadata, but currently it's the other way round - TypeMetadataNode adds ReflectableTypeNode. So that must mean that there's a case where type is reflectable but we don't have metadata for it.

[vitek-karas]

What's the difference between forcedTypes and reflectableTypes - per the change description these should be basically identical now...

[MichalStrehovsky]

reflectableTypes are types that are eligible to be reflection visible, but are not rooted. forcedTypes are rooted.

I forgot to mention that we have a concept of reflection blocked types - types that produce a special System.Type at runtime.

reflectableTypes basically helps compilation phase to decide whether something should be blocked or not.

It unfortunately doesn't match well with the below reflectableFields and reflectableMethods because types have the unfortunate property of becoming visible to reflection just because they exist (unless blocked).

[agocke]

Let me see if I have the right mental model here.

When I was talking about this topic with @sbomer a while back I noted that there's a difference between typeof(Blah) and typeof(Blah).GetMethods() -- the GetMethods() call actually reflects over the contents of the type, but typeof(Blah) doesn't really do anything except get a handle to the type. My thinking was that actually all a typeof is, is a unique token for a given type. If it's never reflected over, in theory all we would need to do is assign unique tokens for types and bake it into an int, and then compile down to the int tokens for runtime analysis.

Is this basically the same idea? That just grabbing a typeof doesn't actually keep the MethodTable or any of the rest of the reflection infrastructure?

[MichalStrehovsky]

I'm also struggling with the relationship between the new node and MetadataManager.GetDependenciesDueToReflectability. Naively, with the new node I would expect that this method is called from the new node and not from various other places.

I'll try to explain with a different example. Consider following code:

GetO().ToString();

[BarAttribute]
struct Foo
{
    public override void ToString() { ... }
}

static object GetO() => (object)default(Foo);

Now consider we compiled this with optimizations on.

When we're in the scanning phase we need to figure out the whole worldview, including reflection analysis. So we scan GetO() and we see it boxes Foo. So we need Foo and it's vtable. Foo also needs the custom attribute on it because we don't wait until someone calls object.GetType().GetCustomAttributes(). We just do it. The important bit is that Foo is considered reflectable during scanning. We implicitly consider all allocated types reflectable (unless reflection blocked). Scanning needs to come up with a complete set of methods and types. If we need a method/type we didn't see during scanning at compile time, that's a compilation failure.

But now we go into compilation. RyuJIT sees Main is calling GetO. GetO is below inlining threshold so we just inline it and devirtualize the ToString call. Crucially, we never allocated a MethodTable for Foo (boxing was elided), so nobody can call GetType on it and do reflection. What this change is doing is prevent the MethodTable of Foo from being forced into compilation.

Now, why do we need to force MethodTables into compilation in the first place? Consider the [BarAttribute] on Foo. The fact that we need a MethodTable of BarAttribute is result of reflection analysis. We don't run reflection analysis during compilation, only during scanning. If scanning didn't communicate to compilation that BarAttribute needs a MethodTable, we wouldn't create it and reflection-activating it would fail at runtime. So we need to tell compilation about some MethodTables, but not all.

You may have also noticed that if we do inline GetO, the MethodTable for BarAttribute is now unnecessary, but we're still going to root it. That's an unfortunate fact of life. Sometimes the decisions of the scanning phase have effects that cannot easily be undone. Similar example is if the scanning phase considers a virtual method used, but at codegen time we devirtualized the uses. Now the vtable slot in all types that implement the virtual method is unused, and points to dead code. We could potentially be able to get rid of inefficiencies like this if we scan multiple times, optimizing more and more, until we reach a fixpoint.

[MichalStrehovsky]

Is this basically the same idea? That just grabbing a typeof doesn't actually keep the MethodTable or any of the rest of the reflection infrastructure?

Grabbing a typeof still grabs the MethodTable because Type.TypeHandle is not marked RUC/RDC and accessing it would throw if MethodTable doesn't exist.

I'm leveraging a RyuJIT optimization in this - if RyuJIT sees typeof(Foo) == typeof(T) and we're compiling unshared code, RyuJIT can statically evaluate this, getting rid of any type comparisons - we don't need a Type here at runtime at all.

[vitek-karas]

I think the terminology here is very confusing.

We have "reflectable method" and "reflectable field" (in AnalysisBasedMetadataManager), these are:

• Fields/Methods for which we've seen actual reflection use
• They have FieldMetadataNode/MethodMetadataNode added to the graph
• If we found RuntimeMapping for these, the compilation will root them

We have "reflectable type" (in AnalysisBasedMetadataManager), these are:

• These are types which we've seen reflection happen on them
• These are types which we've seen reflection happen on some of their methods (generic methos only I think)
• These are types for which we need runtime mapping - which are more or less all types for which we need EEType - so a LOT of them
• Not all of these will have TypeMetadataNode added (although if type has metadata node, then it's reflectable as well) - actually most of them won't have it
• These are not rooted by compilation

We have now new ReflectableTypeNode which is used for:

• Types which we've seen used for reflection (so for example all types marked through data flow)
• Not all of them must have TypeMetadataNode, but all metadata node types must have ReflectableTypeNode - I actually don't know exactly which scenario is the one where type is accessed via reflection but we don't need metadata for it - maybe the typeof (but then we need the type's name, which is metadata)?
• Any type with ReflectableTypeNode is passed as "forced type" to compilation and will be rooted by the compilation

And in the description of this PR there's also the term "reflected type" (as oppose to "reflectable type") which is used for the types for which we've seen reflection so those with ReflectableTypeNode.

I think we should clean this up - and add a comment somewhere which clearly describes what the terms are used for.
I actually don't know what the terms should mean. "Reflected" feels like a good one for cases where we did see reflection access. Which fits the fields and methods above and the new node added in this PR.
"Reflectable" is probably only for "types" and it means there "may" be reflection access but we don't know for sure? When does it happen? What are the implications for the compilation around this?

Otherwise the change looks good - functionally it works and will save size (nice!!), it's just that the code is really hard to read given the current naming scheme.

Side node: Would be great to have some kind of a doc describing the nodes and what is their meaning and most importantly what is their relationship. In this area for example TypeMetadataNode, ReflectableTypeNode, ConstructedEETypeNode and EETypeNode. The problem I see with the nodes/naming is that lot of the nodes are modeled according to the "output", so it uses terms which are used by the runtime (EEType, TypeMetadata, ...). But now we're adding nodes which are modeled according to either the "input" or some intermediate level only defined by the compiler. And it's not clear which node is which and what are the relationships. (It's all even more complicated due to the presense of the two passes - scanner and compiler, and not all nodes are "valid" in both, but most of them are)

[MichalStrehovsky]

• I've rebased the branch on top of main to get rid of the merge conflict.
• I've added some comments.
• Also did a Reflectable -> Reflected rename (I'm ambivalent about that change - we're making it possible to use reflection over the types, but it's no like we demonstrated the thing was reflected on - e.g. if it's rooted from Descriptors or DynamicDependency)

The problem is really only that for fields/methods, they only get visible from reflection if one does an explicit gesture towards that. So we don't have many of those in the system and all of them are deliberate and actually mean "someone did a gesture to make them available from reflection".
For types, they become visible either through an explicit gesture (e.g. Type.GetType("Foo, Bar"), or descriptors, or whatever), or implicitly just because they got allocated and anyone could call object.GetType. The purpose of this pull request is not to make the implicit ones come back as roots after IL scanning, but let them be treeshaken away during compilation because codegen optimizations are capable of restricting the program dependency graph further.

[vitek-karas]

Thanks - the comments are great.
I still think this is rather complicated and really hard to understand by just reading the code, but this makes it better :-)

[MichalStrehovsky]

I still think this is rather complicated and really hard to understand by just reading the code, but this makes it better :-)

I looked if we can make it so that there's only one list of types that feeds into AnalysisBasedMetadataManager, but we currently need two lists due to the mode where each assembly can be compiled into a single obj file. In this mode, the fact that we generated a methodtable doesn't mean we should also generate reflection metadata (this is due to generics that generate methodtables anywhere, but metadata only goes to the one obj file that corresponds to the definition of the uninstantiated type.

[vitek-karas]

Thanks for trying!

[GSPP]

Similar example is if the scanning phase considers a virtual method used, but at codegen time we devirtualized the uses.

If I understand correctly, scanning might keep things alive that later turn dead because codegen optimizes usages away. This can cause unnecessary data to be retained. I understand that the size of the unneeded data is meaningful.

Crazy idea: How about running the pipeline twice? What's kept from the first run is just the information what actually will end up being alive. The code output is simply dropped. And in the second run, the actual output is created. But this time, it is precisely known, what will live.

This obviously has a performance cost so it could be an opt-in mode for use cases in which every percent in size counts. This seems like a sledgehammer solution but it could be very general. Does this make sense?

[MichalStrehovsky]

Yes, eventually the scanning phase would be done by RyuJIT - it can better predict when things will be devirtualized. We don't run RyuJIT for scanning right now because we don't have a way to run it without actually generating the code.

We could certainly run RyuJIT twice and just throw away all the generated code first time. But right now the benefit would be quite limited. We can get a lot more benefit (without the compile time regression drawback) if we do it properly and also allow RyuJIT to collect data it will find useful to do codegen later - but RyuJIT needs to add first class support for running as a scanner.