Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pool the underlying list and dictionary in scopes. #50463

Merged
merged 5 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading;
using Microsoft.Extensions.DependencyInjection.ServiceLookup;

namespace Microsoft.Extensions.DependencyInjection
{
internal class ScopePool
Copy link
Contributor

Choose a reason for hiding this comment

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

How many similar pool implementations do we have across the codebase? Worth having something reusable?

Copy link
Member

@halter73 halter73 Apr 1, 2021

Choose a reason for hiding this comment

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

Getting something that we can actually share internally is probably a good first test for any proposed ObjectPool<T> API. (#49680)

{
// Modest number to re-use. We only really care about reuse for short lived scopes
private static readonly int s_maxQueueSize = Environment.ProcessorCount * 2;

private int _count;
private readonly ConcurrentQueue<State> _queue = new();

public State Rent()
{
if (_queue.TryDequeue(out State state))
{
Interlocked.Decrement(ref _count);
return state;
}
return new State(this);
}

public bool Return(State state)
{
if (Interlocked.Increment(ref _count) > s_maxQueueSize)
{
Interlocked.Decrement(ref _count);
return false;
}

state.Clear();
Copy link
Member

Choose a reason for hiding this comment

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

Should we always call Clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

_queue.Enqueue(state);
return true;
}

public class State
{
private readonly ScopePool _pool;

public IDictionary<ServiceCacheKey, object> ResolvedServices { get; }
public List<object> Disposables { get; set; }

public State(ScopePool pool = null)
{
_pool = pool;
ResolvedServices = pool == null ? new ConcurrentDictionary<ServiceCacheKey, object>() : new Dictionary<ServiceCacheKey, object>();
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
}

internal void Clear()
{
// REVIEW: Should we trim excess here as well?
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
ResolvedServices.Clear();
Disposables?.Clear();
}

public bool Return()
{
return _pool?.Return(this) ?? false;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,15 @@ protected override object VisitScopeCache(ServiceCallSite callSite, RuntimeResol
private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
{
bool lockTaken = false;
IDictionary<ServiceCacheKey, object> resolvedServices = serviceProviderEngine.ResolvedServices;
object sync = serviceProviderEngine.Sync;

// Taking locks only once allows us to fork resolution process
// on another thread without causing the deadlock because we
// always know that we are going to wait the other thread to finish before
// releasing the lock
if ((context.AcquiredLocks & lockType) == 0)
{
Monitor.Enter(resolvedServices, ref lockTaken);
Monitor.Enter(sync, ref lockTaken);
}

try
Expand All @@ -109,7 +109,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte
{
if (lockTaken)
{
Monitor.Exit(resolvedServices);
Monitor.Exit(sync);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,19 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor<object, Expres
private static readonly ParameterExpression ScopeParameter = Expression.Parameter(typeof(ServiceProviderEngineScope));

private static readonly ParameterExpression ResolvedServices = Expression.Variable(typeof(IDictionary<ServiceCacheKey, object>), ScopeParameter.Name + "resolvedServices");
private static readonly ParameterExpression Sync = Expression.Variable(typeof(object), ScopeParameter.Name + "sync");
private static readonly BinaryExpression ResolvedServicesVariableAssignment =
Expression.Assign(ResolvedServices,
Expression.Property(
ScopeParameter,
typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.ResolvedServices), BindingFlags.Instance | BindingFlags.NonPublic)));

private static readonly BinaryExpression SyncVariableAssignment =
Expression.Assign(Sync,
Expression.Property(
ScopeParameter,
typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.Sync), BindingFlags.Instance | BindingFlags.NonPublic)));

private static readonly ParameterExpression CaptureDisposableParameter = Expression.Parameter(typeof(object));
private static readonly LambdaExpression CaptureDisposable = Expression.Lambda(
Expression.Call(ScopeParameter, CaptureDisposableMethodInfo, CaptureDisposableParameter),
Expand Down Expand Up @@ -96,8 +103,9 @@ private Expression<Func<ServiceProviderEngineScope, object>> BuildExpression(Ser
{
return Expression.Lambda<Func<ServiceProviderEngineScope, object>>(
Expression.Block(
new[] { ResolvedServices },
new[] { ResolvedServices, Sync },
ResolvedServicesVariableAssignment,
SyncVariableAssignment,
BuildScopedExpression(callSite)),
ScopeParameter);
}
Expand Down Expand Up @@ -213,7 +221,6 @@ protected override Expression VisitScopeCache(ServiceCallSite callSite, object c
// Move off the main stack
private Expression BuildScopedExpression(ServiceCallSite callSite)
{

ConstantExpression keyExpression = Expression.Constant(
callSite.Cache.Key,
typeof(ServiceCacheKey));
Expand Down Expand Up @@ -257,9 +264,10 @@ private Expression BuildScopedExpression(ServiceCallSite callSite)
// The C# compiler would copy the lock object to guard against mutation.
// We don't, since we know the lock object is readonly.
ParameterExpression lockWasTaken = Expression.Variable(typeof(bool), "lockWasTaken");
ParameterExpression sync = Sync;

MethodCallExpression monitorEnter = Expression.Call(MonitorEnterMethodInfo, resolvedServices, lockWasTaken);
MethodCallExpression monitorExit = Expression.Call(MonitorExitMethodInfo, resolvedServices);
MethodCallExpression monitorEnter = Expression.Call(MonitorEnterMethodInfo, sync, lockWasTaken);
MethodCallExpression monitorExit = Expression.Call(MonitorExitMethodInfo, sync);

BlockExpression tryBody = Expression.Block(monitorEnter, blockExpression);
ConditionalExpression finallyBody = Expression.IfThen(lockWasTaken, monitorExit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ internal sealed class ILEmitResolverBuilder : CallSiteVisitor<ILEmitResolverBuil
private static readonly MethodInfo ResolvedServicesGetter = typeof(ServiceProviderEngineScope).GetProperty(
nameof(ServiceProviderEngineScope.ResolvedServices), BindingFlags.Instance | BindingFlags.NonPublic).GetMethod;

private static readonly MethodInfo ScopeLockGetter = typeof(ServiceProviderEngineScope).GetProperty(
nameof(ServiceProviderEngineScope.Sync), BindingFlags.Instance | BindingFlags.NonPublic).GetMethod;

private static readonly FieldInfo FactoriesField = typeof(ILEmitResolverBuilderRuntimeContext).GetField(nameof(ILEmitResolverBuilderRuntimeContext.Factories));
private static readonly FieldInfo ConstantsField = typeof(ILEmitResolverBuilderRuntimeContext).GetField(nameof(ILEmitResolverBuilderRuntimeContext.Constants));
private static readonly MethodInfo GetTypeFromHandleMethod = typeof(Type).GetMethod(nameof(Type.GetTypeFromHandle));
Expand Down Expand Up @@ -319,6 +322,7 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c
{
LocalBuilder cacheKeyLocal = context.Generator.DeclareLocal(typeof(ServiceCacheKey));
LocalBuilder resolvedServicesLocal = context.Generator.DeclareLocal(typeof(IDictionary<ServiceCacheKey, object>));
LocalBuilder syncLocal = context.Generator.DeclareLocal(typeof(object));
LocalBuilder lockTakenLocal = context.Generator.DeclareLocal(typeof(bool));
LocalBuilder resultLocal = context.Generator.DeclareLocal(typeof(object));

Expand All @@ -339,8 +343,15 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c
// Store resolved services
Stloc(context.Generator, resolvedServicesLocal.LocalIndex);

// Load resolvedServices
Ldloc(context.Generator, resolvedServicesLocal.LocalIndex);
// scope
context.Generator.Emit(OpCodes.Ldarg_1);
// .Sync
context.Generator.Emit(OpCodes.Callvirt, ScopeLockGetter);
// Store syncLocal
Stloc(context.Generator, syncLocal.LocalIndex);
pakrym marked this conversation as resolved.
Show resolved Hide resolved

// Load syncLocal
Ldloc(context.Generator, syncLocal.LocalIndex);
// Load address of lockTaken
context.Generator.Emit(OpCodes.Ldloca_S, lockTakenLocal.LocalIndex);
// Monitor.Enter
Expand Down Expand Up @@ -389,7 +400,7 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c
// return if not
context.Generator.Emit(OpCodes.Brfalse, returnLabel);
// Load resolvedServices
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
Ldloc(context.Generator, resolvedServicesLocal.LocalIndex);
Ldloc(context.Generator, syncLocal.LocalIndex);
// Monitor.Exit
context.Generator.Emit(OpCodes.Call, ExpressionResolverBuilder.MonitorExitMethodInfo);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@ protected ServiceProviderEngine(IEnumerable<ServiceDescriptor> serviceDescriptor
CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite());
CallSiteFactory.Add(typeof(IServiceScopeFactory), new ServiceScopeFactoryCallSite());
RealizedServices = new ConcurrentDictionary<Type, Func<ServiceProviderEngineScope, object>>();
ScopePool = new ScopePool();
}

internal ScopePool ScopePool { get; }

internal ConcurrentDictionary<Type, Func<ServiceProviderEngineScope, object>> RealizedServices { get; }

internal CallSiteFactory CallSiteFactory { get; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading.Tasks;
using Microsoft.Extensions.Internal;

Expand All @@ -15,20 +13,25 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid
// For testing only
internal Action<object> _captureDisposableCallback;

private List<object> _disposables;

private bool _disposed;
private readonly object _disposelock = new object();
private ScopePool.State _state;

// This protects the disposed state
private readonly object _disposeLock = new object();

// This protects resolved services, this is only used if isRoot is false
private readonly object _scopeLock;

public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false)
{
Engine = engine;

// To reduce lock contention for singletons upon resolve we use a concurrent dictionary.
ResolvedServices = isRoot ? new ConcurrentDictionary<ServiceCacheKey, object>() : new Dictionary<ServiceCacheKey, object>();
_state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent();
_scopeLock = isRoot ? null : new object();
}

internal IDictionary<ServiceCacheKey, object> ResolvedServices { get; }
internal IDictionary<ServiceCacheKey, object> ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed();
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved

internal object Sync => _scopeLock;
davidfowl marked this conversation as resolved.
Show resolved Hide resolved

public ServiceProviderEngine Engine { get; }

Expand All @@ -53,7 +56,7 @@ internal object CaptureDisposable(object service)
return service;
}

lock (_disposelock)
lock (_disposeLock)
{
if (_disposed)
{
Expand All @@ -66,16 +69,17 @@ internal object CaptureDisposable(object service)
// sync over async, for the rare case that an object only implements IAsyncDisposable and may end up starving the thread pool.
Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask()).GetAwaiter().GetResult();
}

ThrowHelper.ThrowObjectDisposedException();
}

if (_disposables == null)
{
_disposables = new List<object>();
return service;
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
}

_disposables.Add(service);
_state.Disposables ??= new();

_state.Disposables.Add(service);
}

return service;
}

Expand All @@ -97,6 +101,8 @@ public void Dispose()
}
}
}

ClearState();
}

public ValueTask DisposeAsync()
Expand Down Expand Up @@ -134,9 +140,11 @@ public ValueTask DisposeAsync()
}
}

