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.

Sign up for GitHub

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

Jump to bottom

Fix lock contention #941 #906 #953

Closed
wants to merge 4 commits into from
Closed

Fix lock contention #941 #906 #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
744bd32
Nolock solution for TryGetRegistration, IsRegistered, RegistrationsFo…
avtc Dec 21, 2018
f1bfdf8
Remove recently added excessive method
avtc Dec 21, 2018
8678713
Fix proper case
avtc Dec 21, 2018
5c99efd
Better way of converting coalesce
avtc Dec 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 36 additions & 4 deletions src/Autofac/Core/Registration/ComponentRegistry.cs
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ public class ComponentRegistry : Disposable, IComponentRegistry
/// <summary>
/// Keeps track of the status of registered services.
/// </summary>
private readonly Dictionary<Service, ServiceRegistrationInfo> _serviceInfo = new Dictionary<Service, ServiceRegistrationInfo>();
private readonly ConcurrentDictionary<Service, ServiceRegistrationInfo> _serviceInfo = new ConcurrentDictionary<Service, ServiceRegistrationInfo>();

private readonly ConcurrentDictionary<IComponentRegistration, IEnumerable<IComponentRegistration>> _decorators
= new ConcurrentDictionary<IComponentRegistration, IEnumerable<IComponentRegistration>>();
Expand Down Expand Up @@ -118,9 +118,16 @@ public bool TryGetRegistration(Service service, out IComponentRegistration regis
{
if (service == null) throw new ArgumentNullException(nameof(service));

// get without lock if already registered
var info = GetInitializedServiceInfoOrDefault(service);
if (info != null && info.TryGetRegistration(out registration))
{
return true;
}

lock (_synchRoot)
{
var info = GetInitializedServiceInfo(service);
info = GetInitializedServiceInfo(service);
return info.TryGetRegistration(out registration);
}
}
Expand All @@ -134,6 +141,13 @@ public bool IsRegistered(Service service)
{
if (service == null) throw new ArgumentNullException(nameof(service));

// get without lock if already registered
var info = GetInitializedServiceInfoOrDefault(service);
if (info != null && info.IsRegistered)
{
return true;
}

lock (_synchRoot)
{
return GetInitializedServiceInfo(service).IsRegistered;
Expand Down Expand Up @@ -233,9 +247,16 @@ public IEnumerable<IComponentRegistration> RegistrationsFor(Service service)
{
if (service == null) throw new ArgumentNullException(nameof(service));

// get without lock if already registered
var info = GetInitializedServiceInfoOrDefault(service);
if (info != null)
{
return info.Implementations.ToArray();
}

lock (_synchRoot)
{
var info = GetInitializedServiceInfo(service);
info = GetInitializedServiceInfo(service);
return info.Implementations.ToArray();
}
}
Expand Down Expand Up @@ -373,10 +394,21 @@ private ServiceRegistrationInfo GetServiceInfo(Service service)
return existing;

var info = new ServiceRegistrationInfo(service);
_serviceInfo.Add(service, info);
_serviceInfo.TryAdd(service, info);
return info;
}

/// <summary>
/// Returns ServiceInfo only in case it already exists and initialized for nolock resolution.
/// </summary>
private ServiceRegistrationInfo GetInitializedServiceInfoOrDefault(Service service)
{
ServiceRegistrationInfo existing;
if (_serviceInfo.TryGetValue(service, out existing) && existing.IsInitialized)
return existing;
return null;
}

private EventHandler<ComponentRegisteredEventArgs> GetRegistered()
{
if (Properties.TryGetValue(MetadataKeys.RegisteredPropertyKey, out var registered))
Expand Down
104 changes: 80 additions & 24 deletions src/Autofac/Core/Registration/ServiceRegistrationInfo.cs
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;

namespace Autofac.Core.Registration
{
Expand All @@ -41,21 +42,27 @@ internal class ServiceRegistrationInfo
/// <summary>
/// List of implicit default service implementations. Overriding default implementations are appended to the end,
/// so the enumeration should begin from the end too, and the most default implementation comes last.
/// Used from multithreaded environment, use Interlocked/Volatile for read and write new List each time.
/// </summary>
private readonly List<IComponentRegistration> _defaultImplementations = new List<IComponentRegistration>();
private List<IComponentRegistration> _defaultImplementations = new List<IComponentRegistration>();

/// <summary>
/// List of service implementations coming from sources. Sources have priority over preserve-default implementations.
/// Implementations from sources are enumerated in preserve-default order, so the most default implementation comes first.
/// Used from multithreaded environment, use Interlocked/Volatile for read and write new List each time.
/// </summary>
private List<IComponentRegistration> _sourceImplementations = null;

/// <summary>
/// List of explicit service implementations specified with the PreserveExistingDefaults option.
/// Enumerated in preserve-defaults order, so the most default implementation comes first.
/// Used from multithreaded environment, use Interlocked/Volatile for read and write new List each time.
/// </summary>
private List<IComponentRegistration> _preserveDefaultImplementations = null;

/// <summary>
/// Used from multithreaded environment, use Interlocked/Volatile for read and write each time.
/// </summary>
[SuppressMessage("Microsoft.ApiDesignGuidelines", "CA2213", Justification = "The creator of the compponent registration is responsible for disposal.")]
private IComponentRegistration _defaultImplementation;

Expand All @@ -73,12 +80,25 @@ public ServiceRegistrationInfo(Service service)
_service = service;
}

private volatile bool _isInitialized;

/// <summary>
/// Gets a value indicating whether the first time a service is requested, initialization (e.g. reading from sources)
/// happens. This value will then be set to true. Calling many methods on this type before
/// initialisation is an error.
/// </summary>
public bool IsInitialized { get; private set; }
public bool IsInitialized
{
get
{
return _isInitialized;
}

private set
{
_isInitialized = value;
}
}

/// <summary>
/// Gets the known implementations. The first implementation is a default one.
Expand All @@ -88,15 +108,17 @@ public IEnumerable<IComponentRegistration> Implementations
get
{
RequiresInitialization();
var resultingCollection = Enumerable.Reverse(_defaultImplementations);
if (_sourceImplementations != null)
var resultingCollection = Enumerable.Reverse(Volatile.Read(ref _defaultImplementations));
var sourceImplementations = Volatile.Read(ref _sourceImplementations);
if (sourceImplementations != null)
{
resultingCollection = resultingCollection.Concat(_sourceImplementations);
resultingCollection = resultingCollection.Concat(sourceImplementations);
}

if (_preserveDefaultImplementations != null)
var preserveDefaultImplementations = Volatile.Read(ref _preserveDefaultImplementations);
if (preserveDefaultImplementations != null)
{
resultingCollection = resultingCollection.Concat(_preserveDefaultImplementations);
resultingCollection = resultingCollection.Concat(preserveDefaultImplementations);
}

return resultingCollection;
Expand All @@ -122,54 +144,88 @@ public bool IsRegistered
}

private bool Any =>
_defaultImplementations.Count > 0 ||
_sourceImplementations != null ||
_preserveDefaultImplementations != null;
Volatile.Read(ref _defaultImplementations).Count > 0 ||
Volatile.Read(ref _sourceImplementations) != null ||
Volatile.Read(ref _preserveDefaultImplementations) != null;

public void AddImplementation(IComponentRegistration registration, bool preserveDefaults, bool originatedFromSource)
{
if (preserveDefaults)
{
if (originatedFromSource)
{
if (_sourceImplementations == null)
// called inside of lock, just making List _sourceImplementations thread safe
if (Volatile.Read(ref _sourceImplementations) == null)
{
_sourceImplementations = new List<IComponentRegistration>();
Volatile.Write(ref _sourceImplementations, new List<IComponentRegistration>());
}

_sourceImplementations.Add(registration);
var newSourceImplementations = new List<IComponentRegistration>(Volatile.Read(ref _sourceImplementations));
newSourceImplementations.Add(registration);

Volatile.Write(ref _sourceImplementations, newSourceImplementations);
}
else
{
if (_preserveDefaultImplementations == null)
// called inside of lock, just making List _preserveDefaultImplementations thread safe
if (Volatile.Read(ref _preserveDefaultImplementations) == null)
{
_preserveDefaultImplementations = new List<IComponentRegistration>();
Volatile.Write(ref _preserveDefaultImplementations, new List<IComponentRegistration>());
}

_preserveDefaultImplementations.Add(registration);
var newPreserveDefaultImplementations = new List<IComponentRegistration>(Volatile.Read(ref _preserveDefaultImplementations));
Copy link
Member

Choose a reason for hiding this comment

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

PR #919 attempts to optimize the amount of memory traffic going on in an effort to speed up things like mobile applications, where allocations cost time and battery. Having to allocate a whole new list every time here seems like it might counteract some of those optimizations. We might have to balance the lock contention improvement with the memory usage/optimization improvements.

newPreserveDefaultImplementations.Add(registration);

Volatile.Write(ref _preserveDefaultImplementations, newPreserveDefaultImplementations);
}
}
else
{
if (originatedFromSource)
throw new ArgumentOutOfRangeException(nameof(originatedFromSource));

_defaultImplementations.Add(registration);
var newDefaultImplementations = new List<IComponentRegistration>(Volatile.Read(ref _defaultImplementations));
newDefaultImplementations.Add(registration);

Volatile.Write(ref _defaultImplementations, newDefaultImplementations);
}

_defaultImplementation = null;
Volatile.Write(ref _defaultImplementation, null);
}

public bool TryGetRegistration(out IComponentRegistration registration)
{
RequiresInitialization();

registration = _defaultImplementation ?? (_defaultImplementation =
_defaultImplementations.LastOrDefault() ??
_sourceImplementations?.First() ??
_preserveDefaultImplementations?.First());
registration = Volatile.Read(ref _defaultImplementation);
if (registration != null)
return true;

var defaultImplementations = Volatile.Read(ref _defaultImplementations);
if (defaultImplementations.Count > 0)
{
registration = defaultImplementations[defaultImplementations.Count - 1];
Volatile.Write(ref _defaultImplementation, registration);
return true;
}

var sourceImplementations = Volatile.Read(ref _sourceImplementations);
if (sourceImplementations != null && sourceImplementations.Count > 0)
{
registration = sourceImplementations[0];
Volatile.Write(ref _defaultImplementation, registration);
return true;
}

var preserveDefaultImplementations = Volatile.Read(ref _preserveDefaultImplementations);
if (preserveDefaultImplementations != null && preserveDefaultImplementations.Count > 0)
{
registration = preserveDefaultImplementations[0];
Volatile.Write(ref _defaultImplementation, registration);
return true;
}

return registration != null;
return false;
}

public void Include(IRegistrationSource source)
Expand Down Expand Up @@ -236,7 +292,7 @@ public bool ShouldRecalculateAdaptersOn(IComponentRegistration registration)
// - Have already been initialized
// - Were created via a registration source (because we might be adding an equivalent explicit registration such as Func<T>)
// - Don't contain any registrations (because a registration source was added when no adaptee was present)
return IsInitialized && (_sourceImplementations != null || !Any);
return IsInitialized && (Volatile.Read(ref _sourceImplementations) != null || !Any);
}
}
}