Using Options Validation Source Generator results in increased app size

[eerhardt]

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

1. Clone https://github.com/aspnet/Benchmarks
2. cd src\BenchmarksApps\TodosApi
3. dotnet publish
4. Checkout eerhardt/Benchmarks@c667d28
5. 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.

Related: #97057

cc @MichalStrehovsky @tarekgh @steveharter @DamianEdwards

[dotnet-policy-service]

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

[MichalStrehovsky]

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]

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?).

[MichalStrehovsky]

@eerhardt it should now be possible to re-annotate System.ComponentModel not to use .All. That would make this not regress the size as much as we see now.

It is however potentially a breaking change. I have a commit that simply replaces .All with AllConstructors | AllMethods | AllEvents | AllProperties | AllFields | AllNestedTypes here: b4265dd. That would be a pretty conservative change, but enough to avoid the size regression.

(If you're wondering what's the difference between .All and this one, the difference is in interfaces - if you have something annotated as .All, you can call GetInterface on it and the returned interface is still annotated as .All and anything goes with it. The more verbose variant doesn't have this property and that's how it avoids the massive size regression.)

However, if we're touching this, it might be worth revisiting the annotation here. For example, do we really need .AllNestedTypes too?

The only thing I worry about is that System.ComponentModel has a lot of trimming suppressions on it so it's not statically possible to prove the change would be safe. We might need to go and fully re-annotate it (delete all annotations, delete all suppressions, and see what we come up with now).