dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.04k stars 1.73k forks source link

Proposal: Remove Application.Properties #4864

Closed eerhardt closed 2 years ago

eerhardt commented 2 years ago

Description

We have a couple Obsolete APIs on Microsoft.Maui.Controls.Application:

https://github.com/dotnet/maui/blob/de28ddd2656e8055714a55a58db53efe544e3c9e/src/Controls/src/Core/Application.cs#L114-L127

https://github.com/dotnet/maui/blob/de28ddd2656e8055714a55a58db53efe544e3c9e/src/Controls/src/Core/Application.cs#L267-L280

These APIs bring in a fairly large dependency: DataContractSerializer (and with it, a lot of System.Xml code). This is the only usage of DataContractSerializer in a dotnet new maui application. ~So if we can remove these Obsolete APIs on Application, it will remove DataContractSerializer and a bunch of System.Xml code.~ We should remove the implementations of these methods, so the DataContractSerializer code can be trimmed in an application. We will leave the APIs in place, with an Obsolete attribute to point migrating developers to the corresponding Essentials APIs.

Android

In my local measurements, for a Release Android arm64 application, this change will remove ~7.5% of the IL code. Summing all the (uncompressed) .dll files in the .apk, I see:

Main: 10.9 MB (11,431,696 bytes) Remove Application.Properties: 10.0 MB (10,556,176 bytes)

The main differences can be seen here (left is main and right is a prototype of this change):

image

You can see that 4 whole assemblies are removed. And System.Private.CoreLib and System.Private.Xml get a lot smaller.

iOS

The numbers for an iOS published .ipa file with this change are similar:

Main: 18.0 MB (18,918,198 bytes) Remove Application.Properties: 17.0 MB (17,850,723 bytes)

So we see a 5.6% size reduction (1 full MB) in app size on iOS with this change.

Public API Changes

namespace Microsoft.Maui.Controls
{
    public class Application 
    {
-       [Obsolete("Properties API is obsolete, use Essentials.Preferences instead.")]
+       [Obsolete("Properties API is obsolete, use Essentials.Preferences instead.", error: true)]
-       public IDictionary<string, object> Properties { get; }
+       public IDictionary<string, object> Properties => throw new NotSupportedException();

-       [Obsolete("Properties API is obsolete, use Essentials.Preferences instead.")]
+       [Obsolete("Properties API is obsolete, use Essentials.Preferences instead.", error: true)]
-       public Task SavePropertiesAsync();
+       public Task SavePropertiesAsync() => throw new NotSupportedException();
    }
}

namespace Microsoft.Maui.Controls.Internals
{
-   [EditorBrowsable(EditorBrowsableState.Never)]
-   public interface IDeserializer
-   {
-       Task<IDictionary<string, object>> DeserializePropertiesAsync();
-       Task SerializePropertiesAsync(IDictionary<string, object> properties);
-   }
}

Intended Use-Case

I want my Maui applications to be as small as possible, without unnecessary code that bloats the application.

cc @mattleibow @PureWeen @hartez @drasticactions

drasticactions commented 2 years ago

If we have any tagged obsolete items, be it in Compatibility, Controls, Core, etc, they should be gone, IMO. Considering the platform hasn't hit 1.0, this is the time to start over and remove things we know we don't want to maintain, especially if it will improve performance long term.

mattleibow commented 2 years ago

Obsolete is for compatibility with Xamarin Forms and can be removed in .NET 7. We can't break everything. This API was not obsolete in Xamarin Forms, we obsoleted it in MAUI and will remove in MAUI 2.0.

mattleibow commented 2 years ago

That being said, unless we can determine usage of this property, we cannot just remove. It may not have worked well, but if it is being used, PM needs to decide.

We need a linker step to check for usage of this property and then remove :)

eerhardt commented 2 years ago

We need a linker step to check for usage of this property and then remove :)

A possible alternative to removing the API is to marking the Controls and Compatibility libraries as IsTrimmable=true. That will allow the public API to be trimmed if it isn't used in the app. However, the size reduction won't be significant with that approach because we unconditionally root the internal classes that implement IDeserializer here:

https://github.com/dotnet/maui/blob/de28ddd2656e8055714a55a58db53efe544e3c9e/src/Compatibility/Core/src/AppHostBuilderExtensions.cs#L64

And then also SendSleep() calls into these Properties APIs here:

https://github.com/dotnet/maui/blob/de28ddd2656e8055714a55a58db53efe544e3c9e/src/Controls/src/Core/Application.cs#L351-L355

Given these 2 things, we won't be trimming any DataContractSerializer code with that approach.

eerhardt commented 2 years ago

So I spent the day getting my mac set back up, and getting the numbers for an iOS published .ipa file with this change:

Main: 18.0 MB (18,918,198 bytes) Remove Application.Properties: 17.0 MB (17,850,723 bytes)

So we see a 5.6% size reduction (1 full MB) in app size on iOS with this change. I've updated the OP with this info as well.

cc @davidortinau - any thoughts on this proposal? Would it be acceptable to remove these APIs?

davidortinau commented 2 years ago

This seems worthwhile. The choice to obsolete and point developers to Essentials was already vetted with various customers. We will document this removal and guidance for replacing it with Essentials. @davidbritch

If possible, let's still provide a good compiler error for Xamarin.Forms developers migrating so they get the guidance to update.