maui icon indicating copy to clipboard operation
maui copied to clipboard

[Trimming] Add a feature flag to disable XAML loading at runtime

Open simonrozsival opened this issue 1 year ago • 4 comments

Description of Change

This PR adds a feature flag that allows developers to disable XAML loading at runtime. This is useful when XAML files are compiled using XamlC and the LoadFromXaml and XamlLoader code is never used anyway. The file size savings I'm seeing with NativeAOT on iOS with the feature disabled is 5.1% (378 kB).

Moreover, the XAML loading code is not trimming-friendly. The XAML document can reference arbitrary types and the static analysis cannot find them. For this reason, the feature should be disabled by default for NativeAOT apps.

Discussion points:

  • [x] Naming:
    • MSBuild property: XamlLoadingIsEnabled -> MauiXamlRuntimeParsingSupport
    • Property in code: FeatureFlags.IsXamlLoadingEnabled -> Microsoft.Maui.RuntimeFeature.IsXamlRuntimeParsingSupported
  • [x] Finalize warning messages (suggestions for phrasing or grammar changes are welcome, English is not my first language)
  • [x] ~~How to handle ResourceDictionary.SetAndLoadSource? The current implementation assumes that XamlC will compile all resource dictionaries and add the correct [XamlResourceId] attributes so the codepath that throws exception is never hit. On the other hand, it breaks the principle that whenever there is a call to a method that can throw because the feature is disabled should produce a build time warning. After examining the code in SetAndLoadSource and the code in XamlC that generates the call I wonder if we could avoid calling the method altogether and instead instantiate the generated resource dictionary directly. Is there a specific reason why we're not doing it, or is that something that wasn't worth implementing yet?~~
  • [x] ~~Does it make sense to annotate IResourcesLoader since it is an internal interface? I am a little confused why we have the interface in the first place and why we use it to access ResourceLoader through DI in the first place. Could this setup be simplified?~~

/cc @jonathanpeppers @StephaneDelcroix @mdh1418 @ivanpovazan @vitek-karas

Issues Fixed

Contributes to #18658 and #19397 Once this change is merged it should decrease the number of warnings tracked by the test that should be introduced by #19194

NativeAOT on iOS

Before After Delta
Trimming warnings 64 36 28
AOT warnings 5 5 0
.ipa size 7,425,853 B 7,047,340 B 378,513 B (5.1%)

simonrozsival avatar Dec 08 '23 12:12 simonrozsival

Oh thinking about it more, should XamlCompilationOptions.Skip have [RequiresUnreferencedCode] on it? So you get trimmer warnings if you use that enum value.

jonathanpeppers avatar Dec 08 '23 18:12 jonathanpeppers

