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

Drop original internal XAML resources after merge #12074

Merged
merged 1 commit into from Jul 9, 2023

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Jul 6, 2023

What does the pull request do?

After a resource dictionary is merged using MergeResourceInclude, this PR allows the original dictionary to be dropped from the compilation, reducing the final assembly size.

What is the current behavior?

Currently, MergeResourceInclude basically compiles a resource dictionary twice: the merged version and the original one. Most of the time, the original one isn't needed anymore. That's the case for both built-in themes.

What is the updated/expected behavior with this PR?

With this PR, dictionaries used only via MergeResourceInclude can be optionally dropped. This is enabled for the Themes.Fluent, Themes.Simple and Controls.ColorPicker projects.

How was the solution implemented (if it's not obvious)?

Usage of each dictionary is tracked: if it's used only through merge includes, it's flagged as such.

A new MSBuild property AvaloniaXamlDropMergedDocuments is added to control whether the original dictionaries are dropped once merged. It's opt-in, as we don't want to drop them by default: people might want to include them in other projects we don't know about at compile time, or load them through runtime code. For the basic and fluent themes, these aren't supported scenarios so we can drop them.

Type and method builders for XAML resources are now lazy initialized to avoid creating types for documents that might be dropped later.

Results

Assembly Size before Size after Change
Themes.Fluent 1689 KB 746 KB -56%
Themes.Simple 623 KB 313 KB -50%
Controls.ColorPicker 627 KB 483 KB -23%

Notes

The XamlCompilerTaskExecutor.Compile public overload hasn't been changed to include this option, since that would be a breaking change. Do we really want a new overload just for this: shouldn't the MSBuild task itself be the only valid public entry point? An options object as a single parameter might be a better design, as it's easy to extend in the future. I can open a PR later to implement that if needed.

@avaloniaui-team
Copy link
Contributor

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

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 7, 2023

Initially it was by design, as we wanted to keep original xaml resources visible for manual Style/ResourceInclude.
But then it was desided to support non-publicly visible xaml files, making all internal xaml files in our assemblies actually hidden.

So, yes, now we can safely remove this code without breaking anything.

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 7, 2023

@MrJul Instead of AvaloniaXamlDropMergedDocuments property I would expect compiler automatically exclude all internal xaml resources that were merged. I.e. by checking
x:ClassModifier directive.
When it's internal, it's not accessible by the runtime xaml loaded, and can be trimmed either by .NET or our compilers.

See #11551

@MrJul
Copy link
Member Author

MrJul commented Jul 7, 2023

@MrJul Instead of AvaloniaXamlDropMergedDocuments property I would expect compiler automatically exclude all internal xaml resources that were merged. I.e. by checking x:ClassModifier directive.

That was my original thought and made perfect sense for the themes, but left aside app developers who wanted their own styles to be internal. Plus I thought of it as a behavioral breaking change.

When it's internal, it's not accessible by the runtime xaml loaded, and can be trimmed either by .NET or our compilers.
See #11551

After reading #11551 I realize that internal files are already inaccessible anyways, so my concerns above aren't relevant anymore!

I'll make the change to check for internal and remove AvaloniaXamlDropMergedDocuments .

@MrJul MrJul force-pushed the xaml-drop-merged-resources branch from 823b0a7 to 4405d79 Compare July 7, 2023 09:07
@MrJul MrJul changed the title Avoid compiling merged resources twice Drop original internal XAML resources after merge Jul 7, 2023
@MrJul
Copy link
Member Author

MrJul commented Jul 7, 2023

Made the changes and squashed the commits together, it's quite simpler now without the extra flags.

@avaloniaui-team
Copy link
Contributor

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

@workgroupengineering
Copy link
Contributor

Question. Is it possible to keep xaml in the refs assembly for future use in the AvaloniaVS extension?

@MrJul
Copy link
Member Author

MrJul commented Jul 7, 2023

Question. Is it possible to keep xaml in the refs assembly for future use in the AvaloniaVS extension?

The XAML files themselves (merged or not) were already removed from the assembly after compilation before this PR.

Technically keeping them is possible but what's the use case you have in mind? If it's to refer to the original styles, I think it's better to generate proper debug information (.pdb files) for that. Today, I can open a compiled Avalonia Window in dotPeek and see the original XAML file. Any extension should be able to do the same. (Note that it unfortunately doesn't work for themes currently, probably because source information is lost somewhere while merging.)

@maxkatz6 maxkatz6 added this pull request to the merge queue Jul 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 8, 2023
@maxkatz6 maxkatz6 added this pull request to the merge queue Jul 8, 2023
@workgroupengineering
Copy link
Contributor

Question. Is it possible to keep xaml in the refs assembly for future use in the AvaloniaVS extension?

The XAML files themselves (merged or not) were already removed from the assembly after compilation before this PR.

Technically keeping them is possible but what's the use case you have in mind? If it's to refer to the original styles, I think it's better to generate proper debug information (.pdb files) for that. Today, I can open a compiled Avalonia Window in dotPeek and see the original XAML file. Any extension should be able to do the same. (Note that it unfortunately doesn't work for themes currently, probably because source information is lost somewhere while merging.)

My idea is to allow the AvaloniaVS Extesion to extract the xaml of a control to customize it quickly, without going to search the git repository. All the metadata related (BrowsableAttribute ,EnumPropertyAttribute, etc.. ) to the intellisense and the Xaml of the controls would be kept only in the refs assemblies, so as not to affect the size of the distributed assembly.

Do you think it can do?

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 8, 2023
@maxkatz6 maxkatz6 added this pull request to the merge queue Jul 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 9, 2023
@maxkatz6 maxkatz6 merged commit a1bd915 into AvaloniaUI:master Jul 9, 2023
6 checks passed
@MrJul MrJul deleted the xaml-drop-merged-resources branch July 10, 2023 08:43
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.

None yet

4 participants