Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using undocumented attribute trimming #20906

Open
sbomer opened this issue Jul 16, 2024 · 11 comments · Fixed by #21314
Open

Stop using undocumented attribute trimming #20906

sbomer opened this issue Jul 16, 2024 · 11 comments · Fixed by #21314
Assignees
Milestone

Comments

@sbomer
Copy link
Contributor

sbomer commented Jul 16, 2024

Xamarin-macios should consider removing the _AggressiveAttributeTrimming setting, see dotnet/runtime#88805. This setting is undocumented and has problematic behavior - it can't be statically validated with trim warnings and can introduce correctness problems.

@vitek-karas @MichalStrehovsky

@vitek-karas
Copy link

Android dropped this in dotnet/android#9062 - the size impact was about +20KB on hello world (dotnet/android#9062 (comment)).
We should try to get this in for .NET 9 to make the two platforms similar in behavior - assuming the size impact on iOS is not much worse (very likely won't be).

@sbomer
Copy link
Contributor Author

sbomer commented Jul 23, 2024

@ivanpovazan

@ivanpovazan
Copy link
Contributor

Impact on iOS apps is (measured with Mono):

new ios _AggressiveAttributeTrimming=true _AggressiveAttributeTrimming=false diff (bytes) diff (%)
Size on disk 8201158 8282223 81065 0,99%
.ipa 2864582 2882338 17756 0,62%
new maui _AggressiveAttributeTrimming=true _AggressiveAttributeTrimming=false diff (bytes) diff (%)
Size on disk 37075987 37288812 212825 0,57%
.ipa 12912756 12980754 67998 0,53%

/cc: @rolfbjarne

@vitek-karas
Copy link

The +212825 bytes is interesting - I would suspect that's not just the attributes, but that they cause pulling in additional other code. Might be worth looking into eventually.

But for this I think it's OK to accept 0.5% regression for this.

@rolfbjarne rolfbjarne added this to the .NET 9 milestone Aug 1, 2024
@rolfbjarne
Copy link
Member

I did some testing, with our main test suite (monotouch-test), and a very simple app:

monotouch-test (Release build)

Platform Before After Difference
Mac Catalyst (arm64) 122,828 kiB 123,312 kiB 484 kiB = 0,39 %
Mac Catalyst (x64) 26,888 kiB 27,396 kiB 508 kiB = 1,89 %
iOS 158,836 kiB 159,312 kiB 476 kiB = 0,30 %
macOS (arm64) 129,032 kiB 129,032 kiB 0 kiB = 0,00 %
macOS (x64) 123,048 kiB 123,052 kiB 4 kiB = 0,003%
tvOS 148,204 kiB 148,628 kiB 424 kiB = 0,29 %

Simple test app (Release build)

Platform Before After Difference
Mac Catalyst (arm64) 47,848 kiB 48,020 kiB 172 kiB = 0,36 %
Mac Catalyst (x64) 8,920 kiB 9,072 kiB 152 kiB = 1,70 %
iOS 56,112 kiB 56,268 kiB 156 kiB = 0,28 %
macOS (arm64) 116,924 kiB 116,924 kiB 0 kiB = 0,00 %
macOS (x64) 111,044 kiB 111,044 kiB 0 kiB = 0,00 %
tvOS 55,684 kiB 55,848 kiB 164 kiB = 0,29 %

I then investigated the differences for the Microsoft.iOS.dll assembly, and the difference is exclusively due to additional attributes. Here's a list of the attributes that wouldn't be removed anymore, with a count of each:

15058 System.Runtime.Versioning.SupportedOSPlatformAttribute
 1203 System.ComponentModel.EditorBrowsableAttribute
 1089 System.Runtime.Versioning.UnsupportedOSPlatformAttribute
 1003 System.Runtime.Versioning.ObsoletedOSPlatformAttribute
  226 System.Runtime.CompilerServices.ExtensionAttribute
   31 System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute
   14 System.ObsoleteAttribute
    7 System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute
    6 System.Runtime.CompilerServices.TupleElementNamesAttribute
    5 System.Runtime.CompilerServices.IsReadOnlyAttribute
    3 System.Runtime.Versioning.SupportedOSPlatformGuardAttribute
    2 System.CLSCompliantAttribute
    1 System.Runtime.CompilerServices.CompilerFeatureRequiredAttribute

From what I can tell, there's not a single nullability attribute, which seems to be the original reason for removing support for _AggressiveAttributeTrimming...

@vitek-karas
Copy link

Nullability attributes are just a common example which breaks people. But technically any attribute can break the app - if it looks for it. The problem is that trimming model doesn't handle attributes - it's basically defined as assuming all attributes are preserved if the owning metadata item is preserved. The _AggressiveAttributeTrimming breaks that assumption and thus it breaks the promises made by trimming model.
For example, if the app queries the ObsoleteAttribute via reflection it will not get any trim related warnings, but its behavior will differ between Debug/Release if aggressive attribute trimming is enabled.

Also of note is that this is not a core trimming feature - _AggressiveAttributeTrimming toggles a feature switch System.AggressiveAttributeTrimming which is used in XML files to tell the trimmer to remove some attributes.

In general I would advice against trimming attribute at all (due to the above reasons), but if we think we really need to remove some of them, then we could still do that - but I would do that under a different feature switch. The _AggressiveAttributeTrimming toggles attributes defined in S.P.CoreLib and that set currently includes attributes which are problematic (nullability is a good example).

@rolfbjarne
Copy link
Member

The problem is that trimming model doesn't handle attributes - it's basically defined as assuming all attributes are preserved if the owning metadata item is preserved.

Ideally the trimmer would be able to remove attributes that no code looks for - I really doubt there's any runtime code anywhere that looks for System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute for instance (I guess this is really dotnet/runtime#103934).

In general I would advice against trimming attribute at all (due to the above reasons)

We've always removed numerous attributes (many more in addition to what _AggressiveAttributeTrimming does), so I don't see this as a problem at the moment. I see the rationale for being more conservative, but in this case every iOS app will pay the size penalty if we become more conservative, when currently none of them (that we know of at least) have problems in this area.

but if we think we really need to remove some of them, then we could still do that - but I would do that under a different feature switch.

Yes, I'll do that instead.

@vitek-karas
Copy link

We've always removed numerous attributes (many more in addition to what _AggressiveAttributeTrimming does), so I don't see this as a problem at the moment. I see the rationale for being more conservative, but in this case every iOS app will pay the size penalty if we become more conservative, when currently none of them (that we know of at least) have problems in this area.

I understand. There are differences in expectation, and also the full trimming was not very common before and now with NativeAOT it becomes basically a requirement at times. And this problem is likely more prevalent in full trimming (attributes on types defined by the user).

@vitek-karas
Copy link

I thought about this some more and wanted to add one more thing: One of the reasons why the core trimmer is trying so hard to be as precise as possible and why the team behind it (me included) push for 100% correctness is that it's a trade between "how many apps work out of the box" versus "how many questions and bugs we'll have to deal with". Precise behavior leads to much fewer questions and bugs, but it decreases the number of apps where "things just magically work". Long term, I would argue it's better to be precise as it decreases maintenance cost.

@rolfbjarne
Copy link
Member

Long term, I would argue it's better to be precise as it decreases maintenance cost.

We also have to add people asking "why did my app size grow?" to the maintenance cost equation :)

