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

Performance issues caused by the styling system #5027

Closed
MarchingCube opened this issue Nov 11, 2020 · 20 comments
Closed

Performance issues caused by the styling system #5027

MarchingCube opened this issue Nov 11, 2020 · 20 comments
Milestone

Comments

@MarchingCube
Copy link
Collaborator

MarchingCube commented Nov 11, 2020

When working on optimizing responsiveness of my project I've noticed worrying patterns that show when profiling.

Class selectors are viral and attach to all controls

Selectors in the main theme (fluent for example) like https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Themes.Fluent/DatePicker.xaml#L89 end up being added to all RepeatButton instances. This wouldn't be a huge problem but there is another problem

Style setters are evaluated instantly
Said RepeatButton style has Content property setter: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Themes.Fluent/DatePicker.xaml#L95-L101

This means that every time RepeatButton gets styled this content template will be evaluated, control tree built, path geometry will be parsed and constructed. So detaching and attaching control again will cause this to be built again. When one is creating a lot of such controls dynamically it ends up being fairly noticeable when profiling.

Another kind of setters are the ones that use resources:
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Themes.Fluent/DatePicker.xaml#L94

Such setter will resolve SystemControlBackgroundChromeMediumLowBrush and subscribe to changes (which is not cheap since resource system has to traverse entire tree upwards to find it).

And styles generally have several such setters so it starts to add up and performance starts to degrade.

Important fact is that this style has built a content template and resolved several resources without matching. This style has no effect and in most of the cases it will never be applied. Yet everybody has to pay the price.

Such setup is making Avalonia applications feel sluggish and not responsive when new content is being added. In our case we are already careful and virtualize/recycle as much as possible but still certain content is dynamic and we end up freezing the application when styles are applied.
Such slowdowns take usually less than 100ms but if we are rendering at 60 FPS this means we just skipped 6 frames applying styles and user definitely noticed it.

@grokys
Copy link
Member

grokys commented Nov 11, 2020

Yes, I can see how this is a problem - it's ultimately caused by the flexibility of our styling system. When a style is applied to a control, bindings/values need to be set up as in order to maintain priority between styles - even if that style is never ultimately used.

There are a couple of potential solutions to the problem of "unused" styles off the top of my head:

  • We could add a method of "statically" applying a style such that its selector is only evaluated when the control is added to the tree. An example syntax might be: <Style Selector="RepeatButton.UpButton" SelectorEvaluation="Static">. Where the value of SelectorEvaluation could be Dynamic (default) or Static. This somewhat mirrors the use of static/dynamic resources. I think this could be done pretty easily as we already have a bool subscribe parameter on Selector.Match
  • When applying styles, we could set up the bindings to initially point to a "stub" and which is only "activated" when the style becomes active. I'd need to look into how difficult this would be

Regarding resources - I think we could make it so that resources are only resolved when they're actually needed. Again, I need to take a look into how involved this would be.

In general though, I think the worst of these problems can be fixed.

@ahopper
Copy link
Contributor

ahopper commented Nov 11, 2020

I'd been looking at much the same thing, most of the critical parts of my app have ended up with simplified templates for performance and I was starting to think about creating a theme of just the controls my app needs. Fluent looks great but is heavy.

grokys added a commit that referenced this issue Nov 12, 2020
We don't need to resolve resources for non-matching styles.
grokys added a commit that referenced this issue Nov 12, 2020
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
grokys added a commit that referenced this issue Nov 16, 2020
We shouldn't subscribe to bindings until needed.
grokys added a commit that referenced this issue Nov 16, 2020
As that's where #5027 was showing up most.
grokys added a commit that referenced this issue Nov 16, 2020
When initializing a control, often a lot of styles are applied that will ultimately not be active because a higher-priority style will be active. This has performance implications because we're doing a lot of potentially expensive work for nothing. This commit improves this in two ways:

- In `ValueStore` don't subscribe to bindings on these non-active styles until needed
- To do this we need to add the concept of `ISupportInitialize` to `AvaloniaObject`/`ValueStore` so that the evalulation of active bindings can be done once everything is set up.

Part of fixing #5027.
grokys added a commit that referenced this issue Nov 16, 2020
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
grokys added a commit that referenced this issue Nov 16, 2020
grokys added a commit that referenced this issue Nov 16, 2020
As that's where #5027 was showing up most.
grokys added a commit that referenced this issue Nov 16, 2020
When initializing a control, often a lot of styles are applied that will ultimately not be active because a higher-priority style will be active. This has performance implications because we're doing a lot of potentially expensive work for nothing. This commit improves this in two ways:

- In `ValueStore` don't subscribe to bindings on these non-active styles until needed
- To do this we need to add the concept of `ISupportInitialize` to `AvaloniaObject`/`ValueStore` so that the evalulation of active bindings can be done once everything is set up.

Part of fixing #5027.
grokys added a commit that referenced this issue Nov 16, 2020
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
grokys added a commit that referenced this issue Nov 16, 2020
grokys added a commit that referenced this issue Nov 17, 2020
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
grokys added a commit that referenced this issue Nov 17, 2020
grokys added a commit that referenced this issue Nov 17, 2020
grokys added a commit that referenced this issue Nov 18, 2020
We shouldn't subscribe to bindings until needed.
grokys added a commit that referenced this issue Nov 18, 2020
As that's where #5027 was showing up most.
grokys added a commit that referenced this issue Nov 18, 2020
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
grokys added a commit that referenced this issue Nov 18, 2020
grokys added a commit that referenced this issue Nov 18, 2020
grokys added a commit that referenced this issue Nov 18, 2020
Cuts down the amount of notifications raised when controls' stying is attached/detached.

Part of fixing #5027.
@robloo
Copy link
Contributor

robloo commented Jan 14, 2022

I think Styles themselves need to be re-worked and containerized -- within a hierarchy if necessary. Right now styles are just fragments and there are fragments with selectors for everything. All those fragments are handled separated from what I gather so far.

In WPF a style was nicely containerized and all the 'sub-styles' for pointer-over, enabled, etc. were triggers and handled from within the style itself quite nicely. Even in UWP which moves to named states similar to psudoclasses, all the states are handled within the same style. In effect this means a style would have a single type it applies to that also restricts all the 'sub-styles' from ever being evaluated for other situations.

Switching to a more containerized style system like WPF/UWP would be cleaner in my opinion and match closer to the framework designs: object-oriented visual trees shouldn't have a lot of style fragments. Of course we still need to support the amazing power of selectors to drill down into templates and do things impossible in WPF/UWP.

In practice what does this look like? Well, we simply need a style container with it's own selector. This would hold all the child styles together which are only evaluated if the parent selector is matched.

Example 1: New StyleContainer with it's own selector.

<StyleContainer Selector="Button">
  <Style Selector="Button">
    <Setter Property="Background" Value="{DynamicResource ButtonBackground}" />
    <Setter Property="Foreground" Value="{DynamicResource ButtonForeground}" />
    <Setter Property="Template">
      <ControlTemplate>
        <ContentPresenter x:Name="PART_ContentPresenter"
                          Background="{TemplateBinding Background}"
                          BorderBrush="{TemplateBinding BorderBrush}"
                          BorderThickness="{TemplateBinding BorderThickness}"
                          CornerRadius="{TemplateBinding CornerRadius}"
                          Content="{TemplateBinding Content}"
                          ContentTemplate="{TemplateBinding ContentTemplate}"
                          Padding="{TemplateBinding Padding}"
                          RecognizesAccessKey="True"
                          HorizontalContentAlignment="{TemplateBinding HorizontalContentAlignment}"
                          VerticalContentAlignment="{TemplateBinding VerticalContentAlignment}" />
      </ControlTemplate>
    </Setter>
  </Style>

  <!-- PointerOverState -->  
  <Style Selector="Button:pointerover /template/ ContentPresenter#PART_ContentPresenter">
    <Setter Property="Background" Value="{DynamicResource ButtonBackgroundPointerOver}" />
    <Setter Property="BorderBrush" Value="{DynamicResource ButtonBorderBrushPointerOver}" />
    <Setter Property="TextBlock.Foreground" Value="{DynamicResource ButtonForegroundPointerOver}" />
  </Style>
  
  <Style Selector="Button:pressed  /template/ ContentPresenter#PART_ContentPresenter">
    <Setter Property="Background" Value="{DynamicResource ButtonBackgroundPressed}" />
    <Setter Property="BorderBrush" Value="{DynamicResource ButtonBorderBrushPressed}" />
    <Setter Property="TextBlock.Foreground" Value="{DynamicResource ButtonForegroundPressed}" />
  </Style>

</StyleContainer>

Example 2: Nested styles in a hierarchy. Each Style would have a child Styles list.

<Style Selector="Button">
    <Setter Property="Background" Value="{DynamicResource ButtonBackground}" />
    <Setter Property="Foreground" Value="{DynamicResource ButtonForeground}" />
    <Setter Property="Template">
      <ControlTemplate>
        <ContentPresenter x:Name="PART_ContentPresenter"
                          Background="{TemplateBinding Background}"
                          BorderBrush="{TemplateBinding BorderBrush}"
                          BorderThickness="{TemplateBinding BorderThickness}"
                          CornerRadius="{TemplateBinding CornerRadius}"
                          Content="{TemplateBinding Content}"
                          ContentTemplate="{TemplateBinding ContentTemplate}"
                          Padding="{TemplateBinding Padding}"
                          RecognizesAccessKey="True"
                          HorizontalContentAlignment="{TemplateBinding HorizontalContentAlignment}"
                          VerticalContentAlignment="{TemplateBinding VerticalContentAlignment}" />
      </ControlTemplate>
    </Setter>

  <!-- PointerOverState -->  
  <Style Selector="Button:pointerover /template/ ContentPresenter#PART_ContentPresenter">
    <Setter Property="Background" Value="{DynamicResource ButtonBackgroundPointerOver}" />
    <Setter Property="BorderBrush" Value="{DynamicResource ButtonBorderBrushPointerOver}" />
    <Setter Property="TextBlock.Foreground" Value="{DynamicResource ButtonForegroundPointerOver}" />
  </Style>
  
  <Style Selector="Button:pressed  /template/ ContentPresenter#PART_ContentPresenter">
    <Setter Property="Background" Value="{DynamicResource ButtonBackgroundPressed}" />
    <Setter Property="BorderBrush" Value="{DynamicResource ButtonBorderBrushPressed}" />
    <Setter Property="TextBlock.Foreground" Value="{DynamicResource ButtonForegroundPressed}" />
  </Style>
</Style>

@amwx
Copy link
Contributor

amwx commented Jan 15, 2022

I think Styles themselves need to be re-worked and containerized -- within a hierarchy if necessary. Right now styles are just fragments and there are fragments with selectors for everything. All those fragments are handled separated from what I gather so far.

Personally, I'd lean more toward the second option, "StyleContainer" seems weird to me. Although, what I'd really like, to expand on option 2, is to not need a separate selectors for each element that should react to a specific pseudoclass, and add a Target property to setter, like WPF/UWP have in their triggers or VisualStateManager:

<Style Selector="ListBoxItem:selected">
    <Setter Target="ContentPresenter" Property="Background" Value="{resource}" />
    <Setter Target="SelectionIndicator" Property="IsVisible" Value="True" />
</Style>

That, combined with fixing the LocalValue priority issue in templates would simplify almost every control template file, and reduce the number of Style instances that have to be parsed, loaded, and compared, which I imagine would improve performance and memory overhead.

Not sure how hard this would be to implement though, I'd imagine it requires changes to the Xaml compiler, which is not something I know much about

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

@amwx

I recall some of this is similar to your comments here: #6269 (comment). Ideally we just need to start grouping styles together and eliminating the need for style fragments for control states. That not only makes things logically easier to understand, but it also is a performance benefit and selectors can be smart on what they apply to -- especially for pressed, etc.

"StyleContainer" seems weird to me.

Yes, my StyleContainer is really just a conceptual idea. I can understand that seems weird. The goal is just to stop evaluating selectors for every style and every state for every control. All that information can be codified in a simple hierarchy so only the appropriate child styles need to be considered. Basically, we need to take inspiration from UWP state manager here.

Although, what I'd really like, to expand on option 2, is to not need a separate selectors for each element that should react to a specific pseudoclass, and add a Target property to setter, like WPF/UWP have in their triggers or VisualStateManager:

Agree, 100%. You have a lot more experience with Avalonia styling than I do so your feedback is very valuable here.

reduce the number of Style instances that have to be parsed, loaded, and compared, which I imagine would improve performance and memory overhead.

Yes, that is one of the fundamental points of my thinking as well. All the various states of controls should be grouped together in a single container and then share selectors -- or at least only evaluate child selectors after the parent, etc.