ClearState();
Copy link
Member

@maryamariyan maryamariyan Apr 1, 2021

Choose a reason for hiding this comment

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

Would it make sense to add this ClearState() call on a finally block to the try-catch block above it or it doesnt make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it would.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it's probably better not to try and return this scope to the pool if dispose throws.


return default;

static async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
{
await vt.ConfigureAwait(false);
// vt is acting on the disposable at index i,
Expand All @@ -155,30 +163,48 @@ static async ValueTask Await(int i, ValueTask vt, List<object> toDispose)
((IDisposable)disposable).Dispose();
}
}

ClearState();
}
}

private IDictionary<ServiceCacheKey, object> ScopeDisposed()
{
ThrowHelper.ThrowObjectDisposedException();
return null;
}

private void ClearState()
{
// Dispose the state, which will end up attempting to return the state pool.
// This will return false if the pool is full or if this state object is the root scope
if (_state.Return())
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
{
_state = null;
}
}

private List<object> BeginDispose()
{
List<object> toDispose;
lock (_disposelock)
lock (_disposeLock)
{
if (_disposed)
{
return null;
}

// We've transitioned to the disposed state, so future calls to
// CaptureDisposable will immediately dispose the object.
// No further changes to _state.Disposables, are allowed.
_disposed = true;
toDispose = _disposables;
_disposables = null;

return _state.Disposables;

// Not clearing ResolvedServices here because there might be a compilation running in background
// trying to get a cached singleton service instance and if it won't find
// it it will try to create a new one tripping the Debug.Assert in CaptureDisposable
// and leaking a Disposable object in Release mode
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
}

return toDispose;
}
}
}