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

CollectionView slow/jittering scroll #12130

Closed
angelru opened this issue Dec 15, 2022 · 49 comments
Closed

CollectionView slow/jittering scroll #12130

angelru opened this issue Dec 15, 2022 · 49 comments
Assignees
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/android 🤖 s/duplicate 2️⃣ This issue or pull request already exists t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone

Comments

@angelru
Copy link

angelru commented Dec 15, 2022

Description

When there is a complex layout Scroll does not work efficiently.

Steps to Reproduce

I have this layout, as you can see I have used the recommended bindings OneTime and CompressedLayout.IsHeadless="True".

ItemSizingStrategy has to be in mode: MeasureAllItems. Each element can have a different height.

There are like 30-40 elements, the Scroll is unacceptable, both in debug/release mode. Thus, the application cannot be released to production.

I don't know if I can improve my design, but i can't get it. Any help is welcome.

Link to public reproduction project repository

https://github.com/angelru/CvSlowJittering

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

Android 12

Did you find any workaround?

No response

Relevant log output

No response

@angelru angelru added the t/bug Something isn't working label Dec 15, 2022
@jsuarezruiz jsuarezruiz added legacy-area-perf Startup / Runtime performance area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Dec 15, 2022
@LennoxP90
Copy link

have you tried compiling using AOT?

<RunAOTCompilation>true</RunAOTCompilation>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot>

you app size will be larger but AOT should fix performance issues

@angelru
Copy link
Author

angelru commented Dec 15, 2022

have you tried compiling using AOT?

<RunAOTCompilation>true</RunAOTCompilation>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot>

you app size will be larger but AOT should fix performance issues

the improvement in Scroll is very small, it's still very, very inefficient.
Also, often when I'm scrolling all the time, there comes a time when the application becomes slower and more unstable.

@angelru
Copy link
Author

angelru commented Dec 15, 2022

@LennoxP90
Test my layout with about 40 elements and see how it performs.

@mohachouch
Copy link
Contributor

@angelru Can you try with the same item height if the scroll is smooth

@angelru
Copy link
Author

angelru commented Dec 17, 2022

@angelru Can you try with the same item height if the scroll is smooth

the same issue

@matmork
Copy link

matmork commented Dec 18, 2022

Have you tried changing to a ListView?
also try this Sharpnado.CollectionView

@jsuarezruiz jsuarezruiz added the s/needs-repro Attach a solution or code which reproduces the issue label Dec 19, 2022
@ghost
Copy link

ghost commented Dec 19, 2022

Hi @angelru. We have added the "s/needs-repro" label to this issue, which indicates that we require steps and sample code to reproduce the issue before we can take further action. Please try to create a minimal sample project/solution or code samples which reproduce the issue, ideally as a GitHub repo that we can clone. See more details about creating repros here: https://github.com/dotnet/maui/blob/main/.github/repro.md

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.

@angelru
Copy link
Author

angelru commented Dec 20, 2022

@jsuarezruiz
I already put the repository in the problem description
https://github.com/angelru/CvSlowJittering

@ghost ghost added s/needs-attention Issue has more information and needs another look and removed s/needs-repro Attach a solution or code which reproduces the issue labels Dec 20, 2022
@angelru
Copy link
Author

angelru commented Dec 20, 2022

@mohachouch
With my provided example, were you able to see the lag?

@rachelkang rachelkang added this to the Backlog milestone Dec 20, 2022
@ghost
Copy link

ghost commented Dec 20, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@rachelkang rachelkang removed the s/needs-attention Issue has more information and needs another look label Dec 20, 2022
@mohachouch
Copy link
Contributor

mohachouch commented Dec 20, 2022

@angelru Yes, I tested on Samsung Galaxy Tab Active 2 - Android 9. The collection View scroll is laggy, very very slow @jsuarezruiz

@SailDev
Copy link

SailDev commented Jan 27, 2023

I had the same problem with a "jittery" collection view.

A simple list page structure with a custom "toolbar" control at the top and a vertical collection view below.
The toolbar buttons are represented by a custom button control, which i use all over the place.
The button control itself is based on a grid with some nested grids for background, overlays and text.

Nothing very complex, 5 buttons and a collection view. But scrolling through the collection view felt very strange, especially when reaching the top or bottom of the view.

I spent HOURS, to try every possible settings combination on the collection view and bindings, with no success.
Then i switched to my custom toolbar and button controls, replacing inner controls (i.e. based on SkiaSharp) with simpler representations. Also with no success.

At the end of the day, i added one line to the outer container grid of my custom button control.
HeightRequest="40"
. Now the scrolling behaviour of the collection view is as smooth as expected.

It's interesting, that the reason was not the collection view, but a simple button control somewhere on the same page.

