Pool the underlying list and dictionary in scopes. by davidfowl · Pull Request #50463 · dotnet/runtime

davidfowl · 2021-03-31T01:45:55Z

Today DI scopes allocate a dictionary and an optional list of disposables per request. That dictionary starts off with the default capacity and resizes as scoped services are resolved. Depending on how many scoped services are resolved, this can end up with many resizes per request which result in new array allocations. This change pools those 2 lists assuming that short lived scopes look mostly the same (same number of services resolved and disposed per scope instance).
This change pools a set of scopes assuming they are short lived.
One breaking change is that after disposal, pooled scopes will throw if services are accessed afterwards on the scope.
We could make this an opt in flag but, I'm not sure if it it makes sense. Accessing a scope after it's disposed is broken anyways.

~~PS: Needs testing, getting early feedback.~~
~~TODO: Performance tests~~

Update: @sebastienros confirmed it fixes the issue.

- This change pools a set of scopes assuming they are short lived. - One breaking change is that after disposal, pooled scopes will throw if services are accessed afterwards on the scope.

ghost · 2021-03-31T01:46:00Z

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

Issue Details

This change pools a set of scopes assuming they are short lived.
One breaking change is that after disposal, pooled scopes will throw if services are accessed afterwards on the scope.

PS: Needs testing, getting early feedback.