Bottom line:

  1. Need to remove multiple style fragments for a single control. A control default style should be able to define everything in one style all packaged nicely together like WPF/UWP. This means all the various control states share a selector and some sort of parent/container.
  2. Must stop evaluating selectors everywhere. This issue itself describes a fundamental problem with performance doing so. It also is unnecessary. We can massively simplify things during default style authoring by adding all state information within the style itself. States are very much part of a control style and should not be a separate fragment. Think UWP state manager here.
  3. If someone still wants to make fragmented styles that apply to multiple classes in various ways like CSS -- go for it. These ideas don't restrict that technique. It is a powerful benefit over the WPF/UWP styling system (also the ability to drill down into nearly anything).

@kekekeks
Copy link
Member

StyleContainer is essentially what we are planning to implement as "control themes", but instead of having a selector those would have a target type and be switchable on per-control level using Control.ControlTheme property.

We could also implement StyleContainer separately since it would share 90% of the required infrastructure.

@maxkatz6
Copy link
Member

Isn't styled container basically the same as a nested styles feature? Which is required for control themes/styles anyway.

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

StyleContainer is essentially what we are planning to implement as "control themes"

Good to know ideas are converging :)

but instead of having a selector those would have a target type and be switchable on per-control level

I think we need to keep usage of selectors. They are more powerful and will support better integration/compatibility -- reasons are outlined below. That said, we probably need to add a Key now.

Isn't styled container basically the same as a nested styles feature?

Basically, it's like a style group. It really would only support a single parent-child relationship though and not a full hierarchy. Nesting the styles is more powerful. It's the difference between Example 1 and Example 2 above.


The more I think about it, purely nested styles seems to be the perfect solution to all of this (Example 2 in my original comment above) I don’t think there should be a separate StyleContainer, StyleGroup or ControlTheme at all. Everything should be within Style just like it is in WPF/UWP.

Style will simply need to support nesting – to ANY depth. Style will get a Children or Styles property to hold the collection of ‘sub-styles’ for each control. This has enormous benefits:

  1. Fully backwards compatible with existing styles in XAML or otherwise. It wouldn’t break apps and require rewriting exiting themes. Everything can be upgraded gradually.
  2. Implementation in Avalonia should be quite simple as there are no new classes – except perhaps a new StyleCollection.
  3. Naming isn’t ambiguous – Style remains the container as in WPF/UWP. We aren't introducing new classes to confuse people.
  4. Nested Styles in a hierarchy provide significant benefits as discussed in several places
    1. Only need to evaluate child selectors if the parent matches – a performance benefit for default control styles/templates and the various states (pressed, etc.).
    2. Logically it will make more sense to control authors especially those coming from WPF/UWP as default styles can be fully self-contained. Everything is cleaner with all the various state styles under the same parent style.
    3. This opens the door up for some very-interesting and powerful future areas of expansion. Having a hierarchy in the style itself should allow sharing even more setters for various states where it makes sense. It could be possible to have a depth of 2 or 3 common in future default styles where the states logically branch apart. Instead of having separate styles for "pointer over", "pointer over pressed", for example, "pointer over pressed" would be a child of "pointer over" and "pointer over" a child of the base style. As far as I’m aware this hasn’t been considered before in any XAML styling system and it interests me quite a bit. The possibilities here are exciting to think about!

We still probably need to add a Key property to styles so BasedOn can be done in the future. There are other scenarios where matching by Key might be useful such as a Replaces property so we avoid setting a style only for it to be overridden later and set with another style.

@kekekeks
Copy link
Member

kekekeks commented Jan 15, 2022

I think we need to keep usage of selectors. They are more powerful and will support better integration/compatibility -- reasons are outlined below.

Yes, but they lack a very important feature: the ability to replace styles entirely. If you apply multiple StyleContainer's with the same Selector the styles from the first container would still apply. When you want to change the control template the existing styling can interfere with your new template. It's a common problem with CSS when different CSS frameworks try to set their own styling, so you can't use Bootstrap with Tailwind CSS on the same page.

WPF/UWP don't have that problem because once you change Control's style the previous style no longer applies.

Which is why we want a switchable group of styles associated with a particular control that could be replaced entirely for that particular control.

e. g. in WPF I can write <Button Style="{StaticResource MyAwesomeButtonStyle}"/> with Avalonia I can not do this.

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

@kekekeks In my last paragraph I go over ideas for this. We need to add a Key property like WPF/UWP as well. Then you can do everything you wish. The selector would still be treated as a much more powerful TargetType property from UWP. WPF/UWP support both for the reasons you outlined. One allows a style to specify controls, the other allows controls to specify styles. A key can also allow styles to specify other styles for BasedOn and a potential new Replaces property.

