maui icon indicating copy to clipboard operation
maui copied to clipboard

Border StrokeShape performance issues

Open TheStone03 opened this issue 1 year ago • 8 comments

Description

I have a ResourceDictionary with a style for Borders. in the setters i set the StrokeShape property with a StaticResource that is a RoundRectangle.

I use this border style in a lot of my customControls and it slows the application a lot. (with this style initialization of the border is up to 60ms, without is 1 or 2ms). In the linked repository sample, it change from 4/5ms to 0ms

Steps to Reproduce

-Open the sample repository -In the Resources\Styles\Style.xaml you have to chose what style to use for debug. (if you want to se the perfomance issue, or if you want to see my workaround) -Run and se the output console -Don't just click the refresh button once but click it, at least 3 times to see actual differences

Link to public reproduction project repository

https://github.com/TheStone03/StrokeShapeBug.git

Version with bug

8.0.3

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

No response

Did you find any workaround?

I found a Workaround but is not so clear to me. Instead of having for StaticResource a RoundRectangle to set as value of the StrokeShape, i have a string: "RoundRectangle 10,10,10,10". I Bind this in the Setter.Value and use a CustomConverter for convert the string to a RoundRectangle.

And that's it. I don't know how or why but with this workaround i don't have performance issues anymore.

If it has also to convert i thought it will increase the timing but it doesn't. why? I can't undestand.

Relevant log output

No response

TheStone03 avatar Nov 21 '23 13:11 TheStone03

The reason @TheStone03's workaround works is because it creates a new instance of RoundReactange shape for each element in the page. Normally what happens is that any Shape set via Style is obviously a single instance.

This single instance is then being used everywhere in the app, and that is really concerning for the following reasons.

  1. the instance is added as LogicalChildren which means that every element using this shape will override the BindingContext of the shape (last one wins)
  2. Some properties propagates automatically to LogicalChildren, for example Window (which is part of element visualisation process) will trigger this property changed every time an element using this instance of Shape is being added/removed from the page, and that will cause all the border elements to redraw the shape
  3. On top of that, also Stroke brush has issues, but only the number 1
  4. Memory Leaks

Now, I get this was done to support the following use case:

<Border>
    <Border.StrokeShape>
        <RoundReactangle CornerRadius="{Binding MyCornerRadius}" />
    </Border.StrokeShape>
<Border>

But when that property comes from a single instance (like from Styles.xaml) this is just a bad situation.

Given the state of the art, and considering that both Geometry and Shape implement IShape these are my suggestions:

Solution A

This solution doesn't fix issues number 1 and 3 (which are also a really minor source of performance degradation).

Solution B

This solution fixes all issues but requires telling developers to use a different API in different situations.

  1. Change StrokeShapeTypeConverter to support strings like RoundRectangeGeometry 40 and return corresponding geometry
  2. Change Border.NotifyStrokeShapeChanges class to take into consideration an IShape which is not a VisualElement and in that case avoid adding the logical children
  3. Make BrushTypeConverter return an ImmutableSolidColorBrush which inherits from ImmutableBrush

CC @jsuarezruiz

albyrock87 avatar Nov 23 '23 09:11 albyrock87

@TheStone03 if you're not interested in dynamically changing border shapes (using binding as I mentioned in the above comment / using C# to dynamically manipulate shape properties), you can use the following workaround.

Invoke BorderPerformanceWorkaround.Init(); on top of your MauiProgram.cs.

public static class BorderPerformanceWorkaround
{
    private static readonly PropertyInfo _bindablePropertyPropertyChangingProperty =
        typeof(BindableProperty).GetProperty("PropertyChanging", BindingFlags.Instance | BindingFlags.NonPublic);

    private static readonly PropertyInfo _bindablePropertyPropertyChangedProperty =
        typeof(BindableProperty).GetProperty("PropertyChanged", BindingFlags.Instance | BindingFlags.NonPublic);

    public static void Init()
    {
        _bindablePropertyPropertyChangingProperty.SetValue(Border.StrokeShapeProperty, new BindableProperty.BindingPropertyChangingDelegate((bindable, oldValue, newValue) => { }));
        _bindablePropertyPropertyChangedProperty.SetValue(Border.StrokeShapeProperty, new BindableProperty.BindingPropertyChangedDelegate((bindable, oldValue, newValue) => { }));
        _bindablePropertyPropertyChangingProperty.SetValue(Border.StrokeProperty, new BindableProperty.BindingPropertyChangingDelegate((bindable, oldValue, newValue) => { }));
        _bindablePropertyPropertyChangedProperty.SetValue(Border.StrokeProperty, new BindableProperty.BindingPropertyChangedDelegate((bindable, oldValue, newValue) => { }));
    }
}

