-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[compatibility] IsTrimmable support, fix linker warnings #3161
Conversation
To make `Microsoft.Maui.Controls.Compatibility.dll` "trimmable", we can simply add: [assembly: AssemblyMetadata ("IsTrimmable", "True")] This doesn't mean it actually *works* though! We should enable linker warnings and fix what it finds. To view linker warnings in .NET 6, you can do: > .\bin\dotnet\dotnet.exe build ` .\src\Controls\samples\Controls.Sample.SingleProject\Maui.Controls.Sample.SingleProject.csproj -f net6.0-android ` -c Release ` -p:SuppressTrimAnalysisWarnings=false ` -p:TrimmerSingleWarn=false ` -bl Then open the `msbuild.binlog` file and filter the warnings (that get upgraded to errors) for `src/Compatibility`: src\Compatibility\Core\src\Android\Deserializer.cs(38,6): error IL2026: Microsoft.Maui.Controls.Compatibility.Platform.Android.Deserializer.<>c.<DeserializePropertiesAsync>b__2_0(): Using member 'System.Runtime.Serialization.XmlObjectSerializer.ReadObject(XmlDictionaryReader)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Data Contract Serialization and Deserialization might require types that cannot be statically analyzed. Make sure all of the required types are preserved. src\Compatibility\Core\src\Android\Deserializer.cs(71,6): error IL2026: Microsoft.Maui.Controls.Compatibility.Platform.Android.Deserializer.<>c__DisplayClass3_0.<SerializePropertiesAsync>b__0(): Using member 'System.Runtime.Serialization.XmlObjectSerializer.WriteObject(XmlDictionaryWriter,Object)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Data Contract Serialization and Deserialization might require types that cannot be statically analyzed. Make sure all of the required types are preserved. src\Compatibility\Core\src\Android\AndroidAppIndexProvider.cs(14,5): error IL2072: Microsoft.Maui.Controls.Compatibility.Platform.Android.AndroidAppIndexProvider.AndroidAppIndexProvider(Context): '#0' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 'System.Object System.Activator::CreateInstance(System.Type,System.Object[],System.Object[])'. The return value of method 'System.Type.GetType(String,Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. src\Compatibility\Core\src\AppHostBuilderExtensions.cs(49,4): error IL2091: Microsoft.Maui.Controls.Hosting.MauiAppBuilderExtensions.UseMauiApp<TApp>(MauiAppBuilder): 'TImplementation' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in 'Microsoft.Extensions.DependencyInjection.Extensions.ServiceCollectionDescriptorExtensions.TryAddSingleton<TService,TImplementation>(IServiceCollection)'. The generic parameter 'TApp' of 'Microsoft.Maui.Controls.Hosting.MauiAppBuilderExtensions.UseMauiApp<TApp>(MauiAppBuilder)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. src\Compatibility\Core\src\Android\NativeBindingservice.cs(16,4): error IL2075: Microsoft.Maui.Controls.Compatibility.Platform.Android.NativeBindingService.TrySetBinding(Object,String,BindingBase): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Type System.Object::GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. src\Compatibility\Core\src\Android\ResourceManager.cs(28,4): error IL2026: Microsoft.Maui.Controls.Compatibility.Platform.Android.ResourceManager.FindType(String,String): Using member 'System.Reflection.Assembly.GetTypes()' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed. src\Compatibility\Core\src\Android\ResourceManager.cs(419,4): error IL2070: Microsoft.Maui.Controls.Compatibility.Platform.Android.ResourceManager.GetId(Type,String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.GetFields()'. The parameter 'type' of method 'Microsoft.Maui.Controls.Compatibility.Platform.Android.ResourceManager.GetId(Type,String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. src\Compatibility\Core\src\Android\ResourceManager.cs(432,5): error IL2070: Microsoft.Maui.Controls.Compatibility.Platform.Android.ResourceManager.GetId(Type,String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperties()'. The parameter 'type' of method 'Microsoft.Maui.Controls.Compatibility.Platform.Android.ResourceManager.GetId(Type,String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. I went through applying the new linker attributes to resolve these issues in this assembly. In most cases, I simply added C# attributes to solve warnings. However, a couple places needed code changes: * `Deserializer`, I removed usage of a lambda for a regular method. There isn't a way to decorate lambdas with C# attributes to appease the linker. * `ResourceManager.FindType()`, I replaced System.Linq usage with a `foreach` loop that the linker could better understand (lambdas again!). This should generally improve performance, anyway. Lastly, I had to actually add some attributes to `DependencyService`, otherwise we got crashes at startup like: android.runtime.JavaProxyThrowable: System.MissingMethodException: Arg_NoDefCTor, Microsoft.Maui.Controls.Compatibility.Platform.Android.NativeValueConverterService at System.RuntimeType.CreateInstanceMono(Boolean , Boolean ) at System.RuntimeType.CreateInstanceDefaultCtor(Boolean , Boolean ) at System.Activator.CreateInstance(Type , Boolean , Boolean ) at System.Activator.CreateInstance(Type , Boolean ) at System.Activator.CreateInstance(Type ) at Microsoft.Maui.Controls.DependencyService.Get[INativeValueConverterService](DependencyFetchTarget fetchTarget) at Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.ConvertTo(Object value, Type toType, Func`1 getConverter, IServiceProvider serviceProvider, Exception& exception) at Microsoft.Maui.Controls.Xaml.TypeConversionExtensions.ConvertTo(Object value, Type toType, Func`1 minfoRetriever, IServiceProvider serviceProvider, Exception& exception) at Microsoft.Maui.Controls.Xaml.ValueConverterProvider.Convert(Object value, Type toType, Func`1 minfoRetriever, IServiceProvider serviceProvider) at Microsoft.Maui.Controls.Xaml.AppThemeBindingExtension.Microsoft.Maui.Controls.Xaml.IMarkupExtension<Microsoft.Maui.Controls.BindingBase>.ProvideValue(IServiceProvider serviceProvider) at Maui.Controls.Sample.XamlApp.InitializeComponent() ~~ Results ~~ I built `Maui.Controls.Sample.SingleProject.csproj` with `-p:AndroidUseAssemblyStore=false`, so we can see the size differences of individual assemblies. * Before 30275786 bytes * After 30181399 bytes > apkdiff -f before.apk after.apk Size difference in bytes ([*1] apk1 only, [*2] apk2 only): + 437 assemblies/Microsoft.Maui.Controls.dll + 161 assemblies/x86/System.Private.CoreLib.dll + 160 assemblies/arm64-v8a/System.Private.CoreLib.dll + 139 assemblies/armeabi-v7a/System.Private.CoreLib.dll + 127 assemblies/x86_64/System.Private.CoreLib.dll + 28 assemblies/System.Runtime.dll + 1 assemblies/Maui.Controls.Sample.SingleProject.dll - 1 assemblies/Microsoft.Maui.Controls.Xaml.dll - 15 assemblies/System.Xml.ReaderWriter.dll - 16 assemblies/System.Threading.dll - 231 META-INF/BNDLTOOL.SF - 231 META-INF/MANIFEST.MF - 2,498 assemblies/System.Runtime.Serialization.Xml.dll *1 - 3,523 assemblies/System.IO.IsolatedStorage.dll *1 - 6,341 assemblies/Mono.Android.dll - 8,352 lib/armeabi-v7a/libxamarin-app.so - 8,352 lib/x86/libxamarin-app.so - 8,408 lib/arm64-v8a/libxamarin-app.so - 8,408 lib/x86_64/libxamarin-app.so - 38,104 classes.dex - 72,810 assemblies/Microsoft.Maui.Controls.Compatibility.dll Summary: - 462 Other entries -0.00% (of 12,105,588) - 84,151 Assemblies -0.82% (of 10,293,792) - 38,104 Dalvik executables -0.59% (of 6,486,156) - 33,520 Shared libraries -0.18% (of 18,486,516) - 183,296 Uncompressed assemblies -0.81% (of 22,604,792) - 94,387 Package size difference -0.31% (of 30,275,786) An average of 10 runs on a Pixel 5, seems to show a difference in startup as well: Before: Activity: Displayed 1.676s After: Activity: Displayed 1.642s I think this might help startup by ~34ms?
#if NETSTANDARD2_0 || NETSTANDARD2_1 | ||
namespace System.Diagnostics.CodeAnalysis | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new linker attributes are only in net5.0
and higher. It would be incredibly ugly to sprinkle #if
everywhere we use them.
So I took the approach we used in Xamarin.Android, which is to include an internal
copy of these attributes for target frameworks that don't have them:
https://github.com/xamarin/xamarin-android/tree/main/src-ThirdParty/System.Diagnostics.CodeAnalysis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely the recommended way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it always feels weird to copy source code into your repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're discussing if we should release a NuGet which includes the source files into your project automagically. To make this easier and more "official". But so far there were relatively few cases of it.
using Android.Content; | ||
|
||
namespace Microsoft.Maui.Controls.Compatibility.Platform.Android | ||
{ | ||
public class AndroidAppIndexProvider : IAppIndexingProvider | ||
{ | ||
[UnconditionalSuppressMessage ("Trimming", "IL2035", Justification = AppLinksAssemblyName + ".dll is not always present.")] | ||
[UnconditionalSuppressMessage ("Trimming", "IL2072", Justification = AppLinksAssemblyName + ".dll is not always present.")] | ||
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicConstructors, AppLinksAssemblyName + "." + AppLinksClassName, AppLinksAssemblyName)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably related to dotnet/linker#2392 as well.
But currently this is likely the best way to solve this.
[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "Resource.designer.cs is in the root application assembly, which should be preserved.")] | ||
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth at least creating a bug for this. It's true that by default this is going to be the case (at least I think), but it's still user-configurable, so it is possible to get to the situation where it could be trimmed away.
Since it seems we know statically what types we want from that assembly, this might be solvable - either via source generation or some other mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is OK.
This class is reflecting on a type that the Android build system generates: Resource.designer.cs
It is a class with a bunch of static readonly int
fields that represent Android resource files. The way it is implemented, the main app assembly (should be the root) always contains the class and it is never linked away. It is the C# implementation of the Android R.java
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I change the settings for the app to run the main assembly with "link", would it not trim away even this type (or the members in it)?
// https://github.com/dotnet/runtime/blob/f130138b337b57342e94dabf499b818531effed5/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/DataContract.cs#L31-L32 | ||
internal const string SerializerTrimmerWarning = "Data Contract Serialization and Deserialization might require types that cannot be statically analyzed. Make sure all of the required types are preserved."; | ||
|
||
internal const string NativeBindingService = "This method properly handles missing properties, and there is not a way to preserve them from this method."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good that the code handles missing properties, the question is if missing property can cause application behavior change? If yes - then this is still a problem.
For example:
- Serializers can handle missing properties just fine, they just don't write them out. But this can break apps as the deserialization of such payload may fail due to the missing property.
- On the other hand, a debugger visualizer can handle missing properties and it will not break the app if it doesn't show a value for unused property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this one might cause an issue...
I think this is a feature that came from Xamarin.Forms:
https://docs.microsoft.com/en-us/xamarin/xamarin-forms/platform/native-views/xaml
So you might have .xaml
like:
<ios:UILabel Text="{Binding MyProperty}" />
But then, I don't think that UILabel.Text
would actually get linked away because it's used by other Maui assemblies (that aren't trimmed). This type is in Xamarin.iOS.dll
with all the native UIKit APIs, which is trimmable.
So seems like it would only break if:
- You tried to binding against a specific property Maui doesn't use at all.
- You have XamlC off (users would have to go out of their way to turn it off).
So maybe users aren't likely to hit this -- and this feature is not probably used much. I would probably be fine to see if anyone runs into this during future previews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable - with my comment I just wanted to point out that not crashing is kind of the "min bar" 😉
Description of Change
To make
Microsoft.Maui.Controls.Compatibility.dll
"trimmable", wecan simply add:
This doesn't mean it actually works though! We should enable linker
warnings and fix what it finds.
To view linker warnings in .NET 6, you can do:
Then open the
msbuild.binlog
file and filter the warnings (that getupgraded to errors) for
src/Compatibility
:I went through applying the new linker attributes to resolve these
issues in this assembly.
In most cases, I simply added C# attributes to solve warnings.
However, a couple places needed code changes:
Deserializer
, I removed usage of a lambda for a regular method.There isn't a way to decorate lambdas with C# attributes to appease
the linker.
ResourceManager.FindType()
, I replaced System.Linq usage with aforeach
loop that the linker could better understand (lambdasagain!). This should generally improve performance, anyway.
Lastly, I had to actually add some attributes to
DependencyService
,otherwise we got crashes at startup like:
Results
I built
Maui.Controls.Sample.SingleProject.csproj
with-p:AndroidUseAssemblyStore=false
, so we can see the size differencesof individual assemblies.
Before 30275786 bytes
After 30181399 bytes
An average of 10 runs on a Pixel 5, seems to show a difference in
startup as well:
I think this might help startup by ~34ms?
PR Checklist
Does this PR touch anything that might affect accessibility?
No