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

Make MarkupExtension.ProvideValue method inlinable for StaticResourceExtension and ResolveByNameExtension #17659

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Dec 2, 2024

What does the pull request do?

There is no XAML magic in this PR, just making extensions implementation easier to digest for JIT compiler.
With .NET 9 optimizations, extension object allocation can be eliminated, if it's simple enough for JIT compiler.
I was able to confirm it with Disasmo #17622 (comment)
These changes didn't really improve our existing benchmarks to a measurable degree. Likely because we lazily evaluate most of static resources in our themes.

What is the current behavior?

{StaticResource} object can't be inlined by JIT.

What is the updated/expected behavior with this PR?

{StaticResource} object can be inlined by JIT.

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

Moves complex implementation details to a static stateless method, which we forcefully don't inline. Making instance extension method as simple, as possible (just a simple single call), allowing JIT to eliminate instance object creation completely.
Note: this improvement is in effect only with .NET 9 target.

Example of affected code:

public static object Build_664(IServiceProvider P_0)
{
    var context = CreateContext(P_0);
    SolidColorBrush solidColorBrush;
    SolidColorBrush result = (solidColorBrush = new SolidColorBrush());
    context.PushParent(solidColorBrush);
    StaticResourceExtension staticResourceExtension = new StaticResourceExtension("SystemAltHighColor");
    context.ProvideTargetProperty = SolidColorBrush.ColorProperty;
    object? obj = staticResourceExtension.ProvideValue(context);
    context.ProvideTargetProperty = null;
    XamlDynamicSetter_21(solidColorBrush, obj);
    context.PopParent();
    return result;
}

And generated assembly code (.NET 9). Left 11.2.2, right this PR.
Notable changes:

  1. CORINFO_HELP_NEWSFAST and object allocation is removed.
  2. Before const string key (0xD1FFAB1E) was assigned during object creation, after const string key is directly passed to the static method as a parameter.
    image

@maxkatz6
Copy link
Member Author

maxkatz6 commented Dec 2, 2024

Side note. We should return to the idea of resolving some static resources compile time, if referenced resource is defined in the same file.

@avaloniaui-bot
Copy link

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

@maxkatz6 maxkatz6 added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Dec 2, 2024
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

Awesome! :)

@MrJul MrJul added this pull request to the merge queue Dec 3, 2024
Merged via the queue into master with commit a530bbe Dec 3, 2024
12 checks passed
@MrJul MrJul deleted the inlining-markup-extension branch December 3, 2024 17:39
maxkatz6 added a commit that referenced this pull request Dec 19, 2024
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Dec 19, 2024
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.

3 participants