But in theory I agree, and hopefully we'll get there at some point. Preferably through dotnet/runtime#103934, which I believe is the best option, because then everything becomes automatic.

rolfbjarne added a commit that referenced this issue Sep 26, 2024
The aggressive attribute trimming as exposed by ILLink is undocumented and has
problematic behavior, so disable it (it can't be statically validated) -
ILLink already removed support for it anyway.

However, this has a somewhat significant size increase for our apps, and it
would affect pretty much every app, so re-implement the aggressive attribute
trimming ourselves.

This is accomplished by copying ILLink's list of attributes to remove into our
own xml definition, which we then pass to ILLink.

I tested a release build of monotouch-test for all platforms, and the final
size is almost entirely unchanged, except for a minimal size decrease.

Here's the results for iOS: https://gist.github.com/rolfbjarne/dac3e08620a3ae499ad1fc134765e0d2

Fixes #20906.
@rolfbjarne rolfbjarne modified the milestones: .NET 9, .NET 10 Sep 26, 2024
@rolfbjarne rolfbjarne self-assigned this Sep 26, 2024
@vitek-karas
Copy link

Preferably through dotnet/runtime#103934, which I believe is the best option, because then everything becomes automatic.

Just to set the right expectations, I don't think this will happen any time soon. On top of that it would be a breaking change anyway, so probably opt-in at the beginning. The above efforts are basically to get to a stable well defined state right now.

rolfbjarne added a commit that referenced this issue Oct 10, 2024
The aggressive attribute trimming as exposed by ILLink is undocumented and has
problematic behavior, so disable it (it can't be statically validated) -
ILLink already removed support for it anyway.

However, this has a somewhat significant size increase for our apps, and it
would affect pretty much every app, so re-implement the aggressive attribute
trimming ourselves.

This is accomplished by copying ILLink's list of attributes to remove into our
own xml definition, which we then pass to ILLink.

I tested a release build of monotouch-test for all platforms, and the final
size is almost entirely unchanged, except for a minimal size decrease.

Here's the results for iOS: https://gist.github.com/rolfbjarne/dac3e08620a3ae499ad1fc134765e0d2

Fixes #20906.
rolfbjarne added a commit that referenced this issue Oct 15, 2024
#21314)

The aggressive attribute trimming as exposed by ILLink is undocumented and has
problematic behavior, so disable it (it can't be statically validated) -
ILLink already removed support for it anyway.

However, this has a somewhat significant size increase for our apps, and it
would affect pretty much every app, so re-implement the aggressive attribute
trimming ourselves.

This is accomplished by copying ILLink's list of attributes to remove into our
own xml definition, which we then pass to ILLink.

I tested a release build of monotouch-test for all platforms, and the final
size is almost entirely unchanged, except for a minimal size decrease.

Here's the results for iOS: https://gist.github.com/rolfbjarne/dac3e08620a3ae499ad1fc134765e0d2

Fixes #20906.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants