xamarin / xamarin-macios

.NET for iOS, Mac Catalyst, macOS, and tvOS provide open-source bindings of the Apple SDKs for use with .NET managed languages such as C#
Other
2.44k stars 507 forks source link

Stop using undocumented attribute trimming #20906

Open sbomer opened 1 month ago

sbomer commented 1 month ago

Xamarin-macios should consider removing the _AggressiveAttributeTrimming setting, see https://github.com/dotnet/runtime/issues/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 commented 1 month ago

Android dropped this in https://github.com/dotnet/android/pull/9062 - the size impact was about +20KB on hello world (https://github.com/dotnet/android/pull/9062#discussion_r1658972675). 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 commented 1 month ago

@ivanpovazan

ivanpovazan commented 1 month ago

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 commented 1 month ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/dotnet/runtime/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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.