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

[xaml] improve performance in debug-mode #21460

Merged

Conversation

jonathanpeppers
Copy link
Member

Applies to: #18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip

In reviewing various issues about CollectionView, there is a pattern of:

  • My app is slow (in Debug mode)

  • Oh, but it seems completely OK in Release!

This is due to several features enabled in Debug mode, such as:

  • On mobile, the Mono interpreter is used (enables C# hot reload)

  • Xaml compilation is disabled (enables XAML hot reload)

  • Other debug features are enabled (such as the $(Optimize) flag)

I attached dotnet-trace to the app above on a Pixel 5 while debugging, just to see what I could find. I also had XAML & C# hot reload enabled.

One thing that appears when parsing XAML is:

  2.11s microsoft.maui.controls.xaml!Microsoft.Maui.Controls.Xaml.ApplyPropertiesVisitor.GetBindableProperty(System.Type,string
257.40ms System.Private.CoreLib!System.String.Format(string,object,object)
172.25ms microsoft.maui.controls!Microsoft.Maui.Controls.Xaml.XamlParseException.FormatMessage(string,System.Xml.IXmlLineInfo)

So about ~2 seconds of this app's startup was in
ApplyPropertiesVisitor.GetBindableProperty(). Looking at what is happening:

if (exception == null && bindableFieldInfo == null)
{
    exception =
        new XamlParseException(
            Format("BindableProperty {0} not found on {1}", localName + "Property", elementType.Name), lineInfo);
}
//...
if (throwOnError)
    throw exception; // only use of `exception`

But then throwOnError is actually false in this case, so a XamlParseException is created and not used. I could just reorder the logic to avoid creating a XamlParseException and calling Format().

Next, I noticed the System.Reflection usage:

var bindableFieldInfo = elementType.GetFields(supportedFlags)
    .FirstOrDefault(fi => (fi.IsAssembly || fi.IsPublic) && fi.Name == localName + "Property");

If this were called on many bindable properties on Label, Button, etc., this code would create lots of FieldInfo[] arrays and iterate until a matching property (field) is found.

I think we can change this to avoid creating FieldInfo[] arrays, such as:

var bindableFieldInfo = elementType.GetField(localName + "Property", supportedFlags);
if (bindableFieldInfo is not null && (bindableFieldInfo.IsAssembly || bindableFieldInfo.IsPublic))
{
    //...

With these changes in place, it significantly improves the call:

816.35ms microsoft.maui.controls.xaml!Microsoft.Maui.Controls.Xaml.ApplyPropertiesVisitor.GetBindableProperty(System.Type,string

Saving over about ~1.3 seconds of startup time in debug mode.

This should improve the performance of XAML parsing on all platforms, which is most likely being used while debugging. You can disable Xaml compilation in Release mode, but it is not recommended.

Applies to: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip

In reviewing various issues about `CollectionView`, there is a pattern of:

* My app is slow (in `Debug` mode)

* Oh, but it seems completely OK in `Release`!

This is due to several features enabled in `Debug` mode, such as:

* On mobile, the Mono interpreter is used (enables C# hot reload)

* Xaml compilation is disabled (enables XAML hot reload)

* Other debug features are enabled (such as the `$(Optimize)` flag)

I attached `dotnet-trace` to the app above on a Pixel 5 while
debugging, just to see what I could find. I also had XAML & C# hot
reload enabled.

One thing that appears when parsing XAML is:

      2.11s microsoft.maui.controls.xaml!Microsoft.Maui.Controls.Xaml.ApplyPropertiesVisitor.GetBindableProperty(System.Type,string
    257.40ms System.Private.CoreLib!System.String.Format(string,object,object)
    172.25ms microsoft.maui.controls!Microsoft.Maui.Controls.Xaml.XamlParseException.FormatMessage(string,System.Xml.IXmlLineInfo)

So about ~2 seconds of this app's startup was in
`ApplyPropertiesVisitor.GetBindableProperty()`. Looking at what is
happening:

    if (exception == null && bindableFieldInfo == null)
    {
        exception =
            new XamlParseException(
                Format("BindableProperty {0} not found on {1}", localName + "Property", elementType.Name), lineInfo);
    }
    //...
    if (throwOnError)
        throw exception; // only use of `exception`

But then `throwOnError` is actually `false` in this case, so a
`XamlParseException` is created and not used. I could just reorder the
logic to avoid creating a `XamlParseException` and calling `Format()`.

Next, I noticed the System.Reflection usage:

    var bindableFieldInfo = elementType.GetFields(supportedFlags)
        .FirstOrDefault(fi => (fi.IsAssembly || fi.IsPublic) && fi.Name == localName + "Property");

If this were called on many bindable properties on `Label`, `Button`,
etc., this code would create lots of `FieldInfo[]` arrays and iterate
until a matching property (field) is found.

I think we can change this to avoid creating `FieldInfo[]` arrays,
such as:

    var bindableFieldInfo = elementType.GetField(localName + "Property", supportedFlags);
    if (bindableFieldInfo is not null && (bindableFieldInfo.IsAssembly || bindableFieldInfo.IsPublic))
    {
        //...

With these changes in place, it significantly improves the call:

    816.35ms microsoft.maui.controls.xaml!Microsoft.Maui.Controls.Xaml.ApplyPropertiesVisitor.GetBindableProperty(System.Type,string

Saving over about ~1.3 seconds of startup time in debug mode.

This should improve the performance of XAML parsing on all platforms,
which is most likely being used while debugging. You *can* disable
Xaml compilation in `Release` mode, but it is not recommended.
@jsuarezruiz jsuarezruiz added area-xaml XAML, CSS, Triggers, Behaviors legacy-area-perf Startup / Runtime performance labels Mar 27, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 27, 2024 21:14
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner March 27, 2024 21:14
@StephaneDelcroix
Copy link
Contributor

IIRC, there was a reason using GetFields() instead of GetField(). Could be that one looks up the hierarchy and the other not, or the order, or different versions using different profiles (netstandard1, ...), But it should be caught by unit tests

if (throwOnError)
throw exception;
{
throw new XamlParseException(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it even looks like all invocations of GetBindableProperty use false for throwOnError, so this could be simplified, and the signature changed to return a BP? so the intention are clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's probably a story behind that, but even I can't remember it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the throwOnError parameter. 👍

@StephaneDelcroix StephaneDelcroix enabled auto-merge (squash) March 28, 2024 15:03
@StephaneDelcroix StephaneDelcroix merged commit a7434fc into dotnet:main Mar 29, 2024
47 checks passed
@jonathanpeppers jonathanpeppers deleted the ApplyPropertiesVisitorPerformance branch March 29, 2024 12:00
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.20 fixed-in-9.0.0-preview.4.10690 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants