Skip to content

Commit

Permalink
[net7.0] [controls] fix performance issue in {AppThemeBinding} (#15384)
Browse files Browse the repository at this point in the history
* [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 <[email protected]>
  • Loading branch information
github-actions[bot] and jonathanpeppers authored Jun 1, 2023
1 parent 1199931 commit cd646d5
Showing 1 changed file with 21 additions and 18 deletions.
39 changes: 21 additions & 18 deletions src/Controls/src/Core/AppThemeBinding.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class AppThemeBinding : BindingBase
{
WeakReference<BindableObject> _weakTarget;
BindableProperty _targetProperty;
bool _attached;

internal override BindingBase Clone() => new AppThemeBinding
{
Expand All @@ -21,8 +22,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)
Expand All @@ -31,14 +31,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;
Expand All @@ -51,7 +49,7 @@ void ApplyCore(bool dispatch = false)
{
if (_weakTarget == null || !_weakTarget.TryGetTarget(out var target))
{
DetachEvents();
SetAttached(false);
return;
}

Expand Down Expand Up @@ -123,18 +121,23 @@ 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;
var app = Application.Current;
if (app != null && _attached != value)
{
if (value)
{
// Going from false -> true
app.RequestedThemeChanged += OnRequestedThemeChanged;
}
else
{
// Going from true -> false
app.RequestedThemeChanged -= OnRequestedThemeChanged;
}
_attached = value;
}
}
}
}

0 comments on commit cd646d5

Please sign in to comment.