Improves performance on BindableProperty by albyrock87 · Pull Request #17756 · dotnet/maui

albyrock87 · 2023-09-30T16:15:56Z

Description of Change

Trying to improve Bindable Set/GetValue and bindings performance a little bit more

Improves BindableProperty performance

Rewrite SetterSpecificity to be stored as UInt64
Rewrite (simplify) SetterSpecificityList again to use SortedList but this time using its methods instead of Linq
Use SetterSpecificityList on Bindings too
Minor improvements in BindingExpression SetupPart
Minor improvements in VisualStateManger (especially GoToState by avoiding reading property context multiple times)

Benchmarks Executed with

BenchmarkDotNet v0.13.10, macOS Sonoma 14.2.1 (23C71) [Darwin 23.2.0]
Apple M3 Pro, 1 CPU, 11 logical and 11 physical cores
.NET SDK 8.0.100

SpecificitySetter Benchmark (~58% CPU Time reduction, 48% memory reduction)

Before:

| Method                      | Mean     | Error   | StdDev  | Gen0    | Gen1    | Gen2   | Allocated |
|---------------------------- |---------:|--------:|--------:|--------:|--------:|-------:|----------:|
| SettersFromDifferentSources | 161.0 us | 2.61 us | 2.44 us | 25.1465 | 12.4512 | 6.3477 | 206.98 KB |

After:

| Method                      | Mean     | Error    | StdDev   | Gen0    | Gen1   | Gen2   | Allocated |
|---------------------------- |---------:|---------:|---------:|--------:|-------:|-------:|----------:|
| SettersFromDifferentSources | 68.01 us | 0.229 us | 0.215 us | 13.1836 | 6.5918 | 3.1738 | 108.15 KB |

Binding Benchmark (some small improvements also here)

Before:

| Method           | Mean     | Error    | StdDev   | Gen0   | Gen1   | Gen2   | Allocated |
|----------------- |---------:|---------:|---------:|-------:|-------:|-------:|----------:|
| BindName         | 12.54 us | 0.075 us | 0.070 us | 1.3733 | 0.6866 | 0.0916 |  11.33 KB |
| BindChild        | 17.67 us | 0.061 us | 0.054 us | 2.0752 | 1.0376 | 0.1831 |  17.03 KB |
| BindChildIndexer | 34.11 us | 0.138 us | 0.129 us | 3.1738 | 1.5869 | 0.2441 |  26.56 KB |

After:

| Method           | Mean     | Error    | StdDev   | Gen0   | Gen1   | Gen2   | Allocated |
|----------------- |---------:|---------:|---------:|-------:|-------:|-------:|----------:|
| BindName         | 10.31 us | 0.030 us | 0.028 us | 1.2512 | 0.6256 | 0.0610 |  10.31 KB |
| BindChild        | 15.47 us | 0.060 us | 0.056 us | 1.9531 | 0.9766 | 0.1831 |  16.02 KB |
| BindChildIndexer | 34.67 us | 0.162 us | 0.151 us | 3.1128 | 1.5259 | 0.2441 |  25.55 KB |

TypedBindingBenchmarker

Before:

| Method                | Mean      | Error     | StdDev    | Gen0   | Gen1   | Gen2   | Allocated |
|---------------------- |----------:|----------:|----------:|-------:|-------:|-------:|----------:|
| TypedBindName         |  8.413 us | 0.0326 us | 0.0273 us | 1.0223 | 0.5035 | 0.0305 |   8.36 KB |
| TypedBindChild        | 13.159 us | 0.0473 us | 0.0395 us | 1.6174 | 0.8087 | 0.0916 |  13.28 KB |
| TypedBindChildIndexer | 15.325 us | 0.0632 us | 0.0591 us | 1.8616 | 0.9308 | 0.1221 |  15.23 KB |

After:

