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

Implement compile time MergedDictionaties #9577

Merged
merged 7 commits into from
Dec 11, 2022
Merged

Implement compile time MergedDictionaties #9577

merged 7 commits into from
Dec 11, 2022

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Dec 1, 2022

What does the pull request do?

Replaces:

<ResourceDictionary xmlns='https://github.com/avaloniaui'
                    xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'>
    <SolidColorBrush x:Key='brush1'>Red</SolidColorBrush>
</ResourceDictionary>

and

<ResourceDictionary xmlns='https://github.com/avaloniaui'
                    xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'>
    <SolidColorBrush x:Key='brush2'>Blue</SolidColorBrush>
    <ResourceDictionary.MergedDictionaries>
        <MergeResourceInclude Source='avares://Tests/Resources.xaml'/>
    </ResourceDictionary.MergedDictionaries>
</ResourceDictionary>

with equivalent of:

<ResourceDictionary xmlns='https://github.com/avaloniaui'
                    xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'>
    <SolidColorBrush x:Key='brush1'>Red</SolidColorBrush>
    <SolidColorBrush x:Key='brush2'>Blue</SolidColorBrush>
</ResourceDictionary>

(The first xaml file is still available for the ResourceInclude, at least with current implementation)

Important to mention, that this PR at current state is half efficient, because our theme-variant resources cannot be merged into a global theme. Because we have to attach/detach them in runtime to get working theme switch. This completely not forgotten PR aims to solve this by introducing ThemeDictionaries, which also can be merged into a parent ThemeDictionaries.

Also, no merging for StyleInclude was done in this PR. It might give us a slight improvement, but I doubt it will be any significant due to differences how they are resolved.

Breaking changes

If we remove original resources, then it won't be possible to ResourceInclude parts of the theme.
It wasn't done yet in this PR. Not sure if we need it. But it could save some kilobytes of app size.

Benchmarks

