From 5e77c2a28a278f8e00f31d9692da2543c8086d6e Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Fri, 22 Sep 2023 19:06:10 -0500 Subject: [PATCH] [controls] improve "setter specificity" performance (#17527) Context: https://github.com/dotnet/maui/pull/13818#pullrequestreview-1499240657 Context: https://github.com/jonathanpeppers/lols/issues/4 Fixes: https://github.com/dotnet/maui/issues/17520 A customer noticed my LOLs per second sample was slower in .NET 8 than .NET 7. I could reproduce their results. Digging in, `dotnet-trace` showed one culprit was: .NET 7 8.5% microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.GetValue 1.2% microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.SetValue .NET 8 11.0% microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.GetValue 2.8% microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.SetValue I knew that #13818 had some performance impact, as I noted when reviewing the change. Drilling in further, most of the time is spent calling `SortedList.Last()`. Which makes sense, as `BindableObject.GetValue()` is called *a lot* in a typical .NET MAUI application. Adding some logging, I found my LOLs app most commonly had the following specificity values when `BindableProperty`'s are set: * 5,284 - a single specificity value * 34,306 - two specificity values No `BindableProperty`'s in this app had more than two specificity values. So, an improvement here would be to: * Avoid `SortedList` for the most common calls * Make fields that store up to two specificity values * If a *third* specificity value is required, fall back to using `SortedList`. I introduced a new, internal `SetterSpecificityList` class for this logic. The results of running `BindingBenchmarker`: > .\bin\dotnet\dotnet.exe run --project .\src\Core\tests\Benchmarks\Core.Benchmarks.csproj -c Release -- --filter Microsoft.Maui.Benchmarks.BindingBenchmarker.* ... Before: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | |----------------- |---------:|---------:|---------:|-------:|-------:|----------:| | BindName | 31.67 us | 0.689 us | 2.009 us | 1.7395 | 1.7090 | 14.45 KB | | BindChild | 42.18 us | 0.864 us | 2.548 us | 2.4414 | 2.3804 | 20.16 KB | | BindChildIndexer | 78.37 us | 1.564 us | 3.266 us | 3.5400 | 3.4180 | 29.69 KB | After: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | |----------------- |---------:|---------:|---------:|-------:|-------:|----------:| | BindName | 27.13 us | 0.521 us | 1.016 us | 1.3733 | 1.3428 | 11.33 KB | | BindChild | 37.77 us | 0.845 us | 2.437 us | 2.0752 | 2.0142 | 17.03 KB | | BindChildIndexer | 69.45 us | 1.356 us | 2.859 us | 3.1738 | 3.0518 | 26.56 KB | My original numbers (before specificity changes in #13818) were: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | |----------------- |---------:|---------:|---------:|-------:|-------:|----------:| | BindName | 24.46 us | 0.554 us | 1.624 us | 1.2512 | 1.2207 | 10.23 KB | | BindChild | 33.21 us | 0.743 us | 2.192 us | 1.9226 | 1.8921 | 15.94 KB | | BindChildIndexer | 61.59 us | 1.209 us | 1.952 us | 3.1128 | 3.0518 | 25.47 KB | This gets *some* of the performance back, but not all. The LOLs per second app, testing these changes on a Pixel 5: Before: 376.98 LOLs/s After: 391.44 LOLs/s --- src/Controls/src/Core/BindableObject.cs | 42 +++--- src/Controls/src/Core/SetterSpecificity.cs | 6 +- .../src/Core/SetterSpecificityList.cs | 127 ++++++++++++++++++ src/Controls/src/Core/VisualStateManager.cs | 2 +- .../SetterSpecificityListTests.cs | 71 ++++++++++ 5 files changed, 222 insertions(+), 26 deletions(-) create mode 100644 src/Controls/src/Core/SetterSpecificityList.cs create mode 100644 src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs diff --git a/src/Controls/src/Core/BindableObject.cs b/src/Controls/src/Core/BindableObject.cs index 604cbc99642a..3edd1155391d 100644 --- a/src/Controls/src/Core/BindableObject.cs +++ b/src/Controls/src/Core/BindableObject.cs @@ -124,8 +124,8 @@ void ClearValueCore(BindableProperty property, SetterSpecificity specificity) if (bpcontext == null) return; - var original = bpcontext.Values.LastOrDefault().Value; - var newValue = bpcontext.Values.Count >= 2 ? bpcontext.Values[bpcontext.Values.Keys[bpcontext.Values.Count - 2]] : null; + var original = bpcontext.Values.GetSpecificityAndValue().Value; + var newValue = bpcontext.Values.GetClearedValue(); var changed = !Equals(original, newValue); if (changed) { @@ -158,7 +158,7 @@ public object GetValue(BindableProperty property) var context = property.DefaultValueCreator != null ? GetOrCreateContext(property) : GetContext(property); - return context == null ? property.DefaultValue : context.Values.Last().Value; + return context == null ? property.DefaultValue : context.Values.GetSpecificityAndValue().Value; } internal LocalValueEnumerator GetLocalValueEnumerator() => new LocalValueEnumerator(this); @@ -175,7 +175,7 @@ public bool MoveNext() { if (_propertiesEnumerator.MoveNext()) { - Current = new LocalValueEntry(_propertiesEnumerator.Current.Key, _propertiesEnumerator.Current.Value.Values.LastOrDefault().Value, _propertiesEnumerator.Current.Value.Attributes); + Current = new LocalValueEntry(_propertiesEnumerator.Current.Key, _propertiesEnumerator.Current.Value.Values.GetSpecificityAndValue().Value, _propertiesEnumerator.Current.Value.Attributes); return true; } return false; @@ -213,8 +213,9 @@ internal LocalValueEntry(BindableProperty property, object value, BindableContex { if (properties.TryGetValue(propArray[i], out var context)) { - resultArray[i].IsSet = context.Values.LastOrDefault().Key.CompareTo(SetterSpecificity.DefaultValue) != 0; - resultArray[i].Value = (T)context.Values.LastOrDefault().Value; + var pair = context.Values.GetSpecificityAndValue(); + resultArray[i].IsSet = pair.Key.CompareTo(SetterSpecificity.DefaultValue) != 0; + resultArray[i].Value = (T)pair.Value; } else { @@ -239,7 +240,7 @@ public bool IsSet(BindableProperty targetProperty) return false; if ((bpcontext.Attributes & BindableContextAttributes.IsDefaultValueCreated) == BindableContextAttributes.IsDefaultValueCreated) return true; - return bpcontext.Values.LastOrDefault().Key.CompareTo(SetterSpecificity.DefaultValue) != 0; + return bpcontext.Values.GetSpecificityAndValue().Key.CompareTo(SetterSpecificity.DefaultValue) != 0; } @@ -290,9 +291,9 @@ internal void SetBinding(BindableProperty targetProperty, BindingBase binding, S var context = GetOrCreateContext(targetProperty); //if the value is manually set (has highest specificity than FromBinding), we reassign the specificity so it'll get replaced when the binding is applied - if (context.Values.Last().Key.CompareTo(SetterSpecificity.FromBinding) > 0) + var kvp = context.Values.GetSpecificityAndValue(); + if (kvp.Key.CompareTo(SetterSpecificity.FromBinding) > 0) { - var kvp = context.Values.Last(); context.Values.Remove(kvp.Key); context.Values[SetterSpecificity.FromBinding] = kvp.Value; } @@ -332,17 +333,12 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va { //I wonder if we coulnd't treat bindingcoutext with specificities BindablePropertyContext bpContext = bindable.GetContext(BindingContextProperty); - if (bpContext != null && bpContext.Values.LastOrDefault().Key.CompareTo(SetterSpecificity.ManualValueSetter) >= 0) + if (bpContext != null && bpContext.Values.GetSpecificityAndValue().Key.CompareTo(SetterSpecificity.ManualValueSetter) >= 0) return; - object oldContext = bindable._inheritedContext?.Target; - - if (ReferenceEquals(oldContext, value)) + if (ReferenceEquals(bindable._inheritedContext?.Target, value)) return; - if (bpContext != null && oldContext == null) - oldContext = bpContext.Values.LastOrDefault().Value; - var binding = bpContext?.Bindings.Values.LastOrDefault(); if (binding != null) @@ -580,15 +576,17 @@ internal void SetValueCore(BindableProperty property, object value, SetValueFlag void SetValueActual(BindableProperty property, BindablePropertyContext context, object value, bool currentlyApplying, SetValueFlags attributes, SetterSpecificity specificity, bool silent = false) { - object original = context.Values.LastOrDefault().Value; - var originalSpecificity = context.Values.LastOrDefault().Key; + var pair = context.Values.GetSpecificityAndValue(); + var original = pair.Value; + var originalSpecificity = pair.Key; //if the last value was set from handler, override it if (specificity != SetterSpecificity.FromHandler && originalSpecificity == SetterSpecificity.FromHandler) { context.Values.Remove(SetterSpecificity.FromHandler); - originalSpecificity = context.Values.LastOrDefault().Key; + pair = context.Values.GetSpecificityAndValue(); + originalSpecificity = pair.Key; } //We keep setter of lower specificity so we can unapply @@ -681,7 +679,7 @@ BindablePropertyContext CreateAndAddContext(BindableProperty property) { var defaultValueCreator = property.DefaultValueCreator; var context = new BindablePropertyContext { Property = property }; - context.Values.Add(SetterSpecificity.DefaultValue, defaultValueCreator != null ? defaultValueCreator(this) : property.DefaultValue); + context.Values.SetValue(SetterSpecificity.DefaultValue, defaultValueCreator != null ? defaultValueCreator(this) : property.DefaultValue); if (defaultValueCreator != null) context.Attributes = BindableContextAttributes.IsDefaultValueCreated; @@ -763,7 +761,7 @@ void CoerceValue(BindableProperty property, bool checkAccess) if (bpcontext == null) return; - object currentValue = bpcontext.Values.LastOrDefault().Value; + object currentValue = bpcontext.Values.GetSpecificityAndValue().Value; if (property.ValidateValue != null && !property.ValidateValue(this, currentValue)) throw new ArgumentException($"Value is an invalid value for {property.PropertyName}", nameof(currentValue)); @@ -789,7 +787,7 @@ internal class BindablePropertyContext public Queue DelayedSetters; public BindableProperty Property; - public SortedList Values = new(); + public readonly SetterSpecificityList Values = new(); } diff --git a/src/Controls/src/Core/SetterSpecificity.cs b/src/Controls/src/Core/SetterSpecificity.cs index b4c1a7cb8cd0..480b8978d7f7 100644 --- a/src/Controls/src/Core/SetterSpecificity.cs +++ b/src/Controls/src/Core/SetterSpecificity.cs @@ -4,7 +4,7 @@ namespace Microsoft.Maui.Controls { //NOTE: IDEA: review this: merge FROM into a single int (vsm, manual, dynamicR, binding), and CSS into another - internal readonly struct SetterSpecificity : IComparable + internal readonly struct SetterSpecificity : IComparable, IEquatable { public static readonly SetterSpecificity DefaultValue = new(-1, 0, 0, 0, -1, 0, 0, 0); public static readonly SetterSpecificity VisualStateSetter = new SetterSpecificity(1, 0, 0, 0, 0, 0, 0, 0); @@ -91,9 +91,9 @@ public int CompareTo(SetterSpecificity other) return Type.CompareTo(other.Type); } - public override bool Equals(object obj) => Equals((SetterSpecificity)obj); + public override bool Equals(object obj) => obj is SetterSpecificity s && Equals(s); - bool Equals(SetterSpecificity other) => CompareTo(other) == 0; + public bool Equals(SetterSpecificity other) => CompareTo(other) == 0; public override int GetHashCode() => (Vsm, Manual, DynamicResource, Binding, Style, Id, Class, Type).GetHashCode(); diff --git a/src/Controls/src/Core/SetterSpecificityList.cs b/src/Controls/src/Core/SetterSpecificityList.cs new file mode 100644 index 000000000000..f2299b4dbc7a --- /dev/null +++ b/src/Controls/src/Core/SetterSpecificityList.cs @@ -0,0 +1,127 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Microsoft.Maui.Controls +{ + /// + /// Class for managing up to two Specificity values, and falling back to a SortedList once three values are present. + /// This yields better performance in cases where a BP has one or two Specificity values set. + /// + internal class SetterSpecificityList + { + KeyValuePair? _first; + KeyValuePair? _second; + SortedList? _values; + + public object this[SetterSpecificity key] + { + set => SetValue(key, value); + } + + public void SetValue(SetterSpecificity specificity, object value) + { + if (_first is null || _first.Value.Key == specificity) + { + _first = new KeyValuePair(specificity, value); + if (_values is not null) + _values[specificity] = value; + return; + } + + if (_second is null || _second.Value.Key == specificity) + { + _second = new KeyValuePair(specificity, value); + if (_values is not null) + _values[specificity] = value; + return; + } + + if (_values is null) + { + _values = new() + { + [_first.Value.Key] = _first.Value.Value, + [_second.Value.Key] = _second.Value.Value, + }; + // Clear the fields, to reduce duplication in memory + _first = null; + _second = null; + } + _values[specificity] = value; + } + + public void Remove(SetterSpecificity specificity) + { + _values?.Remove(specificity); + if (_first is not null && _first.Value.Key == specificity) + _first = null; + if (_second is not null && _second.Value.Key == specificity) + _second = null; + } + + public KeyValuePair GetSpecificityAndValue() + { + // Slow path calls SortedList.Last() + if (_values is not null) + return _values.Last(); + + // Fast path accesses _first and _second + if (_first is not null && _second is not null) + { + if (_first.Value.Key.CompareTo(_second.Value.Key) >= 0) + { + return _first.Value; + } + else + { + return _second.Value; + } + } + else if (_first is not null) + { + return _first.Value; + } + else if (_second is not null) + { + return _second.Value; + } + + throw new InvalidOperationException("No BindablePropertyContext Value specified!"); + } + + /// + /// Called by ClearValueCore, returns what the value would be if cleared + /// + public object? GetClearedValue() + { + if (_values is not null) + { + return _values.Count >= 2 ? _values[_values.Keys[_values.Count - 2]] : null; + } + + // Fast path should return the "lower" value + if (_first is not null && _second is not null) + { + if (_second.Value.Key.CompareTo(_first.Value.Key) >= 0) + { + return _first.Value.Value; + } + else + { + return _second.Value.Value; + } + } + else if (_first is not null) + { + return _first.Value.Value; + } + else if (_second is not null) + { + return _second.Value.Value; + } + + return null; + } + } +} diff --git a/src/Controls/src/Core/VisualStateManager.cs b/src/Controls/src/Core/VisualStateManager.cs index 5559ebc5c323..d0d0572e084f 100644 --- a/src/Controls/src/Core/VisualStateManager.cs +++ b/src/Controls/src/Core/VisualStateManager.cs @@ -71,7 +71,7 @@ public static bool GoToState(VisualElement visualElement, string name) var groups = (VisualStateGroupList)visualElement.GetValue(VisualStateGroupsProperty); var context = visualElement.GetContext(VisualStateGroupsProperty); - var vsgSpecificity = context.Values.Keys.Last(); + var vsgSpecificity = context.Values.GetSpecificityAndValue().Key; if (vsgSpecificity == SetterSpecificity.DefaultValue) vsgSpecificity = new SetterSpecificity(); groups.Specificity = vsgSpecificity; diff --git a/src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs b/src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs new file mode 100644 index 000000000000..45681687d307 --- /dev/null +++ b/src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs @@ -0,0 +1,71 @@ +using Xunit; + +namespace Microsoft.Maui.Controls.Core.UnitTests +{ + public class SetterSpecificityListTests + { + [Fact] + public void OneValue() + { + var list = new SetterSpecificityList(); + list.SetValue(SetterSpecificity.ManualValueSetter, nameof(SetterSpecificity.ManualValueSetter)); + + var pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.ManualValueSetter), pair.Value); + Assert.Equal(SetterSpecificity.ManualValueSetter, pair.Key); + + // Add a "default" value + list.SetValue(SetterSpecificity.DefaultValue, nameof(SetterSpecificity.DefaultValue)); + pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.ManualValueSetter), pair.Value); + Assert.Equal(SetterSpecificity.ManualValueSetter, pair.Key); + } + + [Fact] + public void TwoValues() + { + var list = new SetterSpecificityList(); + list.SetValue(SetterSpecificity.DefaultValue, nameof(SetterSpecificity.DefaultValue)); + list.SetValue(SetterSpecificity.ManualValueSetter, nameof(SetterSpecificity.ManualValueSetter)); + + var pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.ManualValueSetter), pair.Value); + Assert.Equal(SetterSpecificity.ManualValueSetter, pair.Key); + + // Remove a value + list.Remove(SetterSpecificity.ManualValueSetter); + pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.DefaultValue), pair.Value); + Assert.Equal(SetterSpecificity.DefaultValue, pair.Key); + } + + [Fact] + public void ThreeValues() + { + var list = new SetterSpecificityList(); + list.SetValue(SetterSpecificity.DefaultValue, nameof(SetterSpecificity.DefaultValue)); + list.SetValue(SetterSpecificity.FromBinding, nameof(SetterSpecificity.FromBinding)); + list.SetValue(SetterSpecificity.ManualValueSetter, nameof(SetterSpecificity.ManualValueSetter)); + + var pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.ManualValueSetter), pair.Value); + Assert.Equal(SetterSpecificity.ManualValueSetter, pair.Key); + + // Remove a value + list.Remove(SetterSpecificity.ManualValueSetter); + pair = list.GetSpecificityAndValue(); + Assert.Equal(nameof(SetterSpecificity.FromBinding), pair.Value); + Assert.Equal(SetterSpecificity.FromBinding, pair.Key); + } + + [Fact] + public void GetClearedValue() + { + var list = new SetterSpecificityList(); + list.SetValue(SetterSpecificity.DefaultValue, nameof(SetterSpecificity.DefaultValue)); + Assert.Equal(nameof(SetterSpecificity.DefaultValue), list.GetClearedValue()); + list.SetValue(SetterSpecificity.ManualValueSetter, nameof(SetterSpecificity.ManualValueSetter)); + Assert.Equal(nameof(SetterSpecificity.DefaultValue), list.GetClearedValue()); + } + } +}