Precise reflection-visible delegate target analysis

[MichalStrehovsky]

Before this change, the compiler considered all methods that are target of a delegate reflection-visible. This is because one can just call Delegate.Method on a delegate instance to obtain a MethodBase of the target. This is rather annoying because most delegates are not actually used with reflection.

In this PR I'm adding analysis of places that use Delegate.Method to reflect on certain delegate type. This builds on top of the existing dataflow analysis within the compiler - we can often see the exact delegate type that was reflected on. When we see that, we make all targets of that specific type reflection-visible.

The advantage is that if nobody ever calls Delegate.Method, no delegates are made reflection visible. If this is used with a certain known delegate type, only the methods pointed to by that specific delegate type are made reflection visible. And if this is used with something typed as Delegate or MulticastDelegate, we fall back to the old behavior that just makes everything reflection visible.

I was hoping this would help ASP.NET but ASP.NET actually uses something typed as Delegate to obtain the MethodInfo and there's more code somewhere in the Task infrastructure (under EventSourceSupport) that also does it. So it unfortunately can't help there.

Cc @dotnet/ilc-contrib

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Before this change, the compiler considered all methods that are target of a delegate reflection-visible. This is because one can just call Delegate.Method on a delegate instance to obtain a MethodBase of the target. This is rather annoying because most delegates are not actually used with reflection.

In this PR I'm adding analysis of places that use Delegate.Method to reflect on certain delegate type. This builds on top of the existing dataflow analysis within the compiler - we can often see the exact delegate type that was reflected on. When we see that, we make all targets of that specific type reflection-visible.

The advantage is that if nobody ever calls Delegate.Method, no delegates are made reflection visible. If this is used with a certain known delegate type, only the methods pointed to by that specific delegate type are made reflection visible. And if this is used with something typed as Delegate or MulticastDelegate, we fall back to the old behavior that just makes everything reflection visible.

I was hoping this would help ASP.NET but ASP.NET actually uses something typed as Delegate to obtain the MethodInfo and there's more code somewhere in the Task infrastructure (under EventSourceSupport) that also does it. So it unfortunately can't help there.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[jkotas]

Any numbers to demonstrate where it helps?

ASP.NET actually uses something typed as Delegate to obtain the MethodInfo

Are these cases fixable?

[MichalStrehovsky]

Any numbers to demonstrate where it helps?

Don't have numbers right now but it's for things like this in CsWinRT:

https://github.com/microsoft/CsWinRT/blob/440e357415d808b9d4993d247f2afadff36fe0db/src/WinRT.Runtime/Marshalers.cs#L1457-L1661

Notice tons of delegates being created under various checks that RyuJIT can optimize. But whole program analysis figures out all of these generic flavors are targets of reflection before the optimizations starts kicking in (because IL scanner doesn't do these optimizations and we create delegates, therefore the target is force-reflection-visible in the whole program view).

ASP.NET actually uses something typed as Delegate to obtain the MethodInfo

Are these cases fixable?

I didn't look at it because this one doesn't look fixable and ASP.NET without EventSourceSupport seems like an academic scenario:

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Line 1687 in 396edb2

TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.Method.Name, 0);

[jkotas]

I didn't look at it because this one doesn't look fixable and ASP.NET without EventSourceSupport seems like an academic scenario:

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Line 1687 in 396edb2

TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.Method.Name, 0);

This one is fixable by introducing a new diagnostic-only API for delegates that uses stacktrace data, that comes with similar disclaimer as stacktrace APIs.

Where is the one in ASP.NET?

[jkotas]

The advantage is that if nobody ever calls Delegate.Method, no delegates are made reflection visible. If this is used with a certain known delegate type, only the methods pointed to by that specific delegate type are made reflection visible

Is this precision useful in practice? Or would be good enough to just check for Delegate.Method call anywhere in the app and give up on this optimization if we see any?

[MichalStrehovsky]

Where is the one in ASP.NET?

This is the full list in BasicMinimalApi:

{[Microsoft.AspNetCore.Routing]Microsoft.AspNetCore.Routing.RouteEndpointDataSource.CreateRouteEndpointBuilder(RouteEntry,RoutePattern,IReadOnlyList`1<Action`1<EndpointBuilder>>,IReadOnlyList`1<Action`1<EndpointBuilder>>)}
{[S.P.CoreLib]System.Threading.Tasks.Task.ScheduleAndStart(bool)}
{[S.P.CoreLib]System.Threading.Tasks.ContinueWithTaskContinuation..ctor(Task,TaskContinuationOptions,TaskScheduler)}

Is this precision useful in practice? Or would be good enough to just check for Delegate.Method call anywhere in the app and give up on this optimization if we see any?

The complete list of callsites in BasicMinimalApi is this, but we know the type of the delegate in most places:

From this I get that people want to use this API and if we can make using it less costly, it's nice. Apps have many delegates. Most are not reflected on. If I were to drop the per-type part from the PR, it wouldn't get much shorter. Most of the analysis we're already getting for free from dataflow.

[jkotas]

[Microsoft.AspNetCore.Routing]Microsoft.AspNetCore.Routing.RouteEndpointDataSource.CreateRouteEndpointBuilder(RouteEntry,RoutePattern,IReadOnlyList1<Action1>,IReadOnlyList1<Action1>)}

Hmm, this one does more than just producing a pretty name - it inspects custom attributes, etc. It may be fixable with source generator, but it does not look easy.

[jkotas]

LGTM. Thank you!

We should open two follow workitems:

• Add a new API to produce diagnostic strings for delegates that piggybacks on data used for stacktraces and use it for tracing.
• Add a new route registration API to ASP.NET with strongly-typed delegates and use it with the minimal API ASP.NET source generator (cc @davidfowl)

If both of these follow up items are implemented, this optimization should be able kick in for ASP.NET apps too.

[MichalStrehovsky]

Filed #96528. I folded it together with a StackFrame API since it's the same kind of issue.

[davidfowl]

Filed dotnet/aspnetcore#53171

[MichalStrehovsky]

Any numbers to demonstrate where it helps?

Don't have numbers right now but it's for things like this in CsWinRT:

I was now able to measure impact in CsWinRT and seeing around 5%. I don't expect ASP.NET to get this much because CsWinRT was all kinds of a pathogenic case, but maybe some.