Author:	davidfowl
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`
Milestone:	-

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

- Allocate the lock. Scoped services can be resolved without a scope when the scope validation flag is off. - Assert the correct lock - Modified test to throw after dispose

...s/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

pakrym · 2021-03-31T15:55:28Z

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

+
+namespace Microsoft.Extensions.DependencyInjection
+{
+    internal class ScopePool


How many similar pool implementations do we have across the codebase? Worth having something reusable?

Getting something that we can actually share internally is probably a good first test for any proposed ObjectPool<T> API. (#49680)

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

...s/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

...s/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

pakrym · 2021-03-31T16:10:56Z

Did you consider pooling entire ServiceProviderEngineScope objects? Or is that too scary?

pakrym · 2021-03-31T16:18:00Z

LGTM, nice and clean.

davidfowl · 2021-03-31T16:44:38Z

Did you consider pooling entire ServiceProviderEngineScope objects? Or is that too scary?

I started that way, but it did seem a little scary and might need to be opt-in if we went that far. I'd like to reduce the object size event more (if possible) so scopes are basically a super lightweight wrapper around this possibly reusable state.

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

...s/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs

maryamariyan · 2021-04-01T15:04:16Z

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

+        {
+            // We lock here since ResolvedServices is always accessed in the scope lock, this means we'll never
+            // try to return to the pool while somebody is trying to access ResolvedServices.
+            lock (_scopeLock)


this method seems to be a noop for root scope, would it make sense to skip for it before taking the lock?

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

maryamariyan · 2021-04-01T15:23:59Z

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

                }
            }

+            ClearState();


Would it make sense to add this ClearState() call on a finally block to the try-catch block above it or it doesnt make a difference?

Yes it would.

Actually, it's probably better not to try and return this scope to the pool if dispose throws.

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

BrennanConroy · 2021-04-01T17:56:23Z

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs

+                return false;
+            }
+
+            state.Clear();


Should we always call Clear?

BrennanConroy · 2021-04-01T18:07:08Z

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

+                // This will return false if the pool is full or if this state object is the root scope
+                if (_state.Return())
+                {
+                    _state = null;


What's the reason for not setting this to null in all cases?

Singletons, and the background compilation thread that uses them. Also it isn't pooled.

eerhardt

I think this looks good. Nice work here!

jkotas · 2021-04-02T00:31:23Z

TODO: Performance tests

Any performance numbers?

sebastienros · 2021-04-02T01:26:18Z

@jkotas on the same application as in the issue I filed, the allocations disappeared from the list. They represented 13% of the total allocations of the app (150MB /sec), and with this branch there were less than 1% AFAIR.

davidfowl · 2021-04-02T01:29:49Z

We (@sebastienros and I) ran a couple of the TE benchmarks to make sure nothing regressed. We saw a very minor RPS improvement and a reduction in allocation rate. In orchard, the allocations basically disappeared from the trace from being the top one but RPS didn't improve much.

Here's a microbenchmark with 10 dependent scoped services:

Before

|         Method |     Mean |   Error |  StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------- |---------:|--------:|--------:|-------:|------:|------:|----------:|
| ScopedServices | 667.8 ns | 9.00 ns | 8.42 ns | 0.3662 |     - |     - |    1.5 KB |

After

|         Method |     Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------- |---------:|---------:|---------:|-------:|------:|------:|----------:|
| ScopedServices | 599.1 ns | 10.60 ns | 12.20 ns | 0.0763 |     - |     - |     320 B |

This benchmark needs more concurrency to represent what ends up happening per request in ASP.NET Core though (where we've mostly removed scoped services).

jkotas · 2021-04-02T01:45:15Z

Thanks.

one but RPS didn't improve much.

This is fairly common for pools like this - they replace GC allocations with cache trashing.

davidfowl · 2021-04-02T03:27:34Z

@jkotas do we have an easy way to measure that? Or do we need a tool like vtune?

jkotas · 2021-04-02T03:50:31Z

Yep, vtune would be a tool to investigate this.

Pool the underlying list and dictionary in scopes.

0c4bea0

- This change pools a set of scopes assuming they are short lived. - One breaking change is that after disposal, pooled scopes will throw if services are accessed afterwards on the scope.

ghost added the area-Extensions-DependencyInjection label Mar 31, 2021

davidfowl requested a review from maryamariyan March 31, 2021 01:45

davidfowl requested review from eerhardt and pakrym March 31, 2021 01:46

davidfowl commented Mar 31, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Outdated Show resolved Hide resolved

davidfowl added 2 commits March 30, 2021 18:57

Fixed race

6569493

More changes

ec75093

- Allocate the lock. Scoped services can be resolved without a scope when the scope validation flag is off. - Assert the correct lock - Modified test to throw after dispose

davidfowl commented Mar 31, 2021

View reviewed changes

...s/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

...s/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs Show resolved Hide resolved

pakrym reviewed Mar 31, 2021

View reviewed changes

...s/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs Outdated Show resolved Hide resolved

PR feedback

8ecde74

davidfowl marked this pull request as ready for review March 31, 2021 23:33

halter73 reviewed Apr 1, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 1, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 1, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 1, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 1, 2021

View reviewed changes

...s/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs Show resolved Hide resolved

maryamariyan reviewed Apr 1, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs Show resolved Hide resolved

maryamariyan reviewed Apr 1, 2021

View reviewed changes

eerhardt reviewed Apr 1, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs Show resolved Hide resolved

PR feedback

aaf95cf

BrennanConroy reviewed Apr 1, 2021

View reviewed changes

eerhardt approved these changes Apr 1, 2021

View reviewed changes

halter73 approved these changes Apr 1, 2021

View reviewed changes

davidfowl merged commit c24bf80 into main Apr 2, 2021

davidfowl added this to the 6.0.0 milestone Apr 2, 2021

jkotas deleted the davidfowl/pool-scoped-dict branch April 2, 2021 01:45

smitpatel mentioned this pull request Apr 5, 2021

[main] Update dependencies from dotnet/runtime dotnet/efcore#24583

Merged

TanayParikh mentioned this pull request Apr 5, 2021

Skipped Test DbContextPoolingTest.Double_dispose_does_not_enter_pool_twice dotnet/efcore#24590

Closed

sebastienros mentioned this pull request Apr 7, 2021

[main] Update dependencies from dotnet/emsdk dotnet/efcore dotnet/runtime dotnet/aspnetcore#31576

Merged

davidfowl mentioned this pull request Apr 7, 2021

Disposing scopes more than once should work #50852

Merged

halter73 mentioned this pull request Apr 14, 2021

Performance difference: mvc, mvc dotnet/aspnetcore#31703

Closed

ghost locked as resolved and limited conversation to collaborators May 2, 2021

Conversation

davidfowl commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pakrym commented Mar 31, 2021

Uh oh!

pakrym commented Mar 31, 2021

Uh oh!

davidfowl commented Mar 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maryamariyan Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Apr 2, 2021

Uh oh!

sebastienros commented Apr 2, 2021

Uh oh!

davidfowl commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

jkotas commented Apr 2, 2021

Uh oh!

davidfowl commented Apr 2, 2021

Uh oh!

jkotas commented Apr 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

davidfowl commented Mar 31, 2021 •

edited

Loading

halter73 Apr 1, 2021 •

edited

Loading

maryamariyan Apr 1, 2021 •

edited

Loading

davidfowl commented Apr 2, 2021 •

edited

Loading