I tried, to extract the issue to a simpler demo project, but unfortunately i ran out of time.

@angelru
Copy link
Author

angelru commented Jan 27, 2023

I had the same problem with a "jittery" collection view.

A simple list page structure with a custom "toolbar" control at the top and a vertical collection view below. The toolbar buttons are represented by a custom button control, which i use all over the place. The button control itself is based on a grid with some nested grids for background, overlays and text.

Nothing very complex, 5 buttons and a collection view. But scrolling through the collection view felt very strange, especially when reaching the top or bottom of the view.

I spent HOURS, to try every possible settings combination on the collection view and bindings, with no success. Then i switched to my custom toolbar and button controls, replacing inner controls (i.e. based on SkiaSharp) with simpler representations. Also with no success.

At the end of the day, i added one line to the outer container grid of my custom button control. HeightRequest="40". Now the scrolling behaviour of the collection view is as smooth as expected.

It's interesting, that the reason was not the collection view, but a simple button control somewhere on the same page.

I tried, to extract the issue to a simpler demo project, but unfortunately i ran out of time.

thanks for this information, if you run my provided sample you can see what happens, I haven't found a way to fix it yet.

@LennoxP90
Copy link

I increased my MONO_GC_PARAMS=nursery-size=128m and place that in a file compiled it as AndroidEnvironment, and my speed issues seem to have resolved itself.

@angelru
Copy link
Author

angelru commented Jan 29, 2023

I increased my MONO_GC_PARAMS=nursery-size=128m and place that in a file compiled it as AndroidEnvironment, and my speed issues seem to have resolved itself.

did you try my sample?

@drasticactions
Copy link
Contributor

@angelru I agree that CollectionView and StackLayout performance can leave much to be desired, especially on iOS and Catalyst. However, debugging your application, I think your primary issue may be how you laid out your CollectionView.

スクリーンショット 2023-02-07 18 14 13

Your CollectionView includes a Bounded VerticalStackLayout, which itself includes its own CollectionView. If you look at how this runs on iOS, these nested views are multiple UICollectionViews embedded inside each other. It "works" (in that, yes, it compiles and runs) but it would never be performant. Nested UICollectionViews are a bad idea.

スクリーンショット 2023-02-07 18 37 08

Looking a breakdown of the UI in Reveal, every time you scroll down the page, the CollectionViews alone are causing each cell to cause the previous one to eject from memory. Removing that nested CollectionView and the scrolling is much improved. While still not great, it's at least usable. Removing the BindableLayout for VerticalStackLayout and replacing it with a grid further improves the performance.

