From 945c4e0605a822153c5c58e71339cb0f1697fdb5 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 28 Apr 2021 21:58:01 -0700 Subject: [PATCH 1/6] Avoid dictionary lookup for singleton services - Stash the instance on the callsite itself and avoid the resolved services lookup. --- .../ServiceLookup/CallSiteRuntimeResolver.cs | 60 ++++++++++--------- .../src/ServiceLookup/ServiceCallSite.cs | 1 + 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index 36de790a24efde..de99a9742cedf5 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Collections.Concurrent; using System.Reflection; using System.Runtime.ExceptionServices; using System.Threading; @@ -62,12 +61,25 @@ protected override object VisitRootCache(ServiceCallSite callSite, RuntimeResolv { var lockType = RuntimeResolverLock.Root; bool lockTaken = false; + ServiceProviderEngineScope serviceProviderEngine = context.Scope.Engine.Root; // using more granular locking (per singleton) for the root Monitor.Enter(callSite, ref lockTaken); try { - return ResolveService(callSite, context, lockType, serviceProviderEngine: context.Scope.Engine.Root); + if (callSite.Value is object value) + { + return value; + } + + object resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext + { + Scope = serviceProviderEngine, + AcquiredLocks = context.AcquiredLocks | lockType + }); + serviceProviderEngine.CaptureDisposable(resolved); + callSite.Value = resolved; + return resolved; } finally { @@ -91,6 +103,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte { bool lockTaken = false; object sync = serviceProviderEngine.Sync; + IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; // Taking locks only once allows us to fork resolution process // on another thread without causing the deadlock because we @@ -103,7 +116,22 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte try { - return ResolveService(callSite, context, lockType, serviceProviderEngine); + // Note: This method has already taken lock by the caller for resolution and access synchronization. + // For root: uses a concurrent dictionary and takes a per singleton lock for resolution. + // For scoped: takes a dictionary as both a resolution lock and a dictionary access lock. + if (resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) + { + return resolved; + } + + resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext + { + Scope = serviceProviderEngine, + AcquiredLocks = context.AcquiredLocks | lockType + }); + serviceProviderEngine.CaptureDisposable(resolved); + resolvedServices.Add(callSite.Cache.Key, resolved); + return resolved; } finally { @@ -114,32 +142,6 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte } } - private object ResolveService(ServiceCallSite callSite, RuntimeResolverContext context, RuntimeResolverLock lockType, ServiceProviderEngineScope serviceProviderEngine) - { - IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; - - // Note: This method has already taken lock by the caller for resolution and access synchronization. - // For root: uses a concurrent dictionary and takes a per singleton lock for resolution. - // For scoped: takes a dictionary as both a resolution lock and a dictionary access lock. - Debug.Assert( - (lockType == RuntimeResolverLock.Root && resolvedServices is ConcurrentDictionary) || - (lockType == RuntimeResolverLock.Scope && Monitor.IsEntered(serviceProviderEngine.Sync))); - - if (resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) - { - return resolved; - } - - resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext - { - Scope = serviceProviderEngine, - AcquiredLocks = context.AcquiredLocks | lockType - }); - serviceProviderEngine.CaptureDisposable(resolved); - resolvedServices.Add(callSite.Cache.Key, resolved); - return resolved; - } - protected override object VisitConstant(ConstantCallSite constantCallSite, RuntimeResolverContext context) { return constantCallSite.DefaultValue; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs index f63f4c43835679..626ecce4f25486 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs @@ -19,6 +19,7 @@ protected ServiceCallSite(ResultCache cache) public abstract Type ImplementationType { get; } public abstract CallSiteKind Kind { get; } public ResultCache Cache { get; } + public object Value { get; set; } public bool CaptureDisposable => ImplementationType == null || From 27fb9e82ac94de0d4248f229bca4f01244cfc6f0 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 28 Apr 2021 22:49:04 -0700 Subject: [PATCH 2/6] Small tweaks - Don't lock if the callsite value is already resolved - Make the value on ServiceCallsite volatile --- .../ServiceLookup/CallSiteRuntimeResolver.cs | 25 ++++++++----------- .../src/ServiceLookup/ServiceCallSite.cs | 4 ++- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index de99a9742cedf5..3c95e054cb4754 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -59,20 +59,24 @@ protected override object VisitConstructor(ConstructorCallSite constructorCallSi protected override object VisitRootCache(ServiceCallSite callSite, RuntimeResolverContext context) { + if (callSite.Value is object value) + { + // Value already calculated, return it directly + return value; + } + var lockType = RuntimeResolverLock.Root; - bool lockTaken = false; ServiceProviderEngineScope serviceProviderEngine = context.Scope.Engine.Root; - // using more granular locking (per singleton) for the root - Monitor.Enter(callSite, ref lockTaken); - try + lock (callSite) { - if (callSite.Value is object value) + // Lock the callsite and check if another thread already cached the value + if (callSite.Value is object resolved) { - return value; + return resolved; } - object resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext + resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext { Scope = serviceProviderEngine, AcquiredLocks = context.AcquiredLocks | lockType @@ -81,13 +85,6 @@ protected override object VisitRootCache(ServiceCallSite callSite, RuntimeResolv callSite.Value = resolved; return resolved; } - finally - { - if (lockTaken) - { - Monitor.Exit(callSite); - } - } } protected override object VisitScopeCache(ServiceCallSite callSite, RuntimeResolverContext context) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs index 626ecce4f25486..66f9da25b78a48 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs @@ -10,6 +10,8 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup /// internal abstract class ServiceCallSite { + private volatile object _value; + protected ServiceCallSite(ResultCache cache) { Cache = cache; @@ -19,7 +21,7 @@ protected ServiceCallSite(ResultCache cache) public abstract Type ImplementationType { get; } public abstract CallSiteKind Kind { get; } public ResultCache Cache { get; } - public object Value { get; set; } + public object Value { get => _value; set => _value = value; } public bool CaptureDisposable => ImplementationType == null || From ae637b6b3535358f382ae5c70b9891ad69848c5b Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 28 Apr 2021 23:32:34 -0700 Subject: [PATCH 3/6] Changed resolved services back to dictionary --- .../src/ScopeState.cs | 8 +++----- .../src/ServiceLookup/CallSiteRuntimeResolver.cs | 3 +-- .../src/ServiceLookup/ServiceProviderEngine.cs | 2 +- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 6 +++--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs index edc06906a8e2d0..14570e149ee107 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Collections.Concurrent; using System.Collections.Generic; using Microsoft.Extensions.DependencyInjection.ServiceLookup; @@ -10,16 +9,15 @@ namespace Microsoft.Extensions.DependencyInjection { internal class ScopeState { - public IDictionary ResolvedServices { get; } + public Dictionary ResolvedServices { get; } public List Disposables { get; set; } public int DisposableServicesCount => Disposables?.Count ?? 0; public int ResolvedServicesCount => ResolvedServices.Count; - public ScopeState(bool isRoot) + public ScopeState() { - // When isRoot is true to reduce lock contention for singletons upon resolve we use a concurrent dictionary. - ResolvedServices = isRoot ? new ConcurrentDictionary() : new Dictionary(); + ResolvedServices = new Dictionary(); } public void Track(ServiceProviderEngine engine) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index 3c95e054cb4754..b0a4611b926c6a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -100,7 +100,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte { bool lockTaken = false; object sync = serviceProviderEngine.Sync; - IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; + Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; // Taking locks only once allows us to fork resolution process // on another thread without causing the deadlock because we @@ -114,7 +114,6 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte try { // Note: This method has already taken lock by the caller for resolution and access synchronization. - // For root: uses a concurrent dictionary and takes a per singleton lock for resolution. // For scoped: takes a dictionary as both a resolution lock and a dictionary access lock. if (resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index d6d804c6484482..ca334392841727 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -19,7 +19,7 @@ internal abstract class ServiceProviderEngine : IServiceProviderEngine, IService protected ServiceProviderEngine(IEnumerable serviceDescriptors) { _createServiceAccessor = CreateServiceAccessor; - Root = new ServiceProviderEngineScope(this, isRoot: true); + Root = new ServiceProviderEngineScope(this); RuntimeResolver = new CallSiteRuntimeResolver(); CallSiteFactory = new CallSiteFactory(serviceDescriptors); CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite()); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index ab31fabff48e4a..fefb711be07bfa 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -16,13 +16,13 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid private bool _disposed; private readonly ScopeState _state; - public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) + public ServiceProviderEngineScope(ServiceProviderEngine engine) { Engine = engine; - _state = new ScopeState(isRoot); + _state = new ScopeState(); } - internal IDictionary ResolvedServices => _state.ResolvedServices; + internal Dictionary ResolvedServices => _state.ResolvedServices; // This lock protects state on the scope, in particular, for the root scope, it protects // the list of disposable entries only, since ResolvedServices is a concurrent dictionary. From f4d8d38e62901ef9de07f0ff0b76cbe93d579fb3 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 29 Apr 2021 03:27:37 -0700 Subject: [PATCH 4/6] Fixed tess - cache callsites - update tests --- .../src/ServiceLookup/CallSiteFactory.cs | 29 ++++++++++++++----- .../ServiceLookup/CallSiteRuntimeResolver.cs | 1 + .../src/ServiceLookup/ServiceCacheKey.cs | 2 +- .../tests/DI.Tests/CallSiteTests.cs | 18 +++++++----- 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs index 19ed4b81091a2d..74d263e207fa22 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs @@ -16,7 +16,7 @@ internal sealed class CallSiteFactory { private const int DefaultSlot = 0; private readonly ServiceDescriptor[] _descriptors; - private readonly ConcurrentDictionary _callSiteCache = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _callSiteCache = new ConcurrentDictionary(); private readonly Dictionary _descriptorLookup = new Dictionary(); private readonly StackGuard _stackGuard; @@ -77,7 +77,7 @@ private void Populate() } internal ServiceCallSite GetCallSite(Type serviceType, CallSiteChain callSiteChain) => - _callSiteCache.TryGetValue(serviceType, out ServiceCallSite site) ? site : + _callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, DefaultSlot), out ServiceCallSite site) ? site : CreateCallSite(serviceType, callSiteChain); internal ServiceCallSite GetCallSite(ServiceDescriptor serviceDescriptor, CallSiteChain callSiteChain) @@ -104,8 +104,6 @@ private ServiceCallSite CreateCallSite(Type serviceType, CallSiteChain callSiteC TryCreateOpenGeneric(serviceType, callSiteChain) ?? TryCreateEnumerable(serviceType, callSiteChain); - _callSiteCache[serviceType] = callSite; - return callSite; } @@ -132,6 +130,11 @@ private ServiceCallSite TryCreateOpenGeneric(Type serviceType, CallSiteChain cal private ServiceCallSite TryCreateEnumerable(Type serviceType, CallSiteChain callSiteChain) { + if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, DefaultSlot), out ServiceCallSite serviceCallSite)) + { + return serviceCallSite; + } + try { callSiteChain.Add(serviceType); @@ -191,7 +194,7 @@ private ServiceCallSite TryCreateEnumerable(Type serviceType, CallSiteChain call resultCache = new ResultCache(cacheLocation, new ServiceCacheKey(serviceType, DefaultSlot)); } - return new IEnumerableCallSite(resultCache, itemType, callSites.ToArray()); + return _callSiteCache[new ServiceCacheKey(serviceType, DefaultSlot)] = new IEnumerableCallSite(resultCache, itemType, callSites.ToArray()); } return null; @@ -211,6 +214,11 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic { if (serviceType == descriptor.ServiceType) { + if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, slot), out ServiceCallSite serviceCallSite)) + { + return serviceCallSite; + } + ServiceCallSite callSite; var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot); if (descriptor.ImplementationInstance != null) @@ -230,7 +238,7 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic throw new InvalidOperationException(SR.InvalidServiceDescriptor); } - return callSite; + return _callSiteCache[new ServiceCacheKey(serviceType, slot)] = callSite; } return null; @@ -241,6 +249,11 @@ private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type if (serviceType.IsConstructedGenericType && serviceType.GetGenericTypeDefinition() == descriptor.ServiceType) { + if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, slot), out ServiceCallSite serviceCallSite)) + { + return serviceCallSite; + } + Debug.Assert(descriptor.ImplementationType != null, "descriptor.ImplementationType != null"); var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot); Type closedType; @@ -258,7 +271,7 @@ private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type return null; } - return CreateConstructorCallSite(lifetime, serviceType, closedType, callSiteChain); + return _callSiteCache[new ServiceCacheKey(serviceType, slot)] = CreateConstructorCallSite(lifetime, serviceType, closedType, callSiteChain); } return null; @@ -406,7 +419,7 @@ private ServiceCallSite[] CreateArgumentCallSites( public void Add(Type type, ServiceCallSite serviceCallSite) { - _callSiteCache[type] = serviceCallSite; + _callSiteCache[new ServiceCacheKey(type, DefaultSlot)] = serviceCallSite; } private struct ServiceDescriptorCacheItem diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index b0a4611b926c6a..81f19d9ad52a6f 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -102,6 +102,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte object sync = serviceProviderEngine.Sync; Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; + // 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 diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs index b8b5dde5dba068..cbcee470f50883 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs @@ -5,7 +5,7 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { - internal struct ServiceCacheKey: IEquatable + internal struct ServiceCacheKey : IEquatable { public static ServiceCacheKey Empty { get; } = new ServiceCacheKey(null, 0); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs index c83e27790b637d..da75b6af9a4a62 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/CallSiteTests.cs @@ -89,9 +89,11 @@ public void BuiltExpressionWillReturnResolvedServiceWhenAppropriate( var compiledCallSite = CompileCallSite(callSite, provider); var compiledCollectionCallSite = CompileCallSite(collectionCallSite, provider); - var service1 = Invoke(callSite, provider); - var service2 = compiledCallSite(provider.Root); - var serviceEnumerator = ((IEnumerable)compiledCollectionCallSite(provider.Root)).GetEnumerator(); + using var scope = (ServiceProviderEngineScope)provider.CreateScope(); + + var service1 = Invoke(callSite, scope); + var service2 = compiledCallSite(scope); + var serviceEnumerator = ((IEnumerable)compiledCollectionCallSite(scope)).GetEnumerator(); Assert.NotNull(service1); Assert.True(compare(service1, service2)); @@ -114,10 +116,12 @@ public void BuiltExpressionCanResolveNestedScopedService() var callSite = provider.CallSiteFactory.GetCallSite(typeof(ServiceC), new CallSiteChain()); var compiledCallSite = CompileCallSite(callSite, provider); - var serviceC = (ServiceC)compiledCallSite(provider.Root); + using var scope = (ServiceProviderEngineScope)provider.CreateScope(); + + var serviceC = (ServiceC)compiledCallSite(scope); Assert.NotNull(serviceC.ServiceB.ServiceA); - Assert.Equal(serviceC, Invoke(callSite, provider)); + Assert.Equal(serviceC, Invoke(callSite, scope)); } [Theory] @@ -371,9 +375,9 @@ public void Dispose() } } - private static object Invoke(ServiceCallSite callSite, ServiceProviderEngine provider) + private static object Invoke(ServiceCallSite callSite, ServiceProviderEngineScope scope) { - return CallSiteRuntimeResolver.Resolve(callSite, provider.Root); + return CallSiteRuntimeResolver.Resolve(callSite, scope); } private static Func CompileCallSite(ServiceCallSite callSite, ServiceProviderEngine engine) From b891ae1cbc345a1025b7ba45db520812df84e1a7 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 29 Apr 2021 23:37:29 -0700 Subject: [PATCH 5/6] PR feedback and cleanup --- .../src/ServiceLookup/CallSiteFactory.cs | 17 ++++++++++------- .../ServiceLookup/CallSiteRuntimeResolver.cs | 1 - .../src/ServiceLookup/ConstantCallSite.cs | 4 ++-- .../src/ServiceLookup/ServiceCacheKey.cs | 2 +- .../src/ServiceLookup/ServiceCallSite.cs | 4 +--- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs index 74d263e207fa22..82ee191aade689 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs @@ -130,7 +130,8 @@ private ServiceCallSite TryCreateOpenGeneric(Type serviceType, CallSiteChain cal private ServiceCallSite TryCreateEnumerable(Type serviceType, CallSiteChain callSiteChain) { - if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, DefaultSlot), out ServiceCallSite serviceCallSite)) + ServiceCacheKey callSiteKey = new ServiceCacheKey(serviceType, DefaultSlot); + if (_callSiteCache.TryGetValue(callSiteKey, out ServiceCallSite serviceCallSite)) { return serviceCallSite; } @@ -191,10 +192,10 @@ private ServiceCallSite TryCreateEnumerable(Type serviceType, CallSiteChain call ResultCache resultCache = ResultCache.None; if (cacheLocation == CallSiteResultCacheLocation.Scope || cacheLocation == CallSiteResultCacheLocation.Root) { - resultCache = new ResultCache(cacheLocation, new ServiceCacheKey(serviceType, DefaultSlot)); + resultCache = new ResultCache(cacheLocation, callSiteKey); } - return _callSiteCache[new ServiceCacheKey(serviceType, DefaultSlot)] = new IEnumerableCallSite(resultCache, itemType, callSites.ToArray()); + return _callSiteCache[callSiteKey] = new IEnumerableCallSite(resultCache, itemType, callSites.ToArray()); } return null; @@ -214,7 +215,8 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic { if (serviceType == descriptor.ServiceType) { - if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, slot), out ServiceCallSite serviceCallSite)) + ServiceCacheKey callSiteKey = new ServiceCacheKey(serviceType, slot); + if (_callSiteCache.TryGetValue(callSiteKey, out ServiceCallSite serviceCallSite)) { return serviceCallSite; } @@ -238,7 +240,7 @@ private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type servic throw new InvalidOperationException(SR.InvalidServiceDescriptor); } - return _callSiteCache[new ServiceCacheKey(serviceType, slot)] = callSite; + return _callSiteCache[callSiteKey] = callSite; } return null; @@ -249,7 +251,8 @@ private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type if (serviceType.IsConstructedGenericType && serviceType.GetGenericTypeDefinition() == descriptor.ServiceType) { - if (_callSiteCache.TryGetValue(new ServiceCacheKey(serviceType, slot), out ServiceCallSite serviceCallSite)) + ServiceCacheKey callSiteKey = new ServiceCacheKey(serviceType, slot); + if (_callSiteCache.TryGetValue(callSiteKey, out ServiceCallSite serviceCallSite)) { return serviceCallSite; } @@ -271,7 +274,7 @@ private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type return null; } - return _callSiteCache[new ServiceCacheKey(serviceType, slot)] = CreateConstructorCallSite(lifetime, serviceType, closedType, callSiteChain); + return _callSiteCache[callSiteKey] = CreateConstructorCallSite(lifetime, serviceType, closedType, callSiteChain); } return null; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index 81f19d9ad52a6f..b0a4611b926c6a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -102,7 +102,6 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte object sync = serviceProviderEngine.Sync; Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; - // 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 diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ConstantCallSite.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ConstantCallSite.cs index 206d2691a878fc..a27cb110389d34 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ConstantCallSite.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ConstantCallSite.cs @@ -8,7 +8,7 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup internal sealed class ConstantCallSite : ServiceCallSite { private readonly Type _serviceType; - internal object DefaultValue { get; } + internal object DefaultValue => Value; public ConstantCallSite(Type serviceType, object defaultValue): base(ResultCache.None) { @@ -18,7 +18,7 @@ public ConstantCallSite(Type serviceType, object defaultValue): base(ResultCache throw new ArgumentException(SR.Format(SR.ConstantCantBeConvertedToServiceType, defaultValue.GetType(), serviceType)); } - DefaultValue = defaultValue; + Value = defaultValue; } public override Type ServiceType => DefaultValue?.GetType() ?? _serviceType; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs index cbcee470f50883..f2c3baa2a6d471 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs @@ -5,7 +5,7 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { - internal struct ServiceCacheKey : IEquatable + internal readonly struct ServiceCacheKey : IEquatable { public static ServiceCacheKey Empty { get; } = new ServiceCacheKey(null, 0); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs index 66f9da25b78a48..626ecce4f25486 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs @@ -10,8 +10,6 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup /// internal abstract class ServiceCallSite { - private volatile object _value; - protected ServiceCallSite(ResultCache cache) { Cache = cache; @@ -21,7 +19,7 @@ protected ServiceCallSite(ResultCache cache) public abstract Type ImplementationType { get; } public abstract CallSiteKind Kind { get; } public ResultCache Cache { get; } - public object Value { get => _value; set => _value = value; } + public object Value { get; set; } public bool CaptureDisposable => ImplementationType == null || From 345c0b7d63b430586f32213df9c98655233c4e67 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 29 Apr 2021 23:39:47 -0700 Subject: [PATCH 6/6] Remove extra whitespace --- .../src/ServiceLookup/CallSiteRuntimeResolver.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index b0a4611b926c6a..7479759f2b527a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -101,7 +101,6 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte bool lockTaken = false; object sync = serviceProviderEngine.Sync; Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; - // 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