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

[core] use factory methods for registering services #16741

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

jonathanpeppers
Copy link
Member

In .NET 8 Preview 7, we noticed a regression in startup around:

.NET 8 Preview 6:
45.35ms microsoft.maui!Microsoft.Maui.Hosting.MauiAppBuilder.Build()
.NET 8 Preview 7:
62.17ms microsoft.maui!Microsoft.Maui.Hosting.MauiAppBuilder.Build()

This may have been related to:

dotnet/runtime#87183

Which should be improved by:

dotnet/runtime#89964

In reviewing the traces, we noticed several places where MAUI was registering services like:

services.AddTransient<IFoo, Foo>();

Where we could instead do:

services.AddTransient<IFoo>(_ => new Foo());

Where this registration avoids any System.Reflection work at startup. Microsoft.Extensions.DI can just call the factory method instead.

I expanded upon BannedSymbols.txt like we did in e7812b0, to ban cases of AddTransient and AddScoped that might be used accidentally.

This resulted in banning the slow versions of these APIs like:

Microsoft.Maui.Hosting.ImageSourceServiceCollectionExtensions.AddService
Microsoft.Maui.Hosting.MauiHandlersCollectionExtensions.AddHandler
Microsoft.Maui.Hosting.MauiHandlersCollectionExtensions.TryAddHandler

I also introduced public, fast versions of methods in MauiHandlersCollectionExtensions.

With this change in place, I could see the difference in:

Before:
11.24ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine..ctor
After:
 6.29ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine..ctor

Which resulted in an overall faster startup of a dotnet new maui app on a Pixel 5:

Average(ms): 691
Std Err(ms): 3.00370142028502
Std Dev(ms): 9.49853789918334
Average(ms): 687.8
Std Err(ms): 4.04365071576553
Std Dev(ms): 12.7871463239892

This is an average of 10 runs. Note that this was built with main, and so we have an out-of-date AOT profile compared to the net8.0 branch.

@jonathanpeppers jonathanpeppers added the legacy-area-perf Startup / Runtime performance label Aug 14, 2023
@samhouts samhouts added this to the .NET 8 GA milestone Aug 15, 2023
@jonathanpeppers jonathanpeppers marked this pull request as ready for review August 17, 2023 14:29
@jonathanpeppers jonathanpeppers requested review from a team as code owners August 17, 2023 14:29
Eilon
Eilon previously approved these changes Aug 18, 2023
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Looks good. At least with your updated change there should be no behavior regression at all, even if perhaps it turns out the CellRenderer is not needed a tall.

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Besides the removal of the Cell registration, this is good to go! And maybe a rebase/merge from main to fix the CI builds.

In .NET 8 Preview 7, we noticed a regression in startup around:

    .NET 8 Preview 6:
    45.35ms microsoft.maui!Microsoft.Maui.Hosting.MauiAppBuilder.Build()
    .NET 8 Preview 7:
    62.17ms microsoft.maui!Microsoft.Maui.Hosting.MauiAppBuilder.Build()

This may have been related to:

dotnet/runtime#87183

Which should be improved by:

dotnet/runtime#89964

In reviewing the traces, we noticed several places where MAUI was
registering services like:

    services.AddTransient<IFoo, Foo>();

Where we could instead do:

    services.AddTransient<IFoo>(_ => new Foo());

Where this registration avoids any System.Reflection work at startup.
Microsoft.Extensions.DI can just call the factory method instead.

I expanded upon `BannedSymbols.txt` like we did in e7812b0, to ban
cases of `AddTransient` and `AddScoped` that might be used accidentally.

This resulted in banning the slow versions of these APIs like:

    Microsoft.Maui.Hosting.ImageSourceServiceCollectionExtensions.AddService
    Microsoft.Maui.Hosting.MauiHandlersCollectionExtensions.AddHandler
    Microsoft.Maui.Hosting.MauiHandlersCollectionExtensions.TryAddHandler

I also introduced public, fast versions of methods in
`MauiHandlersCollectionExtensions`.

With this change in place, I could see the difference in:

    Before:
    11.24ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine..ctor
    After:
     6.29ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine..ctor

Which resulted in an overall faster startup of a `dotnet new maui` app
on a Pixel 5:

    Average(ms): 691
    Std Err(ms): 3.00370142028502
    Std Dev(ms): 9.49853789918334
    Average(ms): 687.8
    Std Err(ms): 4.04365071576553
    Std Dev(ms): 12.7871463239892

