Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[controls] improve "setter specificity" performance #17527

Merged
merged 1 commit into from
Sep 23, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Sep 20, 2023

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 (smaller ms/KB is better):

> .\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 (higher is better):

Before:
376.98 LOLs/s
After:
391.44 LOLs/s

image

Comment on lines -338 to -344
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found oldContext wasn't used below, Roslyn actually recommended removing this.

@jonathanpeppers
Copy link
Member Author

This also seems to help startup, a dotnet new maui app on a Pixel 5 testing this change with dotnet-trace attached:

Before:
105.36ms microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.SetValueCore(Microsoft.Maui.Controls.BindableProperty...
After:
89.79ms microsoft.maui.controls!Microsoft.Maui.Controls.BindableObject.SetValueCore(Microsoft.Maui.Controls.BindableProperty..

So maybe ~15ms improved for this call?

@jonathanpeppers
Copy link
Member Author

A customer just shared some results comparing their app startup 7 vs 8:

https://discord.com/channels/732297728826277939/732297837953679412/1154161770169114654

image

@PureWeen PureWeen added this to the .NET 8 GA milestone Sep 20, 2023
@StephaneDelcroix
Copy link
Contributor

can't we get the same perf bump by just avoiding calling Last() ?

@StephaneDelcroix
Copy link
Contributor

I'd go with that, and if you have a chart indicating where the time is spend, we can look at how to improve this further

@jonathanpeppers
Copy link
Member Author

can't we get the same perf bump by just avoiding calling Last() ?

Something like list[list.Count-1] is already done by Last():

https://source.dot.net/#System.Linq/System/Linq/Last.cs,76

I think that list[list.Count-1] would be roughly the same as calling Last(). But we can try it.

Some other ideas, we could look at next:

  • Invert the sorting. Is First() faster than Last()?
  • Is there some other collection type that would work better?

@mattleibow
Copy link
Member

Can you also just store last? So first, second and last.

@Eilon Eilon added the legacy-area-perf Startup / Runtime performance label Sep 21, 2023
@jonathanpeppers
Copy link
Member Author

Can you also just store last? So first, second and last.

I have tested some benchmarks, but haven't proven it 100% helps yet. I think we should do this in a future PR.

@PureWeen
Copy link
Member

/rebase

Context: dotnet#13818 (review)
Context: jonathanpeppers/lols#4
Fixes: dotnet#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 dotnet#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 dotnet#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
@github-actions github-actions bot force-pushed the SetterSpecificityPerformance branch from c33c0eb to ad4b4d1 Compare September 22, 2023 14:54
@PureWeen PureWeen enabled auto-merge (squash) September 22, 2023 14:54
@PureWeen PureWeen merged commit 5e77c2a into dotnet:main Sep 23, 2023
@jonathanpeppers jonathanpeppers deleted the SetterSpecificityPerformance branch September 23, 2023 01:02
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-rc.2.9373 Look for this fix in 8.0.0-rc.2.9373! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-8.0.0-rc.2.9373 Look for this fix in 8.0.0-rc.2.9373! legacy-area-perf Startup / Runtime performance t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve performance issues introduced by Setter Specificity PR
6 participants