Add DynamicallyAccessedMemberTypes.AllMethods/.AllFields/.AllEvents/etc

[MichalStrehovsky]

We currently use DynamicallyAccessedMemberTypes.All in a couple places within libraries. Most uses that I know of fall back to this because they need to access non-public members recursively from their bases and .All is the only DAMT that allows this.

The existing DAMT annotations are modeled after reflection APIs/BindingFlags and that doesn't gel well with code that iterates base types and looks at private stuff in the bases.

We defined .All to be quite broad (it e.g. includes all methods on any interface the targeted type implements). It can hurt size badly.

I'm looking at the Stage2 app where dataflow came up with a ton of reflection roots. I did a quick hack to make .All at least not root everything on implemented interfaces (we're unlikely to rely on this anywhere) and I'm seeing 6.7% savings from that alone. It is probably too late to scale back what .All means (even though I'd really be surprised if anyone relies on the fact that .All on a class keeps all methods on interfaces), but if we can migrate code from using .All to do something more targeted, we'll get the 6.7% size savings and likely even more.

Click to expand the diff for the hack I did

Baseline:
Compare:

diff --git a/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs b/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
index 6780cb56192..b139a4421d2 100644
--- a/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
+++ b/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
@@ -469,7 +469,7 @@ private static void GetAllOnType(TypeDesc type, bool declaredOnly, List<TypeSyst
                 foreach (DefType interfaceType in type.TryGetExplicitlyImplementedInterfaces())
                 {
                     members.Add(interfaceType);
-                    GetAllOnType(interfaceType, declaredOnly: false, members, types);
+                    //GetAllOnType(interfaceType, declaredOnly: false, members, types);
                 }
             }
 
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
index 79c0cf011bf..43d089dddac 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
@@ -155,7 +155,7 @@ public static DependencyList ProcessTypeGetTypeDataflow(NodeFactory factory, Flo
             // annotations to interfaces separately if dealing with DAMT.All or DAMT.Interfaces.
             if (annotation.HasFlag(DynamicallyAccessedMemberTypes.Interfaces))
             {
-                var annotationToApplyToInterfaces = annotation == DynamicallyAccessedMemberTypes.All ? annotation : DynamicallyAccessedMemberTypes.Interfaces;
+                var annotationToApplyToInterfaces = /*annotation == DynamicallyAccessedMemberTypes.All ? annotation :*/ DynamicallyAccessedMemberTypes.Interfaces;
                 foreach (var iface in type.RuntimeInterfaces)
                 {
                     if (flowAnnotations.GetTypeAnnotation(iface).HasFlag(annotationToApplyToInterfaces))

API Proposal

namespace System.Diagnostics.CodeAnalysis;

public enum DynamicallyAccessedMemberTypes
{
    // Existing members
    // ...

    // New members:
    AllConstructors = 0x4000 | PublicConstructors | NonPublicConstructors,
    AllMethods = 0x8000 | PublicMethods | NonPublicMethods,
    AllFields = 0x10000 | PublicFields | NonPublicFields,
    AllNestedTypes = 0x20000 | PublicNestedTypes | NonPublicNestedTypes,
    AllProperties = 0x40000 | PublicProperties | NonPublicProperties,
    AllEvents = 0x80000 | PublicEvents | NonPublicEvents,
}

The proposed All* members would do the obvious thing: they will keep all members of the specified kind on the type and its bases. They will not go to interfaces.

Cc @dotnet/illink @eerhardt

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We currently use DynamicallyAccessedMemberTypes.All in a couple places within libraries. Most uses that I know of fall back to this because they need to access non-public members recursively from their bases and .All is the only DAMT that allows this.

The existing DAMT annotations are modeled after reflection APIs/BindingFlags and that doesn't gel well with code that iterates base types and looks at private stuff in the bases.

We defined .All to be quite broad (it e.g. includes all methods on any interface the targeted type implements). It can hurt size badly.

I'm looking at the Stage2 app where dataflow came up with a ton of reflection roots. I did a quick hack to make .All at least not root everything on implemented interfaces (we're unlikely to rely on this anywhere) and I'm seeing 6.7% savings from that alone. It is probably too late to scale back what .All means (even though I'd really be surprised if anyone relies on the fact that .All on a class keeps all methods on interfaces), but if we can migrate code from using .All to do something more targeted, we'll get the 6.7% size savings and likely even more.

Click to expand the diff for the hack I did

Baseline:
Compare:

diff --git a/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs b/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
index 6780cb56192..b139a4421d2 100644
--- a/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
+++ b/src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
@@ -469,7 +469,7 @@ private static void GetAllOnType(TypeDesc type, bool declaredOnly, List<TypeSyst
                 foreach (DefType interfaceType in type.TryGetExplicitlyImplementedInterfaces())
                 {
                     members.Add(interfaceType);
-                    GetAllOnType(interfaceType, declaredOnly: false, members, types);
+                    //GetAllOnType(interfaceType, declaredOnly: false, members, types);
                 }
             }
 
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
index 79c0cf011bf..43d089dddac 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
@@ -155,7 +155,7 @@ public static DependencyList ProcessTypeGetTypeDataflow(NodeFactory factory, Flo
             // annotations to interfaces separately if dealing with DAMT.All or DAMT.Interfaces.
             if (annotation.HasFlag(DynamicallyAccessedMemberTypes.Interfaces))
             {
-                var annotationToApplyToInterfaces = annotation == DynamicallyAccessedMemberTypes.All ? annotation : DynamicallyAccessedMemberTypes.Interfaces;
+                var annotationToApplyToInterfaces = /*annotation == DynamicallyAccessedMemberTypes.All ? annotation :*/ DynamicallyAccessedMemberTypes.Interfaces;
                 foreach (var iface in type.RuntimeInterfaces)
                 {
                     if (flowAnnotations.GetTypeAnnotation(iface).HasFlag(annotationToApplyToInterfaces))

API Proposal

namespace System.Diagnostics.CodeAnalysis;

public enum DynamicallyAccessedMemberTypes
{
    // Existing members
    // ...

    // New members:
    AllConstructors = 0x4000 | PublicConstructors | NonPublicConstructors,
    AllMethods = 0x8000 | PublicMethods | NonPublicMethods,
    AllFields = 0x10000 | PublicFields | NonPublicFields,
    AllNestedTypes = 0x20000 | PublicNestedTypes | NonPublicNestedTypes,
    AllProperties = 0x40000 | PublicProperties | NonPublicProperties,
    AllEvents = 0x80000 | PublicEvents | NonPublicEvents,
}

The proposed All* members would do the obvious thing: they will keep all members of the specified kind on the type and its bases. They will not go to interfaces.

Cc @dotnet/illink @eerhardt

Author:	MichalStrehovsky
Assignees:	-
Labels:	linkable-framework, area-Tools-ILLink
Milestone:	-

[eerhardt]

I'm looking at the Stage2 app where dataflow came up with a ton of reflection roots. I did a quick hack to make .All at least not root everything on implemented interfaces (we're unlikely to rely on this anywhere) and I'm seeing 6.7% savings from that alone.

A big size issue with the Stage2 app is that the JwtBearer authentication code pulls in Newtonsoft.Json, which then pulls in a ton of stuff (System.Xml, System.Data.Common, etc).

Just removing the JwtBearer dependency takes the app from 27.5 MB => 18.3 MB (on win-x64). My guess is you won't see such a big size savings (6.7%) once Newtonsoft.Json is trimmed from the app. Can you try your hack again on the changes in aspnet/Benchmarks@main...eerhardt:RemoveJwtFromTodosApi and measure the gains?

AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2035 is tracking removing the Newtonsoft.Json dependency in the JwtBearer code and using System.Text.Json instead.

[MichalStrehovsky]

About 1% with JwtBearer removed. Same saving in Stage1.

Note that this is the bottom range of potential savings - the hack I did actually removes very little (only methods on interface types) - it just happens to have a surprisingly large impact on Stage2 and I would bet a small amount of money that it doesn't actually break anything.

If we were to re-annotate places in framework with the more restricted annotations instead of .All, we would get at least the 1% in savings, but likely more.

The top of the potential savings for Stage2 (with JwtBearer removed) is 2.5% (I did another hack where I made .All do nothing - this obviously breaks the app).

Given this is maybe 1.5-2% savings for Stage2 with JwtBearer removed, the main question would be whether the pattern that made things pathological for Stage2 as it is right now could happen elsewhere. I don't have a good other app to try this on.

The bad condition the Stage2 app hit this on was "make a bunch of generic interface methods reflection visible even though you didn't really need to do that and then implement those interfaces on a ton of types". The pattern that triggered it on Stage2 was us annotating this:

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs

Lines 18 to 19 in 7c00b17

	[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
	public class DbConnectionStringBuilder : IDictionary, ICustomTypeDescriptor

And Npgsql implementing a IDictionary on a type that is going to be annotated as .All due to the base:

https://github.com/npgsql/npgsql/blob/9b03f3b6009f4f141b8f5e0cb9e0cee489f9cc93/src/Npgsql/NpgsqlConnectionStringBuilder.cs#L21

This made IEnumerable and a ton of other interfaces reflection visible and a lot more costly, for no reason (we don't actually need those interfaces to be reflection visible, we just don't have a way to express that).

[vitek-karas]

I like the proposed behavior of the new enum values. I don't have a strong opinion on the priority of this though - the use cane in DbConnectionStringBuilder is unfortunate, but the root cause is actually TypeDescriptor, so maybe we should look into that and try to see if we can reduce the All annotation there (and if these new additions would help).
This might have positive effect on apps using TypeDescriptor, so typically UI stuff (MAUI).