This is an average of 10 runs. Note that this was built with main, and
so we have an out-of-date AOT profile compared to the `net8.0` branch.
@@ -7,6 +7,7 @@
<RootNamespace>Microsoft.Maui.DeviceTests</RootNamespace>
<AssemblyName>Microsoft.Maui.Controls.DeviceTests</AssemblyName>
<NoWarn>$(NoWarn),CA1416</NoWarn>
<IsTestProject>true</IsTestProject>
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This disables the BannedApis analyzer for certain projects, added in: e7812b0

It's fine to use these in tests.

public static IImageSourceServiceCollection AddService<TImageSource, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TImageSourceService>(this IImageSourceServiceCollection services)
where TImageSource : IImageSource
where TImageSourceService : class, IImageSourceService<TImageSource>
{
#pragma warning disable RS0030 // Do not use banned APIs, the current method is also banned
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a overload that doesn't use this ? And use it ourselfs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use the wrong one, it's a "banned API" and we'd get a build error. This one doesn't seem to be highly used, but I'm open.

@rmarinho rmarinho enabled auto-merge (squash) August 23, 2023 11:21
@rmarinho rmarinho requested review from rmarinho and removed request for Eilon and jfversluis August 23, 2023 11:22
@PureWeen
Copy link
Member

PureWeen commented Aug 23, 2023

@jonathanpeppers @mattleibow so one thing I realized about this PR is that this will disable our ability to do dependency injection with handlers. I was more hoping we were going to move to instantiating all the handlers via DependencyInjection.ActivatorUtilities.CreateInstance or service resolution vs moving to factories.

#4129

I realize users could re-register the handler but that's a bit awkward.

Will we eventually be able to move these back once the issue that regressed with runtime is fixed?

@jonathanpeppers
Copy link
Member Author

this will disable our ability to do dependency injection with handlers

@PureWeen no, it shouldn't. There is an example how to do this in e7812b0:

services.TryAddSingleton<IMauiHandlersFactory>(sp => new MauiHandlersFactory(sp.GetServices<HandlerRegistration>()));

If you want to do constructor injection, you just have to pass the parameters in.

Keep in mind customers won't have to do any of this in their apps. They can register things however they like.

@jonathanpeppers
Copy link
Member Author

Will we eventually be able to move these back once the issue that regressed with runtime is fixed?

We should avoid DI features that use System.Reflection -- if we want a fast startup time. They might have a source generator one day, but that is some future .NET 9 thing.

@rmarinho rmarinho requested a review from PureWeen August 23, 2023 15:23
@PureWeen PureWeen requested a review from Eilon August 23, 2023 15:51
@PureWeen
Copy link
Member

this will disable our ability to do dependency injection with handlers

@PureWeen no, it shouldn't. There is an example how to do this in e7812b0:

services.TryAddSingleton<IMauiHandlersFactory>(sp => new MauiHandlersFactory(sp.GetServices<HandlerRegistration>()));

If you want to do constructor injection, you just have to pass the parameters in.

Keep in mind customers won't have to do any of this in their apps. They can register things however they like.

yea, I was thinking from our performance analysis work in NET6 we'd mostly determined it didn't really matter. I was just hopeful we'd be able to avoid needing to be prescriptive about how to register.

If this is something users need to be aware of in general, we might also look at updating our docs https://learn.microsoft.com/en-us/dotnet/architecture/maui/dependency-injection#registration

Or eventually adding to the queue of things that would be neat for a MAUI analyzer :-)

@rmarinho rmarinho merged commit b0bba51 into dotnet:main Aug 23, 2023
34 checks passed
@jonathanpeppers jonathanpeppers deleted the ServiceFactoryAllTheThings branch August 23, 2023 16:31
PureWeen added a commit that referenced this pull request Aug 29, 2023
This reverts commit b0bba51.

# Conflicts:
#	src/Controls/src/Xaml/Hosting/AppHostBuilderExtensions.cs
#	src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt
PureWeen added a commit that referenced this pull request Aug 30, 2023
* Revert "[core] factory methods for registering services, part 2 (#17004)"

This reverts commit 79a44ff.

* Revert "[android] fix DI registration for Frame, ListView, TableView (#16989)"

This reverts commit 136f21d.

* Revert "[core] use factory methods for registering services (#16741)"

This reverts commit b0bba51.

# Conflicts:
#	src/Controls/src/Xaml/Hosting/AppHostBuilderExtensions.cs
#	src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt

* Revert "Revert "[core] factory methods for registering services, part 2 (#17004)""

This reverts commit ed9a1fc.

# Conflicts:
#	eng/BannedSymbols.txt
#	src/BlazorWebView/src/SharedSource/BlazorWebViewServiceCollectionExtensions.cs

* Restore some changes (so hopefully it builds now?)

Diff is also smaller

---------

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) area-controls-general General issues that span multiple controls, or common base classes such as View or Element and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants