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
- Don't allocate disposable list until used
- Only use it for transient objects
- Use _resolvedServices as a lockObject to avoid allocating a lockObj

Author: davidfowl

src/Microsoft.Framework.DependencyInjection/Internal/ConcurrentDictionaryExtensions.cs

@@ -0,0 +1,33 @@
+using System;
+using System.Diagnostics;
+
+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<TKey, TValue, TArg>(this ConcurrentDictionary<TKey, TValue> dictionary, TKey key, Func<TKey, TArg, TValue> valueFactory, TArg arg)
+        {
+            Debug.Assert(dictionary != null);
+            Debug.Assert(key != null);
+            Debug.Assert(valueFactory != null);
+
+            while (true)
+            {
+                TValue value;
+                if (dictionary.TryGetValue(key, out value))
+                {
+                    return value;
+                }
+
+                value = valueFactory(key, arg);
+                if (dictionary.TryAdd(key, value))
+                {
+                    return value;
+                }
+            }
+        }
+
+    }
+}

src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs

@@ -17,14 +17,13 @@ namespace Microsoft.Framework.DependencyInjection
     /// </summary>
     internal class ServiceProvider : IServiceProvider, IDisposable
     {
-        private readonly object _sync = new object();
-
-        private readonly Func<Type, Func<ServiceProvider, object>> _createServiceAccessor;
         private readonly ServiceProvider _root;
         private readonly ServiceTable _table;
 
         private readonly Dictionary<IService, object> _resolvedServices = new Dictionary<IService, object>();
-        private ConcurrentBag<IDisposable> _disposables = new ConcurrentBag<IDisposable>();
+        private List<IDisposable> _transientDisposables;
+
+        private static readonly Func<Type, ServiceProvider, Func<ServiceProvider, object>> _createServiceAccessor = CreateServiceAccessor;
 
         public ServiceProvider(IEnumerable<ServiceDescriptor> serviceDescriptors)
         {
@@ -34,38 +33,35 @@ public ServiceProvider(IEnumerable<ServiceDescriptor> 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
         internal ServiceProvider(ServiceProvider parent)
         {
             _root = parent._root;
             _table = parent._table;
-
-            // Caching to avoid allocating a Func<,> object per call to GetService
-            _createServiceAccessor = CreateServiceAccessor;
         }
 
+        // Reusing _resolvedServices as an implementation detail of the lock
+        private object SyncObject => _resolvedServices;
+
         /// <summary>
         /// Gets the service object of the specified type.
         /// </summary>
         /// <param name="serviceType"></param>
         /// <returns></returns>
         public object GetService(Type serviceType)
         {
-            var realizedService = _table.RealizedServices.GetOrAdd(serviceType, _createServiceAccessor);
+            var realizedService = _table.RealizedServices.GetOrAdd(serviceType, _createServiceAccessor, this);
             return realizedService.Invoke(this);
         }
 
-        private Func<ServiceProvider, object> CreateServiceAccessor(Type serviceType)
+        private static Func<ServiceProvider, object> CreateServiceAccessor(Type serviceType, ServiceProvider serviceProvider)
         {
-            var callSite = GetServiceCallSite(serviceType, new HashSet<Type>());
+            var callSite = serviceProvider.GetServiceCallSite(serviceType, new HashSet<Type>());
             if (callSite != null)
             {
-                return RealizeService(_table, serviceType, callSite);
+                return RealizeService(serviceProvider._table, serviceType, callSite);
             }
 
             return _ => null;
@@ -145,14 +141,27 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet<Type> callSi
 
         public void Dispose()
         {
-            var disposables = Interlocked.Exchange(ref _disposables, null);
-
-            if (disposables != null)
+            lock (SyncObject)
             {
-                foreach (var disposable in disposables)
+                if (_transientDisposables != null)
+                {
+                    foreach (var disposable in _transientDisposables)
+                    {
+                        disposable.Dispose();
+                    }
+
+                    _transientDisposables.Clear();
+                }
+
+                // 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)
                 {
-                    disposable.Dispose();
+                    (entry.Value as IDisposable)?.Dispose();
                 }
+
+                _resolvedServices.Clear();
             }
         }
 
@@ -163,7 +172,15 @@ private object CaptureDisposable(object service)
                 var disposable = service as IDisposable;
                 if (disposable != null)
                 {
-                    _disposables.Add(disposable);
+                    lock (SyncObject)
+                    {
+                        if (_transientDisposables == null)
+                        {
+                            _transientDisposables = new List<IDisposable>();
+                        }
+
+                        _transientDisposables.Add(disposable);
+                    }
                 }
             }
             return service;
@@ -255,11 +272,11 @@ 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))
                     {
-                        resolved = provider.CaptureDisposable(_serviceCallSite.Invoke(provider));
+                        resolved = _serviceCallSite.Invoke(provider);
                         provider._resolvedServices.Add(_key, resolved);
                     }
                 }
@@ -284,12 +301,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,
@@ -302,7 +315,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);
@@ -312,7 +325,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);

test/Microsoft.Framework.DependencyInjection.Tests/ScopingContainerTestBase.cs

@@ -76,16 +76,39 @@ public void DisposingScopeDisposesService()
         {
             var container = CreateContainer();
             FakeService disposableService;
+            FakeService transient1;
+            FakeService transient2;
+            FakeService transient3;
+            FakeService singleton;
+
+            transient3 = (FakeService)container.GetService<IFakeService>();
 
             var scopeFactory = container.GetService<IServiceScopeFactory>();
             using (var scope = scopeFactory.CreateScope())
             {
                 disposableService = (FakeService)scope.ServiceProvider.GetService<IFakeScopedService>();
+                transient1 = (FakeService)scope.ServiceProvider.GetService<IFakeService>();
+                transient2 = (FakeService)scope.ServiceProvider.GetService<IFakeService>();
+                singleton = (FakeService)scope.ServiceProvider.GetService<IFakeSingletonService>();
 
                 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);
+                Assert.True(transient3.Disposed);
+            }
         }
 
         [Fact]