Published size regression: todosapipublishaot, todosapipublishaot, todosapipublishaot

[pr-benchmarks]

Scenario	Environment	Date	Old Build (KB)	New Build (KB)	Change	Deviation	StDev	Dependencies
todosapipublishaot [Fixed]	Goldilocks Stage 2 (NativeAOT) 2- Native Aot Arm 28 Linux	06/07/2024 09:49:15	61,773	63,379	2.00 % (1,606) 👎	401.00 σ	4	Changes Name Version Diff Microsoft.AspNetCore.App 9.0.0-preview.6.24305.3 -> 9.0.0-preview.6.24306.2 8adff2c...73067c1 Microsoft.NETCore.App 9.0.0-preview.6.24281.1 -> 9.0.0-preview.6.24306.3 dotnet/runtime@3750ac5...87ca274
todosapipublishaot [Fixed]	Goldilocks Stage 2 (NativeAOT - Server GC) 2- Native Aot Arm 28 Linux	06/07/2024 09:50:41	61,773	63,379	2.00 % (1,606) 👎	401.00 σ	4	Changes Name Version Diff Microsoft.AspNetCore.App 9.0.0-preview.6.24305.3 -> 9.0.0-preview.6.24306.2 8adff2c...73067c1 Microsoft.NETCore.App 9.0.0-preview.6.24281.1 -> 9.0.0-preview.6.24306.3 dotnet/runtime@3750ac5...87ca274
todosapipublishaot [Fixed]	Goldilocks Stage 2 (NativeAOT) 7- Native Aot Intel Windows	06/07/2024 12:31:57	107,574	110,953	3.00 % (3,379) 👎	8,447.50 σ	0	Changes Name Version Diff Microsoft.AspNetCore.App 9.0.0-preview.6.24305.3 -> 9.0.0-preview.6.24306.5 8adff2c...1051726 Microsoft.NETCore.App 9.0.0-preview.6.24281.1 -> 9.0.0-preview.6.24306.8 dotnet/runtime@3750ac5...f6a7ebb

PowerBI Dashboard

@sebastienros
@eerhardt

[eerhardt]

This looks like a 2% regression in the TodosApi app (Stage 2).

I narrowed this down to dotnet/runtime@3750ac5...b49c04fcff68/. The one that stands out the most is dotnet/runtime#102902.

I have a before and after mstat. It appears the biggest changes are:

1. A bunch of SZGenericArrayEnumerators are now being kept:
   

2. The Enumerator.Reset() method is being kept on a bunch of Enumerators:

It looks like dotnet/runtime#102902 could be causing it:

See the line: https://github.com/dotnet/runtime/pull/102902/files#diff-b7da6b90947be2bdebc35f367d9dc2c5bb66dd3be881af8a2d6cbb9c18108a7fR668

AttributeProviderFactory = static () => typeof({{property.DeclaringType.FullyQualifiedName}}).GetMember({{FormatStringLiteral(property.MemberName)}}, {{InstanceMemberBindingFlagsVariableName}}) is { Length: > 0 } results ? results[0] : null,

cc @MichalStrehovsky @eiriktsarpalis

[eiriktsarpalis]

Hmm, my expectation was that the AttributeProviderFactory would get trimmed away unless the JsonPropertyInfo.AttributeProvider was being explicitly invoked by the application. It might either be that my assumption was incorrect or that the application uses JsonPropertyInfo.AttributeProvider somehow.

[eerhardt]

TodosApi-before.zip
TodosApi-after.zip
TodosApi-after-dmgl.zip

Those are the mstats I captured.

[MichalStrehovsky]

Hmm, my expectation was that the AttributeProviderFactory would get trimmed away unless the JsonPropertyInfo.AttributeProvider was being explicitly invoked by the application. It might either be that my assumption was incorrect or that the application uses JsonPropertyInfo.AttributeProvider somehow.

Looking at the code that Eric posted, if the code above is needed (initializing PropertyName, JsonPropertyName, etc.), the code to initialize the AttributeProviderFactory will also be there, even if nobody reads the field. It's very difficult to get rid of dead writes in trimming because there can be side effects, etc.

GetMember is one of the worst APIs to call because it's annotated as "keep everything on the Type" and there is no intrinsic handling of it in analysis (so even if we see the textual name of the member, we still keep everything because this API is used so rarely and has obscure corner cases analysis would need to implement, like support for wildcards). If this is looking for properties, can this use the mainstream API GetProperty? (Or ideally not use reflection since we're source generating...).

It is possible all of this is caused by the GetMember call since the annotation is so broad.

https://github.com/dotnet/runtime/blob/0686ce61ed1e1cb3cb420281a0154efa5d0d00d5/src/libraries/System.Private.CoreLib/src/System/Type.cs#L242-L264

Not sure if the array enumerators and IEnumerable.Reset are all caused by this but they probably are. Opening the roots view from the diff view in latest sizoscope will show things that are unique to one of these in different colors so it's much easier to find the thing that introduced the problem. But it needs DGML from both before and after and the above ZIP doesn't have before.

[MichalStrehovsky]

I triggered a rt-sz run on the PR and it does show some regression, but not for the array enumerators. Could be the array enumerators are a different issue, or it just needs a different app to hit. The rt-sz run is here, the mstat files can be downloaded from the report: MichalStrehovsky/rt-sz#28

This is how the new sizoscope feature looks like (notice slightly different color):

[eiriktsarpalis]

(Or ideally not use reflection since we're source generating...).

For context, the OpenAPI use case needs a mechanism for resolving custom attributes from the type graph (that is attributes not explicitly recognized by the source generator such as System.ComponentModel.DataAnnotations).

If this is looking for properties, can this use the mainstream API GetProperty?

It is looking for either properties or fields. What would be the benefits of switching to GetProperty and GetField in this case?

[eiriktsarpalis]

If this is looking for properties, can this use the mainstream API GetProperty?

It is looking for either properties or fields. What would be the benefits of switching to GetProperty and GetField in this case?

FWIW I just remembered that the reason I ended up with GetMembers here is that GetProperty and GetField can result in AmbiguousMatchException in case of shadowed members.

[MichalStrehovsky]

It is looking for either properties or fields. What would be the benefits of switching to GetProperty and GetField in this case?

If this was typeof(SomeConcreteType).GetProperty("SomeStringLiteral"), trimming would treat this as intrinsic and preserve just this one property.

Since it's GetMember, there's no intrinsic handling for this API and it will keep all properties, all fields, all methods, all nested types, all events on SomeConcreteType and its bases.

FWIW I just remembered that the reason I ended up with GetMembers here is that GetProperty and GetField can result in AmbiguousMatchException in case of shadowed members.

GetMembers has some absurd behaviors compared to the other Get APIs and there are considerations to do breaking change in them (dotnet/runtime#98533). Not sure if it affects the semantic you're using here, but I though it's worth mentioning.

[eiriktsarpalis]

I put up dotnet/runtime#103275 that investigates use of GetField and GetProperty. I'm not entirely convinced this would improve size substantially though, a delegate would be generated for every property that is part of the JSON contract so this shouldn't result in any trimming happening for your typical DTO. I'm also somewhat concerned that it could result in AmbiguousMatchException creeping in for cases not currently covered by testing.

[eerhardt]

Looks like dotnet/runtime#103275 fixed it. Now that we got a new runtime build:

Looking at sizoscope of before/after, I see the SZGenericArrayEnumerators are only in "before", as well as the Reset() methods that were showing up in the inital regression.

The total size looks slightly smaller than before the regression, so some other changes must have been made as well to reduce the size.