-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove indirection in DependencyInjection #52140
Conversation
# Conflicts: # src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs # src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs # src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs # src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsThe ServiceProviderEngine is a pretty thick abstraction and most of it didn't serve a purpose and can be inlined into the service provider removing the ICallback interface with it.
|
{ | ||
Func<ServiceProviderEngineScope, object> realizedService = _expressionResolverBuilder.Build(callSite); | ||
RealizedServices[callSite.ServiceType] = realizedService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These calls were redundant.
...ft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs
Show resolved
Hide resolved
@pakrym - can you resolve the conflicts on this PR? |
Will do, missed them. |
…tion # Conflicts: # src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs # src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionContainerBuilderTestExtensions.cs
Lets get this for preview6 |
public RuntimeServiceProviderEngine(IEnumerable<ServiceDescriptor> serviceDescriptors) : base(serviceDescriptors) | ||
{ | ||
} | ||
public static RuntimeServiceProviderEngine Instance { get; } = new RuntimeServiceProviderEngine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do this, it isn't stateless right? I see it's stateless.
@@ -8,28 +8,30 @@ | |||
|
|||
namespace Microsoft.Extensions.DependencyInjection.ServiceLookup | |||
{ | |||
internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAsyncDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's rename this to ServiceProviderScope
on a next PR
The ServiceProviderEngine is a pretty thick abstraction and most of it didn't serve a purpose and can be inlined into the service provider removing the ICallback interface with it.
Shrink it down to only the service accessor building logic.
Remove ScopeState as it was an extra allocation per scope.