From 6a0ad267242bbcb19657403616e7352735033d8e Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 14:19:17 +0100 Subject: [PATCH 01/10] Reduce DI Allocations Resolves #288 --- .../ServiceLookup/ServiceScope.cs | 17 +++++++++++++ .../ServiceLookup/ServiceScopeFactory.cs | 24 ++++++++++++++++++- .../ServiceProvider.cs | 18 +++++++------- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs index 8cf3348b..21cf797d 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs @@ -8,20 +8,37 @@ namespace Microsoft.Framework.DependencyInjection.ServiceLookup internal class ServiceScope : IServiceScope { private readonly ServiceProvider _scopedProvider; + private readonly ServiceScopeFactory _parentFactory; public ServiceScope(ServiceProvider scopedProvider) { _scopedProvider = scopedProvider; } + internal ServiceScope(ServiceProvider scopedProvider, ServiceScopeFactory parentFactory) + { + _scopedProvider = scopedProvider; + _parentFactory = parentFactory; + } public IServiceProvider ServiceProvider { get { return _scopedProvider; } } + internal void Reset() + { + // Double wash + _scopedProvider.Dispose(); + } + public void Dispose() { _scopedProvider.Dispose(); + + if (_parentFactory != null) + { + _parentFactory.PoolScope(this); + } } } } diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs index fc9cb773..cb2dd83e 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs @@ -1,10 +1,16 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Collections.Concurrent; + namespace Microsoft.Framework.DependencyInjection.ServiceLookup { internal class ServiceScopeFactory : IServiceScopeFactory { + private static int _maxPooled = 32 * Environment.ProcessorCount; + private static readonly ConcurrentQueue _scopePool = new ConcurrentQueue(); + private readonly ServiceProvider _provider; public ServiceScopeFactory(ServiceProvider provider) @@ -14,7 +20,23 @@ public ServiceScopeFactory(ServiceProvider provider) public IServiceScope CreateScope() { - return new ServiceScope(new ServiceProvider(_provider)); + ServiceScope scope; + if (_scopePool.TryDequeue(out scope)) + { + scope.Reset(); + return scope; + } + return new ServiceScope(new ServiceProvider(_provider), this); + } + + internal void PoolScope(ServiceScope scope) + { + // Benign race condition + if (_scopePool.Count < _maxPooled) + { + _scopePool.Enqueue(scope); + } } + } } diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index b07b4dbf..b3d7510a 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -19,12 +19,12 @@ internal class ServiceProvider : IServiceProvider, IDisposable { private readonly object _sync = new object(); - private readonly Func> _createServiceAccessor; + private Func> _createServiceAccessor; private readonly ServiceProvider _root; private readonly ServiceTable _table; private readonly Dictionary _resolvedServices = new Dictionary(); - private ConcurrentBag _disposables = new ConcurrentBag(); + private ConcurrentQueue _disposables = new ConcurrentQueue(); public ServiceProvider(IEnumerable serviceDescriptors) { @@ -145,15 +145,13 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi public void Dispose() { - var disposables = Interlocked.Exchange(ref _disposables, null); - - if (disposables != null) + IDisposable disposable; + while (_disposables.TryDequeue(out disposable)) { - foreach (var disposable in disposables) - { - disposable.Dispose(); - } + disposable.Dispose(); } + _createServiceAccessor = null; + _resolvedServices.Clear(); } private object CaptureDisposable(object service) @@ -163,7 +161,7 @@ private object CaptureDisposable(object service) var disposable = service as IDisposable; if (disposable != null) { - _disposables.Add(disposable); + _disposables.Enqueue(disposable); } } return service; From 3f44164cf74d7672f51fe649296feb93f6bad1e2 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 14:30:19 +0100 Subject: [PATCH 02/10] Don't clear _createServiceAccessor Or should it be reinitalised each time? --- src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index b3d7510a..e5f3b108 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -150,7 +150,6 @@ public void Dispose() { disposable.Dispose(); } - _createServiceAccessor = null; _resolvedServices.Clear(); } From a5746e4733b1da5b5254563749ed793e5e002144 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 18:49:08 +0100 Subject: [PATCH 03/10] Restore readonly --- src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index e5f3b108..b5b2b734 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -19,7 +19,7 @@ internal class ServiceProvider : IServiceProvider, IDisposable { private readonly object _sync = new object(); - private Func> _createServiceAccessor; + private readonly Func> _createServiceAccessor; private readonly ServiceProvider _root; private readonly ServiceTable _table; From 572fc71289c547214969805f5d4a4432a6d0605f Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 18:58:57 +0100 Subject: [PATCH 04/10] Bag back rather than Queue --- .../ServiceProvider.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index b5b2b734..7f765f1c 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -24,7 +24,7 @@ internal class ServiceProvider : IServiceProvider, IDisposable private readonly ServiceTable _table; private readonly Dictionary _resolvedServices = new Dictionary(); - private ConcurrentQueue _disposables = new ConcurrentQueue(); + private ConcurrentBag _disposables = new ConcurrentBag(); public ServiceProvider(IEnumerable serviceDescriptors) { @@ -146,7 +146,7 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi public void Dispose() { IDisposable disposable; - while (_disposables.TryDequeue(out disposable)) + while (_disposables.TryTake(out disposable)) { disposable.Dispose(); } @@ -160,7 +160,7 @@ private object CaptureDisposable(object service) var disposable = service as IDisposable; if (disposable != null) { - _disposables.Enqueue(disposable); + _disposables.Add(disposable); } } return service; From 09e07f714cd35e008e3cee141d961633ec0effb4 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 20:09:24 +0100 Subject: [PATCH 05/10] review changes as per @rynowak comments --- .../ServiceLookup/ServiceScope.cs | 8 ++------ .../ServiceLookup/ServiceScopeFactory.cs | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs index 21cf797d..694228e5 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs @@ -9,12 +9,8 @@ internal class ServiceScope : IServiceScope { private readonly ServiceProvider _scopedProvider; private readonly ServiceScopeFactory _parentFactory; - - public ServiceScope(ServiceProvider scopedProvider) - { - _scopedProvider = scopedProvider; - } - internal ServiceScope(ServiceProvider scopedProvider, ServiceScopeFactory parentFactory) + + public ServiceScope(ServiceProvider scopedProvider, ServiceScopeFactory parentFactory) { _scopedProvider = scopedProvider; _parentFactory = parentFactory; diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs index cb2dd83e..f830e9b4 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs @@ -8,7 +8,7 @@ namespace Microsoft.Framework.DependencyInjection.ServiceLookup { internal class ServiceScopeFactory : IServiceScopeFactory { - private static int _maxPooled = 32 * Environment.ProcessorCount; + private static int _capacity = 32 * Environment.ProcessorCount; private static readonly ConcurrentQueue _scopePool = new ConcurrentQueue(); private readonly ServiceProvider _provider; @@ -32,7 +32,7 @@ public IServiceScope CreateScope() internal void PoolScope(ServiceScope scope) { // Benign race condition - if (_scopePool.Count < _maxPooled) + if (_scopePool.Count < _capacity) { _scopePool.Enqueue(scope); } From 7194bcbf4589ea117bf85a6e64258e129e9ac025 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 20:42:49 +0100 Subject: [PATCH 06/10] Single dispose, backing disposable, throw errors --- .../ServiceLookup/ServiceScope.cs | 3 +-- .../ServiceProvider.cs | 24 +++++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs index 694228e5..111b0f49 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScope.cs @@ -23,8 +23,7 @@ public IServiceProvider ServiceProvider internal void Reset() { - // Double wash - _scopedProvider.Dispose(); + _scopedProvider.Reset(); } public void Dispose() diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index 7f765f1c..224f8146 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -25,6 +25,7 @@ internal class ServiceProvider : IServiceProvider, IDisposable private readonly Dictionary _resolvedServices = new Dictionary(); private ConcurrentBag _disposables = new ConcurrentBag(); + private ConcurrentBag _offlineDisposables; public ServiceProvider(IEnumerable serviceDescriptors) { @@ -143,14 +144,29 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi } } + internal void Reset() + { + if (_offlineDisposables == null) + { + throw new InvalidOperationException("ServiceProvider reused before being disposed"); + } + _disposables = _offlineDisposables; + _offlineDisposables = null; + } + public void Dispose() { - IDisposable disposable; - while (_disposables.TryTake(out disposable)) + var disposables = Interlocked.Exchange(ref _disposables, null); + if(disposables != null) { - disposable.Dispose(); + IDisposable disposable; + while (_disposables.TryTake(out disposable)) + { + disposable.Dispose(); + } + _resolvedServices.Clear(); + _offlineDisposables = disposables; } - _resolvedServices.Clear(); } private object CaptureDisposable(object service) From 1b0d07b7c172f4c27d1bb1acbd63c620e80e29f3 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 14 Sep 2015 20:47:00 +0100 Subject: [PATCH 07/10] Empty correct object --- .../ServiceProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index 224f8146..d84fc6c7 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -157,10 +157,10 @@ internal void Reset() public void Dispose() { var disposables = Interlocked.Exchange(ref _disposables, null); - if(disposables != null) + if (disposables != null) { IDisposable disposable; - while (_disposables.TryTake(out disposable)) + while (disposables.TryTake(out disposable)) { disposable.Dispose(); } From 9bef883cc9630f3a69a4f9c9531e32c02f1be659 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Tue, 15 Sep 2015 00:19:06 +0100 Subject: [PATCH 08/10] Use resx for exception, make internal method public --- .../Properties/Resources.Designer.cs | 8 ++++++++ .../Resources.resx | 3 +++ .../ServiceProvider.cs | 6 ++++-- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/Properties/Resources.Designer.cs b/src/Microsoft.Framework.DependencyInjection/Properties/Resources.Designer.cs index ce83a23a..fc40e62f 100644 --- a/src/Microsoft.Framework.DependencyInjection/Properties/Resources.Designer.cs +++ b/src/Microsoft.Framework.DependencyInjection/Properties/Resources.Designer.cs @@ -106,6 +106,14 @@ internal static string FormatTypeCannotBeActivated(object p0, object p1) return string.Format(CultureInfo.CurrentCulture, GetString("TypeCannotBeActivated"), p0, p1); } + /// + /// Pooled type '{0}' reused before correct disposal. + /// + internal static string FormatPooledTypeReusedBeforeDisposal(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("PooledTypeReusedBeforeDisposal"), p0); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.Framework.DependencyInjection/Resources.resx b/src/Microsoft.Framework.DependencyInjection/Resources.resx index 5cfd5693..9ad3ddd4 100644 --- a/src/Microsoft.Framework.DependencyInjection/Resources.resx +++ b/src/Microsoft.Framework.DependencyInjection/Resources.resx @@ -137,4 +137,7 @@ Cannot instantiate implementation type '{0}' for service type '{1}'. + + Pooled type '{0}' reused before correct disposal. + \ No newline at end of file diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index d84fc6c7..b250bd49 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -144,11 +144,13 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi } } - internal void Reset() + public void Reset() { if (_offlineDisposables == null) { - throw new InvalidOperationException("ServiceProvider reused before being disposed"); + throw new InvalidOperationException( + Resources.FormatPooledTypeReusedBeforeDisposal( + nameof(ServiceProvider))); } _disposables = _offlineDisposables; _offlineDisposables = null; From 556009641a58730a2d894e842febaa712132e38d Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Wed, 16 Sep 2015 03:18:23 +0100 Subject: [PATCH 09/10] Added Pool Test --- .../ServiceProvider.cs | 2 +- .../ScopingContainerTestBase.cs | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index b250bd49..2be92ff9 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -144,7 +144,7 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi } } - public void Reset() + internal void Reset() { if (_offlineDisposables == null) { diff --git a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs index 465756f5..7bbfb0eb 100644 --- a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs +++ b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs @@ -88,6 +88,34 @@ public void DisposingScopeDisposesService() Assert.True(disposableService.Disposed); } + [Fact] + public void PooledDisposedScopesResolveDifferentInstances() + { + var container = CreateContainer(); + FakeService disposableService1; + FakeService disposableService2; + + var scopeFactory = container.GetService(); + using (var scope = scopeFactory.CreateScope()) + { + disposableService1 = (FakeService)scope.ServiceProvider.GetService(); + + Assert.False(disposableService1.Disposed); + } + + Assert.True(disposableService1.Disposed); + + using (var scope = scopeFactory.CreateScope()) + { + disposableService2 = (FakeService)scope.ServiceProvider.GetService(); + + Assert.False(disposableService2.Disposed); + } + + Assert.True(disposableService2.Disposed); + Assert.NotEqual(disposableService1, disposableService2); + } + [Fact] public void SingletonServicesComeFromRootContainer() { From c64fb6099806b835cb622740356efe3da5054757 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 17 Sep 2015 15:10:34 +0100 Subject: [PATCH 10/10] Make _halfCapacity actually half --- .../ServiceLookup/ServiceScopeFactory.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs index 1835d852..2f4fcca3 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceLookup/ServiceScopeFactory.cs @@ -8,14 +8,16 @@ namespace Microsoft.Framework.DependencyInjection.ServiceLookup { internal class ServiceScopeFactory : IServiceScopeFactory { - private int _capacity = 32 * Environment.ProcessorCount; - private int _halfCapacity = 32 * Environment.ProcessorCount; + private readonly int _capacity; + private readonly int _halfCapacity; private readonly ConcurrentQueue _scopePool = new ConcurrentQueue(); private readonly ServiceProvider _provider; public ServiceScopeFactory(ServiceProvider provider) { + _capacity = 32 * Environment.ProcessorCount; + _halfCapacity = _capacity / 2; _provider = provider; }