Skip to content
This repository was archived by the owner on Nov 2, 2018. It is now read-only.

[Design] Reduce DI Allocations #289

Closed
wants to merge 11 commits into from

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Microsoft.Framework.DependencyInjection/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -137,4 +137,7 @@
<data name="TypeCannotBeActivated" xml:space="preserve">
<value>Cannot instantiate implementation type '{0}' for service type '{1}'.</value>
</data>
<data name="PooledTypeReusedBeforeDisposal" xml:space="preserve">
<value>Pooled type '{0}' reused before correct disposal.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,37 @@ namespace Microsoft.Framework.DependencyInjection.ServiceLookup
internal class ServiceScope : IServiceScope
{
private readonly ServiceProvider _scopedProvider;

public ServiceScope(ServiceProvider scopedProvider)
private readonly ServiceScopeFactory _parentFactory;

public ServiceScope(ServiceProvider scopedProvider, ServiceScopeFactory parentFactory)
{
_scopedProvider = scopedProvider;
_parentFactory = parentFactory;
}

public IServiceProvider ServiceProvider
{
get { return _scopedProvider; }
}

internal void Reset()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why internal here? It would also be helpful to capture your explanation of this in comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to write... Its a method that is explicitly bonded to the pooling of ServiceScopeFactory so if the class were made public rather than internal for whatever reason the exposed api would be for it to function as a standalone class (which is why I left public constructor in); whereas the "internals" only work as a class pair.

Not hugely fussed on visibility though, can make public

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it internal

{
_scopedProvider.Reset();
}

public void Dispose()
{
if (_scopedProvider.Disposed)
{
return;
}

_scopedProvider.Dispose();

if (_parentFactory != null)
{
_parentFactory.PoolScope(this);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,46 @@
// 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 readonly int _capacity;
private readonly int _halfCapacity;
private readonly ConcurrentQueue<ServiceScope> _scopePool = new ConcurrentQueue<ServiceScope>();

private readonly ServiceProvider _provider;

public ServiceScopeFactory(ServiceProvider provider)
{
_capacity = 32 * Environment.ProcessorCount;
_halfCapacity = _capacity / 2;
_provider = provider;
}

public IServiceScope CreateScope()
{
return new ServiceScope(new ServiceProvider(_provider));
ServiceScope scope;
// maintain unused buffer of _halfCapacity as partial defense against badly behaving user code
if (_scopePool.Count > _halfCapacity && _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 < _capacity)
{
_scopePool.Enqueue(scope);
}
}

}
}
27 changes: 26 additions & 1 deletion src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ internal ServiceProvider(ServiceProvider parent)
_root = parent._root;
_table = parent._table;
}

// Reusing _resolvedServices as an implementation detail of the lock
private object SyncObject => _resolvedServices;

Expand Down Expand Up @@ -139,10 +138,27 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet<Type> callSi
}
}

internal bool Disposed { get; private set; }

internal void Reset()
{
lock (SyncObject)
{
if (!Disposed)
{
throw new InvalidOperationException(
Resources.FormatPooledTypeReusedBeforeDisposal(
nameof(ServiceProvider)));
}
Disposed = false;
}
}

public void Dispose()
{
lock (SyncObject)
{
Disposed = true;
if (_transientDisposables != null)
{
foreach (var disposable in _transientDisposables)
Expand All @@ -151,6 +167,7 @@ public void Dispose()
}

_transientDisposables.Clear();
_transientDisposables = null;
}

// PERF: We've enumerating the dictionary so that we don't allocate to enumerate.
Expand Down Expand Up @@ -179,6 +196,10 @@ private object CaptureDisposable(object service)
{
lock (SyncObject)
{
if (Disposed)
{
throw new ObjectDisposedException(nameof(ServiceProvider));
}
if (_transientDisposables == null)
{
_transientDisposables = new List<IDisposable>();
Expand Down Expand Up @@ -279,6 +300,10 @@ public virtual object Invoke(ServiceProvider provider)
object resolved;
lock (provider._resolvedServices)
{
if (provider.Disposed)
{
throw new ObjectDisposedException(nameof(ServiceProvider));
}
if (!provider._resolvedServices.TryGetValue(_key, out resolved))
{
resolved = _serviceCallSite.Invoke(provider);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,34 @@ public void SelfResolveThenDispose()
(container as IDisposable)?.Dispose();
}

[Fact]
public void PooledDisposedScopesResolveDifferentInstances()
{
var container = CreateContainer();
FakeService disposableService1;
FakeService disposableService2;

var scopeFactory = container.GetService<IServiceScopeFactory>();
using (var scope = scopeFactory.CreateScope())
{
disposableService1 = (FakeService)scope.ServiceProvider.GetService<IFakeScopedService>();

Assert.False(disposableService1.Disposed);
}

Assert.True(disposableService1.Disposed);

using (var scope = scopeFactory.CreateScope())
{
disposableService2 = (FakeService)scope.ServiceProvider.GetService<IFakeScopedService>();

Assert.False(disposableService2.Disposed);
}

Assert.True(disposableService2.Disposed);
Assert.NotEqual(disposableService1, disposableService2);
}

[Fact]
public void SingletonServicesComeFromRootContainer()
{
Expand Down