-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Pool the underlying list and dictionary in scopes. #50463
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Threading; | ||
using Microsoft.Extensions.DependencyInjection.ServiceLookup; | ||
|
||
namespace Microsoft.Extensions.DependencyInjection | ||
{ | ||
internal class ScopePool | ||
{ | ||
// Modest number to re-use. We only really care about reuse for short lived scopes | ||
private const int s_maxQueueSize = 128; | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private int _count; | ||
private readonly ConcurrentQueue<State> _queue = new(); | ||
|
||
public State Rent() | ||
{ | ||
if (_queue.TryDequeue(out State state)) | ||
{ | ||
Interlocked.Decrement(ref _count); | ||
return state; | ||
} | ||
return new State(this); | ||
} | ||
|
||
public bool Return(State state) | ||
{ | ||
if (Interlocked.Increment(ref _count) > s_maxQueueSize) | ||
{ | ||
Interlocked.Decrement(ref _count); | ||
return false; | ||
} | ||
|
||
state.Clear(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we always call Clear? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? |
||
_queue.Enqueue(state); | ||
return true; | ||
} | ||
|
||
public class State | ||
{ | ||
private readonly ScopePool _pool; | ||
|
||
public IDictionary<ServiceCacheKey, object> ResolvedServices { get; } | ||
public List<object> Disposables { get; set; } | ||
|
||
public State(ScopePool pool = null) | ||
{ | ||
_pool = pool; | ||
// When pool is null, we're in the global scope which doesn't need pooling. | ||
// To reduce lock contention for singletons upon resolve we use a concurrent dictionary. | ||
ResolvedServices = pool == null ? new ConcurrentDictionary<ServiceCacheKey, object>() : new Dictionary<ServiceCacheKey, object>(); | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
internal void Clear() | ||
{ | ||
// REVIEW: Should we trim excess here as well? | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
eerhardt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ResolvedServices.Clear(); | ||
Disposables?.Clear(); | ||
} | ||
|
||
public bool Return() | ||
{ | ||
return _pool?.Return(this) ?? false; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,7 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Threading.Tasks; | ||
using Microsoft.Extensions.Internal; | ||
|
||
|
@@ -15,20 +13,23 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid | |
// For testing only | ||
internal Action<object> _captureDisposableCallback; | ||
|
||
private List<object> _disposables; | ||
|
||
private bool _disposed; | ||
private readonly object _disposelock = new object(); | ||
private ScopePool.State _state; | ||
|
||
// This lock protects state on the scope, in particular, for the root scope, it protects | ||
// the list of disposable entries only as ResolvedServices is a concurrent dictionary | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// For other scopes, it protects ResolvedServices and the list of disposables | ||
private readonly object _scopeLock = new object(); | ||
|
||
public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) | ||
{ | ||
Engine = engine; | ||
|
||
// To reduce lock contention for singletons upon resolve we use a concurrent dictionary. | ||
ResolvedServices = isRoot ? new ConcurrentDictionary<ServiceCacheKey, object>() : new Dictionary<ServiceCacheKey, object>(); | ||
_state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent(); | ||
} | ||
|
||
internal IDictionary<ServiceCacheKey, object> ResolvedServices { get; } | ||
internal IDictionary<ServiceCacheKey, object> ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed(); | ||
maryamariyan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
internal object Sync => _scopeLock; | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public ServiceProviderEngine Engine { get; } | ||
|
||
|
@@ -53,7 +54,7 @@ internal object CaptureDisposable(object service) | |
return service; | ||
} | ||
|
||
lock (_disposelock) | ||
lock (_scopeLock) | ||
{ | ||
if (_disposed) | ||
{ | ||
|
@@ -66,16 +67,17 @@ internal object CaptureDisposable(object service) | |
// sync over async, for the rare case that an object only implements IAsyncDisposable and may end up starving the thread pool. | ||
Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask()).GetAwaiter().GetResult(); | ||
} | ||
|
||
ThrowHelper.ThrowObjectDisposedException(); | ||
} | ||
|
||
if (_disposables == null) | ||
{ | ||
_disposables = new List<object>(); | ||
return service; | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
_disposables.Add(service); | ||
_state.Disposables ??= new List<object>(); | ||
|
||
_state.Disposables.Add(service); | ||
} | ||
|
||
return service; | ||
} | ||
|
||
|
@@ -97,6 +99,8 @@ public void Dispose() | |
} | ||
} | ||
} | ||
|
||
ClearState(); | ||
} | ||
|
||
public ValueTask DisposeAsync() | ||
|
@@ -115,7 +119,7 @@ public ValueTask DisposeAsync() | |
ValueTask vt = asyncDisposable.DisposeAsync(); | ||
if (!vt.IsCompletedSuccessfully) | ||
{ | ||
return Await(i, vt, toDispose); | ||
return Await(this, i, vt, toDispose); | ||
} | ||
|
||
// If its a IValueTaskSource backed ValueTask, | ||
|
@@ -134,9 +138,11 @@ public ValueTask DisposeAsync() | |
} | ||
} | ||
|
||
ClearState(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it would. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it's probably better not to try and return this scope to the pool if dispose throws. |
||
|
||
return default; | ||
|
||
static async ValueTask Await(int i, ValueTask vt, List<object> toDispose) | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask vt, List<object> toDispose) | ||
{ | ||
await vt.ConfigureAwait(false); | ||
// vt is acting on the disposable at index i, | ||
|
@@ -155,30 +161,53 @@ static async ValueTask Await(int i, ValueTask vt, List<object> toDispose) | |
((IDisposable)disposable).Dispose(); | ||
} | ||
} | ||
|
||
scope.ClearState(); | ||
} | ||
} | ||
|
||
private IDictionary<ServiceCacheKey, object> ScopeDisposed() | ||
{ | ||
ThrowHelper.ThrowObjectDisposedException(); | ||
return null; | ||
} | ||
|
||
private void ClearState() | ||
{ | ||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this method seems to be a noop for root scope, would it make sense to skip for it before taking the lock? |
||
{ | ||
// Dispose the state, which will end up attempting to return the state pool. | ||
// This will return false if the pool is full or if this state object is the root scope | ||
if (_state.Return()) | ||
{ | ||
_state = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for not setting this to null in all cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Singletons, and the background compilation thread that uses them. Also it isn't pooled. |
||
} | ||
} | ||
} | ||
|
||
private List<object> BeginDispose() | ||
{ | ||
List<object> toDispose; | ||
lock (_disposelock) | ||
lock (_scopeLock) | ||
{ | ||
if (_disposed) | ||
{ | ||
return null; | ||
} | ||
|
||
// We've transitioned to the disposed state, so future calls to | ||
// CaptureDisposable will immediately dispose the object. | ||
// No further changes to _state.Disposables, are allowed. | ||
_disposed = true; | ||
toDispose = _disposables; | ||
_disposables = null; | ||
|
||
return _state.Disposables; | ||
|
||
// Not clearing ResolvedServices here because there might be a compilation running in background | ||
// trying to get a cached singleton service instance and if it won't find | ||
// it it will try to create a new one tripping the Debug.Assert in CaptureDisposable | ||
// and leaking a Disposable object in Release mode | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return toDispose; | ||
} | ||
} | ||
} |
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.
How many similar pool implementations do we have across the codebase? Worth having something reusable?
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.
Getting something that we can actually share internally is probably a good first test for any proposed
ObjectPool<T>
API. (#49680)