From 3b3f74f30eac5a5477d6dc0485c3b0dd39483790 Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Mon, 17 Apr 2023 14:18:12 -0500
Subject: [PATCH] [controls] fix performance issue in {AppThemeBinding}

Context: https://github.com/dotnet/maui/issues/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:

https://github.com/dotnet/runtime/issues/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.
---
 src/Controls/src/Core/AppThemeBinding.cs | 37 ++++++++++++------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/Controls/src/Core/AppThemeBinding.cs b/src/Controls/src/Core/AppThemeBinding.cs
index 092e505e8775..086f1d3cb2f6 100644
--- a/src/Controls/src/Core/AppThemeBinding.cs
+++ b/src/Controls/src/Core/AppThemeBinding.cs
@@ -8,6 +8,7 @@ class AppThemeBinding : BindingBase
 	{
 		WeakReference<BindableObject> _weakTarget;
 		BindableProperty _targetProperty;
+		bool _attached;
 
 		internal override BindingBase Clone() => new AppThemeBinding
 		{
@@ -22,8 +23,7 @@ internal override void Apply(bool fromTarget)
 		{
 			base.Apply(fromTarget);
 			ApplyCore();
-
-			AttachEvents();
+			SetAttached(true);
 		}
 
 		internal override void Apply(object context, BindableObject bindObj, BindableProperty targetProperty, bool fromBindingContextChanged = false)
@@ -32,14 +32,12 @@ internal override void Apply(object context, BindableObject bindObj, BindablePro
 			_targetProperty = targetProperty;
 			base.Apply(context, bindObj, targetProperty, fromBindingContextChanged);
 			ApplyCore();
-
-			AttachEvents();
+			SetAttached(true);
 		}
 
 		internal override void Unapply(bool fromBindingContextChanged = false)
 		{
-			DetachEvents();
-
+			SetAttached(false);
 			base.Unapply(fromBindingContextChanged);
 			_weakTarget = null;
 			_targetProperty = null;
@@ -52,7 +50,7 @@ void ApplyCore(bool dispatch = false)
 		{
 			if (_weakTarget == null || !_weakTarget.TryGetTarget(out var target))
 			{
-				DetachEvents();
+				SetAttached(false);
 				return;
 			}
 
@@ -124,18 +122,21 @@ target is VisualElement ve &&
 			};
 		}
 
-		void AttachEvents()
+		void SetAttached(bool value)
 		{
-			DetachEvents();
-
-			if (Application.Current != null)
-				Application.Current.RequestedThemeChanged += OnRequestedThemeChanged;
-		}
-
-		void DetachEvents()
-		{
-			if (Application.Current != null)
-				Application.Current.RequestedThemeChanged -= OnRequestedThemeChanged;
+			if (_attached != value)
+			{
+				if (_attached = value)
+				{
+					if (Application.Current != null)
+						Application.Current.RequestedThemeChanged += OnRequestedThemeChanged;
+				}
+				else
+				{
+					if (Application.Current != null)
+						Application.Current.RequestedThemeChanged -= OnRequestedThemeChanged;
+				}
+			}
 		}
 	}
 }
\ No newline at end of file