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.

Sign up for GitHub

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

Jump to bottom

[C] use ResourcesChanged to propagate Theme #21229

Merged

StephaneDelcroix merged 3 commits into main from appthemebindingwithoutsubscription

Mar 20, 2024

Contributor

StephaneDelcroix commented Mar 15, 2024 •

edited by samhouts

Loading

Description of Change

instead of subscribing to ThemeChanged event, use the ResourcesChanged mechanism of DynamicResource to propagate the change

Issues Fixed

related to Rework AppThemeBinding to use IApplication and/or Window.Parent #8713
alternative to Propagate the app theme to all children #19931
fixes [Android] CollectionVIew scrolling performance on MAUI lags compared to XF #18505
fixes choppy scrolling with CollectionView #21019

StephaneDelcroix requested a review from a team as a code owner

March 15, 2024 09:17

StephaneDelcroix requested review from mattleibow and jsuarezruiz

March 15, 2024 09:18

Contributor

MartyIX commented Mar 15, 2024

Is there a benchmark or some observation how much faster this is, please? We have an app where there are may AppThemeBindings, so I'm very curious.

StephaneDelcroix force-pushed the appthemebindingwithoutsubscription branch from 02e7fa3 to 340e53e Compare

March 15, 2024 10:02

StephaneDelcroix commented

View reviewed changes

src/Controls/src/Core/Application/Application.cs

		@@ -101,6 +101,15 @@ public IAppLinks AppLinks

		_singleWindowMainPage = value;

		if (value is not null)

Contributor Author

StephaneDelcroix Mar 15, 2024

this fixes another bug. resources changed at application level weren't propagated to MainPage

Member

mattleibow Mar 15, 2024

I think if you are just using MainPage, then you get parented to a window during the creating of the UI - so once the app is ready, it calls CreateWindow and assigns the MainPage to the window.

The _singleWindowMainPage is just a placeholder while the app is still being constructed. Changing MainPage later literally just replaces the content of the first window:

Windows[0].Page = value;

What you have observed might just be because MainPage really is a delayed construction and assigned. Now this is to say it may be fine what you are doing here, but will it result in a double listener registration once the window is created and the MainPage parented to the window?

Contributor Author

StephaneDelcroix Mar 18, 2024

maybe, but that doesn't work in unit tests

StephaneDelcroix commented

View reviewed changes

src/Controls/src/Core/Shell/Shell.cs

               				return;
               			if (args.NewHandler == null)
+              #pragma warning disable CS0618 // Type or member is obsolete

Contributor Author

StephaneDelcroix Mar 15, 2024

well, it's not anymore in this version of the PR, but maybe it should

StephaneDelcroix commented

View reviewed changes

src/Controls/tests/Core.UnitTests/AppThemeTests.cs

@@ @@ -34,6 +34,7 @@ public void ThemeChangeUsingSetAppThemeColor() @@
               			{
               				Text = "Green on Light, Red on Dark"
               			};
+              			app.LoadPage(new ContentPage {Content = label});

Contributor Author

StephaneDelcroix Mar 15, 2024 •

edited

Loading

apptheme propagation REQUIRES that the control is parented, which is fine, as otherwise it wouldn't be on screen

jsuarezruiz added the area-xaml label

jsuarezruiz reviewed

View reviewed changes

Microsoft.Maui-dev.sln

               EndProject
               Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "UITest.NUnit", "src\TestUtils\src\UITest.NUnit\UITest.NUnit.csproj", "{A307B624-48D4-494E-A70D-5B3CDF6620CF}"
               EndProject
-              Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Controls.SourceGen.UnitTests", "src\Controls\tests\SourceGen.UnitTests\Conrtrols.SourceGen.UnitTests\Controls.SourceGen.UnitTests.csproj", "{06747B55-91DB-47F5-B7A2-56526C28A0D3}"

Contributor

jsuarezruiz Mar 15, 2024

😮

jsuarezruiz reviewed

View reviewed changes

src/Controls/src/Core/AppThemeBinding.cs Outdated

               		WeakReference<BindableObject> _weakTarget;
               		BindableProperty _targetProperty;
-              		bool _attached;
+              		// bool _attached;

Contributor

jsuarezruiz Mar 15, 2024

It is not used anywhere, I think we can eliminate this variable.

jsuarezruiz reviewed

View reviewed changes

src/Controls/src/Core/AppThemeBinding.cs