| Method                | Mean      | Error     | StdDev    | Gen0   | Gen1   | Gen2   | Allocated |
|---------------------- |----------:|----------:|----------:|-------:|-------:|-------:|----------:|
| TypedBindName         |  8.467 us | 0.1409 us | 0.1177 us | 0.8850 | 0.4425 | 0.0305 |   7.34 KB |
| TypedBindChild        | 12.850 us | 0.0384 us | 0.0359 us | 1.4954 | 0.7477 | 0.0610 |  12.27 KB |
| TypedBindChildIndexer | 14.922 us | 0.0593 us | 0.0525 us | 1.7395 | 0.8698 | 0.1221 |  14.22 KB |

Conclusions

Original PR introducing SetterSpecificity #13818

ghost · 2023-09-30T16:16:02Z

Hey there @albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

src/Controls/src/Core/Interactivity/TriggerBase.cs

jfversluis · 2023-10-02T10:50:24Z

/azp run

azure-pipelines · 2023-10-02T10:50:43Z

Azure Pipelines successfully started running 3 pipeline(s).

albyrock87 · 2023-10-06T11:48:19Z

@StephaneDelcroix @jonathanpeppers I've updated PR with some more improvements on the binding part and a general cleanup + documentation on the SetterSpecificity struct.

jonathanpeppers

I think we need some review from @StephaneDelcroix about the general idea of packing into a ulong.

src/Controls/src/Core/SetterSpecificity.cs

jonathanpeppers · 2023-10-09T16:21:29Z

src/Controls/src/Core/SetterSpecificityList.cs

+#if NETSTANDARD
+			return Keys[index];
+#else
+			return GetKeyAtIndex(index);


Do you have an idea how much time is spent now in GetKeyAtIndex()?

I was using dotnet-trace on this app: https://github.com/jonathanpeppers/lols

The same time required to access a TKey[] array index, so it shouldn't be a concern.
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections/src/System/Collections/Generic/SortedList.cs#L561-L566

On the opposite side, in NETSTANDARD that Keys[index] has some overhead because it creates a wrapper object to access keys as an IList<TKey> which we could solve via reflection considering that those private properties cannot change due to serialization matters.