Edit: And I will add that if a control specifies a style by key directly <Button Style="{StaticResource MyAwesomeButtonStyle}"/> that is highest priority and no other styles should be applied even if they match by selector. That is possible to do with nested styles now that they are all contained in a single parent.

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

For this situation <Button Style="{StaticResource MyAwesomeButtonStyle}"/> and this comment:

And I will add that if a control specifies a style by key directly that is highest priority and no other styles should be applied even if they match by selector. That is possible to do with nested styles now that they are all contained in a single parent.

You might say: what about styles that don't replace the template but just change something simple like background or corner radius? The above would seem not to pull in the default style with its template and therefore would not work.

In UWP it is common for all base styles to be named with a key 'DefaultButtonStyle', 'DefaultTextBlockStyle', etc. Then in your 'MyAwesomeButtonStyle`. We should do the same and then app styles would be declared as follows:

<Style Selector="Button" Key="MyAwesomeButtonStyle" BasedOn="DefaultButtonStyle">
   <Setter Property="CornerRadius" Value="8" />
</Style>

This is actually BETTER than WPF/UWP where there is some ambiguity in the style resolution system especially for default styles. This is even worse in the WinUI library which actually breaks some things due to missing design/features in this area. Avalonia would get around all of these problems by explicitly requiring styles to be based on the default (choosing if they need it along with the template).

@kekekeks
Copy link
Member

The selector would still be treated as a much more powerful TargetType property from UWP.

Well, a Style with x:Key will have to be placed into ResourceDictionary. So anything complex in Selector just won't have any sense since such Style would be only usable as a property value.

@kekekeks
Copy link
Member

This is actually BETTER than WPF/UWP where there is some ambiguity in the style resolution system especially for default styles.

My initial plan was to define "default" style (ControlTheme in the initial proposal) from a theme using other styles:

<Style Selector="Button">
   <Setter Property="Style" Value="{StaticResource FluentThemeButtonStyle}" />
</Style>

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

Well, a Style with x:Key will have to be placed into ResourceDictionary. So anything complex in Selector just won't have any sense since such Style would be only usable as a property value.

That requirement is completely arbitrary and can be changed. Key is just a property of style as I describe above and would be taken into account during style matching. There is nothing that actually prevents adding a key to style as far as I understand. Just the style system needs to be extended which is absolutely necessary.

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

My initial plan was to define "default" style (ControlTheme in the initial proposal) from a theme using other styles:

Yes, I saw that. Its similar but the BasedOn syntax is easier and more intuitive IMO. Working with keys is familiar to everyone.

@amwx
Copy link
Contributor

amwx commented Jan 15, 2022

I recall some of this is similar to your comments here: #6269 (comment). Ideally we just need to start grouping styles together and eliminating the need for style fragments for control states. That not only makes things logically easier to understand, but it also is a performance benefit and selectors can be smart on what they apply to -- especially for pressed, etc.

