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

Refactor style apply #9411

Merged
merged 3 commits into from
Nov 22, 2022
Merged

Refactor style apply #9411

merged 3 commits into from
Nov 22, 2022

Conversation

grokys
Copy link
Member

@grokys grokys commented Nov 10, 2022

What does the pull request do?

Refactors how we attach styles:

  • Removes the IStyler service and the Styler implementation
    • Having this as a pluggable service was only really done for unit testing purposes, but it meant that we had to do a service lookup each time a control wanted to style itself
  • Moves the logic for applying styles and control themes from Styler into StyledElement
  • Removes the style TryAttach method from the public API
    • I want to allow the possibility of refactoring this code later without breaking a public API

Also adds a fix for #8549:

  • Don't apply styling in TopLevel constructor. This isn't a good idea for a number of reasons, one of which is demonstrated by Window gets :is(Control) style applied twice #8549
  • Instead apply styling when showing a window, or if it's needed call StyledElement.ApplyStyling manually, e.g. in unit tests.

Performance Regression

This PR removes style caching for now. This will need to be added back in future, but:

  • The old method of caching styles inside individual Style/Styles objects wasn't very efficient. We'll need to implement something better, that also helps speed up applying control themes
  • Now we have control themes we have a lot less styles than when style caching was added so probably less important

I modified a benchmark in this PR to test this; it adds a lot of styles for various controls in a nested tree and instantiates and styles a single TextBox:

Before:

Method Mean Error StdDev Gen 0 Allocated
Apply_Detach_Styles 68.18 us 1.275 us 1.065 us 3.7842 15 KB

After:

Method Mean Error StdDev Gen 0 Allocated
Apply_Detach_Styles 109.4 us 0.89 us 0.84 us 4.2725 17 KB

So we have a pretty serious performance regression in the worst case, though this only affects applications with a lot of styles. For typical applications which use mostly control themes it shouldn't be noticeable.

I don't want to address this in this PR as it's a separate feature, but will open a PR to add back style caching using a new method in the near future, which should improve things beyond what we had before.

Depends on #8600
Fixes #8549

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0026033-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@grokys grokys changed the title WIP: Refactor styles apply WIP: Refactor style apply Nov 21, 2022
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0026420-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0026423-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

Now tries to simulate an application with a lot of styles applied at different points in the logical tree.

Make `StyledElement.ApplyStyling` a public API in order to do this.
- Removes the `IStyler` service and the `Styler` implementation
- Moves the logic for applying styles and control themes into `StyledElement`
- Removes the style `TryAttach` method from the public API
- Removes style caching for now - this will need to be added back
Applying styling in the constructor isn't a good idea as demonstrated by #8549..

Instead apply styling when showing a window, or if it's needed call `ApplyStyling` manually, e.g. in unit tests.

Fixes #8549
@grokys grokys changed the title WIP: Refactor style apply Refactor style apply Nov 21, 2022
@grokys grokys marked this pull request as ready for review November 21, 2022 11:31
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0026429-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 merged commit 9f9e99d into master Nov 22, 2022
@maxkatz6 maxkatz6 deleted the refactor/styles branch November 22, 2022 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window gets :is(Control) style applied twice
3 participants