From 7fb611ec5aa9ef9c7080b20a3a386481f873bff6 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 12 Apr 2021 23:34:59 -0700 Subject: [PATCH 1/9] Use a thread static instead of concurrent queue for the scope pool - The ConcurrentQueue showed a 48% regression on AMD scenarios and a 4% regression on citrine linux. - Removed interface dispatch on clear --- .../src/ScopePool.cs | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index b077306e5a5a9f..4c6447537f19b5 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -5,40 +5,42 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; -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 MaxQueueSize = 128; - - private int _count; - private readonly ConcurrentQueue _queue = new(); + // Stash the scope state on a thread static + [ThreadStatic] + private static State t_tlsScopeState; public State Rent() { - if (_queue.TryDequeue(out State state)) + State state = null; + + // Try to get a scope from TLS + if (t_tlsScopeState != null) { - Interlocked.Decrement(ref _count); - return state; + state = t_tlsScopeState; + t_tlsScopeState = null; } - return new State(this); + + return state ?? new State(this); } public bool Return(State state) { - if (Interlocked.Increment(ref _count) > MaxQueueSize) + if (t_tlsScopeState == null) { - Interlocked.Decrement(ref _count); - return false; + // Stash the state back in TLS + state.Clear(); + t_tlsScopeState = state; + + return true; } - state.Clear(); - _queue.Enqueue(state); - return true; + return false; } public class State @@ -61,7 +63,7 @@ internal void Clear() // This should only get called from the pool Debug.Assert(_pool != null); // REVIEW: Should we trim excess here as well? - ResolvedServices.Clear(); + ((Dictionary)ResolvedServices).Clear(); Disposables?.Clear(); } From 4cfb93072b5cffa37a538c9f5669e94994dfa231 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 13 Apr 2021 00:44:39 -0700 Subject: [PATCH 2/9] Add a maximum size --- .../src/ScopePool.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index 4c6447537f19b5..63dc6361ddc62e 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -15,6 +15,9 @@ internal class ScopePool [ThreadStatic] private static State t_tlsScopeState; + // More than 1000 scopes services seems like too many to cache + private const int MaxResolvedServicesCount = 1000; + public State Rent() { State state = null; @@ -31,7 +34,7 @@ public State Rent() public bool Return(State state) { - if (t_tlsScopeState == null) + if (state.Size <= MaxResolvedServicesCount && t_tlsScopeState == null) { // Stash the state back in TLS state.Clear(); @@ -50,6 +53,8 @@ public class State public IDictionary ResolvedServices { get; } public List Disposables { get; set; } + public int Size => ((Dictionary)ResolvedServices).Count; + public State(ScopePool pool = null) { _pool = pool; From fd1e6b3f93173403cef1a6411362c99825d2c8b5 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 13 Apr 2021 02:17:53 -0700 Subject: [PATCH 3/9] Added comment about the size and removed null tlsCache check on return --- .../Microsoft.Extensions.DependencyInjection/src/ScopePool.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index 63dc6361ddc62e..6b853157cf7dfd 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -15,7 +15,7 @@ internal class ScopePool [ThreadStatic] private static State t_tlsScopeState; - // More than 1000 scopes services seems like too many to cache + // More than 1000 scopes services seems like too many to cache (~36KB in size) private const int MaxResolvedServicesCount = 1000; public State Rent() @@ -34,7 +34,7 @@ public State Rent() public bool Return(State state) { - if (state.Size <= MaxResolvedServicesCount && t_tlsScopeState == null) + if (state.Size <= MaxResolvedServicesCount) { // Stash the state back in TLS state.Clear(); From 99d57f341925bb98ecaed81a78ffd302de50e7d1 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 13 Apr 2021 10:08:01 -0700 Subject: [PATCH 4/9] Made some adjustments based on PR feedback - Cache 100 scoped entries maximum - Keep track of the maximum capacity up to a limit --- .../src/ScopePool.cs | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index 6b853157cf7dfd..c7d3fae401cc86 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -14,27 +14,33 @@ internal class ScopePool // Stash the scope state on a thread static [ThreadStatic] private static State t_tlsScopeState; + private volatile int _maxScopeSize; - // More than 1000 scopes services seems like too many to cache (~36KB in size) - private const int MaxResolvedServicesCount = 1000; + // Roughly ~3.6KB + private const int MaxResolvedServicesCount = 100; + + // Below default ~85K LOH threshold, avoid large temporary garbage + private const int MaxCapacity = 2000; public State Rent() { - State state = null; - // Try to get a scope from TLS - if (t_tlsScopeState != null) + State state = t_tlsScopeState; + + if (state != null) { - state = t_tlsScopeState; + // Clear the state t_tlsScopeState = null; } - return state ?? new State(this); + return state ?? new State(this, _maxScopeSize); } public bool Return(State state) { - if (state.Size <= MaxResolvedServicesCount) + var size = state.Size; + + if (size <= MaxResolvedServicesCount) { // Stash the state back in TLS state.Clear(); @@ -43,6 +49,9 @@ public bool Return(State state) return true; } + // Clamp the size between the min and max + _maxScopeSize = Math.Min(MaxCapacity, Math.Max(size, _maxScopeSize)); + return false; } @@ -55,12 +64,12 @@ public class State public int Size => ((Dictionary)ResolvedServices).Count; - public State(ScopePool pool = null) + public State(ScopePool pool = null, int initialCapacity = 0) { _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() : new Dictionary(); + ResolvedServices = pool == null ? new ConcurrentDictionary() : new Dictionary(initialCapacity); } internal void Clear() From 35db644bf5bf3bbccaba4e5f2a12f2dbd777de7e Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 14 Apr 2021 10:24:22 -0700 Subject: [PATCH 5/9] Remove pooling, just track sizes --- .../src/ScopePool.cs | 90 ------------------- .../src/ScopeTracker.cs | 61 +++++++++++++ .../ServiceLookup/ServiceProviderEngine.cs | 4 +- .../ServiceProviderEngineScope.cs | 53 ++++------- .../ServiceProviderEngineScopeTests.cs | 12 --- 5 files changed, 81 insertions(+), 139 deletions(-) delete mode 100644 src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs create mode 100644 src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs deleted file mode 100644 index c7d3fae401cc86..00000000000000 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ /dev/null @@ -1,90 +0,0 @@ -// 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.Diagnostics; -using Microsoft.Extensions.DependencyInjection.ServiceLookup; - -namespace Microsoft.Extensions.DependencyInjection -{ - internal class ScopePool - { - // Stash the scope state on a thread static - [ThreadStatic] - private static State t_tlsScopeState; - private volatile int _maxScopeSize; - - // Roughly ~3.6KB - private const int MaxResolvedServicesCount = 100; - - // Below default ~85K LOH threshold, avoid large temporary garbage - private const int MaxCapacity = 2000; - - public State Rent() - { - // Try to get a scope from TLS - State state = t_tlsScopeState; - - if (state != null) - { - // Clear the state - t_tlsScopeState = null; - } - - return state ?? new State(this, _maxScopeSize); - } - - public bool Return(State state) - { - var size = state.Size; - - if (size <= MaxResolvedServicesCount) - { - // Stash the state back in TLS - state.Clear(); - t_tlsScopeState = state; - - return true; - } - - // Clamp the size between the min and max - _maxScopeSize = Math.Min(MaxCapacity, Math.Max(size, _maxScopeSize)); - - return false; - } - - public class State - { - private readonly ScopePool _pool; - - public IDictionary ResolvedServices { get; } - public List Disposables { get; set; } - - public int Size => ((Dictionary)ResolvedServices).Count; - - public State(ScopePool pool = null, int initialCapacity = 0) - { - _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() : new Dictionary(initialCapacity); - } - - internal void Clear() - { - // This should only get called from the pool - Debug.Assert(_pool != null); - // REVIEW: Should we trim excess here as well? - ((Dictionary)ResolvedServices).Clear(); - Disposables?.Clear(); - } - - public bool Return() - { - return _pool?.Return(this) ?? false; - } - } - } -} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs new file mode 100644 index 00000000000000..b1545a0d5b9a25 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs @@ -0,0 +1,61 @@ +// 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 Microsoft.Extensions.DependencyInjection.ServiceLookup; + +namespace Microsoft.Extensions.DependencyInjection +{ + internal class ScopeTracker + { + // We keep track of the number of the maximum scoped services resolved + private volatile int _maxResolvedServices; + private volatile int _maxDisposableServices; + + // Below default ~85K LOH threshold, avoid large temporary garbage + private const int MaxCapacity = 2000; + + public State Allocate() => new State(this, _maxResolvedServices, _maxDisposableServices); + + private bool Track(State state) + { + // Clamp the size between the min and max + _maxResolvedServices = Math.Min(MaxCapacity, Math.Max(state.ResolvedServicesSize, _maxResolvedServices)); + _maxDisposableServices = Math.Min(MaxCapacity, Math.Max(state.Disposables.Count, _maxDisposableServices)); + + return false; + } + + public class State + { + private readonly ScopeTracker _tracker; + + public IDictionary ResolvedServices { get; } + public List Disposables { get; set; } + + public int ResolvedServicesSize => ((Dictionary)ResolvedServices).Count; + + public State(ScopeTracker tracker = null, int initialCapacity = 0, int initialDisposableCapacity = 0) + { + _tracker = tracker; + + // When tracker is null, we're not tracking the number of resolved services. Also + // to reduce lock contention for singletons upon resolve we use a concurrent dictionary. + + ResolvedServices = tracker == null ? new ConcurrentDictionary() : new Dictionary(initialCapacity); + + if (initialDisposableCapacity > 0) + { + Disposables ??= new List(initialDisposableCapacity); + } + } + + public void Track() + { + _tracker?.Track(this); + } + } + } +} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index 48bab2f5fa5101..e9480f2da6bd59 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -25,10 +25,10 @@ protected ServiceProviderEngine(IEnumerable serviceDescriptor CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite()); CallSiteFactory.Add(typeof(IServiceScopeFactory), new ServiceScopeFactoryCallSite()); RealizedServices = new ConcurrentDictionary>(); - ScopePool = new ScopePool(); + ScopeTracker = new ScopeTracker(); } - internal ScopePool ScopePool { get; } + internal ScopeTracker ScopeTracker { get; } internal ConcurrentDictionary> RealizedServices { get; } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index b29b4016bc0f5b..e66e7be97b0940 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -14,22 +14,20 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid internal Action _captureDisposableCallback; private bool _disposed; - 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, since ResolvedServices is a concurrent dictionary. - // For other scopes, it protects ResolvedServices and the list of disposables - private readonly object _scopeLock = new object(); + private ScopeTracker.State _state; public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) { Engine = engine; - _state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent(); + _state = isRoot ? new ScopeTracker.State() : engine.ScopeTracker.Allocate(); } - internal IDictionary ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed(); + internal IDictionary ResolvedServices => _state.ResolvedServices; - internal object Sync => _scopeLock; + // This lock protects state on the scope, in particular, for the root scope, it protects + // the list of disposable entries only, since ResolvedServices is a concurrent dictionary. + // For other scopes, it protects ResolvedServices and the list of disposables + internal object Sync => _state; public ServiceProviderEngine Engine { get; } @@ -54,7 +52,7 @@ internal object CaptureDisposable(object service) return service; } - lock (_scopeLock) + lock (Sync) { if (_disposed) { @@ -164,40 +162,25 @@ static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask } } - private IDictionary 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) + // Don't attempt to dispose if we're already disposed + if (_disposed) { - // Don't attempt to dispose if we're already disposed - if (_state == null) - { - return; - } + return; + } - // ResolvedServices is never cleared for singletons because there might be a compilation running in background - // trying to get a cached singleton service. If it doesn't find it - // it will try to create a new one which will result in an ObjectDisposedException. + // ResolvedServices is never cleared for singletons because there might be a compilation running in background + // trying to get a cached singleton service. If it doesn't find it + // it will try to create a new one which will result in an ObjectDisposedException. - // 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; - } - } + // Track statistics about the scope (number of disposable objects and number of disposed services) + _state.Track(); } private List BeginDispose() { - lock (_scopeLock) + lock (Sync) { if (_disposed) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs index 223c6bfd7bc8a9..95fde1c98e8eaa 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Collections.Generic; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Xunit; @@ -10,17 +9,6 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { public class ServiceProviderEngineScopeTests { - [Fact] - public void ResolvedServicesAfterDispose_ThrowsObjectDispose() - { - var engine = new FakeEngine(); - var serviceProviderEngineScope = new ServiceProviderEngineScope(engine); - serviceProviderEngineScope.ResolvedServices.Add(new ServiceCacheKey(typeof(IFakeService), 0), null); - serviceProviderEngineScope.Dispose(); - - Assert.Throws(() => serviceProviderEngineScope.ResolvedServices); - } - [Fact] public void DoubleDisposeWorks() { From 6da21a135602809cb4fb45bab7284f65da4920d9 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 15 Apr 2021 07:57:37 -0700 Subject: [PATCH 6/9] Remove pre-sizing logic - Added an event that we can ask customers to turn on to see if scope in real applications are mostly homogeneous --- .../src/DependencyInjectionEventSource.cs | 15 +++++++++ .../src/ScopeTracker.cs | 29 ++++------------ .../ServiceProviderEngineScope.cs | 33 +++++-------------- 3 files changed, 31 insertions(+), 46 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs index fdca10e7696204..f3ebf4823e1a0b 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs @@ -56,6 +56,21 @@ public void DynamicMethodBuilt(string serviceType, int methodSize) WriteEvent(4, serviceType, methodSize); } + [Event(5, Level = EventLevel.Verbose)] + public void ScopeDisposed(int scopedServicesResolved, int disposableServices) + { + WriteEvent(5, scopedServicesResolved, disposableServices); + } + + [NonEvent] + public void ScopeDisposed(ScopeTracker.State state) + { + if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) + { + ScopeDisposed(state.ResolvedServicesCount, state.DisposableServicesCount); + } + } + [NonEvent] public void ServiceResolved(Type serviceType) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs index b1545a0d5b9a25..e1e4fbc9a10f82 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs @@ -10,22 +10,11 @@ namespace Microsoft.Extensions.DependencyInjection { internal class ScopeTracker { - // We keep track of the number of the maximum scoped services resolved - private volatile int _maxResolvedServices; - private volatile int _maxDisposableServices; + public State Allocate() => new State(this); - // Below default ~85K LOH threshold, avoid large temporary garbage - private const int MaxCapacity = 2000; - - public State Allocate() => new State(this, _maxResolvedServices, _maxDisposableServices); - - private bool Track(State state) + private void Track(State state) { - // Clamp the size between the min and max - _maxResolvedServices = Math.Min(MaxCapacity, Math.Max(state.ResolvedServicesSize, _maxResolvedServices)); - _maxDisposableServices = Math.Min(MaxCapacity, Math.Max(state.Disposables.Count, _maxDisposableServices)); - - return false; + DependencyInjectionEventSource.Log.ScopeDisposed(state); } public class State @@ -35,21 +24,17 @@ public class State public IDictionary ResolvedServices { get; } public List Disposables { get; set; } - public int ResolvedServicesSize => ((Dictionary)ResolvedServices).Count; + public int DisposableServicesCount => Disposables?.Count ?? 0; + public int ResolvedServicesCount => ((Dictionary)ResolvedServices).Count; - public State(ScopeTracker tracker = null, int initialCapacity = 0, int initialDisposableCapacity = 0) + public State(ScopeTracker tracker = null) { _tracker = tracker; // When tracker is null, we're not tracking the number of resolved services. Also // to reduce lock contention for singletons upon resolve we use a concurrent dictionary. - ResolvedServices = tracker == null ? new ConcurrentDictionary() : new Dictionary(initialCapacity); - - if (initialDisposableCapacity > 0) - { - Disposables ??= new List(initialDisposableCapacity); - } + ResolvedServices = tracker == null ? new ConcurrentDictionary() : new Dictionary(); } public void Track() diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index e66e7be97b0940..9d268651501d2c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -95,8 +95,6 @@ public void Dispose() } } } - - ClearState(); } public ValueTask DisposeAsync() @@ -115,7 +113,7 @@ public ValueTask DisposeAsync() ValueTask vt = asyncDisposable.DisposeAsync(); if (!vt.IsCompletedSuccessfully) { - return Await(this, i, vt, toDispose); + return Await(i, vt, toDispose); } // If its a IValueTaskSource backed ValueTask, @@ -134,11 +132,9 @@ public ValueTask DisposeAsync() } } - ClearState(); - return default; - static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask vt, List toDispose) + static async ValueTask Await(int i, ValueTask vt, List toDispose) { await vt.ConfigureAwait(false); // vt is acting on the disposable at index i, @@ -157,27 +153,9 @@ static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask ((IDisposable)disposable).Dispose(); } } - - scope.ClearState(); } } - private void ClearState() - { - // Don't attempt to dispose if we're already disposed - if (_disposed) - { - return; - } - - // ResolvedServices is never cleared for singletons because there might be a compilation running in background - // trying to get a cached singleton service. If it doesn't find it - // it will try to create a new one which will result in an ObjectDisposedException. - - // Track statistics about the scope (number of disposable objects and number of disposed services) - _state.Track(); - } - private List BeginDispose() { lock (Sync) @@ -187,11 +165,18 @@ private List BeginDispose() return null; } + // Track statistics about the scope (number of disposable objects and number of disposed services) + _state.Track(); + // 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; + // ResolvedServices is never cleared for singletons because there might be a compilation running in background + // trying to get a cached singleton service. If it doesn't find it + // it will try to create a new one which will result in an ObjectDisposedException. + return _state.Disposables; } } From 629f04314a54df87f8b816bc65bf77b867b985f0 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 15 Apr 2021 08:22:21 -0700 Subject: [PATCH 7/9] Add the hashcode of the tracker so that we can detect scope sizes per container instance. --- .../src/DependencyInjectionEventSource.cs | 8 ++++---- .../src/ScopeTracker.cs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs index f3ebf4823e1a0b..1ed26cd69d6534 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs @@ -57,17 +57,17 @@ public void DynamicMethodBuilt(string serviceType, int methodSize) } [Event(5, Level = EventLevel.Verbose)] - public void ScopeDisposed(int scopedServicesResolved, int disposableServices) + public void ScopeDisposed(int trackerHashCode, int scopedServicesResolved, int disposableServices) { - WriteEvent(5, scopedServicesResolved, disposableServices); + WriteEvent(5, trackerHashCode, scopedServicesResolved, disposableServices); } [NonEvent] - public void ScopeDisposed(ScopeTracker.State state) + public void ScopeDisposed(ScopeTracker tracker, ScopeTracker.State state) { if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) { - ScopeDisposed(state.ResolvedServicesCount, state.DisposableServicesCount); + ScopeDisposed(tracker.GetHashCode(), state.ResolvedServicesCount, state.DisposableServicesCount); } } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs index e1e4fbc9a10f82..f0e809285202d8 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs @@ -14,7 +14,7 @@ internal class ScopeTracker private void Track(State state) { - DependencyInjectionEventSource.Log.ScopeDisposed(state); + DependencyInjectionEventSource.Log.ScopeDisposed(this, state); } public class State From 41603ca0a25210729f66c5db9a5a2ab4f60f5e8a Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 15 Apr 2021 08:47:13 -0700 Subject: [PATCH 8/9] Simplify --- .../src/DependencyInjectionEventSource.cs | 8 ++-- .../src/ScopeState.cs | 30 ++++++++++++ .../src/ScopeTracker.cs | 46 ------------------- .../ServiceLookup/ServiceProviderEngine.cs | 3 -- .../ServiceProviderEngineScope.cs | 6 +-- 5 files changed, 37 insertions(+), 56 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs delete mode 100644 src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs index 1ed26cd69d6534..19616694cd0be1 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs @@ -57,17 +57,17 @@ public void DynamicMethodBuilt(string serviceType, int methodSize) } [Event(5, Level = EventLevel.Verbose)] - public void ScopeDisposed(int trackerHashCode, int scopedServicesResolved, int disposableServices) + public void ScopeDisposed(int serviceProviderHashCode, int scopedServicesResolved, int disposableServices) { - WriteEvent(5, trackerHashCode, scopedServicesResolved, disposableServices); + WriteEvent(5, serviceProviderHashCode, scopedServicesResolved, disposableServices); } [NonEvent] - public void ScopeDisposed(ScopeTracker tracker, ScopeTracker.State state) + public void ScopeDisposed(ServiceProviderEngine engine, ScopeState state) { if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) { - ScopeDisposed(tracker.GetHashCode(), state.ResolvedServicesCount, state.DisposableServicesCount); + ScopeDisposed(engine.GetHashCode(), state.ResolvedServicesCount, state.DisposableServicesCount); } } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs new file mode 100644 index 00000000000000..48003ee002caa1 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs @@ -0,0 +1,30 @@ +// 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 Microsoft.Extensions.DependencyInjection.ServiceLookup; + +namespace Microsoft.Extensions.DependencyInjection +{ + internal class ScopeState + { + public IDictionary ResolvedServices { get; } + public List Disposables { get; set; } + + public int DisposableServicesCount => Disposables?.Count ?? 0; + public int ResolvedServicesCount => ((Dictionary)ResolvedServices).Count; + + public ScopeState(bool isRoot) + { + // When isRoot is true to reduce lock contention for singletons upon resolve we use a concurrent dictionary. + ResolvedServices = isRoot ? new ConcurrentDictionary() : new Dictionary(); + } + + public void Track(ServiceProviderEngine engine) + { + DependencyInjectionEventSource.Log.ScopeDisposed(engine, this); + } + } +} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs deleted file mode 100644 index f0e809285202d8..00000000000000 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeTracker.cs +++ /dev/null @@ -1,46 +0,0 @@ -// 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 Microsoft.Extensions.DependencyInjection.ServiceLookup; - -namespace Microsoft.Extensions.DependencyInjection -{ - internal class ScopeTracker - { - public State Allocate() => new State(this); - - private void Track(State state) - { - DependencyInjectionEventSource.Log.ScopeDisposed(this, state); - } - - public class State - { - private readonly ScopeTracker _tracker; - - public IDictionary ResolvedServices { get; } - public List Disposables { get; set; } - - public int DisposableServicesCount => Disposables?.Count ?? 0; - public int ResolvedServicesCount => ((Dictionary)ResolvedServices).Count; - - public State(ScopeTracker tracker = null) - { - _tracker = tracker; - - // When tracker is null, we're not tracking the number of resolved services. Also - // to reduce lock contention for singletons upon resolve we use a concurrent dictionary. - - ResolvedServices = tracker == null ? new ConcurrentDictionary() : new Dictionary(); - } - - public void Track() - { - _tracker?.Track(this); - } - } - } -} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index e9480f2da6bd59..d6d804c6484482 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -25,11 +25,8 @@ protected ServiceProviderEngine(IEnumerable serviceDescriptor CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite()); CallSiteFactory.Add(typeof(IServiceScopeFactory), new ServiceScopeFactoryCallSite()); RealizedServices = new ConcurrentDictionary>(); - ScopeTracker = new ScopeTracker(); } - internal ScopeTracker ScopeTracker { get; } - internal ConcurrentDictionary> RealizedServices { get; } internal CallSiteFactory CallSiteFactory { get; } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 9d268651501d2c..ab31fabff48e4a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -14,12 +14,12 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid internal Action _captureDisposableCallback; private bool _disposed; - private ScopeTracker.State _state; + private readonly ScopeState _state; public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) { Engine = engine; - _state = isRoot ? new ScopeTracker.State() : engine.ScopeTracker.Allocate(); + _state = new ScopeState(isRoot); } internal IDictionary ResolvedServices => _state.ResolvedServices; @@ -166,7 +166,7 @@ private List BeginDispose() } // Track statistics about the scope (number of disposable objects and number of disposed services) - _state.Track(); + _state.Track(Engine); // We've transitioned to the disposed state, so future calls to // CaptureDisposable will immediately dispose the object. From ffb7a2c887cecd459a313d3d826a80b92f0bcef2 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 15 Apr 2021 08:48:13 -0700 Subject: [PATCH 9/9] Remove cast --- .../Microsoft.Extensions.DependencyInjection/src/ScopeState.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs index 48003ee002caa1..edc06906a8e2d0 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs @@ -14,7 +14,7 @@ internal class ScopeState public List Disposables { get; set; } public int DisposableServicesCount => Disposables?.Count ?? 0; - public int ResolvedServicesCount => ((Dictionary)ResolvedServices).Count; + public int ResolvedServicesCount => ResolvedServices.Count; public ScopeState(bool isRoot) {