Propagate the app theme to all children by mattleibow · Pull Request #19931 · dotnet/maui

mattleibow · 2024-01-16T20:36:26Z

Description of Change

Subscribing to Application.RequestedThemeChanged is fairly expensive because it uses a weak event. This PR tries an alternative by propagating the value to all children.

Issues Fixed

Related to Rework AppThemeBinding to use IApplication and/or Window.Parent #8713
Fixes [Android] CollectionVIew scrolling performance on MAUI lags compared to XF #18505

jonathanpeppers

This generally looks good, my Before:

vs After:

(NOTE just look at the % as the trace duration is not the same)

Traces taken while scrolling a sample on #18505: matthew.zip

jonathanpeppers · 2024-01-17T17:08:39Z

Is something broken, though? A lot of the lanes are red.

mattleibow · 2024-01-17T17:50:19Z

Is this really like the top went from 18% to 1.1%? like almost a 17% improvement???

mattleibow · 2024-01-17T17:50:41Z

Is something broken, though? A lot of the lanes are red.

Yeah, I have broken some tests I think because of someting, checking now.

mattleibow · 2024-01-17T18:08:59Z

This is crazy! Before, it was 45% just inflating (and all the bindings) items:

Inflation is still the top item, but now is only 8.6%!

mattleibow · 2024-01-17T21:50:01Z

 		void SetAttached(bool value)
 		{
-			var app = Application.Current;
-			if (app != null && _attached != value)
+			if (_attached == value)
+				return;
+
+			_attached = value;
+
+			if (_weakTarget?.TryGetTarget(out var target) == true && target is VisualElement ve)


This method does have a significant change hidden in here. Previously, all AppThemeBinding instances would be subscribed to the app. This could be hundreds of items in complex UIs.

This is the main point of this PR. Since the value propagates from the app and the binding only observes the propagation, if there is a break in the chain - like a button that is not attached to the UI - then the button will never get any theme updates. Not sure how often this happens in reality where a control has to respond to theme updates before being attached to the UI.

This also creates a world in which the initial binding operation asks the Application/OS at the time, so if the theme changes and then a new binding is created (on the same control or a new control) that new binding will have a different theme. I don't like this, but maybe it is not too bad and it is not really significant because as soon as the element is attached to the UI it will be correct.

mattleibow · 2024-01-17T21:51:39Z

+			// If there is no VisualElement (OR no theme set because it is not attached to the UI),
+			// then try the current app. If that fails, just ask the OS for the current theme.
+			if (appTheme == AppTheme.Unspecified)
+				appTheme = Application.Current?.RequestedTheme ?? AppInfo.RequestedTheme;


I am not 100% sure I like the way that an element just starts scanning the universe to find a value. I would like the consistency where if the VisualElement is not attached, then it does not have a theme and thus falls back to the default value. However, this may be some type of breaking change because bindings previously just all hooked into the global app/OS.

mattleibow · 2024-01-17T21:53:45Z

+		AppTheme IRequestedThemeController.RequestedTheme
+		{
+			get => RequestedTheme;
+			set { /* do nothing as this API is not really meant to work this way */ }


This setter is not useful as there is nothing that sets this value from a parent. Applications do not have parents.

mattleibow · 2024-01-17T21:57:41Z

+		[Fact(Skip = "The current implementation actively choses to have different values.")]
+		public void UnattachedVisualElementBindingIsConsistent()


This test is intentionally skipped for now because it demonstrates what I believe to be the best behaviour - see previous comments.

mattleibow · 2024-01-17T21:58:33Z

-			SetAppTheme(AppTheme.Dark);
+			app.LoadPage(new ContentPage { Content = label });


Here is the demonstration of the main change. The theme requires that the element be attached to a window which in turn is attached to an application.

mattleibow · 2024-01-17T22:00:23Z

+		public void ThemeChangeUsingSetAppThemeColorNonVisualElement()
+		{
+			var element = new NonVisualElement
+			{
+				Text = "Green on Light, Red on Dark"
+			};
+
+			element.SetAppThemeColor(NonVisualElement.ColorProperty, Colors.Green, Colors.Red);
+			Assert.Equal(Colors.Green, element.Color);
+
+			EmulatePlatformThemeChange(AppTheme.Dark);
+
+			Assert.Equal(Colors.Red, element.Color);
+		}


Some elements, like Shell and MenuItems - which are not in the main UI hierarchy - do not need a host Window because they are directly connected to the Application. I would like to make all things not need to use the main application as this not only makes things neater but also does not require Application.Current. But, this isextra work for this already too-long PR.

mattleibow · 2024-01-17T22:02:25Z

+		[Fact, Category(TestCategory.Memory)]
+		public async Task VisualElementDoesNotLeak()
+		{
+			(WeakReference Label, WeakReference Binding) CreateReference()
+			{
+				var element = new Label { Text = "Green on Light, Red on Dark" };
+
+				var binding = new AppThemeBinding { Light = Colors.Green, Dark = Colors.Red };
+
+				element.SetBinding(Label.TextColorProperty, binding);
+
+				Assert.True(binding.IsApplied);
+
+				return (new WeakReference(element), new WeakReference(binding));
+			}
+
+			var (element, binding) = CreateReference();
+
+			// GC
+			await TestHelpers.Collect();
+
+			Assert.False(element.IsAlive, "Label should not be alive!");
+			Assert.False(binding.IsAlive, "AppThemeBinding should not be alive!");
+		}


This test specifically test to see if the GC can pick up a VisualElement and AppThemeBinding that are connected. Even though the binding only has a weak reference to the target element, the event that it attaches is not weak and _could_leak. But, I do not think it actually does. @jonathanpeppers is this test valid for testing this type of leak?

For the test above, is there an Application instance that will be around for the lifetime of the test? That seems like the piece that's missing, if not.

mattleibow · 2024-01-17T22:08:55Z

+				// This logic here does also have a strong reference to the target object when
+				// applied, however this does not appear to be a problem in my tests. I also
+				// tested with making the _weakTarget field be a normal reference and still
+				// did not leak.
+
+				if (value)
+					ve.RequestedThemeChanged += OnRequestedThemeChanged;
+				else
+					ve.RequestedThemeChanged -= OnRequestedThemeChanged;


This is not a weak event, even though the target object is a weak reference. My tests pass, so I am not sure if there was a reason that is gone now and we can trust this test, or it was defensive coding. I checked the OG PR from forms and it was always a weak event:

529c8e9#diff-a465dad5f7c3fe7c7ae4a52720c7ca8846cf243930297f34d15036a4397d1ea9R162-R171

and so was the OG implemntation of AppThemeBinding:

6c40121#diff-c0e940f176dec7f36005d68d0754e576b9ba344b21b6be8b4348028acd07f256R7

RequestedThemeChanged uses WeakEventManager, right? That means += won't keep strong references.

I think it should keep using WeakEventManager as random customer code might use that event.

kcrg · 2024-01-23T11:26:26Z

@mattleibow I just tried this build in big app with tons of controls and its works much better than standard 8.0.6. I even can see page transition animations when tapping between bottom tabs xD

PavloLukianets · 2024-01-23T11:46:17Z

@kcrg which < MauiVersion > did u specify? i am currently trying it and can't get a nuget from pr

PavloLukianets · 2024-01-23T12:14:45Z

@mattleibow is the only way for it to work is to copy over manually the nugets to the project?

kcrg · 2024-01-23T12:32:18Z

@kcrg which < MauiVersion > did u specify? i am currently trying it and can't get a nuget from pr

You can download zip with nugets, put them in some folder and just create local nuget repo in Visual Studio that points to that folder with nugets.

<MauiVersion>8.0.6-ci.net8.25004</MauiVersion>

PavloLukianets · 2024-01-23T12:34:10Z

yep, did exactly that, thanks

BaY1251 · 2024-01-30T01:36:44Z

I try #18505 with Microsoft.Maui.Controls-v8.0.100-dev build from dev/propagate-theme. There seems to be no significant improvement in scrolling performance, more needed to do?
nupkg.zip
microsoft.maui.resizetizer.8.0.100-dev.zip

jonathanpeppers

Can you target the net9.0 branch instead?

@mattleibow

Context: dotnet#18505 Context: dotnet#19931 Context: https://github.com/dotnet/maui/files/13251041/MauiCollectionView.zip In the above sample, a lot of time while scrolling a `CollectionView` on Android is spent in `{AppThemeBinding}` and `Application.RequestedThemeChanged`'s underlying `WeakEventManager`. As the top item in a `CollectionView` scrolls offscreen, it is "recycled". This refreshes the `BindingContext` of `{AppThemeBinding}, subscribing and unsubscribing to the `Application.RequestedThemeChanged` event. @mattleibow has a PR that is acceptable for .NET 9, but we are wanting to see what we can do for .NET 8 servicing. Can we make a *faster* `WeakEventManager`? `WeakEventManager` has two performance concerns: 1. It's core data structure is a `Dictionary<string, List<Subscription>>`, requiring string lookups prior to any operations. 2. It uses `System.Reflection.MethodInfo` for invocation. These are completely reasonable, given `WeakEventManager`'s flexibility. It can handle multiple events of different `EventHandler` types. If we restrict ourselves to a single `EventHandler<T>` type, we can: 1. Use a plain `List<T>`. 2. Just call the `EventHandler<T>` directly. No System.Reflection. I tested these changes by parameterizing the existing `WeakEventManagerTests` for both classes and getting them to pass. A benchmark comparing the new `WeakEventHandler<T>` to `WeakEventManager`: | Method | Mean | Error | StdDev | Gen0 | Allocated | |----------------- |---------:|---------:|---------:|-------:|----------:| | WeakEventHandler | 14.02 us | 0.329 us | 0.965 us | 1.8005 | 14.95 KB | | WeakEventManager | 46.13 us | 0.922 us | 1.025 us | 6.4087 | 52.70 KB | I replaced usage of `WeakEventManager` in a single place, `Application.RequestedThemeChanged`. And then in a real-world scenario, the `MauiCollectionView` sample above, scrolling on a Pixel 5: (13%) Microsoft.Maui!Microsoft.Maui.WeakEventManager.RemoveEventHandler(string,object,System.Reflection.MemberInfo) (8.8%) Microsoft.Maui!Microsoft.Maui.WeakEventHandler<TEventArgs_REF>.RemoveEventHandler(System.EventHandler`1<TEventArgs_REF>) A 4.2% improvement is noticeable while scrolling. I think I can *feel* the difference. This should improve the performance of creating or scrolling any control using `{AppThemeBinding}`. Note that `Styles.xaml` in the project template makes use of `{AppThemeBinding}`, so this is likely *every* control in a lot of .NET MAUI applications. Obviously this won't be as dramatic as the improvement in dotnet#19931, but it's *something* and seems safe and reasonable to service to .NET 8. If this change works out, we can consider: * Use `BannedApiAnalyzers` to "ban" `WeakEventManager` in this codebase. * Switch all usage over to `WeakEventHandler<T>` instead. * We can leave `WeakEventManager` in place indefinitely, as it's public. It's "fine" if it's current API is useful to MAUI developers. The only downside is if a class has multiple events, it will require multiple `WeakEventHandler<T>` objects.

StephaneDelcroix · 2024-02-21T13:01:17Z

the theme change could be notified to the AppThemeBinding using the OnParentResourcesChanged mechanism. I'll see if that's possible, and what it saves

MartinLichtblau · 2024-02-26T07:28:27Z

Feedback: Major performance improvement! Collectionview was unusable before. Please release this fix for net8. Many devs say this is the biggest reason they can't recommend using MAUI (see Is MAUI still bad?)

FlavioGoncalves-Cayas · 2024-03-11T15:06:41Z

This looks promising. Might be the CollectionView performance fix everyone has been waiting for. Did you decide if it will be released for .net 8 yet?
@Redth @mattleibow

instead of subscribing to ThemeChanged event, use the ResourcesChanged mechanism of DynamicResource to propagate the change - related to #8713 - alternative to #19931 - fixes #18505

* [C] use ResourcesChanged to propagate Theme instead of subscribing to ThemeChanged event, use the ResourcesChanged mechanism of DynamicResource to propagate the change - related to #8713 - alternative to #19931 - fixes #18505 * fix test * use a const string

mattleibow changed the title ~~Propagate the theme to all children~~ Propagate the app theme to all children Jan 16, 2024

mattleibow requested a review from jonathanpeppers January 16, 2024 20:42

jonathanpeppers reviewed Jan 17, 2024

View reviewed changes

mattleibow force-pushed the dev/propagate-theme branch 2 times, most recently from 6ccc599 to 2a8b802 Compare January 17, 2024 21:13

Propagate the theme to all children

7ac8269

mattleibow force-pushed the dev/propagate-theme branch from 2a8b802 to 7ac8269 Compare January 17, 2024 21:56

mattleibow commented Jan 17, 2024

View reviewed changes

mattleibow mentioned this pull request Jan 17, 2024

[Android] CollectionVIew scrolling performance on MAUI lags compared to XF #18505

Closed

jsuarezruiz added area-xaml XAML, CSS, Triggers, Behaviors legacy-area-perf Startup / Runtime performance labels Jan 22, 2024

This comment was marked as off-topic.

Sign in to view

PureWeen mentioned this pull request Feb 4, 2024

Subscribes to the property chain upto the application for theme binding. #8957

Closed

jonathanpeppers reviewed Feb 6, 2024

View reviewed changes

jonathanpeppers mentioned this pull request Feb 8, 2024

[core] introduce WeakEventHandler<T>, internal for now #20440

Closed

StephaneDelcroix mentioned this pull request Mar 15, 2024

[C] use ResourcesChanged to propagate Theme #21229

Merged

PureWeen closed this Mar 24, 2024

mattleibow deleted the dev/propagate-theme branch March 26, 2024 10:12

github-actions Bot locked and limited conversation to collaborators Apr 26, 2024

Eilon added perf/general 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

		[Fact(Skip = "The current implementation actively choses to have different values.")]
		public void UnattachedVisualElementBindingIsConsistent()

		SetAppTheme(AppTheme.Dark);
		app.LoadPage(new ContentPage { Content = label });

Conversation

mattleibow commented Jan 16, 2024

Description of Change

Issues Fixed

Uh oh!

jonathanpeppers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers commented Jan 17, 2024

Uh oh!

mattleibow commented Jan 17, 2024

Uh oh!

mattleibow commented Jan 17, 2024

Uh oh!

mattleibow commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcrg commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PavloLukianets commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PavloLukianets commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcrg commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PavloLukianets commented Jan 23, 2024

Uh oh!

BaY1251 commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

StephaneDelcroix commented Feb 21, 2024

Uh oh!

MartinLichtblau commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FlavioGoncalves-Cayas commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jonathanpeppers left a comment •

edited

Loading

kcrg commented Jan 23, 2024 •

edited

Loading

PavloLukianets commented Jan 23, 2024 •

edited

Loading

PavloLukianets commented Jan 23, 2024 •

edited

Loading

kcrg commented Jan 23, 2024 •

edited

Loading

BaY1251 commented Jan 30, 2024 •

edited

Loading

MartinLichtblau commented Feb 26, 2024 •

edited

Loading