@jonathanpeppers the RUC attribute cannot be applied to enum values (it's just for classes, methods, and constructors). Could we produce these warnings whenever XamlC runs into one of these attributes? One similar scenario: We should probably emit warning when MauiEnableXamlLoading is disabled and MauiXamlCValidateOnly is enabled.

simonrozsival avatar Dec 12 '23 15:12 simonrozsival

Ok, yeah it kind of makes sense you can't put ILLink attributes on an enum. It's only because we have a custom MSBuild task that rewrites IL -- that makes this even possible.

So, I think we could manually warn in a couple places:

https://github.com/dotnet/maui/blob/7acd5c2990aa0241bf896c743d81d7f716b05868/src/Controls/src/Build.Tasks/XamlCTask.cs#L87-L88

https://github.com/dotnet/maui/blob/7acd5c2990aa0241bf896c743d81d7f716b05868/src/Controls/src/Build.Tasks/XamlCTask.cs#L102-L103

https://github.com/dotnet/maui/blob/7acd5c2990aa0241bf896c743d81d7f716b05868/src/Controls/src/Build.Tasks/XamlCTask.cs#L132-L133

I think we could pass in the MSBuild property to this task to know if the new feature flag is enabled. It might should even be an error?

jonathanpeppers avatar Dec 12 '23 15:12 jonathanpeppers

Ok, yeah it kind of makes sense you can't put ILLink attributes on an enum.

This has been discussed several times in the past, but it's difficult. In IL enums look like ints (ldc.0 and so on). The only way to recognize enums is by looking at the type of a parameter they're passed into. So doable, but not super robust... so for now we avoided investing into it.

vitek-karas avatar Dec 12 '23 16:12 vitek-karas

@simonrozsival as you noticed locally, template tests are also failing on CI with:

  Failed PublishNativeAOTCheckWarnings("maui","net9.0-ios","ios-arm64") [1 m 7 s]
  Error Message:
   System.ArgumentNullException : Value cannot be null. (Parameter 'format')
  Stack Trace:
     at System.ArgumentNullException.Throw(String paramName)
   at System.String.FormatHelper(IFormatProvider provider, String format, ReadOnlySpan`1 args)

this is due to a new version of MSBuild.StructuredLogger API, and as discussed offline seems to be fixed by bumping the version of the package.

This was already done on main with: https://github.com/dotnet/maui/commit/8821d48ce279fab529965dfada09cd6c0e819b6d

  • As a short-term solution I suppose you can just cherry-pick that commit
  • As a long-term solution (to avoid versioning problems) @jonathanpeppers should we wire integration tests references with darc, so we are always up-to-date?

ivanpovazan avatar Jan 19 '24 17:01 ivanpovazan

should we wire integration tests references with darc, so we are always up-to-date?

MSBuild.StructuredLogger is third party (although Kirill works for MSFT). I would guess that main got updated because @dependabot updated it?

The general problem here happens if MSBuild changes the .binlog format. This will probably happen a couple times during .NET 9 previews, it happened during 6, 7, 8.

jonathanpeppers avatar Jan 19 '24 18:01 jonathanpeppers

There are still some failing tests, but they don't seem unique to this PR, and I can see the same failures in other CI runs.

simonrozsival avatar Jan 20 '24 08:01 simonrozsival

should we wire integration tests references with darc, so we are always up-to-date?

MSBuild.StructuredLogger is third party (although Kirill works for MSFT).

I would guess that main got updated because @dependabot updated it?

Looks like it was a manual bump.

The general problem here happens if MSBuild changes the .binlog format. This will probably happen a couple times during .NET 9 previews, it happened during 6, 7, 8.

I've always had to bump manually (also any new package versions have to be manually mirorred to the dotnet-public first - https://github.com/dotnet/arcade/blob/main/Documentation/MirroringPackages.md)

rolfbjarne avatar Jan 22 '24 16:01 rolfbjarne

I revisited the condition in ResourceDictionary which was supposed to ensure that the developers have the same experience in Debug and Release configurations and I simplified the code:

  • we now compile all XAML files by default (https://github.com/dotnet/maui/pull/19390)
  • the codepath which can throw will only run of the developer opts out of XAML compilation (both in Debug and Release/NativeAOT)
  • if developer opts out of XAML compilation for some file, they will get a build warning from the source generator or from XamlC (https://github.com/dotnet/maui/pull/20127)

On top of that, I updated the source generator to generate [UnconditionalSuppressMessage] for all generated InitializeComponent() methods - they always contain calls to LoadFromXaml which will always produce IL2026. XamlC later removes the attribute so that we don't suppress trimming warnings from ILC when it compiles the generated IL.

Please have another look, @StephaneDelcroix.

simonrozsival avatar Jan 24 '24 14:01 simonrozsival

/rebase

simonrozsival avatar Jan 29 '24 13:01 simonrozsival

/rebase

mattleibow avatar Feb 06 '24 14:02 mattleibow

If this is green, I think we are OK to merge:

image

I am testing how green CI is, in general, at:

  • https://github.com/dotnet/maui/pull/20387
  • https://github.com/dotnet/maui/pull/20388

jonathanpeppers avatar Feb 06 '24 15:02 jonathanpeppers

Agreed.

FWIW, the last tip of net9 branch https://github.com/dotnet/maui/commit/9c03adcef4fe852f14738825a9cba8df2312b8a7 got merged with macOS and iOS Controls (v16.4) failures, due to missing workloads:

Executing: /Users/builder/azdo/_work/3/s/bin/dotnet/dotnet build "../../src/Controls/samples/Controls.Sample.UITests/Controls.Sample.UITests.csproj" --framework net9.0-ios --configuration Release /p:BuildIpa=true /bl:/Users/builder/azdo/_work/3/a/logs/Controls.Sample.UITests-Release-ios.binlog /tl
MSBuild version 17.10.0-preview-24101-01+07fd5d51f for .NET
  Determining projects to restore...
/Users/builder/azdo/_work/3/s/bin/dotnet/sdk/9.0.100-preview.1.24101.2/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.ImportWorkloads.targets(38,5): error NETSDK1178: The project depends on the following workload packs that do not exist in any of the workloads available in this installation: Microsoft.NETCore.App.Runtime.AOT.Cross.net8.ios-arm64 Microsoft.NETCore.App.Runtime.AOT.Cross.net8.iossimulator-arm64 Microsoft.NETCore.App.Runtime.AOT.Cross.net8.iossimulator-x64 [/Users/builder/azdo/_work/3/s/src/Graphics/src/Graphics/Graphics.csproj::TargetFramework=net8.0-ios]

in https://dev.azure.com/xamarin/public/_build/results?buildId=107057&view=logs&j=bdef7eb6-2ec3-5ca2-56c7-25b2b8e714f2&t=f852c752-4869-5efa-8563-adc59bcd5d54&l=1011

ivanpovazan avatar Feb 06 '24 15:02 ivanpovazan