I thought I shared something similar too, but couldn't remember where. Speaking on the performance, just some numbers from my sample app:
Loading the Fluent v2 theme (which is every Avalonia control, except ContextMenu, plus everything I've done), results in 1763 Style instances
image

There are almost 6000 Selector variants generated, but I'm also not understanding why there's 3x as many Selectors as Style instances.
image

Granted, changing to a containerized Styling system doesn't fix this, I think that's more in the lazily loading styles category only if the Selector matches, and the Styles instances could probably be cut down doing what WinUI does by using an MSBuild script to merge all their xaml files at build, but now I'm just thinking aloud so I digress...

@amwx
Copy link
Contributor

amwx commented Jan 15, 2022

I think we need to keep usage of selectors. They are more powerful and will support better integration/compatibility -- reasons are outlined below.

Yes, but they lack a very important feature: the ability to replace styles entirely. If you apply multiple StyleContainer's with the same Selector the styles from the first container would still apply. When you want to change the control template the existing styling can interfere with your new template. It's a common problem with CSS when different CSS frameworks try to set their own styling, so you can't use Bootstrap with Tailwind CSS on the same page.

I think I kind of see the issue with keeping Selector here. They are more powerful yes, but because you can drill down into the template (Selector="Button /template/ ContentPresenter") means some may still apply even if you don't want them to.

Also, Selectors can nest type descendants:

<Style Selector="Menu MenuItem">
</Style>

If you used a style key in either of these cases, should it apply to Menu, MenuItem (or Button or ContentPresenter template item), or both? I'd assume the last Control type in the Selector, but this becomes a grey area and may cause some confusion. Granted, I really like this ability to target descendants, and I don't think the WPF TargetType system is powerful enough to do something like that.

Not arguing for or against Style Keys or Selectors, but at this conceptual stage its a little difficult to see how they function together.

@robloo
Copy link
Contributor

robloo commented Jan 15, 2022

I think I kind of see the issue with keeping Selector here.

Selector can have restricted functionality. But I would definitely not change the syntax. Just require Selector to support only types.

but at this conceptual stage its a little difficult to see how they function together.

There are directly equivalents for both in WPF/UWP so this isn't a new idea. See the table below summarizing all ideas.

UWP Avalonia
Style Style
TargetType Selector
Key Key
BasedOn BasedOn
- Replaces

If you used a style key in either of these cases, should it apply to Menu, MenuItem (or Button or ContentPresenter template item), or both? I'd assume the last Control type in the Selector, but this becomes a grey area and may cause some confusion.

There would be no issue here as far as I can see. We all agree in the base case that Selector is equivalent to TargetType. Therefore, Selector type is matched first, then the key is matched. If you specify a key with an incompatible type an exception is thrown. Again, this is no different than WPF/UWP. In other words, the same key can be used to reference this style anywhere the type is Menu or MenuItem (or a derivative thereorof). So it is usable for both and I don't think we would ever need to make the assumption you mention.

Granted, changing to a containerized Styling system doesn't fix this, I think that's more in the lazily loading styles category only if the Selector matches,

It wouldn't do anything for load times, yes, but evaluation of selectors would be much quicker. Children are only evaluated if the parent matches. Setters are ignored entirely for all the secondary state styles (pressed, etc.) except for the precise controls that need them. As I understand it that is the majority of the performance issue.

@amwx
Copy link
Contributor

amwx commented Jan 16, 2022

I think I kind of see the issue with keeping Selector here.

Selector can have restricted functionality. But I would definitely not change the syntax. Just require Selector to support only types.

but at this conceptual stage its a little difficult to see how they function together.

There are directly equivalents for both in WPF/UWP so this isn't a new idea. See the table below summarizing all ideas.
UWP Avalonia
Style Style
TargetType Selector
Key Key
BasedOn BasedOn

  • Replaces
    

If you used a style key in either of these cases, should it apply to Menu, MenuItem (or Button or ContentPresenter template item), or both? I'd assume the last Control type in the Selector, but this becomes a grey area and may cause some confusion.

There would be no issue here as far as I can see. We all agree in the base case that Selector is equivalent to TargetType. Therefore, Selector type is matched first, then the key is matched. If you specify a key with an incompatible type an exception is thrown. Again, this is no different than WPF/UWP. In other words, the same key can be used to reference this style anywhere the type is Menu or MenuItem (or a derivative thereorof). So it is usable for both and I don't think we would ever need to make the assumption you mention.

Ahhh ok, now I understand. I still think there could be some conflicts with the more complex Selectors, but yes treating it the same as TargetType with the order of operations you said makes sense

Granted, changing to a containerized Styling system doesn't fix this, I think that's more in the lazily loading styles category only if the Selector matches,

It wouldn't do anything for load times, yes, but evaluation of selectors would be much quicker. Children are only evaluated if the parent matches. Setters are ignored entirely for all the secondary state styles (pressed, etc.) except for the precise controls that need them. As I understand it that is the majority of the performance issue.

Actually, I have to hand it to XamlX, it's quite fast at parsing everything when loading, on my machine it's 5-10ms in debug mode to load the entire fluent v2 theme from all the axaml files, which if I understand correctly in loading, all the resources and templates are built and connected - so that's quite good, IMO. I was more speaking on not only containerizing styles so that less is evaluated when ApplyTemplate() is called, but expanding that to not parse and load Styles at all until a Selector matches that type - reducing memory footprint for things a specific app may never use. I'm kind of blending the original issue discussion with the current one.

@grokys grokys mentioned this issue Jun 10, 2022
2 tasks
@maxkatz6 maxkatz6 added this to the 11.0 milestone Jun 19, 2022
@grokys
Copy link
Member

grokys commented Jul 29, 2022

I think we can close this now that #8263 has been merged!

@grokys grokys closed this as completed Jul 29, 2022
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

No branches or pull requests

7 participants