Optimize ServiceCollectionDescriptorExtensions TryAdd

[eerhardt]

See the conversation here: https://github.com/dotnet/runtime/pull/44696/files#r523781023

We are currently looping over the IServiceCollection looking if a ServiceDescriptor has already been added for this ServiceType. Not only are we going through the whole collection for each new service added, but we are also making interface dispatches for each element.

Instead, we should introduce a new interface that ServiceCollection implements, so we can fast path this check.

While introducing this new interface, we should analyze if there are other paths on the startup path that can be optimized as well, in case we need other new methods.

cc @davidfowl @stephentoub @benaadams

Proposed API

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
+     public bool TryAdd(ServiceDescriptor descriptor);
+     public bool TryAddEnumerable(ServiceDescriptor descriptor);
+     public bool TryReplace(ServiceDescriptor descriptor);
+     public void RemoveAll(Type serviceType);
    }
}

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Description:	See the conversation here: https://github.com/dotnet/runtime/pull/44696/files#r523781023 We are currently looping over the IServiceCollection looking if a ServiceDescriptor has already been added for this ServiceType. Not only are we going through the whole collection for each new service added, but we are also making interface dispatches for each element. Instead, we should introduce a new interface that ServiceCollection implements, so we can fast path this check. While introducing this new interface, we should analyze if there are other paths on the startup path that can be optimized as well, in case we need other new methods. cc @davidfowl @stephentoub @benaadams
Author:	eerhardt
Assignees:	-
Labels:	area-Extensions-DependencyInjection, untriaged
Milestone:	-

[lateapexearlyspeed]

Hi, I would ask some questions if this issue is allowed to improve now, thanks.

1. In ServiceCollectionDescriptorExtensions.cs , besides of checking ServiceType for most of TryAdd(), TryAddEnumerable() checks both ServiceType and ImplementationType.
2. Consider ServiceCollection keeps implementing previous IList<> interface

Based on conversation, should ServiceCollection add new Dictionary<ServiceType, collection of serviceDescriber> field and keep old List field? This will have synchronous action when calling modified related method for example RemoveAll() ?
Or other solution ?
Thanks!

[benaadams]

@stephentoub In answer to your question #44696 (comment)

How many times are these methods called in a typical startup? The fact that you said it's saving ~750 closures suggests it's called to add that many services, and this is O(N^2) (for each service looking at every other service already added)?

Looking at the startup call counts; I'm going to say yes...

Can see it growing per call (not completely in order)

[davidfowl]

We should look into this as part of 6.0.0 in the name of startup performance. It'll also require a new API which we should try to get approved.

[davidfowl]

Assigning to myself to do.

[davidfowl]

I can think of 2 options here:

1. Define a new interface in abstractions, implement it on ServiceCollection, check for the interface in the various extension methods and delegate to the interface.
2. Move the concrete ServiceCollection type to the abstractions package and add the methods there. We would add a type forward to avoid the breaking change and detect the concrete type in the extension methods and optimize there.

I prefer option 2 as it avoids interface dispatch and avoids introducing a new type.

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
+     public void TryAdd(ServiceDescriptor descriptor);
+     public void TryAddEnumerable(ServiceDescriptor descriptor);
+     public void Replace(ServiceDescriptor descriptor);
+     public void RemoveAll(Type serviceType);
    }
}

Thoughts @eerhardt and @maryamariyan?

[eerhardt]

I prefer option 2 as it avoids interface dispatch and avoids introducing a new type.

How common is it for others to implement their own ServiceCollection?

What do you think about option 3:

3. Introduce a new abstract class ServiceCollectionBase in Abstractions that ServiceCollection inherits from that implements all methods on IServiceCollection and the above proposed methods.

[davidfowl]

Option 3 sounds fine as well but my hottake is that nobody writes their own IServiceCollection so it's not worth it.

[eerhardt]

my hottake is that nobody writes their own IServiceCollection

Ok, then Option 2 sounds good to me.

[terrajobst]

OK, I've updated the proposal to include the API

[maryamariyan]

Ok, then Option 2 sounds good to me.

LGTM. Just some thoughts for API review: Replace -> TryReplace ?

Even though the existing TryAdd APIs:

runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.cs

Lines 67 to 75 in eeb185f

	/// <summary>
	/// Adds the specified <paramref name="descriptor"/> to the <paramref name="collection"/> if the
	/// service type hasn't already been registered.
	/// </summary>
	/// <param name="collection">The <see cref="IServiceCollection"/>.</param>
	/// <param name="descriptor">The <see cref="ServiceDescriptor"/> to add.</param>
	public static void TryAdd(
	this IServiceCollection collection,
	ServiceDescriptor descriptor)

don't return bool, would we want to make these new ones return bool?

[davidfowl]

K, but how would the new types on ServiceCollection be fast pathed? Using is tests in the IServiceCollection extension methods? (Which would reduce to limited subset of types)

Yep. I'd wager that 98% of use cases are ServiceCollection instances. Though I don't have a problem trying to make this work for Lamar as well.

[benaadams]

Though I don't have a problem trying to make this work for Lamar as well.

