From 189b2574aa34c75bb413604dd59d6ed52f0a3e2f Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 16 Sep 2015 02:30:06 -0700 Subject: [PATCH 1/5] Changes to improve performance - Track disposables in a list with lock instead of a concurrent bag - Allocate a single _createServiceAccessor instead of one per instance - Removed syncObject and just locked the _resolvedServices --- .../ConcurrentDictionaryExtensions.cs | 43 +++++++++++++++++++ .../ServiceProvider.cs | 43 ++++++++++--------- .../ScopingContainerTestBase.cs | 8 ++++ 3 files changed, 73 insertions(+), 21 deletions(-) create mode 100644 src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs diff --git a/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs b/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs new file mode 100644 index 00000000..288926e4 --- /dev/null +++ b/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs @@ -0,0 +1,43 @@ +using System; + +namespace System.Collections.Concurrent +{ + internal static class ConcurrentDictionaryExtensions + { + // From https://github.com/dotnet/corefx/issues/394#issuecomment-69494764 + // This lets us pass a state parameter allocation free GetOrAdd + internal static TValue GetOrAdd(this ConcurrentDictionary dictionary, TKey key, Func valueFactory, TArg arg) + { + if (dictionary == null) + { + throw new ArgumentNullException(nameof(dictionary)); + } + + if (key == null) + { + throw new ArgumentNullException(nameof(key)); + } + + if (valueFactory == null) + { + throw new ArgumentNullException(nameof(valueFactory)); + } + + while (true) + { + TValue value; + if (dictionary.TryGetValue(key, out value)) + { + return value; + } + + value = valueFactory(key, arg); + if (dictionary.TryAdd(key, value)) + { + return value; + } + } + } + + } +} diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index b07b4dbf..64d47c06 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -17,14 +17,11 @@ namespace Microsoft.Framework.DependencyInjection /// internal class ServiceProvider : IServiceProvider, IDisposable { - private readonly object _sync = new object(); - - private readonly Func> _createServiceAccessor; private readonly ServiceProvider _root; private readonly ServiceTable _table; private readonly Dictionary _resolvedServices = new Dictionary(); - private ConcurrentBag _disposables = new ConcurrentBag(); + private readonly List _disposables = new List(); public ServiceProvider(IEnumerable serviceDescriptors) { @@ -34,9 +31,6 @@ public ServiceProvider(IEnumerable serviceDescriptors) _table.Add(typeof(IServiceProvider), new ServiceProviderService()); _table.Add(typeof(IServiceScopeFactory), new ServiceScopeService()); _table.Add(typeof(IEnumerable<>), new OpenIEnumerableService(_table)); - - // Caching to avoid allocating a Func<,> object per call to GetService - _createServiceAccessor = CreateServiceAccessor; } // This constructor is called exclusively to create a child scope from the parent @@ -44,9 +38,6 @@ internal ServiceProvider(ServiceProvider parent) { _root = parent._root; _table = parent._table; - - // Caching to avoid allocating a Func<,> object per call to GetService - _createServiceAccessor = CreateServiceAccessor; } /// @@ -56,16 +47,16 @@ internal ServiceProvider(ServiceProvider parent) /// public object GetService(Type serviceType) { - var realizedService = _table.RealizedServices.GetOrAdd(serviceType, _createServiceAccessor); + var realizedService = _table.RealizedServices.GetOrAdd(serviceType, (type, sp) => CreateServiceAccessor(type, sp), this); return realizedService.Invoke(this); } - private Func CreateServiceAccessor(Type serviceType) + private static Func CreateServiceAccessor(Type serviceType, ServiceProvider sp) { - var callSite = GetServiceCallSite(serviceType, new HashSet()); + var callSite = sp.GetServiceCallSite(serviceType, new HashSet()); if (callSite != null) { - return RealizeService(_table, serviceType, callSite); + return RealizeService(sp._table, serviceType, callSite); } return _ => null; @@ -145,25 +136,35 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi public void Dispose() { - var disposables = Interlocked.Exchange(ref _disposables, null); - - if (disposables != null) + lock (_disposables) { - foreach (var disposable in disposables) + foreach (var disposable in _disposables) { disposable.Dispose(); } + + _disposables.Clear(); + _resolvedServices.Clear(); } } private object CaptureDisposable(object service) { + // Skip capturing disposables for the root + if (_root == this) + { + return service; + } + if (!object.ReferenceEquals(this, service)) { var disposable = service as IDisposable; if (disposable != null) { - _disposables.Add(disposable); + lock (_disposables) + { + _disposables.Add(disposable); + } } } return service; @@ -255,7 +256,7 @@ public ScopedCallSite(IService key, IServiceCallSite serviceCallSite) public virtual object Invoke(ServiceProvider provider) { object resolved; - lock (provider._sync) + lock (provider._resolvedServices) { if (!provider._resolvedServices.TryGetValue(_key, out resolved)) { @@ -312,7 +313,7 @@ private static Expression Lock(Expression providerExpression, Expression body) { // The C# compiler would copy the lock object to guard against mutation. // We don't, since we know the lock object is readonly. - var syncField = Expression.Field(providerExpression, "_sync"); + var syncField = Expression.Field(providerExpression, "_resolvedServices"); var lockWasTaken = Expression.Variable(typeof(bool), "lockWasTaken"); var monitorEnter = Expression.Call(MonitorEnterMethodInfo, syncField, lockWasTaken); diff --git a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs index 465756f5..057ccbbf 100644 --- a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs +++ b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs @@ -76,16 +76,24 @@ public void DisposingScopeDisposesService() { var container = CreateContainer(); FakeService disposableService; +- FakeService transient1; + FakeService transient2; var scopeFactory = container.GetService(); using (var scope = scopeFactory.CreateScope()) { disposableService = (FakeService)scope.ServiceProvider.GetService(); + transient1 = (FakeService)scope.ServiceProvider.GetService(); + transient2 = (FakeService)scope.ServiceProvider.GetService(); Assert.False(disposableService.Disposed); + Assert.False(transient1.Disposed); + Assert.False(transient2.Disposed); } Assert.True(disposableService.Disposed); + Assert.True(transient1.Disposed); + Assert.True(transient2.Disposed); } [Fact] From cbc58a2581127aabab9d19517d9b3a897fa9c595 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 16 Sep 2015 02:39:12 -0700 Subject: [PATCH 2/5] Bleh --- .../ScopingContainerTestBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs index 057ccbbf..8faa259a 100644 --- a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs +++ b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs @@ -76,7 +76,7 @@ public void DisposingScopeDisposesService() { var container = CreateContainer(); FakeService disposableService; -- FakeService transient1; + FakeService transient1; FakeService transient2; var scopeFactory = container.GetService(); From 2526128dcc87f4b40701678eb061aad42e6b9b85 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 16 Sep 2015 09:25:14 -0700 Subject: [PATCH 3/5] CR feedback --- .../Internal/ConcurrentDictionaryExtensions.cs | 18 ++++-------------- .../ServiceProvider.cs | 10 ++++++---- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs b/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs index 288926e4..abb83f00 100644 --- a/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs +++ b/src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; namespace System.Collections.Concurrent { @@ -8,20 +9,9 @@ internal static class ConcurrentDictionaryExtensions // This lets us pass a state parameter allocation free GetOrAdd internal static TValue GetOrAdd(this ConcurrentDictionary dictionary, TKey key, Func valueFactory, TArg arg) { - if (dictionary == null) - { - throw new ArgumentNullException(nameof(dictionary)); - } - - if (key == null) - { - throw new ArgumentNullException(nameof(key)); - } - - if (valueFactory == null) - { - throw new ArgumentNullException(nameof(valueFactory)); - } + Debug.Assert(dictionary != null); + Debug.Assert(key != null); + Debug.Assert(valueFactory != null); while (true) { diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index 64d47c06..7dd12d79 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -23,6 +23,8 @@ internal class ServiceProvider : IServiceProvider, IDisposable private readonly Dictionary _resolvedServices = new Dictionary(); private readonly List _disposables = new List(); + private static readonly Func> _createServiceAccessor = CreateServiceAccessor; + public ServiceProvider(IEnumerable serviceDescriptors) { _root = this; @@ -47,16 +49,16 @@ internal ServiceProvider(ServiceProvider parent) /// public object GetService(Type serviceType) { - var realizedService = _table.RealizedServices.GetOrAdd(serviceType, (type, sp) => CreateServiceAccessor(type, sp), this); + var realizedService = _table.RealizedServices.GetOrAdd(serviceType, _createServiceAccessor, this); return realizedService.Invoke(this); } - private static Func CreateServiceAccessor(Type serviceType, ServiceProvider sp) + private static Func CreateServiceAccessor(Type serviceType, ServiceProvider serviceProvider) { - var callSite = sp.GetServiceCallSite(serviceType, new HashSet()); + var callSite = serviceProvider.GetServiceCallSite(serviceType, new HashSet()); if (callSite != null) { - return RealizeService(sp._table, serviceType, callSite); + return RealizeService(serviceProvider._table, serviceType, callSite); } return _ => null; From 799c134985c1d4e5d75bbaa64ad15940279fe81a Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 16 Sep 2015 09:54:47 -0700 Subject: [PATCH 4/5] Don't allocate disposable list until used - Only use it for transient objects - Use _resolvedServices as a lockObject to avoid allocating a lockObj --- .../ServiceProvider.cs | 44 ++++++++++++------- .../ScopingContainerTestBase.cs | 11 +++++ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index 7dd12d79..8efa2046 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -21,7 +21,7 @@ internal class ServiceProvider : IServiceProvider, IDisposable private readonly ServiceTable _table; private readonly Dictionary _resolvedServices = new Dictionary(); - private readonly List _disposables = new List(); + private List _transientDisposables; private static readonly Func> _createServiceAccessor = CreateServiceAccessor; @@ -42,6 +42,9 @@ internal ServiceProvider(ServiceProvider parent) _table = parent._table; } + // Reusing _resolvedServices as an implementation detail of the lock + private object SyncObject => _resolvedServices; + /// /// Gets the service object of the specified type. /// @@ -138,14 +141,24 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet callSi public void Dispose() { - lock (_disposables) + lock (SyncObject) { - foreach (var disposable in _disposables) + if (_transientDisposables != null) + { + foreach (var disposable in _transientDisposables) + { + disposable.Dispose(); + } + + _transientDisposables.Clear(); + } + + // Nuke the other service types + foreach (var item in _resolvedServices.Values) { - disposable.Dispose(); + (item as IDisposable)?.Dispose(); } - _disposables.Clear(); _resolvedServices.Clear(); } } @@ -163,9 +176,14 @@ private object CaptureDisposable(object service) var disposable = service as IDisposable; if (disposable != null) { - lock (_disposables) + lock (SyncObject) { - _disposables.Add(disposable); + if (_transientDisposables == null) + { + _transientDisposables = new List(); + } + + _transientDisposables.Add(disposable); } } } @@ -262,7 +280,7 @@ public virtual object Invoke(ServiceProvider provider) { if (!provider._resolvedServices.TryGetValue(_key, out resolved)) { - resolved = provider.CaptureDisposable(_serviceCallSite.Invoke(provider)); + resolved = _serviceCallSite.Invoke(provider); provider._resolvedServices.Add(_key, resolved); } } @@ -287,12 +305,8 @@ public virtual Expression Build(Expression providerExpression) keyExpression, resolvedExpression); - var captureDisposableExpression = Expression.Assign( - resolvedExpression, - Expression.Call( - providerExpression, - CaptureDisposableMethodInfo, - _serviceCallSite.Build(providerExpression))); + var assignExpression = Expression.Assign( + resolvedExpression, _serviceCallSite.Build(providerExpression)); var addValueExpression = Expression.Call( resolvedServicesExpression, @@ -305,7 +319,7 @@ public virtual Expression Build(Expression providerExpression) new[] { resolvedExpression }, Expression.IfThen( Expression.Not(tryGetValueExpression), - Expression.Block(captureDisposableExpression, addValueExpression)), + Expression.Block(assignExpression, addValueExpression)), resolvedExpression); return Lock(providerExpression, blockExpression); diff --git a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs index 8faa259a..5c94a5eb 100644 --- a/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs +++ b/test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs @@ -78,6 +78,7 @@ public void DisposingScopeDisposesService() FakeService disposableService; FakeService transient1; FakeService transient2; + FakeService singleton; var scopeFactory = container.GetService(); using (var scope = scopeFactory.CreateScope()) @@ -85,15 +86,25 @@ public void DisposingScopeDisposesService() disposableService = (FakeService)scope.ServiceProvider.GetService(); transient1 = (FakeService)scope.ServiceProvider.GetService(); transient2 = (FakeService)scope.ServiceProvider.GetService(); + singleton = (FakeService)scope.ServiceProvider.GetService(); Assert.False(disposableService.Disposed); Assert.False(transient1.Disposed); Assert.False(transient2.Disposed); + Assert.False(singleton.Disposed); } Assert.True(disposableService.Disposed); Assert.True(transient1.Disposed); Assert.True(transient2.Disposed); + Assert.False(singleton.Disposed); + + var disposableContainer = container as IDisposable; + if (disposableContainer != null) + { + disposableContainer.Dispose(); + Assert.True(singleton.Disposed); + } } [Fact] From 92a2c7ff2efb1d2ee7dedd53a92560be415de0dd Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 16 Sep 2015 10:33:47 -0700 Subject: [PATCH 5/5] CR Feedback --- .../ServiceProvider.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs index 8efa2046..69c50965 100644 --- a/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs +++ b/src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs @@ -153,10 +153,12 @@ public void Dispose() _transientDisposables.Clear(); } - // Nuke the other service types - foreach (var item in _resolvedServices.Values) + // PERF: We've enumerating the dictionary so that we don't allocate to enumerate. + // .Values allocates a KeyCollection on the heap, enumerating the dictionary allocates + // a struct enumerator + foreach (var entry in _resolvedServices) { - (item as IDisposable)?.Dispose(); + (entry.Value as IDisposable)?.Dispose(); } _resolvedServices.Clear();