(I know I'm talking about iOS when I think everyone else here is talking about Android, but my guess if you look at the underlying views under it would similar with whatever controls are being used to render it.)

TL;DR I agree those views have performance issues, but IMO, even if it were working well, your code would be problematic.

@angelru
Copy link
Author

angelru commented Feb 7, 2023

@angelru I agree that CollectionView and StackLayout performance can leave much to be desired, especially on iOS and Catalyst. However, debugging your application, I think your primary issue may be how you laid out your CollectionView.

スクリーンショット 2023-02-07 18 14 13

Your CollectionView includes a Bounded VerticalStackLayout, which itself includes its own CollectionView. If you look at how this runs on iOS, these nested views are multiple UICollectionViews embedded inside each other. It "works" (in that, yes, it compiles and runs) but it would never be performant. Nested UICollectionViews are a bad idea.

スクリーンショット 2023-02-07 18 37 08

Looking a breakdown of the UI in Reveal, every time you scroll down the page, the CollectionViews alone are causing each cell to cause the previous one to eject from memory. Removing that nested CollectionView and the scrolling is much improved. While still not great, it's at least usable. Removing the BindableLayout for VerticalStackLayout and replacing it with a grid further improves the performance.

(I know I'm talking about iOS when I think everyone else here is talking about Android, but my guess if you look at the underlying views under it would similar with whatever controls are being used to render it.)

TL;DR I agree those views have performance issues, but IMO, even if it were working well, your code would be problematic.

but I need the bindable layouts, do you suggest that I change them to Grid BindableLayout ?

@drasticactions
Copy link
Contributor

@angelru "BindableLayout" in it of itself is not bad, it's how you're using it.

You have a VerticalStackLayout with a BindableLayout of CollectionViews, which itself is inside a CollectionView. That is causing your CollectionView items to unload and reload from memory every time you scroll. In general, AFAIK (@hartez may know more) it's preferred to have one CollectionView, max, per page.

You could refactor out that nested CollectionView with another BindableLayout (StackView, Grid, etc) and try with that and see what it does? It probably will still not be great (You can see the other issues filed for CollectionView performance, it does have issues) but it should be at least usable enough to keep working on your app.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 2, 2023
Context: dotnet#12130
Context: https://github.com/angelru/CvSlowJittering

When profiling the above customer app while scrolling, 4% of the time
is spent doing dictionary lookups:

    (4.0%) System.Private.CoreLib!System.Collections.Generic.Dictionary<TKey_REF,TValue_REF>.FindValue(TKey_REF)

Observing the call stack, some of these are coming from culture-aware
string lookups in MAUI:

* `microsoft.maui!Microsoft.Maui.PropertyMapper.GetProperty(string)`
* `microsoft.maui!Microsoft.Maui.WeakEventManager.AddEventHandler(System.EventHandler`1<TEventArgs_REF>,string)`
* `microsoft.maui!Microsoft.Maui.CommandMapper.GetCommand(string)`

Which show up as a mixture of `string` comparers:

    (0.98%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.71%) System.Private.CoreLib!System.String.GetNonRandomizedHashCode()
    (0.31%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri
    (0.01%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.GetStringComparer(object)

In cases of `Dictionary<string, TValue>` or `HashSet<string>`, we can
use `StringComparer.Ordinal` for faster dictionary lookups.

Unfortunately, there is no code analyzer for this:

dotnet/runtime#52399

So, I manually went through the codebase and found all the places.

I now only see the *fast* string comparers in this sample:

    (1.3%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.35%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri

Which is about ~0.36% better than before.

This should slightly improve the performance of handlers & all
controls on all platforms.

I also fixed `Microsoft.Maui.Graphics.Text.TextColors` to use
`StringComparer.OrdinalIgnoreCase` -- and removed a
`ToUpperInvariant()` call.
jonathanpeppers added a commit that referenced this issue May 3, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering

When profiling the above customer app while scrolling, 4% of the time
is spent doing dictionary lookups:

    (4.0%) System.Private.CoreLib!System.Collections.Generic.Dictionary<TKey_REF,TValue_REF>.FindValue(TKey_REF)

Observing the call stack, some of these are coming from culture-aware
string lookups in MAUI:

* `microsoft.maui!Microsoft.Maui.PropertyMapper.GetProperty(string)`
* `microsoft.maui!Microsoft.Maui.WeakEventManager.AddEventHandler(System.EventHandler`1<TEventArgs_REF>,string)`
* `microsoft.maui!Microsoft.Maui.CommandMapper.GetCommand(string)`

Which show up as a mixture of `string` comparers:

    (0.98%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.71%) System.Private.CoreLib!System.String.GetNonRandomizedHashCode()
    (0.31%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri
    (0.01%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.GetStringComparer(object)

In cases of `Dictionary<string, TValue>` or `HashSet<string>`, we can
use `StringComparer.Ordinal` for faster dictionary lookups.

Unfortunately, there is no code analyzer for this:

dotnet/runtime#52399

So, I manually went through the codebase and found all the places.

I now only see the *fast* string comparers in this sample:

    (1.3%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.35%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri

Which is about ~0.36% better than before.

This should slightly improve the performance of handlers & all
controls on all platforms.

I also fixed `Microsoft.Maui.Graphics.Text.TextColors` to use
`StringComparer.OrdinalIgnoreCase` -- and removed a
`ToUpperInvariant()` call.
@Odaronil
Copy link

Odaronil commented May 3, 2023

My God, the process is finally underway 💯 Really looking forward to good performance with ListView/CollectionView!!!

@angelru
Copy link
Author

angelru commented May 3, 2023

It's exciting to see the performance of the CollectionView being worked on when scrolling,

Since all this will not be seen until .NET 8, is there any way to integrate to .NET 7? Do we have to point our applications to the .NET 8 preview? can we install/update nugets?

@jonathanpeppers
Copy link
Member

In a future Visual Studio preview, there will be a .NET 8 Preview optional checkbox. And you can just change your project to net8.0 to try it.

In a more distant future, you'll be able to change <MauiVersion> to try new versions (even in different projects), but the groundwork for this is only in .NET 8+.

jonathanpeppers added a commit to dotnet/java-interop that referenced this issue May 4, 2023
Context: dotnet/maui#12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a .NET MAUI customer sample while scrolling on a Pixel 5, I
see ~2.2% of the time spent in:

    (2.2%) mono.android!Android.Views.View.IOnFocusChangeListenerImplementor..ctor()

https://github.com/dotnet/maui/blob/2041476e78452891029f4d2bd806c45be42f4878/src/Core/src/Handlers/View/ViewHandler.Android.cs#L14

MAUI subscribes to `Android.Views.View.FocusChange` for every view
placed on the screen -- which happens while scrolling in this sample.

Reviewing, the generated code for this constructor still uses the
outdated `JNIEnv` APIs:

    public IOnFocusChangeListenerImplementor () : base (global::Android.Runtime.JNIEnv.StartCreateInstance ("mono/android/view/View_OnFocusChangeListenerImplementor", "()V"), JniHandleOwnership.TransferLocalRef)
    {
        global::Android.Runtime.JNIEnv.FinishCreateInstance (((global::Java.Lang.Object) this).Handle, "()V");
    }

Which we can change to use the newer/faster Java.Interop APIs:

    public unsafe IOnFocusChangeListenerImplementor () : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
    {
        const string __id = "()V";
        if (((global::Java.Lang.Object) this).Handle != IntPtr.Zero)
            return;
        var h = JniPeerMembers.InstanceMethods.StartCreateInstance (__id, ((object) this).GetType (), null);
        SetHandle (h.Handle, JniHandleOwnership.TransferLocalRef);
        JniPeerMembers.InstanceMethods.FinishCreateInstance (__id, this, null);
    }

These are better because the equivalent call to `JNIEnv.FindClass()`
is cached among other things.

After these changes, I instead see:

    (0.81%) mono.android!Android.Views.View.IOnFocusChangeListenerImplementor..ctor()

This should improve the performance of all C# events that wrap Java
listeners -- and all .NET MAUI views on Android.

These changes also stop emitting the `[Register]` attribute for
`*Implementor` classes:

    [global::Android.Runtime.Register ("mono/android/view/View_OnFocusChangeListenerImplementor")]

Which is not needed, because `*Implementor` classes are generated,
internal implementation details.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 4, 2023
Context: dotnet#12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a .NET MAUI customer sample while scrolling on a Pixel 5, I
see some interesting time being spent in:

    (0.76%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.54%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This sample has a `<Border/>` inside a `<CollectionView/>` and so you
can see this work happening while scrolling.

Specifically, I found a couple places we had code like:

    _borderPaint.StrokeWidth = _strokeThickness;
    _borderPaint.StrokeJoin = _strokeLineJoin;
    _borderPaint.StrokeCap = _strokeLineCap;
    _borderPaint.StrokeMiter = _strokeMiterLimit * 2;
    if (_borderPathEffect != null)
        _borderPaint.SetPathEffect(_borderPathEffect);

This calls from C# to Java 5 times. Creating a new method in
`PlatformInterop.java` allowed me to reduce it to 1.

I also found:

    void SetDefaultBackgroundColor()
    {
        using (var background = new TypedValue())
        {
            if (_context == null || _context.Theme == null || _context.Resources == null)
                return;

            if (_context.Theme.ResolveAttribute(global::Android.Resource.Attribute.WindowBackground, background, true))
            {
                var resource = _context.Resources.GetResourceTypeName(background.ResourceId);
                var type = resource?.ToLowerInvariant();

                if (type == "color")
                {
                    var color = new AColor(ContextCompat.GetColor(_context, background.ResourceId));
                    _backgroundColor = color;
                }
            }
        }
    }

This is doing a lot of unnecessary stuff: looking up a resource by
name, etc. I found a very simple Java example we could put in
`PlatformInterop.java`:

https://stackoverflow.com/a/14468034

After these changes, I now see:

    (0.28%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.04%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This improves the performance of any `<Border/>` (and other shapes) on
Android, and drops about ~1% of the CPU time while scrolling in this
example.
jonpryor pushed a commit to dotnet/java-interop that referenced this issue May 5, 2023
Context: dotnet/maui#12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a .NET MAUI customer sample while scrolling on a Pixel 5,
I see ~2.2% of the time spent in the
`IOnFocusChangeListenerImplementor` constructor, due to
[subscription to the `View.FocusChange` event][0]:

	(2.2%) mono.android!Android.Views.View.IOnFocusChangeListenerImplementor..ctor()

MAUI subscribes to [`Android.Views.View.FocusChange` event][1] for
every view placed on the screen, which happens while scrolling in
this sample.

Reviewing, the generated code for the
`IOnFocusChangeListenerImplementor` constructor, we see it still uses
outdated `JNIEnv` APIs:

	public IOnFocusChangeListenerImplementor ()
	    : base (global::Android.Runtime.JNIEnv.StartCreateInstance ("mono/android/view/View_OnFocusChangeListenerImplementor", "()V"), JniHandleOwnership.TransferLocalRef)
	{
	    global::Android.Runtime.JNIEnv.FinishCreateInstance (((global::Java.Lang.Object) this).Handle, "()V");
	}

Which we can change to use the newer/faster Java.Interop APIs:

	public unsafe IOnFocusChangeListenerImplementor ()
	    : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
	{
	    const string __id = "()V";
	    if (((global::Java.Lang.Object) this).Handle != IntPtr.Zero)
	        return;
	    var h = JniPeerMembers.InstanceMethods.StartCreateInstance (__id, ((object) this).GetType (), null);
	    SetHandle (h.Handle, JniHandleOwnership.TransferLocalRef);
	    JniPeerMembers.InstanceMethods.FinishCreateInstance (__id, this, null);
	}

These are better because the equivalent call to `JNIEnv.FindClass()`
is cached, among other things.

After these changes, I now see:

	(0.81%) mono.android!Android.Views.View.IOnFocusChangeListenerImplementor..ctor()

This should improve the performance of all C# events that wrap Java
listeners, including and all .NET MAUI views on Android.

Additionally, stop emitting the `[Register]` attribute for
`*Implementor` classes:

	[global::Android.Runtime.Register ("mono/android/view/View_OnFocusChangeListenerImplementor")]
	partial class IOnFocusChangeListenerImplementor {/* … */}

The `[Register]` attribute is not needed, because `*Implementor`
classes are generated internal implementation details.

[0]: https://github.com/dotnet/maui/blob/2041476e78452891029f4d2bd806c45be42f4878/src/Core/src/Handlers/View/ViewHandler.Android.cs#L14
[1]: https://learn.microsoft.com/en-us/dotnet/api/android.views.view.focuschange?view=xamarin-android-sdk-13
jonathanpeppers added a commit that referenced this issue May 8, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a .NET MAUI customer sample while scrolling on a Pixel 5, I
see some interesting time being spent in:

    (0.76%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.54%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This sample has a `<Border/>` inside a `<CollectionView/>` and so you
can see this work happening while scrolling.

Specifically, I found a couple places we had code like:

    _borderPaint.StrokeWidth = _strokeThickness;
    _borderPaint.StrokeJoin = _strokeLineJoin;
    _borderPaint.StrokeCap = _strokeLineCap;
    _borderPaint.StrokeMiter = _strokeMiterLimit * 2;
    if (_borderPathEffect != null)
        _borderPaint.SetPathEffect(_borderPathEffect);

This calls from C# to Java 5 times. Creating a new method in
`PlatformInterop.java` allowed me to reduce it to 1.

I also found:

    void SetDefaultBackgroundColor()
    {
        using (var background = new TypedValue())
        {
            if (_context == null || _context.Theme == null || _context.Resources == null)
                return;

            if (_context.Theme.ResolveAttribute(global::Android.Resource.Attribute.WindowBackground, background, true))
            {
                var resource = _context.Resources.GetResourceTypeName(background.ResourceId);
                var type = resource?.ToLowerInvariant();

                if (type == "color")
                {
                    var color = new AColor(ContextCompat.GetColor(_context, background.ResourceId));
                    _backgroundColor = color;
                }
            }
        }
    }

This is doing a lot of unnecessary stuff: looking up a resource by
name, etc. I found a very simple Java example we could put in
`PlatformInterop.java`:

https://stackoverflow.com/a/14468034

After these changes, I now see:

    (0.28%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.04%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This improves the performance of any `<Border/>` (and other shapes) on
Android, and drops about ~1% of the CPU time while scrolling in this
example.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 8, 2023
Context: dotnet#12130
Context: https://github.com/angelru/CvSlowJittering
Context: https://github.com/jonathanpeppers/lols

Testing a customer sample and my LOLs per second sample, I could see a
lot of time (5.1%) spent in `PrepareForTextViewArrange()`:

    1.01s (5.1%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui
    635.99ms (3.2%) mono.android!Android.Views.View.get_Context()

Most of the time is spent just calling `View.Context` to be able to do:

    internal static int MakeMeasureSpecExact(this Context context, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)context!.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

In eea91d3, I added an overload for `ToPixels()` that allows you to
get the same value with an `Android.Views.View` -- avoiding the need
to call `View.Context`.

So we can instead do:

    internal static int MakeMeasureSpecExact(this PlatformView view, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)view.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

After these changes, it seems to be ~2.7% better:

    420.68ms (2.4%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui

The `View.Context` call is now completely gone.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue May 8, 2023
Context: dotnet#12130
Context: https://github.com/angelru/CvSlowJittering
Context: https://github.com/jonathanpeppers/lols

Testing a customer sample and my LOLs per second sample, I could see a
lot of time (5.1%) spent in `PrepareForTextViewArrange()`:

    1.01s (5.1%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui
    635.99ms (3.2%) mono.android!Android.Views.View.get_Context()

Most of the time is spent just calling `View.Context` to be able to do:

    internal static int MakeMeasureSpecExact(this Context context, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)context!.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

In eea91d3, I added an overload for `ToPixels()` that allows you to
get the same value with an `Android.Views.View` -- avoiding the need
to call `View.Context`.

So we can instead do:

    internal static int MakeMeasureSpecExact(this PlatformView view, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)view.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

After these changes, it seems to be ~2.7% better:

    420.68ms (2.4%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui

The `View.Context` call is now completely gone.
jonathanpeppers added a commit that referenced this issue May 8, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering
Context: https://github.com/jonathanpeppers/lols

Testing a customer sample and my LOLs per second sample, I could see a
lot of time (5.1%) spent in `PrepareForTextViewArrange()`:

    1.01s (5.1%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui
    635.99ms (3.2%) mono.android!Android.Views.View.get_Context()

Most of the time is spent just calling `View.Context` to be able to do:

    internal static int MakeMeasureSpecExact(this Context context, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)context!.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

In eea91d3, I added an overload for `ToPixels()` that allows you to
get the same value with an `Android.Views.View` -- avoiding the need
to call `View.Context`.

So we can instead do:

    internal static int MakeMeasureSpecExact(this PlatformView view, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)view.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

After these changes, it seems to be ~2.7% better:

    420.68ms (2.4%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui

The `View.Context` call is now completely gone.
@jonathanpeppers
Copy link
Member

jonathanpeppers commented May 9, 2023

I believe we have enough changes to close this one, testing the above sample with my script here:

https://github.com/jonathanpeppers/maui-scrolling-performance

.NET 7:

05-09 09:11:49.830 16727 16727 I DOTNET  : Frame(s) that took ~5ms, count: 5
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~6ms, count: 24
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~7ms, count: 5
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~8ms, count: 37
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~9ms, count: 29
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~10ms, count: 2
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~11ms, count: 2
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~12ms, count: 3
05-09 09:11:49.831 16727 16727 I DOTNET  : Frame(s) that took ~14ms, count: 1
05-09 09:11:49.832 16727 16727 I DOTNET  : Frame(s) that took ~19ms, count: 2
05-09 09:11:49.832 16727 16727 I DOTNET  : Frame(s) that took ~21ms, count: 2
05-09 09:11:49.832 16727 16727 I DOTNET  : Average frame time: 8.33ms
05-09 09:11:49.832 16727 16727 I DOTNET  : No. of slow frames: 4

.NET 8 net8.0 branch + main on top:

05-09 09:15:10.010 17241 17241 I DOTNET  : Frame(s) that took ~4ms, count: 1
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~5ms, count: 8
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~6ms, count: 31
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~7ms, count: 3
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~8ms, count: 12
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~9ms, count: 2
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~10ms, count: 3
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~11ms, count: 3
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~12ms, count: 1
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~15ms, count: 2
05-09 09:15:10.011 17241 17241 I DOTNET  : Frame(s) that took ~18ms, count: 1
05-09 09:15:10.011 17241 17241 I DOTNET  : Average frame time: 7.28ms
05-09 09:15:10.011 17241 17241 I DOTNET  : No. of slow frames: 1

Scrolling feels a lot better. I tested these on a Pixel 5 (released in Oct 2020).

The list of performance changes, I think led to the improvement here:

The recent fixes for memory leaks, likely also helped.

We can always make this even better, but I think this is enough to close this one for now. Thanks!

What do I do if my app still scrolls slowly?

First, try it on the latest .NET 8 release and see if things have improved.

If not, file a new issue with a sample app, and we can profile the app and see what we find. You can also try the instructions at https://aka.ms/profile-maui to profile it yourself.

@samhouts samhouts added the s/duplicate 2️⃣ This issue or pull request already exists label May 12, 2023
maxkatz6 pushed a commit to AvaloniaUI/Avalonia.Essentials that referenced this issue May 15, 2023
Context: dotnet/maui#12130
Context: https://github.com/angelru/CvSlowJittering

When profiling the above customer app while scrolling, 4% of the time
is spent doing dictionary lookups:

    (4.0%) System.Private.CoreLib!System.Collections.Generic.Dictionary<TKey_REF,TValue_REF>.FindValue(TKey_REF)

Observing the call stack, some of these are coming from culture-aware
string lookups in MAUI:

* `microsoft.maui!Microsoft.Maui.PropertyMapper.GetProperty(string)`
* `microsoft.maui!Microsoft.Maui.WeakEventManager.AddEventHandler(System.EventHandler`1<TEventArgs_REF>,string)`
* `microsoft.maui!Microsoft.Maui.CommandMapper.GetCommand(string)`

Which show up as a mixture of `string` comparers:

    (0.98%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.71%) System.Private.CoreLib!System.String.GetNonRandomizedHashCode()
    (0.31%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri
    (0.01%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.GetStringComparer(object)

In cases of `Dictionary<string, TValue>` or `HashSet<string>`, we can
use `StringComparer.Ordinal` for faster dictionary lookups.

Unfortunately, there is no code analyzer for this:

dotnet/runtime#52399

So, I manually went through the codebase and found all the places.

I now only see the *fast* string comparers in this sample:

    (1.3%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.GetHashCode(string)
    (0.35%) System.Private.CoreLib!System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer.Equals(string,stri

Which is about ~0.36% better than before.

This should slightly improve the performance of handlers & all
controls on all platforms.

I also fixed `Microsoft.Maui.Graphics.Text.TextColors` to use
`StringComparer.OrdinalIgnoreCase` -- and removed a
`ToUpperInvariant()` call.
rmarinho pushed a commit that referenced this issue May 30, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a .NET MAUI customer sample while scrolling on a Pixel 5, I
see some interesting time being spent in:

    (0.76%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.54%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This sample has a `<Border/>` inside a `<CollectionView/>` and so you
can see this work happening while scrolling.

Specifically, I found a couple places we had code like:

    _borderPaint.StrokeWidth = _strokeThickness;
    _borderPaint.StrokeJoin = _strokeLineJoin;
    _borderPaint.StrokeCap = _strokeLineCap;
    _borderPaint.StrokeMiter = _strokeMiterLimit * 2;
    if (_borderPathEffect != null)
        _borderPaint.SetPathEffect(_borderPathEffect);

This calls from C# to Java 5 times. Creating a new method in
`PlatformInterop.java` allowed me to reduce it to 1.

I also found:

    void SetDefaultBackgroundColor()
    {
        using (var background = new TypedValue())
        {
            if (_context == null || _context.Theme == null || _context.Resources == null)
                return;

            if (_context.Theme.ResolveAttribute(global::Android.Resource.Attribute.WindowBackground, background, true))
            {
                var resource = _context.Resources.GetResourceTypeName(background.ResourceId);
                var type = resource?.ToLowerInvariant();

                if (type == "color")
                {
                    var color = new AColor(ContextCompat.GetColor(_context, background.ResourceId));
                    _backgroundColor = color;
                }
            }
        }
    }

This is doing a lot of unnecessary stuff: looking up a resource by
name, etc. I found a very simple Java example we could put in
`PlatformInterop.java`:

https://stackoverflow.com/a/14468034

After these changes, I now see:

    (0.28%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.OnDraw(Android.Graphics.Drawables.Shapes.Shape,Android.Graphics.Canv
    (0.04%) microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()

This improves the performance of any `<Border/>` (and other shapes) on
Android, and drops about ~1% of the CPU time while scrolling in this
example.
rmarinho pushed a commit that referenced this issue May 30, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering
Context: https://github.com/jonathanpeppers/lols

Testing a customer sample and my LOLs per second sample, I could see a
lot of time (5.1%) spent in `PrepareForTextViewArrange()`:

    1.01s (5.1%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui
    635.99ms (3.2%) mono.android!Android.Views.View.get_Context()

Most of the time is spent just calling `View.Context` to be able to do:

    internal static int MakeMeasureSpecExact(this Context context, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)context!.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

In eea91d3, I added an overload for `ToPixels()` that allows you to
get the same value with an `Android.Views.View` -- avoiding the need
to call `View.Context`.

So we can instead do:

    internal static int MakeMeasureSpecExact(this PlatformView view, double size)
    {
        // Convert to a native size to create the spec for measuring
        var deviceSize = (int)view.ToPixels(size);
        return MeasureSpecMode.Exactly.MakeMeasureSpec(deviceSize);
    }

After these changes, it seems to be ~2.7% better:

    420.68ms (2.4%) microsoft.maui!Microsoft.Maui.ViewHandlerExtensions.PrepareForTextViewArrange(Microsoft.Maui.IViewHandler,Microsoft.Maui

The `View.Context` call is now completely gone.
github-actions bot pushed a commit that referenced this issue Jun 1, 2023
Context: #12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a customer sample app, I noticed a lot of time spent in
`{AppThemeBinding}` and `WeakEventManager` while scrolling:

    2.08s (17%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.Apply(object,Microsoft.Maui.Controls.BindableObject,Micr...
    2.05s (16%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.AttachEvents()
    2.04s (16%) microsoft.maui!Microsoft.Maui.WeakEventManager.RemoveEventHandler(System.EventHandler`1<TEventArgs_REF>,string)

16% is a *lot* to notice while scrolling. Sometimes I've made
improvements where I only shaved off 3% of the total time.

What is going on here is:

* Default `maui` template has lots of `{AppThemeBinding}` in the
  default `Styles.xaml`. This supports Light vs Dark theming.

* `{AppThemeBinding}` subscribes to `Application.RequestedThemeChanged`

* Making every MAUI view subscribe to this event -- potentially
  multiple times.

* Subscribers are a `Dictionary<string, List<Subscriber>>`, where
  there is a dictionary lookup followed by a O(N) search for
  unsubscribe operations.

I spent a little time investigating if we can make a faster
`WeakEventManager`, in general:

dotnet/runtime#61517

I did not immediately see a way to make "weak events" fast, but I did
see a way to make this scenario fast.

Before:
* For any `{AppThemeBinding}`, it calls both:
    * `RequestedThemeChanged -= OnRequestedThemeChanged` O(N) time
    * `RequestedThemeChanged += OnRequestedThemeChanged` constant time
* Where the `-=` is notably slower, due to possibly 100s of subscribers.

After:
* Create an `_attached` boolean, so we know know the "state" if it is
  attached or not.
* New bindings only call `+=`, where `-=` will now only be called by
  `{AppThemeBinding}` in *rare* cases.
* Most .NET MAUI apps do not "unapply" bindings, but `-=` would only
  be used in that case.

After this change, the following method disappeared from `dotnet-trace`
output completely:

    2.05s (16%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.AttachEvents()

Meaning that `AppThemeBinding.AttachEvents()` is now so fast that 0%
(basically no time) is spent inside this method.

I also could notice a difference in general startup time of the sample
app. An average of 10 runs on a Pixel 5:

    Before:
    Average(ms): 967.7
    Std Err(ms): 4.62132737064436
    Std Dev(ms): 14.6139203045133
    After:
    Average(ms): 958.9
    Std Err(ms): 3.22645316098034
    Std Dev(ms): 10.2029407525478

So I could notice a ~10ms improvement to startup in this app, and
scrolling seemed a bit better as well.

Note that I don't think this completely solves #12130, as things still
seem sluggish to me when scrolling. But it is a reasonable improvement
to start with that benefits all .NET MAUI apps on all platforms.
rmarinho pushed a commit that referenced this issue Jun 1, 2023
* [controls] fix performance issue in {AppThemeBinding}

Context: #12130
Context: https://github.com/angelru/CvSlowJittering

Profiling a customer sample app, I noticed a lot of time spent in
`{AppThemeBinding}` and `WeakEventManager` while scrolling:

    2.08s (17%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.Apply(object,Microsoft.Maui.Controls.BindableObject,Micr...
    2.05s (16%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.AttachEvents()
    2.04s (16%) microsoft.maui!Microsoft.Maui.WeakEventManager.RemoveEventHandler(System.EventHandler`1<TEventArgs_REF>,string)

16% is a *lot* to notice while scrolling. Sometimes I've made
improvements where I only shaved off 3% of the total time.

What is going on here is:

* Default `maui` template has lots of `{AppThemeBinding}` in the
  default `Styles.xaml`. This supports Light vs Dark theming.

* `{AppThemeBinding}` subscribes to `Application.RequestedThemeChanged`

* Making every MAUI view subscribe to this event -- potentially
  multiple times.

* Subscribers are a `Dictionary<string, List<Subscriber>>`, where
  there is a dictionary lookup followed by a O(N) search for
  unsubscribe operations.

I spent a little time investigating if we can make a faster
`WeakEventManager`, in general:

dotnet/runtime#61517

I did not immediately see a way to make "weak events" fast, but I did
see a way to make this scenario fast.

Before:
* For any `{AppThemeBinding}`, it calls both:
    * `RequestedThemeChanged -= OnRequestedThemeChanged` O(N) time
    * `RequestedThemeChanged += OnRequestedThemeChanged` constant time
* Where the `-=` is notably slower, due to possibly 100s of subscribers.

After:
* Create an `_attached` boolean, so we know know the "state" if it is
  attached or not.
* New bindings only call `+=`, where `-=` will now only be called by
  `{AppThemeBinding}` in *rare* cases.
* Most .NET MAUI apps do not "unapply" bindings, but `-=` would only
  be used in that case.

After this change, the following method disappeared from `dotnet-trace`
output completely:

    2.05s (16%) microsoft.maui.controls!Microsoft.Maui.Controls.AppThemeBinding.AttachEvents()

Meaning that `AppThemeBinding.AttachEvents()` is now so fast that 0%
(basically no time) is spent inside this method.

I also could notice a difference in general startup time of the sample
app. An average of 10 runs on a Pixel 5:

    Before:
    Average(ms): 967.7
    Std Err(ms): 4.62132737064436
    Std Dev(ms): 14.6139203045133
    After:
    Average(ms): 958.9
    Std Err(ms): 3.22645316098034
    Std Dev(ms): 10.2029407525478

So I could notice a ~10ms improvement to startup in this app, and
scrolling seemed a bit better as well.

Note that I don't think this completely solves #12130, as things still
seem sluggish to me when scrolling. But it is a reasonable improvement
to start with that benefits all .NET MAUI apps on all platforms.

* PR feedback

---------

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) 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-controls-collectionview CollectionView, CarouselView, IndicatorView p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/android 🤖 s/duplicate 2️⃣ This issue or pull request already exists t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

No branches or pull requests