They would have to change their inheritect for ServiceRegistry to inherit from ServiceCollection and then rather than being a straight typecheck in the extensions its now an inheritence walk (still faster than O(N²) that it is currently); and then isn't it essentally making in the IServiceCollection interface irrelevant as you'd need to always be a strongly typed ServiceCollection?

OTOH if a default interface was also added (and ServiceCollection implements the methods); then on .NET Core 3.1+ the IServiceCollection is still viable (Lamar does have a .NET5.0 build)

    public interface IServiceCollection : IList<ServiceDescriptor>
    {
#if NETCOREAPP3_1_OR_GREATER
        public bool TryAdd(ServiceDescriptor descriptor) => ServiceCollectionDescriptorExtensions.TryAdd(this, descriptor);
        public bool TryAddEnumerable(ServiceDescriptor descriptor) => ServiceCollectionDescriptorExtensions.TryAddEnumerable(this, descriptor);
        public bool TryReplace(ServiceDescriptor descriptor) => ServiceCollectionDescriptorExtensions.TryReplace(this, descriptor);
        public void RemoveAll(Type serviceType) => ServiceCollectionDescriptorExtensions.RemoveAll(this, serviceType);
#endif
    }

The extensions can still have a typecheck for ServiceCollection on pre-netcoreapp3.1 platforms; best of both worlds?

[davidfowl]

Right, I don't think there's an either or here. We should do the move and the cast anyways. We can use DIMs on IServiceCollection as well on .NET Core specific frameworks. I'm just saying that besides Lamar, I don't think anyone extends this type and it forces them to cross compile for the performance boost anyways. As long as that's fine, we can do both things.

[benaadams]

Checking implementations using grep.app serach : IServiceCollection for C# are a few implementations and extensions:

Azure/azure-webjobs-sdk

Microsoft.Azure.WebJobs.Host/Hosting/TrackedServiceCollection.cs

internal class TrackedServiceCollection : IServiceCollection, ITrackedServiceCollection

dotnet/maui

Hosting/IMauiServiceCollection.cs

public interface IMauiServiceCollection : IServiceCollection

Hosting/Internal/MauiServiceCollection.cs

class MauiServiceCollection : IMauiServiceCollection

Hosting/IMauiHandlersCollection.cs

public interface IMauiHandlersCollection : IMauiServiceCollection

Hosting/ImageSources/IImageSourceServiceCollection.cs

public interface IImageSourceServiceCollection : IServiceCollection

etc

OrchardCMS/OrchardCore

OrchardCore/Shell/Builders/FeatureAwareServiceCollection.cs

public class FeatureAwareServiceCollection : IServiceCollection

sevenTiny/Bamboo

SevenTiny.Bantina.Spring/DependencyInjection/ServiceCollection.cs

public class ServiceCollection : IServiceCollection

davidrevoledo/AspNetCore.AutoHealthCheck
AspNetCore.AutoHealthCheck.Abstractions/IAutoHealthCheckBuilder.c

public interface IAutoHealthCheckBuilder : IServiceCollection

blqw/blqw.DI
blqw.DI.Startup/ServiceCollection.cs

internal class ServiceCollection : IServiceCollection

asimmon/Pillar
Pillar.Ioc/ServiceCollection.cs

public class ServiceCollection : IServiceCollection

gmf520/osharp-v4
OSharp.Core/Dependency/ServiceCollection.cs

public class ServiceCollection : IServiceCollection

[benaadams]

Also a bunch that don't start : IServiceCollection but has something in the middle like
: List<ServiceDescriptor>, IServiceCollection

[davidfowl]

Check their target frameworks as well

[benaadams]

Check their target frameworks as well

Ah, get you, you mean if they target netstandard2.0 then they wouldn't get the perf boost from the DIMs as they'd need to also a target netcoreapp3.1+ TFM?

OTH they wouldn't get the performance improvement anyway as they don't inherit from ServiceCollection 🤷‍♂️

[bartonjs]

Video

Approved as proposed.

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceCollection
    {
+     public bool TryAdd(ServiceDescriptor descriptor);
+     public bool TryAddEnumerable(ServiceDescriptor descriptor);
+     public bool TryReplace(ServiceDescriptor descriptor);
+     public void RemoveAll(Type serviceType);
    }
}

[davidfowl]

We decided not to add new APIs here. We moved the type and are keeping these internal for now,

[terrajobst]

Should we close this issue then?

[eerhardt]

Moving a type to a different assembly is still an "API" change, right?

[terrajobst]

That's fair.

[wasabii]

@davidfowl I am one of those that implement their own IServiceCollection. It is not uncommon when creating non-MS DI integrations. My IServiceCollection wraps my DI's container's builder, to expose 'fake' service registrations out of it, so it 'looks like' services registered outside of MS DI are present.

Because so many MS libraries try to call TryAdd* and then duplicate crap if they don't see it already present.

Without this, I end up with a billion registrations of IOptions stuff, for instance.

If there were Try* methods available to me, I'd implement those directly on my IServiceCollection, instead of exposing a manufactured fake list of stuff.