-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[controls] improve "setter specificity" performance (#17527)
Context: #13818 (review) Context: jonathanpeppers/lols#4 Fixes: #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
- Loading branch information
1 parent
087fb7b
commit 5e77c2a
Showing
5 changed files
with
222 additions
and
26 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace Microsoft.Maui.Controls | ||
| { | ||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| internal class SetterSpecificityList | ||
| { | ||
| KeyValuePair<SetterSpecificity, object>? _first; | ||
| KeyValuePair<SetterSpecificity, object>? _second; | ||
| SortedList<SetterSpecificity, object>? _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<SetterSpecificity, object>(specificity, value); | ||
| if (_values is not null) | ||
| _values[specificity] = value; | ||
| return; | ||
| } | ||
|
|
||
| if (_second is null || _second.Value.Key == specificity) | ||
| { | ||
| _second = new KeyValuePair<SetterSpecificity, object>(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<SetterSpecificity, object> 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!"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Called by ClearValueCore, returns what the value would be if cleared | ||
| /// </summary> | ||
| 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; | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } | ||
| } |