+              			public void OnAppThemeChanged()
+              			{
+              				Binding.Apply(false);

Contributor

jsuarezruiz Mar 15, 2024

I'm curious about the performance impact, especially on CollectionView scrolling. Could we perhaps have an UITest with this example #18505 where we can display the minimum fps in a Label by scrolling and verifying that the value is greater than n (where n is value higher than before)?

Member

mattleibow Mar 15, 2024 •

edited

Loading

We may need @jonathanpeppers to run this PR in a release app with profiling. Our UI tests have a lot of overhead and appium things running.

mattleibow reviewed

View reviewed changes

src/Controls/src/Core/AppThemeBinding.cs Outdated

+              			{
+              				_parent = parent;
+              				Binding = binding;
+              				this.SetDynamicResource(AppThemeProperty, "__ApplicationTheme__");

Member

mattleibow Mar 15, 2024

Maybe this string should be in some sort of const place and maybe also have a __MAUI__ prefix.

Contributor Author

StephaneDelcroix Mar 18, 2024 •

edited

Loading

__MAUI__MatthewIsAwesome__ApplicationTheme__ ?

mattleibow reviewed

View reviewed changes

src/Controls/src/Core/AppThemeBinding.cs

@@ @@ -8,10 +8,38 @@ namespace Microsoft.Maui.Controls @@
               {
               	class AppThemeBinding : BindingBase
               	{
+              		class AppThemeProxy : Element
+              		{
+              			public AppThemeProxy(Element parent, AppThemeBinding binding)

Member

mattleibow Mar 15, 2024

I see the AppThemeBinding uses a weak reference for the target: WeakReference<BindableObject> _weakTarget; Is this now creating a string reference that might cause issues? Maybe the AppThemeBinding does not need to be weak?

Contributor Author

StephaneDelcroix Mar 18, 2024

the proxy might prevent the AppThemeBinding to GC, but the AppThemeBinding doesn't hold a strong ref to the BO.
in this case, I trust the GC to do the job

Contributor Author

StephaneDelcroix Mar 28, 2024

looks like I was wrong

mattleibow reviewed

View reviewed changes

src/Controls/src/Core/AppThemeBinding.cs

+              				((IElementDefinition)parent).AddResourcesChangedListener(OnParentResourcesChanged);
+              			}
+              			public static BindableProperty AppThemeProperty = BindableProperty.Create("AppTheme", typeof(AppTheme), typeof(AppThemeBinding), AppTheme.Unspecified,

Member

mattleibow Mar 15, 2024

Will it be more expensive to have a separate proxy Element object with a single extra property vs just adding this to maybe the base Element?

Contributor Author

StephaneDelcroix Mar 18, 2024

we can't add that to the base Element, as an element might need have multiple AppThemebindings, and then multiples proxies. moving that to the element will require some sort of event subscription, and that's what we're avoiding here

mattleibow reviewed

View reviewed changes

Member

mattleibow left a comment

This is looking better than mine with all my extra things, and this looks cleaner to. I tried to cover all the cases of all the things, but just making it work when the theme changes AND the element is parented is better. I tried to make it always work even when there is a floating element.

I also just dumped a bunch of thoughts and questions on this. More trying to see what you were thinking and if there is any gotchas.

PureWeen requested a review from jonathanpeppers

March 15, 2024 15:10

jonathanpeppers reviewed

View reviewed changes

Member

jonathanpeppers left a comment

This seems to significantly improve {AppThemeBinding}. I tested this app on a Pixel 5:

https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip

AppThemeBinding.zip

Before I was seeing:

With calling method:

(Used git revert -n 340e53e6)

Then tried the change in this PR:

Just compare the %, as the traces have different durations.

Scrolling the sample app, it "feels" better too.


          [C] use ResourcesChanged to propagate Theme

2982b41

instead of subscribing to ThemeChanged event, use the ResourcesChanged
mechanism of DynamicResource to propagate the change

- related to #8713
- alternative to #19931
- fixes #18505

StephaneDelcroix force-pushed the appthemebindingwithoutsubscription branch from 340e53e to 2982b41 Compare

March 18, 2024 08:51

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request


          [android] avoid OnLayout() for Label

f5d4177

Context: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: dotnet#21229 (review)

In profiling scrolling of an app with a `<CollectionView/>` and 12
`<Label/>`s, we see time spent in:

    1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback *at all*
if `Label.TextType` is not `TextType.Text`. The bulk of all `Label`'s
can avoid this call?

To do this:

* Write a new `PlatformAppCompatTextView.java` that override `onLayout()`

* It only calls `onLayoutFormatted()` if a `isFormatted` `boolean`
  field is `true`

* We can set `isFormatted` when `Label.TextType` changes to something
  other than `TextType.Text`.

With this change in place, the above `MauiTextView.OnLayout()` call is
completely gone from `dotnet-trace` output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all `Label`s on Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.

jonathanpeppers mentioned this pull request

[android] avoid OnLayout() for Label #21291

Merged


          fix test

4dd87d7

rmarinho requested review from mattleibow, jsuarezruiz and jonathanpeppers

March 19, 2024 10:29

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request


          [android] avoid OnLayout() for Label

e6216a5

Context: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: dotnet#21229 (review)

In profiling scrolling of an app with a `<CollectionView/>` and 12
`<Label/>`s, we see time spent in:

    1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback *at all*
if `Label.TextType` is not `TextType.Text`. The bulk of all `Label`'s
can avoid this call?

To do this:

* Write a new `PlatformAppCompatTextView.java` that override `onLayout()`

* It only calls `onLayoutFormatted()` if a `isFormatted` `boolean`
  field is `true`

* We can set `isFormatted` when `Label.TextType` changes to something
  other than `TextType.Text`.

With this change in place, the above `MauiTextView.OnLayout()` call is
completely gone from `dotnet-trace` output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all `Label`s on Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.


          use a const string

88e554e

jonathanpeppers approved these changes

View reviewed changes

StephaneDelcroix enabled auto-merge (squash)

March 20, 2024 13:34

jsuarezruiz approved these changes

View reviewed changes

StephaneDelcroix merged commit e0b85ca into main

47 checks passed

StephaneDelcroix deleted the appthemebindingwithoutsubscription branch

March 20, 2024 14:06

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request


          [controls] improve perf of "merged" ResourceDictionary lookups

c6a793e

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

I profiled the above sample with `dotnet-trace` with the following PRs
applied locally:

* dotnet#21229
* dotnet#21291

While scrolling, a lot of time is spent in `ResourceDictionary`
lookups on an Android Pixel 5 device:

    2.0% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

Drilling in, I can see System.Linq's `Reverse()` method:

    0.56% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.MoveNext()
    0.14% System.Linq!System.Linq.Enumerable.Reverse(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>..ctor(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.Dispose()

`Reverse()` can be problematic as it can sometimes create a copy of
the entire collection, in order to sort in reverse. We can juse use a
reverse `for`-loop instead.

The indexer, we can also avoid a double-lookup:

    if (dict.ContainsKey(index))
        return dict[index];

And instead do:

    if (dict.TryGetValue(index, out var value))
        return value;

The MAUI project template seems to setup a few "merged"
`ResourceDictionary` as it contains `Styles.xaml`, so this is why this
code path is being hit.

I wrote a BenchmarkDotNet benchmark, and it indicates the collection
is being copied, as the 872 bytes of allocation occur:

| Method      | key         | Mean      | Error    | StdDev   | Gen0   | Allocated |
|------------ |------------ |----------:|---------:|---------:|-------:|----------:|
| TryGetValue | key0        |  11.45 ns | 0.026 ns | 0.023 ns |      - |         - |
| Indexer     | key0        |  24.72 ns | 0.133 ns | 0.118 ns |      - |         - |
| TryGetValue | merged99,99 | 117.06 ns | 2.334 ns | 2.497 ns | 0.1042 |     872 B |
| Indexer     | merged99,99 | 145.60 ns | 2.737 ns | 2.286 ns | 0.1042 |     872 B |

With these changes in place, I see less time spent inside:

    0.91% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

The benchmark no longer allocates either:

| Method      | key         | Mean      | Error     | StdDev    | Allocated |
|------------ |------------ |----------:|----------:|----------:|----------:|
| TryGetValue | key0        |  11.92 ns |  0.094 ns |  0.084 ns |         - |
| Indexer     | merged99,99 |  23.12 ns |  0.418 ns |  0.391 ns |         - |
| Indexer     | key0        |  24.20 ns |  0.485 ns |  0.453 ns |         - |
| TryGetValue | merged99,99 |  29.09 ns |  0.296 ns |  0.262 ns |         - |

This should improve the performance "parenting" of any MAUI view on
all platforms -- as well as scrolling `CollectionView`.

jonathanpeppers mentioned this pull request

[controls] improve perf of "merged" ResourceDictionary lookups #21334

Merged

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request


          [android] avoid OnLayout() for Label

c8aebdc

Context: dotnet#18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: dotnet#21229 (review)

In profiling scrolling of an app with a `<CollectionView/>` and 12
`<Label/>`s, we see time spent in:

    1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback *at all*
if `Label.FormattedText` is not `null`. The bulk of all `Label`'s can
avoid this call?

To do this:

* Write a new `PlatformAppCompatTextView.java` that override `onLayout()`

* It only calls `onLayoutFormatted()` if a `isFormatted` `boolean`
  field is `true`

* We can set `isFormatted` if a formatted string is such as:
  `isFormatted = !(text instanceof String)`

With this change in place, the above `MauiTextView.OnLayout()` call is
completely gone from `dotnet-trace` output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all non-formatted `Label`s on
Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.

DDHSchmidt mentioned this pull request

[BUG] StatusBarBehavior.SetAppThemeColor(...) throws NullReferenceExceptions on MAUI Nightly builds CommunityToolkit/Maui#1776

Closed

2 tasks

jonathanpeppers added a commit that referenced this pull request


          [android] avoid OnLayout() for Label (#21291)

83960b0

Context: #18505
Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip
Context: #21229 (review)

In profiling scrolling of an app with a `<CollectionView/>` and 12
`<Label/>`s, we see time spent in:

    1.9% Microsoft.Maui!Microsoft.Maui.Platform.MauiTextView.OnLayout(bool,int,int,int,int)

This is a callback from Java to C#, which has a performance cost.
Reviewing the code, we would only need to make this callback *at all*
if `Label.FormattedText` is not `null`. The bulk of all `Label`'s can
avoid this call?

To do this:

* Write a new `PlatformAppCompatTextView.java` that override `onLayout()`

* It only calls `onLayoutFormatted()` if a `isFormatted` `boolean`
  field is `true`

* We can set `isFormatted` if a formatted string is such as:
  `isFormatted = !(text instanceof String)`

With this change in place, the above `MauiTextView.OnLayout()` call is
completely gone from `dotnet-trace` output. Scrolling the sample also
"feels" a bit snappier.

This should improve the performance of all non-formatted `Label`s on
Android.

This is the mininum amount of API changes possible -- which seems like
what we should go for if we ship this change in .NET 8 servicing.

StephaneDelcroix pushed a commit that referenced this pull request


          [controls] improve perf of "merged" ResourceDictionary lookups (#21334)

8ca7648

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

I profiled the above sample with `dotnet-trace` with the following PRs
applied locally:

* #21229
* #21291

While scrolling, a lot of time is spent in `ResourceDictionary`
lookups on an Android Pixel 5 device:

    2.0% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

Drilling in, I can see System.Linq's `Reverse()` method:

    0.56% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.MoveNext()
    0.14% System.Linq!System.Linq.Enumerable.Reverse(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>..ctor(System.Collections.Generic.IEnumerable`1<TSource_REF>)
    0.04% System.Linq!System.Linq.Enumerable.ReverseIterator<TSource_REF>.Dispose()

`Reverse()` can be problematic as it can sometimes create a copy of
the entire collection, in order to sort in reverse. We can juse use a
reverse `for`-loop instead.

The indexer, we can also avoid a double-lookup:

    if (dict.ContainsKey(index))
        return dict[index];

And instead do:

    if (dict.TryGetValue(index, out var value))
        return value;

The MAUI project template seems to setup a few "merged"
`ResourceDictionary` as it contains `Styles.xaml`, so this is why this
code path is being hit.

I wrote a BenchmarkDotNet benchmark, and it indicates the collection
is being copied, as the 872 bytes of allocation occur:

| Method      | key         | Mean      | Error    | StdDev   | Gen0   | Allocated |
|------------ |------------ |----------:|---------:|---------:|-------:|----------:|
| TryGetValue | key0        |  11.45 ns | 0.026 ns | 0.023 ns |      - |         - |
| Indexer     | key0        |  24.72 ns | 0.133 ns | 0.118 ns |      - |         - |
| TryGetValue | merged99,99 | 117.06 ns | 2.334 ns | 2.497 ns | 0.1042 |     872 B |
| Indexer     | merged99,99 | 145.60 ns | 2.737 ns | 2.286 ns | 0.1042 |     872 B |

With these changes in place, I see less time spent inside:

    0.91% Microsoft.Maui.Controls!Microsoft.Maui.Controls.ResourceDictionary.TryGetValue(string,object&)

The benchmark no longer allocates either:

| Method      | key         | Mean      | Error     | StdDev    | Allocated |
|------------ |------------ |----------:|----------:|----------:|----------:|
| TryGetValue | key0        |  11.92 ns |  0.094 ns |  0.084 ns |         - |
| Indexer     | merged99,99 |  23.12 ns |  0.418 ns |  0.391 ns |         - |
| Indexer     | key0        |  24.20 ns |  0.485 ns |  0.453 ns |         - |
| TryGetValue | merged99,99 |  29.09 ns |  0.296 ns |  0.262 ns |         - |

This should improve the performance "parenting" of any MAUI view on
all platforms -- as well as scrolling `CollectionView`.

PureWeen mentioned this pull request

Bugzilla44166 now failing after ThemeResourcesChange fix #21495

Closed

eth-ellis mentioned this pull request

[Android] CollectionViewwith multiple bindable layouts (CardWithComplexContent) performance issues #21580

Open

github-actions bot locked and limited conversation to collaborators

samhouts added fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457 labels

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-xaml fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457