Open MichalStrehovsky opened 1 year ago
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.
Author: | MichalStrehovsky |
---|---|
Assignees: | - |
Labels: | `linkable-framework`, `area-Tools-ILLink` |
Milestone: | - |
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 https://github.com/aspnet/Benchmarks/compare/main...eerhardt:RemoveJwtFromTodosApi and measure the gains?
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/2035 is tracking removing the Newtonsoft.Json dependency in the JwtBearer code and using System.Text.Json instead.
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:
And Npgsql implementing a IDictionary on a type that is going to be annotated as .All
due to the base:
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).
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).
AllConstructors | PublicMethods | NonPublicMethods
to imply AllMethods
.namespace System.Diagnostics.CodeAnalysis;
public enum DynamicallyAccessedMemberTypes
{
// Existing members
// ...
// New members:
PublicConstructorsWithInherited = PublicConstructors | 0x100000,
PublicNestedTypesWithInherited = PublicNestedTypes | 0x200000,
NonPublicConstructorsWithInherited = NonPublicConstructors | 0x4000,
NonPublicMethodsWithInherited = NonPublicMethods | 0x8000,
NonPublicFieldsWithInherited = NonPublicFields | 0x10000,
NonPublicNestedTypesWithInherited = NonPublicNestedTypes | 0x20000,
NonPublicPropertiesWithInherited = NonPublicProperties | 0x40000,
NonPublicEventsWithInherited = NonPublicEvents | 0x80000,
AllConstructors = PublicConstructorsWithInherited | NonPublicConstructorsWithInherited,
AllMethods = PublicMethods | NonPublicMethodsWithInherited,
AllFields = PublicFields | NonPublicFieldsWithInherited,
AllNestedTypes = PublicNestedTypesWithInherited | NonPublicNestedTypesWithInherited,
AllProperties = PublicProperties | NonPublicPropertiesWithInherited,
AllEvents = PublicEvents | NonPublicEventsWithInherited,
}
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 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, ListAPI Proposal
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