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

Improve MauiFactory performance by avoiding reflection to get base classes #24775

Closed
wants to merge 1 commit into from

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Sep 15, 2024

Description of Change

A minor improvement to a piece of code executed on every ToHandler or ToPlatform.

Speedscope Before
image

Speedscope After
image

Extra note
I had some code ready to cache the entire Type => ServiceDescriptor(s) and make this even faster, but it breaks a unit test which honestly I don't understand the purpose of:

public void CanChangeHandlerRegistration()
{
var mauiApp = MauiApp.CreateBuilder()
.ConfigureMauiHandlers(handlers => handlers.AddHandler<ButtonStub, ButtonHandlerStub>())
.Build();
var specificHandler = mauiApp.Services.GetRequiredService<IMauiHandlersFactory>().GetHandler(typeof(ButtonStub));
Assert.IsType<ButtonHandlerStub>(specificHandler);
mauiApp.Services.GetRequiredService<IMauiHandlersFactory>().GetCollection().AddHandler<ButtonStub, AlternateButtonHandlerStub>();
var alternateHandler = mauiApp.Services.GetRequiredService<IMauiHandlersFactory>().GetHandler(typeof(ButtonStub));
Assert.IsType<AlternateButtonHandlerStub>(alternateHandler);
}

@albyrock87 albyrock87 requested a review from a team as a code owner September 15, 2024 11:50
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 15, 2024
@tj-devel709 tj-devel709 added the area-core-hosting Extensions / Hosting / AppBuilder / Startup label Sep 16, 2024
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen requested review from jonathanpeppers and removed request for jfversluis September 17, 2024 03:11
PureWeen
PureWeen previously approved these changes Sep 20, 2024
@PureWeen PureWeen enabled auto-merge (squash) September 20, 2024 19:49
auto-merge was automatically disabled September 21, 2024 21:34

Head branch was pushed to by a user without write access

@albyrock87 albyrock87 force-pushed the maui-factory-perf branch 3 times, most recently from bb8e201 to 01281ff Compare September 22, 2024 18:39
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Comment on lines +383 to +390
new WarningsPerCode
{
Code = "IL2070",
Messages = new List<string>
{
"Microsoft.Maui.Hosting.Internal.MauiFactory.ServiceBaseTypesFactory(Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.Interfaces' in call to 'System.Type.GetInterfaces()'. The parameter '#0' of method 'Microsoft.Maui.Hosting.Internal.MauiFactory.ServiceBaseTypesFactory(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.",
}
},
Copy link
Member

Choose a reason for hiding this comment

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

We should have 0 trimmer warnings going forward in the net9.0 branch. Should we target net9.0 for this change instead?

Ideally, no new trimmer warnings are introduced, and this might be some side-effect of not using .NET 9.

@albyrock87
Copy link
Contributor Author

Declined this PR because it completely changed in
.net9
e2245e0#diff-b0a3fe29b001746098b741a1cdf8ac9a03545fc1617550b3ceb45afee5688670

@albyrock87 albyrock87 closed this Sep 23, 2024
@albyrock87 albyrock87 deleted the maui-factory-perf branch September 23, 2024 21:37
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-core-hosting Extensions / Hosting / AppBuilder / Startup community ✨ Community Contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants