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

[XC] only generate the services that'll be used #19945

Merged
merged 2 commits into from Feb 14, 2024
Merged

Conversation

StephaneDelcroix
Copy link
Contributor

Description of Change

MarkupExtensions, ValueProviders and some TypeConverters require a serviceProvider. Only add the services that are actually required. This will reduce IL size, time to JIT, number of allocations, ...

Also add a warning when users do not attribute their MarkupExctensions

Issues Fixed

@@ -10,6 +10,7 @@
namespace Microsoft.Maui.Controls
{
/// <include file="../../docs/Microsoft.Maui.Controls/ReferenceTypeConverter.xml" path="Type[@FullName='Microsoft.Maui.Controls.ReferenceTypeConverter']/Docs/*" />
[RequireService([typeof(IReferenceProvider), typeof(IProvideParentValues)])]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the only IExtendedTypeConverter that needs to be attributed, all the others have a compiled counterpart

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

What is an example of the IL before & after? I have two apps and inspecting assemblies in ILSpy, but I'm not sure if I see the different yet.

Comment on lines 22 to 23
[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
public sealed class RequireServiceAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

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

If we add a new API, should this go to .NET 9?

@StephaneDelcroix
Copy link
Contributor Author

What is an example of the IL before & after? I have two apps and inspecting assemblies in ILSpy, but I'm not sure if I see the different yet.

calls for x:Reference and x:StaticResource should produce different IL and no longer push an array with all parents

@jonathanpeppers
Copy link
Member

Somehow it seems a bit slower? I do see IL changes though:

image

Before:

image

After:

image

The most expensive method now is:

image

Before, I don't think that was showing up as much.

dotnet-dsrouter.exe_20240117_093642 is after: xaml-services.zip

@jonathanpeppers
Copy link
Member

The part that is concerning is somehow all InitializeComponent() are all slower?

@jonathanpeppers
Copy link
Member

Ok, I retested before & after, that latest commit appears to fix what was slow before:

Before:

image

After:

image

The numbers vary between runs, but I see different IL which should be faster (this is the InitializeComponent() of Resources\\Styles\\Styles.xaml in the project template:

image

@jonathanpeppers
Copy link
Member

Here is a .zip with speedscope files and assemblies inside: stephane.zip

@StephaneDelcroix
Copy link
Contributor Author

/rebase

simonrozsival
simonrozsival previously approved these changes Jan 22, 2024
@samhouts samhouts added the area/Xaml </> Controls - XAML, CSS, Gestures, Triggers, Behaviors label Jan 25, 2024
@StephaneDelcroix StephaneDelcroix changed the base branch from main to net9.0 February 7, 2024 12:39
@StephaneDelcroix StephaneDelcroix dismissed simonrozsival’s stale review February 7, 2024 12:39

The base branch was changed.

@rmarinho
Copy link
Member

/rebase

@@ -243,6 +243,11 @@
<data name="XDataTypeSyntax" xml:space="preserve">
<value>x:DataType expects a string literal, an {{x:Type}} markup or {{x:Nul}l}.</value>
</data>
<data name="UnattributedMarkupType" xml:space="preserve">
<value>Consider attributing the markup extension "{0}" with [RequireService] or [AcceptEmptyServiceProvider] if it doesn't require any.</value>
Copy link
Member

Choose a reason for hiding this comment

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

I created ticket because I think our localization is broken and these aren't getting translated, just a note I wanted to had so we can keep track of it.

#20515

MarkupExtensions, ValueProviders and some TypeConverters require a
serviceProvider. Only add the services that are actually required. This
will reduce IL size, time to JIT, number of allocations, ...

Also add a warning when users do not attribute their MarkupExctensions

- fixes #19650
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Failing tests not related

@StephaneDelcroix StephaneDelcroix merged commit f690a51 into net9.0 Feb 14, 2024
45 of 50 checks passed
@StephaneDelcroix StephaneDelcroix deleted the fix_19650 branch February 14, 2024 11:24
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/Xaml </> Controls - XAML, CSS, Gestures, Triggers, Behaviors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[X] Simplify XamlServiceProvider setup code generated by XamlC when invoking markup extensions
5 participants