FIX Memory leak in DefaultRegisteredServicesTracker

[snaumenko-st]

• Replaced ConcurrentBag with ConcurrentQueue. It's causing memory leaks if left undisposed somewhere.
• ClearRegistrations is not necessary anymore.
• Decreased allocations on List.Insert(0,...) and .ToList() calls

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.56%. Comparing base (ec4907f) to head (bb9e961).
Report is 9 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1423      +/-   ##
===========================================
- Coverage    78.57%   78.56%   -0.02%     
===========================================
  Files          200      200              
  Lines         5713     5709       -4     
  Branches      1168     1168              
===========================================
- Hits          4489     4485       -4     
  Misses         712      712              
  Partials       512      512              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alistairjevans]

Thanks; can you please run our benchmarks for a comparison here, particularly the ones that touch on child unit-of-work scopes with their own dependencies? I'm thinking at least ChildScopeResolveBenchmark and ConcurrencyNestedScopeBenchmark. These sort of collection changes can have unintended perf implications.

[botinko]

In the test environment we are very active in creating containers and child containers. We have found a memory leak. A lot of objects can't be GC'ed because the are active reference from ConcurrentBag internal structures (it store data in buckets in thread local storage). So eventually all IComponentRegistration will be gc rooted to static thread local.

ClearRegistrations wont help much. Just one non-disposed container or child container (which can happens in real world full of bugs) can create a fake gc root for all ConcurrentBag instances.

For me, having ConcurrentBag holding IComponentRegistration, that can hold anything in my app is already a performance bug.

We will make bench for sure, when we finished local testing in our env.

[snaumenko-st]

Thanks; can you please run our benchmarks for a comparison here, particularly the ones that touch on child unit-of-work scopes with their own dependencies? I'm thinking at least ChildScopeResolveBenchmark and ConcurrencyNestedScopeBenchmark. These sort of collection changes can have unintended perf implications.

Yes, sure thing. Here you are:

BenchmarkDotNet v0.13.10, Windows 10 (10.0.19045.4651/22H2/2022Update)
11th Gen Intel Core i7-11800H 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.303
  [Host]     : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2

ChildScopeResolveBenchmark

Method	Mean Before	Mean After
Resolve	16.944 μs	16.449 μs
ResolveNeverRegisteredFromChild	7.952 μs	7.411 μs

ConcurrencyBenchmark

Method	ResolveTaskCount	ResolvesPerTask	Mean Before	Mean After
MultipleResolvesOnMultipleTasks	100	100	646.6 μs	632.3 μs

ConcurrencyNestedScopeBenchmark

Method	ConcurrentRequests	RepeatCount	Mean Before	Mean After
MultipleResolvesOnMultipleTasks	100	10	1.004 ms	954.7 μs

DeepGraphResolveBenchmark

Method	Mean Before	Mean After
Resolve	7.423 μs	7.169 μs

RootContainerResolveBenchmark

Method	Mean Before	Mean After
OperatorNew	2.131 ns	1.990 ns
NonSharedReflectionResolve	208.615 ns	198.548 ns
NonSharedDelegateResolve	205.002 ns	187.762 ns
SharedResolve	165.256 ns	159.633 ns

[alistairjevans]

Great, thanks, sorry for the delay!

[snaumenko-st]

This change actually reverses the order of the Registrations collection unfortunately. However unless somebody depends on that it should be safe.

[alistairjevans]

It was a concurrentbag before right? Which is already technically an unordered set I believe.

[snaumenko-st]

Yes, you are right, but in fact they are selected in reverse order of inserts. So in our case this fact helped to reveal some corner cases of duplicate registrations with different configs, for example. Some unit tests were failing because of that. It's not an issue I believe, but who knows how others are using this interface.