Using Options Validation Source Generator results in increased app size

eerhardt commented 1 month ago

Switching the ASP.NET Core's TodosApi benchmark app (a.k.a. "Stage 2") to use the Microsoft.Extensions.Options validation source generator causes the app to go from 18.1 MB to 19.5 MB on win-x64.

Repro steps

Clone https://github.com/aspnet/Benchmarks
cd src\BenchmarksApps\TodosApi
dotnet publish
Checkout https://github.com/eerhardt/Benchmarks/commit/c667d2822941e70df147d4e83c1338aa106a690f
dotnet publish

Expected result

The 2 publishes should result in roughly the same size app

Actual result

The 2nd app is almost 8% larger (18.1 => 19.5).

Logs

You can open the following in sizoscope to view the difference between the apps.

todos-before.zip todos-after.zip

Doing an initial investigation, I believe what is happening is that including System.ComponentModel code into the graph causes ICustomTypeDescriptor to be included, which since Npgsql's ConnectionStringBuilder (and our System.Data.Common ConnectionStringBuilder) implements ICustomTypeDescriptor. In order to make that usage trim-compatible, we added [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] to the base DbConnectionStringBuilder. That class is also an IDictionary, which means all interface methods on the collection types are being preserved - across every collection in the app.

cc @MichalStrehovsky @tarekgh @steveharter @DamianEdwards

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-extensions-options See info in area-owners.md if you want to be subscribed.

MichalStrehovsky commented 1 month ago

Yes, I believe your analysis is correct. I consider DynamicallyAccessedMembers.All actively harmful, I wish we invested into tools that allow people not to use .All (#88512) and we switched to them. Switching to them would be a breaking change however.

DynamicallyAccessedMembers.All was implemented as "whatever ILLinker currently does when we ask it to fully preserve a type" and unfortunately we didn't question those behaviors and their necessity. As a result, DAM.All preserves everything on all interfaces implemented on the type. Because we have trimming suppressions all over the product to the tune of https://github.com/dotnet/aspnetcore/blob/08b60af1bca8cffff8ba0a72164fb7505ffe114d/src/SignalR/common/Shared/ReflectionHelper.cs#L50-L71 and APIs such as GetMemberWithSameDefinitionAs, there's very little the compiler can do to avoid uncontrolled generic expansion if a foundational generic interface gets marked as reflected on. We're forced to generate all of this garbage on all possible instantiations of the interface. Foundational interface methods should ideally not be reflected on.

I think the only thing we could do here is implement #88512 and then to a breaking change that restricts the annotation on DbConnectionStringBuilder to be what's actually needed (I don't think we need reflectable methods on interfaces the type implements - we probably need something like reflectable methods on the type, but not on its interfaces - that's considerably less viral).

Cc @dotnet/illink @dotnet/ilc-contrib

MichalStrehovsky commented 1 month ago

I have a prototype that keeps the size with Options Validation Source Generator more under control. As I alluded, it's composed of two things: implementation of #88512 and switching DAMT.All in TypeConverter to the new annotation (that's the breaking change).

The above repro project with Options Validator Source Generator as-is in the main branch: 19.5 MB. The above project with the two fixes: 18.4 MB.

I put my prototype at https://github.com/dotnet/runtime/compare/main...MichalStrehovsky:runtime:BAK_fix106366prototype?expand=1.

I think this demonstrates that #88512 can be useful. I'm convinced enough that I'm going to push getting this through API review and implemented. The prototype just needs a bit of cleanup and tests. The implementation is basically done.

This leaves us with the question whether we'd be okay doing the breaking change in DAMT annotations on TypeConverter/DbConnectionStringBuilder once the new API exists.

What's the breaking change? We currently annotate these APIs with DAM annotations to cover our use of reflection on this. Someone else might take advantage of this and do their own reflection. E.g. they might pass these things to locations annotated as DAMT.All. If we make a change in the annotations these places could break. Not sure how likely that would be. This is uncharted territories. The other question is how confident we would be in changing these - we have many suppression in place that will prevent automatic detection of annotation mismatches. The replacement annotation I chose is very conservative (E.g. do we need .AllFields?).

dotnet / runtime