simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 154 forks source link

Degraded Register performance when AllowOverridingRegistrations is set to true #985

Closed erdalsivri closed 10 months ago

erdalsivri commented 10 months ago

In our test performance profiling sessions we've noticed that AllowOverridingRegistrations might be partly responsible for slowness in our tests. The code seems to be doing a linear search on providers to remove existing ones. Since our container is pretty heavy (with thousands of registrations for each test case), this RemoveAll call showed up in our profiling sessions. This is obviously not a problem on prod because we use a single container and do the registrations once on startup but in our test, we use a separate container for each test case and register everything (our tests are more like integration tests than unit tests).

Here is a screenshot from a profiling session that ran 150 tests. RemoveAll was called 5M times and its predicate was called over a billion items (see op_Equality and get_ServiceType). Comparison is of course very fast but when called billions of thems, it adds up to a significant amount of time

image

I am not sure if it is easy or worthwhile to improve this code for the majority of the users (maybe we are an outlier) but it'd be very useful if you could provide methods like RegisterWithOverride, which would allow us to pay this cost only when we are actually overriding registrations in test setup.

dotnetjunkie commented 10 months ago

Have you tried setting AllowOverridingRegistrations to true at a later point in the registration process. If you can, first build your 'production' container and when finished, set AllowOverridingRegistrations to true and start making your test-specific overrides after that.

What would be the performance characteristics when taking that approach?

dotnetjunkie commented 10 months ago

After going through the code more, I believe that it shouldn't make much difference whether AllowOverridingRegistrations is true or false. When it is false, the call to ThrowWhenOverlappingRegistrationsExist would cause an O(n) (linear search) operation iterating all providers, while when set to true, the this.providers.RemoveAll causes an O(n) operation.

Although not easy, I rather try to improve this part of the code base rather than complicating the API with a RegisterWithOverride method, that would only exist for performance reasons (which would probably be even more complicated to implement).

But let me know what the performance difference is in your case if you delay setting AllowOverridingRegistrations to true (if possible).

erdalsivri commented 10 months ago

Thanks Steven! I had always assumed Container properties would be set once on startup and are never to be changed again. I did try setting AllowOverridingRegistrations later (before test overrides). However I didn't see a significant change in test runtime. However, as you mentioned if there is a similar linear search in AllowOverridingRegistrations = false path as well, then it is not surprising that disabling it hasn't improved things.

It'll be nice if there is an easy way to improve those linear searches. Otherwise feel free to close the bug.

dotnetjunkie commented 10 months ago

I had always assumed Container properties would be set once on startup and are never to be changed again

Some properties van only be set before the first registration is made (e.g. ConstructorResolutionBehavior, DependencyInjectionBehavior, PropertySelectionBehavior, and DefaultLifestyle), while others can be changed until the container has been locked (e.g. UseStrictLifestyleMismatchBehavior and ResolveUnregisteredConcreteTypes), while others, such as AllowOverridingRegistrations and SuppressLifestyleMismatchVerification have no restriction at all (although they might as well be blocked from changing when the container is locked, because it makes little sense to make changes to the options when the container is locked).

However I didn't see a significant change in test runtime.

That's unfortunate, but a little bit expected. As a single GenericRegistrationEntry instance is used for all registrations of single generic interface (e.g. ICommandHandler<T>) it can become quite a bottleneck. This is something that is likely impacting other users as well, although most of them might not be aware of it. I'll see what I can do to improve performance. I'll keep you posted.

dotnetjunkie commented 10 months ago

@erdalsivri, what is your biggest set with of registrations you make for a specific generic interface and what size does it have?

erdalsivri commented 10 months ago

We have handlers of 3 shapes (3 generic interfaces). One has 317 implementers, the second one has 879 implementers and the last one has 87 implementers). These are all registered to the DI container. Something similar to: container.Register<IHandler<FooRequest, FooResponse>, SomeHandler<FooRequest, FooResponse>> (but dynamically). Not sure if that matters but these are also decorated by two generic decorators

dotnetjunkie commented 10 months ago

Hi @erdalsivri,

I made some optimizations to the GenericRegistrationEntry class making the performance in adding registrations (in most cases) linear and, hopefully, much faster when making many closed-generic registrations.

I pushed a beta version to NuGet and am hoping you have the time to do a test run with beta version on your test suite to check if you see a difference in performance.

Let me know what your findings are.

I ran some benchmarks with Benchmark.NET to understand how 5.4.2 performs and compared that with my perf improvements (5.4.3). I tested the performance with a test of 1, 10, 100, 500, and 1000 registrations, both in 'normal' (n) mode and 'override' (o) mode. I plotted the results in this graph:

Graph showing the performance of registering closed generic types for the same generic abstraction

What you can see is that (unfortunately) with 5.4.2 adding registrations in 'override' mode is much faster compared to the normal mode. Still, both show an exponential line. This changes with the beta 5.4.3. Here the performance of both modes are identical and have a linear characteristic.

Of course, nothing is free. The memory usage of Simple Injector goes (slightly) up, and GenericRegistrationEntry got quite a lot more complex.

erdalsivri commented 10 months ago

Wow, thanks for the quick optimization Steven! I tested the beta package in one of our test projects and it shaved off almost a minute from the total test runtime (about 12-13% improvement): 7:30m vs 6:35m 🥳

dotnetjunkie commented 10 months ago

Happy to hear it's working out. Also because this is likely the most I can squeeze out of this part of the registration API.

I will publish the RTM version of v5.4.3 later this week. I'll let you know when I did.

dotnetjunkie commented 10 months ago

v5.4.3 has been published to NuGet