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
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;
private readonly ServiceScopeFactory _parentFactory;

public ServiceScope(ServiceProvider scopedProvider)
{
_scopedProvider = scopedProvider;
}
internal ServiceScope(ServiceProvider scopedProvider, ServiceScopeFactory parentFactory)
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 for this? will the public constructor ever be used with these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppose not, missed it was an internal class

{
_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

{
// Double wash
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on this? what does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dispose again before reuse incase any extra disposables were added - overkill probably, but am concerned about information leakage between objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phrased another way, original dispose is to drop any references so they can be GC'ed as the object is now kept; second Reset dispose is to make it safe for reuse.

I could swap _disposables to a backing object to make any adds error; then restore it in the Reset instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change

_scopedProvider.Dispose();
}

public void Dispose()
{
_scopedProvider.Dispose();

if (_parentFactory != null)
{
_parentFactory.PoolScope(this);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

_capacity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

private static readonly ConcurrentQueue<ServiceScope> _scopePool = new ConcurrentQueue<ServiceScope>();

private readonly ServiceProvider _provider;

public ServiceScopeFactory(ServiceProvider provider)
Expand All @@ -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);
}
}

}
}
11 changes: 4 additions & 7 deletions src/Microsoft.Framework.DependencyInjection/ServiceProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -145,15 +145,12 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet<Type> callSi

public void Dispose()
{
var disposables = Interlocked.Exchange(ref _disposables, null);

if (disposables != null)
IDisposable disposable;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was done to avoid a double-dispose if Dispose() is called concurrently. With the current code it won't double-dispose any services but could call Clear() twice - is this a concern?

I suspect we're not too concerned about this scenario in practice. If there's any code you have operating on an SP while it's being disposed, you already have a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would just empty any extras; or do nothing if there were none. I have a little concern as its a concurrentbag which suggests items can be added in parallel; or out of order and with delays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact next comment is about the forced double dispose.

while (_disposables.TryTake(out disposable))
{
foreach (var disposable in disposables)
{
disposable.Dispose();
}
disposable.Dispose();
}
_resolvedServices.Clear();
Copy link
Member

Choose a reason for hiding this comment

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

This needs a unit test - something where we can prove that that the following works correctly:

  • Create a scoped SP
  • Resolve a scoped service (instance 1)
  • Dispose scoped SP
  • Resolve a scoped service (instance 2)
  • Assert that 1 and 2 aren't the same instance

}

private object CaptureDisposable(object service)
Expand Down