public class SortedList<TKey, TValue> : IDictionary<TKey, TValue>, IDictionary, IReadOnlyDictionary<TKey, TValue> where TKey : notnull { private TKey[] keys; // Do not rename (binary serialization) private TValue[] values; // Do not rename (binary serialization)

Anyway I think that with MAUI the goal is always to use the latest target framework available, so I think the current implementation is acceptable.

.NET Standard is not really a supported platform and is just used by the Visual Studio IDE for the analyzers and/or compilers. All other systems should be using the netX.0 TFMs.

Do you have an idea how much time is spent now in GetKeyAtIndex()?

The reason I ask, is because SortedList usage shows up quite a bit in dotnet-trace for me in the past. Before #17527, like ~1.4% of time in my LOLs sample was spent just iterating a SortedList.

These were the old .speedscope files:

net7vnet8rc1.zip

I got rid of 1, 2 and 3 by eliminating Linq.
7 and 8 can be eliminated by giving a initial capacity to the sorted list.

Anyway, there is some space for improvement, I'm working on a benchmark project to get the best result possible.
I'll get back to you in a couple of days. Thanks.

The .zip file above has files you can open in https://speedscope.app/

When you finish your next iteration, I should have some time to dotnet-trace these changes and compare. Thanks!

@jonathanpeppers I've finished up doing an ad hoc implementation which brings great results.

You can see benchmark results in this repo.

I've updated the PR and also the description above with new benchmark results.

I've added a section under #if DEBUG in SetterSpecificity to simplify debugging.

I've also added all required unit tests to verify the new ad-hoc implementation.

I've also tried to include Lols page inside MAUI solution in order to try it out directly LolsPage.zip, but the number of lols/s keeps decreasing every time I enter the page, which is weird (it makes me think of memory leaks somewhere).

One last consideration regarding NET8 GA: a general practice (for example with buttons) is to define a style with the BG color and then use VSM to change it when pressed or disabled.
This makes 3 setters:

default

style

VSM

Which unfortunately with the current implemention would hit the usage of SortedList.

(Thank you for working on this!)

Today I've also added on the other repo an implementation based on an unordered linked list.
https://github.com/albyrock87/setter-specificity-list-benchmarks/blob/master/SetterSpecificityListAdHocLinked.cs
It looks better from performance side.
That's probably due to the low number of Setters but putting it here gave me a worse result in terms of memory usage on the first benchmark (129KB vs 121KB).

Method Mean Error StdDev Gen0 Gen1 Allocated

SettersFromDifferentSources 147.9 us 1.84 us 1.72 us 20.9961 8.3008 129.39 KB

Method Mean Error StdDev Gen0 Gen1 Allocated

BindName 19.68 us 0.423 us 1.248 us 1.6785 1.6479 10.31 KB

BindChild 30.06 us 0.601 us 1.255 us 2.5940 2.5635 16.02 KB

BindChildIndexer 54.53 us 1.068 us 1.663 us 4.1504 4.0894 25.55 KB

What really makes the difference between the two implementations in terms of speed is basically the Array.Copy which apparently costs more than iterating on a few items (Log(N) vs N is not that relevant with low numbers).

The last benchmark there has a huge difference in terms of speed: 1,654.5us vs 772.2us.

I'll leave this choice up to you: for now I've kept the AdHoc implementation similar to a SortedList.

src/Core/tests/Benchmarks/Benchmarks/SetterBenchmarker.cs

jonathanpeppers · 2023-10-09T16:24:20Z

/azp run

azure-pipelines · 2023-10-09T16:24:43Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2023-10-11T21:41:29Z

/azp run

azure-pipelines · 2023-10-11T21:41:50Z

Azure Pipelines successfully started running 3 pipeline(s).

* Test ignore * Test off-by-one * Test print out missing category * Cleanup --------- Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>

avalable -> available

### Description of Change Replace Wait() calls with method which Asserts the results of the Task<bool> so we have a clearer idea where the tests fail; remove extra post-Wait() assertions. Move non-extension methods to AssertHelpers. Remove random timings. ### Issues Fixed Fixes dotnet#19376

* [iOS] Fix Flyout Header breaking clicking over items The Flyout Header height was being defined as `ArrangedHeaderViewHeightWithNoMargin` + `HeaderTopMargin`, but the `HeaderView.MeasuredHeight` (value on which `ArrangedHeaderViewHeightWithNoMargin` is based on) does include the margin so we were accounting for it twice and making the header go over the items below it, making them not clickable. When there is no margin, `HeaderTopMargin` represents the safe area and it should not be accounted for either. Fixes dotnet#17965 * Rename confusing properties Rename `ArrangedHeaderViewHeightWithNoMargin` and `MeasuredHeaderViewHeightWithNoMargin`, to `ArrangedHeaderViewHeightWithMargin` and `MeasuredHeaderViewHeightWithMargin`, since both include the top margin value: https://github.com/dotnet/maui/blob/main/src/Core/src/Layouts/LayoutExtensions.cs#L27. * Fix tests * Fix safe area margin bottom value * Fix margins * Polish flyout header and content offsets * Remove extra space * Simplify content layout * Small code cleanup * Fix condition * Simplify test * More tests * Test fix * Add ArrangedHeaderViewHeightWithOutMargin * Broken * Fix scrollview inset * Fix all tests * Fix scroll view content inset and expand tests * Fix new test cases for Android * Skip FlyoutHeaderContentAndFooterAllMeasureCorrectly for Android since there is a bug that makes tests fail * Enable more tests for iOS * Honor IgnoreSafeArea * Add missing null check * Fix CollectionView support * Add check by ItemsView for CV * Add tests scrolling using different content control types * Log more info on new tests * Use requested value for assertion * Consider epsilon for scrolled position * Better test logging * Fix spacing * Another indenting issue * Fix namespaces

… a relative position (dotnet#19371) * Fix * Test * Apply feedback * Switch Button to BoxView * - fix tests * - add upper limit constraints * - switch to wait for element * Update src/Controls/samples/Controls.Sample.UITests/Issues/Issue19329.xaml Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com> * - switch to click for catalyst * Update src/Controls/tests/UITests/Tests/Issues/Issue19329.cs Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com> * - add comments --------- Co-authored-by: Shane Neuville <shneuvil@microsoft.com> Co-authored-by: Shane Neuville <shane94@hotmail.com>

Disables the native cell recycling mechanisms in favor of the renderer/handler reuse that's already built into .NET MAUI. Adds special casing to ensure that the behavior changes only apply to TableView; similar problems may exist in ListView, but that's beyond the scope of this change. These changes also break down the TableView's cell creation methods for easier maintenance.

* Update dependencies from https://github.com/dotnet/xharness build 20231220.1 Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Common , Microsoft.DotNet.XHarness.TestRunners.Xunit From Version 9.0.0-prerelease.23616.1 -> To Version 9.0.0-prerelease.23620.1 * Update dependencies from https://github.com/dotnet/xharness build 20231220.1 Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Common , Microsoft.DotNet.XHarness.TestRunners.Xunit From Version 9.0.0-prerelease.23616.1 -> To Version 9.0.0-prerelease.23620.1 * Try update catalyst to use the xharness installed --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Rui Marinho <me@ruimarinho.net>

StephaneDelcroix · 2024-01-10T08:34:50Z

I'm not yet 100% sure that's where we want to go, but it's all internal cooking, doesn't prevent us for going in any other direction in the future, and solves a real problem. I'll give it a test and eventually merge. thanks for the hard work

- Rewrite `SetterSpecificity` to be stored as UInt64 - Rewrite `SetterSpecificityList` with a faster implementation - Use `SetterSpecificityList` on `Bindings` too - Minor improvements in `BindingExpression.SetupPart` - Fixes `BindingCondition.TearDown` trying to remove `Manual` setter instead of Binding one - Fixes `GridLayoutManagerTests` on non-US cultures - Improves performance of `VisualStateManager.GoToState` - Added more benchmarks

jonathanpeppers · 2024-01-10T14:24:00Z

FYI I changed this PR to target the net9.0 branch instead.

jonathanpeppers · 2024-01-10T14:40:34Z

They are merging main into net9.0 shortly, so we are hoping the conflicts (and extra commits) will go away then.

/cc @rmarinho

StephaneDelcroix · 2024-01-11T08:04:05Z

/rebase

albyrock87 requested a review from a team as a code owner September 30, 2023 16:15

albyrock87 requested review from hartez and mattleibow September 30, 2023 16:15

ghost added the community ✨ Community Contribution label Sep 30, 2023

albyrock87 force-pushed the bindable-set-value-performance-improvement branch from 9e5afa6 to d2403e5 Compare September 30, 2023 16:34

albyrock87 commented Sep 30, 2023

View reviewed changes

src/Controls/src/Core/Interactivity/TriggerBase.cs Show resolved Hide resolved

albyrock87 force-pushed the bindable-set-value-performance-improvement branch from d2403e5 to 31647c9 Compare October 1, 2023 09:04

albyrock87 changed the title ~~Improves Bindable SetValue performance~~ Improves performance on SetterSpecificity and SetterSpecificityList Oct 1, 2023

albyrock87 force-pushed the bindable-set-value-performance-improvement branch from 31647c9 to eb29423 Compare October 1, 2023 09:11

PureWeen requested review from StephaneDelcroix and jonathanpeppers and removed request for hartez and mattleibow October 1, 2023 20:34

jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Oct 2, 2023

albyrock87 force-pushed the bindable-set-value-performance-improvement branch 2 times, most recently from 66717b7 to 9a5ed3a Compare October 6, 2023 11:41

albyrock87 changed the title ~~Improves performance on SetterSpecificity and SetterSpecificityList~~ Improves performance on BindableProperty Oct 6, 2023

jonathanpeppers reviewed Oct 9, 2023

View reviewed changes

samhouts added this to the Under Consideration milestone Oct 9, 2023

albyrock87 force-pushed the bindable-set-value-performance-improvement branch from 9a5ed3a to 09a520c Compare October 11, 2023 10:23

jonathanpeppers mentioned this pull request Oct 11, 2023

.NET 8 slower than .NET 7? jonathanpeppers/lols#4

Closed

albyrock87 force-pushed the bindable-set-value-performance-improvement branch from 09a520c to 28876c0 Compare October 14, 2023 11:51

hartez and others added 14 commits January 4, 2024 09:21

Fix incorrect call in memory tests

6c75478

Hopefully found the last remaining Wait references

eaadd24

Fix searchbar cancel button color test on Windows

75daafe

Ensure views have size in Android tests so we know they've been laid out

020af1d

Ensure image in stream test actually loads before checking color

f7f3997

Simplify handler GC tests

fe8a2e3

- fix DetailsViewPopOverLayoutIsCorrectForIdiom

7898fc2

[Windows] Fix verification of test count in CI (dotnet#19697)

c3221bf

* Test ignore * Test off-by-one * Test print out missing category * Cleanup --------- Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>

Update eng/README.md (dotnet#19735)

dda8dd3

avalable -> available

github-actions bot force-pushed the bindable-set-value-performance-improvement branch from 85bf14a to 2285dd8 Compare January 10, 2024 08:47

StephaneDelcroix approved these changes Jan 10, 2024

View reviewed changes

StephaneDelcroix enabled auto-merge (squash) January 10, 2024 14:23

jonathanpeppers changed the base branch from main to net9.0 January 10, 2024 14:23

Merge branch 'net9.0' into bindable-set-value-performance-improvement

c342e58

StephaneDelcroix merged commit 1ce36ff into dotnet:net9.0 Jan 11, 2024

albyrock87 deleted the bindable-set-value-performance-improvement branch January 11, 2024 20:47

github-actions bot locked and limited conversation to collaborators Feb 11, 2024

samhouts removed this from the Under Consideration milestone Jul 1, 2024

samhouts added fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.100-preview.1.9973 labels Aug 2, 2024

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
BindName	19.68 us	0.423 us	1.248 us	1.6785	1.6479	10.31 KB
BindChild	30.06 us	0.601 us	1.255 us	2.5940	2.5635	16.02 KB
BindChildIndexer	54.53 us	1.068 us	1.663 us	4.1504	4.0894	25.55 KB

Comments

Conversation

albyrock87 commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Uh oh!

ghost commented Sep 30, 2023

Uh oh!

Uh oh!

jfversluis commented Oct 2, 2023

Uh oh!

azure-pipelines bot commented Oct 2, 2023

Uh oh!

albyrock87 commented Oct 6, 2023

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

albyrock87 Oct 11, 2023 • 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!

mattleibow Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

albyrock87 Oct 13, 2023 • 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 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

albyrock87 Oct 14, 2023 • 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!

MartyIX Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

albyrock87 Oct 16, 2023 • 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!

Uh oh!

jonathanpeppers commented Oct 9, 2023

Uh oh!

azure-pipelines bot commented Oct 9, 2023

Uh oh!

mattleibow commented Oct 11, 2023

Uh oh!

azure-pipelines bot commented Oct 11, 2023

Uh oh!

StephaneDelcroix commented Jan 10, 2024

Uh oh!

jonathanpeppers commented Jan 10, 2024

Uh oh!

jonathanpeppers commented Jan 10, 2024

Uh oh!

StephaneDelcroix commented Jan 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

albyrock87 commented Sep 30, 2023 •

edited

Loading

albyrock87 Oct 11, 2023 •

edited

Loading

albyrock87 Oct 13, 2023 •

edited

Loading

albyrock87 Oct 14, 2023 •

edited

Loading

albyrock87 Oct 16, 2023 •

edited

Loading