albyrock87 avatar Nov 23 '23 10:11 albyrock87

@albyrock87 thanks for your solution and clear description. Testing your workaround the performance seems equals to the one i get with my workaround(converter). measuring in CPU ticks, your solutions is a little bit better than mine.

Do you suggest to use yours for a better code or logic? or is not so important if i use one or the other? Thanks

TheStone03 avatar Nov 23 '23 10:11 TheStone03

@TheStone03 I would say it is a matter of pros&cons.

Your solution

  • pros: you can selectively pick the approach you prefer
  • cons: it requires you to write custom code around the application (harder to remove)

My solution

  • pros: no custom code (easy to remove once they fix the issue)
  • cons: you cannot bind on border's shape properties nor running animation on that (imagine an animation which keeps changing the corner radius of a round rectangle)

albyrock87 avatar Nov 23 '23 10:11 albyrock87

Ok. Thank you so much.

TheStone03 avatar Nov 23 '23 10:11 TheStone03

@TheStone03 I had another idea, may you try to do this simple workaround and let me know if it fixes the perf issue?

<RoundRectangle x:Key="CornerRadius" BindingContext="{x:Null}" CornerRadius="10,10,10,10" />

I'm basically suggesting to set BindingContext="{x:Null}" so that the automatic inheritance won't be applied.

Edit: and the same should be done on the Brush attached to Stroke property.

albyrock87 avatar Jan 01 '24 15:01 albyrock87

@TheStone03 I had another idea, may you try to do this simple workaround and let me know if it fixes the perf issue?

<RoundRectangle x:Key="CornerRadius" BindingContext="{x:Null}" CornerRadius="10,10,10,10" />

I'm basically suggesting to set BindingContext="{x:Null}" so that the automatic inheritance won't be applied.

Edit: and the same should be done on the Brush attached to Stroke property.

Hi @albyrock87, I tested your new workaround in my sample project and I found that your previous workaround is always the best in terms of performances in my case.

Thank you for your time.

TheStone03 avatar Jan 03 '24 07:01 TheStone03

This causes major performance issues. Without the workaround my App goes up linearly about 100-200ms (starting from ~300ms) loading times of each page after each navigation to a transient page that has Borders with Strokeshapes. So after about 10 navigations, I have loading times of over 2 seconds and it doesnt stop there. This does disappear when using @albyrock87's workaround and the loading times stay steady.

borrmann avatar Jan 27 '24 12:01 borrmann

The AutoDisconnectBehavior from my memory toolkit appears to fix this issue on iOS and Android. You can give it a go on Windows and see if it helps: https://github.com/AdamEssenmacher/MemoryToolkit.Maui

AdamEssenmacher avatar Feb 02 '24 09:02 AdamEssenmacher

The AutoDisconnectBehavior from my memory toolkit appears to fix this issue on iOS and Android. You can give it a go on Windows and see if it helps: https://github.com/AdamEssenmacher/MemoryToolkit.Maui

Thanks for your efforts but i "rage quitted" MAUI for to many problems. So i can't test your toolkit.

TheStone03 avatar Feb 02 '24 10:02 TheStone03

This problem seems to have been fixed by https://github.com/dotnet/maui/pull/21151 (Solution A)

taenito avatar Apr 11 '24 07:04 taenito

Verified this issue with Visual Studio 17.10.0 Preview 4 ( 8.0.21).I cannot repro this issue.

ninachen03 avatar Apr 24 '24 07:04 ninachen03

Hi @TheStone03. We have added the "s/try-latest-version" label to this issue, which indicates that we'd like you to try and reproduce this issue on the latest available public version. This can happen because we think that this issue was fixed in a version that has just been released, or the information provided by you indicates that you might be working with an older version.

You can install the latest version by installing the latest Visual Studio (Preview) with the .NET MAUI workload installed. If the issue still persists, please let us know with any additional details and ideally a reproduction project provided through a GitHub repository.

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.