I run couple of different benchmarks that depend on our XAML themes + a new one.
Results are based on this branch and master branch (which already has #9413 included). I planned to do another run of benchmarks on a commit before #9413 was merged, but there are too many other PRs were merged in between (including the great interface removal (tm)) making it not very reliable.

FindFluentControlTheme

A new benchmark, which shows the most improvement.
Before this PR resource lookup had to iterate over each ResourceInclude looking for the control theme. The more controls (and ResourceIncludes), the slower it is.
After this PR it just looks up merged dictionary (partially merged due to limitations explained above). Shouldn't be much slower with no matter how many controls we have.

There also is the same benchmark for Simple theme, but it shows same relative improvement, so I removed it to avoid informational noise.

Master:

Method type Mean Error StdDev Median Gen 0 Gen 1 Allocated
FindFluentControlTheme Button 599.51 ns 5.229 ns 4.891 ns 597.86 ns - - -
FindFluentControlTheme DatePicker 104.66 ns 0.515 ns 0.481 ns 104.70 ns - - -
FindFluentControlTheme TextBox 246.04 ns 0.802 ns 0.751 ns 245.77 ns - - -

This PR with merged resource dictionaries:

Method type Mean Error StdDev Gen 0 Gen 1 Allocated
FindFluentControlTheme Button 36.93 ns 0.386 ns 0.342 ns - - -
FindFluentControlTheme DatePicker 35.85 ns 0.158 ns 0.132 ns - - -
FindFluentControlTheme TextBox 40.43 ns 0.111 ns 0.104 ns - - -
InitFluentTheme/InitSimpleTheme

This benchmark creates a fluent/simple theme and resolves a single resource from there.
Not a huge difference, except Simple them. No idea what happened there, but I suppose it became even more simple (IL-wise).

Master:

Method Mean Error StdDev Median Gen 0 Gen 1 Allocated
InitFluentTheme 475,002.16 ns 4,808.775 ns 4,498.132 ns 473,144.19 ns 52.2461 21.9727 991,560 B
InitSimpleTheme 88,004.18 ns 1,678.955 ns 1,488.350 ns 87,441.86 ns 11.9629 3.1738 227,264 B

This PR with merged resource dictionaries

Method Mean Error StdDev Gen 0 Gen 1 Allocated
InitFluentTheme 416,758.14 ns 1,167.943 ns 1,092.494 ns 48.3398 21.4844 910,240 B
InitSimpleTheme 57,204.09 ns 110.930 ns 92.631 ns 7.1411 1.7700 134,984 B
Create control benchmarks

SimpleTheme is used.
Marginal improvement, but still an improvement.

Master:

Method Mean Error StdDev Gen 0 Gen 1 Allocated
CreateCalendar 4,654.810 μs 52.3468 μs 43.7119 μs 195.3125 93.7500 3,633 KB
CreateCalendarWithLoaded 4,437.421 μs 25.1641 μs 23.5385 μs 195.3125 93.7500 3,658 KB
CreateControl 3.575 μs 0.0186 μs 0.0165 μs 0.1450 - 3 KB
CreateDecorator 3.679 μs 0.0152 μs 0.0127 μs 0.1450 - 3 KB
CreateScrollViewer 53.876 μs 0.0771 μs 0.0644 μs 3.6621 0.5493 68 KB
CreateButton 21.217 μs 0.0864 μs 0.0808 μs 1.3428 0.0610 25 KB
CreateTextBox 240.633 μs 1.1244 μs 0.9968 μs 10.7422 2.9297 203 KB

This PR with merged resource dictionaries:

Method Mean Error StdDev Gen 0 Gen 1 Allocated
CreateCalendar 4,141.446 μs 22.8560 μs 21.3795 μs 195.3125 93.7500 3,633 KB
CreateCalendarWithLoaded 4,094.625 μs 13.3628 μs 11.1585 μs 195.3125 93.7500 3,658 KB
CreateControl 2.921 μs 0.0162 μs 0.0143 μs 0.1450 - 3 KB
CreateDecorator 2.969 μs 0.0081 μs 0.0076 μs 0.1450 - 3 KB
CreateScrollViewer 49.632 μs 0.2777 μs 0.2462 μs 3.6621 - 68 KB
CreateButton 19.855 μs 0.1860 μs 0.1649 μs 1.3428 0.0610 25 KB
CreateTextBox 231.171 μs 1.1218 μs 0.9944 μs 10.9863 2.9297 203 KB

Other metrics affected by this PR:

  • Avalonia.Themes.Fluent binary size: increased from 3.3mb to 3.6mb. Because we basically duplicate resource definitions now by saving original files in the assembly. If we remove original ones, it will be improved, but users won't be able to include parts of the theme.

@maxkatz6 maxkatz6 changed the title Implement compile time MergedDictionaties [Experiment] Implement compile time MergedDictionaties Dec 1, 2022
@maxkatz6 maxkatz6 changed the base branch from fix-parent-service-provider to master December 1, 2022 06:16
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0026789-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-nightly/nuget/v3/index.json) [PRBUILDID]

@timunie
Copy link
Contributor

timunie commented Dec 1, 2022

Can I still swap out a resource dictionary during runtime? I use it for example for translations

@kekekeks
Copy link
Member

kekekeks commented Dec 1, 2022

As long as you use regular ResourceInclude and not MergeResourceInclude

@maxkatz6 maxkatz6 changed the title [Experiment] Implement compile time MergedDictionaties Шmplement compile time MergedDictionaties Dec 4, 2022
@maxkatz6 maxkatz6 changed the title Шmplement compile time MergedDictionaties Implement compile time MergedDictionaties Dec 4, 2022
@maxkatz6 maxkatz6 marked this pull request as ready for review December 4, 2022 13:40
@maxkatz6 maxkatz6 requested review from grokys and kekekeks December 4, 2022 13:40
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027010-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-nightly/nuget/v3/index.json) [PRBUILDID]

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

I can't speak for the implementation (this is @kekekeks' area, and yours I guess ;) ) but I've been wanting this feature for ages!

There's been some internal discussion about whether this should be a separate element or a compiler directive, i.e. <MergeResourceInclude> or something like <ResourceInclude x:Merge="True">.

I think the main reason that <MergeResourceInclude> was chosen was that it won't cause errors in designers that aren't aware of this directive, but someone may want to give more details here.

@maxkatz6 maxkatz6 enabled auto-merge December 11, 2022 17:53
@maxkatz6 maxkatz6 merged commit 1c79687 into master Dec 11, 2022
@maxkatz6 maxkatz6 deleted the merge-resources branch December 11, 2022 19:49
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027448-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-nightly/nuget/v3/index.json) [PRBUILDID]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants