Consider analyzing call sites to methods to determine if portions of the callee can be trimmed

[stephentoub]

I've seen this in a few cases in corelib. For example, methods that take a StringComparison and then have six cases in a switch, one for each of Ordinal, OrdinalIgnoreCase, InvariantCulture, InvariantCultureIgnoreCase, CurrentCulture, and CurrentCultureIgnoreCase. Each of those ends up having a different supporting implementation. But in a default Blazor wasm app, a given method generally only ended up having a couple of callers, often always with the same harcoded value, e.g. LastIndexOf(..., StringComparison.OrdinalIgnoreCase). The linker could in theory examine all call sites, and if all were const values (or could otherwise be determined via flow analysis), determine which code paths in the callee were and were not hittable, and then trim implementation accordingly. It's also likely a very expensive thing to do.

[MichalStrehovsky]

The analyzer would also have to prove that the method is not a target of a delegate (relatively easy) and not a target of reflection (hard).

Basically every method that is reachable in the app needs to be considered target of reflection because of APIs like StackFrame::GetMethod. We could consider only enabling this if these APIs are not used in the app, but even a C# Hello World uses StackFrame::GetMethod.

Our only chance of being able to do these optimizations reliably at least some times was dotnet/runtime#11811 (comment) but that was won't fixed.

[stephentoub]

I'm not following. If we have to consider every method to be reachable via reflection, how can we trim anything? We mark things as reachable via reflection either due to recognition of certain patterns or due to dynamic dependency attributes, either of which would be considered call sites with arbitrary inputs.

[MichalStrehovsky]

If we have to consider every method to be reachable via reflection, how can we trim anything?

Yes, we wouldn't be able to trim anything in that case, but that's not what I wrote.

I wrote "every method that is reachable in the app needs to be considered target of reflection". Once the method is in the dependency graph because it can be reached (either via static callgraph, or reflection analysis, or ILLinkTrim.xml) we need to consider it as reflection invokable with arbitrary arguments.

If you can get a MethodInfo for the method and there's MethodInfo.Invoke somewhere in the app, it's difficult to prove that a certain method will never be that MethodInfo. StackFrame::GetMethod is a way to get a MethodInfo for pretty much any method that is in the app.

[stephentoub]

That feels very inconsistent to me. We're comfortable enough removing a method because we don't see any meaningful way for app code to get to it, which means we've found all the ways an app could reasonably get to it to invoke it, but then we're not comfortable saying those are the only ways the app could get to it to invoke it.

[MichalStrehovsky]

Yes, we're comfortable removing otherwise unreferenced methods, but once a method is referenced, we don't make assumptions about it because there are "reflection" APIs to obtain things that are currently executing within the app (same way as e.g. we don't go and strip metadata off types that we don't see as explicit targets of reflection because anyone can just call Object.GetType on something and get a reflection object that allows e.g. querying custom attributes or names of the types).

We could declare StackFrame::GetMethod as a trimming unfriendly API. Not sure if there are others in that category. This would likely enable these optimizations. If we do that we could also e.g. strip all parameter names on methods that we don't see as explicit targets of reflection.

[vitek-karas]

@MichalStrehovsky didn't we take a different approach by now? Linker (and especially NativeAOT) clearly differentiates between methods accessed via reflection and those which are not access via reflection. The above optimization could be done on those considered not accessed via reflection.

That said such optimization it pretty difficult to implement - it requires global analysis with probably multi-pass analysis of methods. That is something linker does in only very limited scopes, not across entire app. That said, I agree that there might be cases where this helps quite a bit.

[MichalStrehovsky]

Yes, we marked all of reflection that gets a MethodBase from a stackwalk as RUC: dotnet/runtime#59851. This allows us to optimize these methods in any way we want.

[MichalStrehovsky]

We might want to tweak the RequiredUnreferencedCode message if we do more than this - Metadata for the method might be incomplete or removed doesn't cover "Code could be removed".