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.

Sign up for GitHub

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

Jump to bottom

Style specificity #13818

Merged
merged 31 commits into from
Jul 20, 2023
Merged

Style specificity #13818

merged 31 commits into from
Jul 20, 2023

Conversation

StephaneDelcroix
Copy link
Contributor

@StephaneDelcroix StephaneDelcroix commented Mar 10, 2023 •
edited
Loading

Description of Change

This changes the way values are applied to BindableProperties. Behind the scenes, every value set is associated with a specificity, and each value is kept, allowing proper rollback when unapplying.

We still can refine the specificities, but here is how they're compared right now:

  • DefaultValue has the lowest priority
  • Everything coming from a Style is low priority
  • Binding, DynamicResource, Manual (in that order)
  • values set from VSM have a higher priority

then everything coming from the Handlers has a special priority. it is always applied, but is overridden by almost everything else

There are some new unit test showcasing what this does https://github.com/dotnet/maui/pull/13818/files#diff-a55f411e3279ac65c4d17ddf4f3596a307c697d7d18fe1e2d82c95ce6b7081a5R1011

Issues Fixed

Fixes #11082

@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@jsuarezruiz jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Mar 10, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@drasticactions drasticactions mentioned this pull request Apr 20, 2023
Closed
@StephaneDelcroix StephaneDelcroix marked this pull request as ready for review April 27, 2023 09:19
@Redth Redth requested a review from mattleibow May 3, 2023 14:40
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

1 similar comment
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@chabiss chabiss self-requested a review June 12, 2023 23:14
chabiss
chabiss approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@chabiss chabiss left a comment

Choose a reason for hiding this comment

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

:shipit:

PureWeen
PureWeen reviewed Jul 19, 2023
View reviewed changes
src/Controls/tests/DeviceTests/Elements/Frame/FrameTests.cs
// If the frame's height changed (it wrapped some text), ensure it hasn't shrunk
Assert.True(frameControlSize.Height >= originalFrameHeight);
});
{
Copy link
Member

Choose a reason for hiding this comment

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

This is just an indent fix

PureWeen
PureWeen reviewed Jul 19, 2023
View reviewed changes
src/Controls/tests/DeviceTests/Elements/Frame/FrameTests.cs
@@ -302,7 +302,7 @@ public async Task FrameIncludesPadding()
Assert.Equal(expected, layoutFrame.Height, 1.0d);
}

#if !ANDROID
#if !ANDROID && !IOS
Copy link
Member

Choose a reason for hiding this comment

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

This test deals with window resizing which isn't applicable to iOS. I updated the iOS tests a bit on this PR to always run inside a Modal View Controller. Because of that change these tests started failing because the iOS view isn't going to change size when you change the width/height on the window

@PureWeen PureWeen merged commit 83398c3 into main Jul 20, 2023
@PureWeen PureWeen deleted the styleSpecificity branch July 20, 2023 00:32
PureWeen added a commit that referenced this pull request Jul 21, 2023
@PureWeen
Revert "Style specificity (#13818)"
ea992bb 
This reverts commit 83398c3.
PureWeen added a commit that referenced this pull request Jul 24, 2023
@PureWeen
Revert "Style specificity (#13818)"
cee834f 
This reverts commit 83398c3.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Aug 9, 2023
@jonathanpeppers
Revert "Style specificity (dotnet#13818)"
56f8223 
This reverts commit 83398c3.
@MartyIX MartyIX mentioned this pull request Aug 10, 2023
Closed
This was referenced Sep 14, 2023
Closed
Closed
@jonathanpeppers jonathanpeppers mentioned this pull request Sep 15, 2023
Closed
@PureWeen PureWeen mentioned this pull request Sep 20, 2023
Closed
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Sep 20, 2023
@jonathanpeppers
[controls] improve "setter specificity" performance
c33c0eb 
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
@jonathanpeppers jonathanpeppers mentioned this pull request Sep 20, 2023
Merged
github-actions bot pushed a commit to jonathanpeppers/maui that referenced this pull request Sep 22, 2023
@jonathanpeppers @PureWeen
[controls] improve "setter specificity" performance
ad4b4d1 
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
PureWeen pushed a commit that referenced this pull request Sep 23, 2023
@jonathanpeppers
[controls] improve "setter specificity" performance (#17527)
5e77c2a 
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
@albyrock87 albyrock87 mentioned this pull request Sep 30, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@jknaudt21 jknaudt21 jknaudt21 approved these changes

@chabiss chabiss chabiss approved these changes

@jfversluis jfversluis jfversluis approved these changes

@Redth Redth Redth approved these changes

@mattleibow mattleibow Awaiting requested review from mattleibow

Assignees
No one assigned
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Setter Specificity
9 participants
@StephaneDelcroix @PureWeen @Redth @jonathanpeppers @jfversluis @chabiss @jknaudt21 @jsuarezruiz @samhouts