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

SingleInstance lock #635

Closed
nillis opened this issue Apr 13, 2015 · 3 comments
Closed

SingleInstance lock #635

nillis opened this issue Apr 13, 2015 · 3 comments

Comments

@nillis
Copy link

nillis commented Apr 13, 2015

We were encountering problems on our highload multithreaded application with complex dependency graph caused by the LifetimeScope lock.

Problem:
The lock is provided to work around the non-threadsafe Dictionary. Although this lock is cheap, it's acquired for each SingleInstance resolve operation and shared across threads. In our case, this caused problems (high-cpu) when GC occurs and the triggering thread was blocking all other threads.

Solution:
For 4.0: replace the Dictionary with a ConcurrentDictionary and remove the Monitor lock
For 3.5: replace the Monitor lock with a ReaderWriterLockSlim lock (https://gist.github.com/nillis/0c63930229ac9164ff09)

Are there other known cases where this problem arises? Are there any pitfalls on the proposed solution?

@tillig
Copy link
Member

tillig commented Apr 13, 2015

We've had a couple of reports of this sort of thing in the past but we've had some challenges in addressing it:

  • Solutions have usually introduced problems in other areas. For example, it may solve the single instance problem but cause issues with things that aren't single instance. Or maybe it solves the performance problem but adds a memory usage issue. That kind of thing.
  • We've not really seen any hard data to back up the issue. For example, in my personal experience at work we also have a pretty high volume app and we were doing "memory dump" style "performance testing" trying to catch memory dumps at random intervals to see if we could find hot spots under load. However, when we actually attached a profiler to it, it turned out we were catching memory dumps at the wrong spot and the optimization actually needed to happen elsewhere.

Not that I'm saying there's no opportunity to optimize, just that we want to be very careful here.

Do you have any profiler data you could share - even a screen shot from ANTS or something like that? Before we dive into solving the problem, we just want to make sure that there's something backing up the issue. It'll also be good to ensure that once we issue a fix, we can see the hot spot go away to know we've fixed it.

Note that we're neck deep in ASP.NET v5 (vNext?) and the whole "DNX" thing right now, so we probably won't be able to do much with this until that's settled out. Lots of moving pieces. That also means it'd be solved in the 4.x codeline, not back-ported to 3.x.

@nillis
Copy link
Author

nillis commented Apr 13, 2015

The lock is only used when resolving Shared instances (f.e. SingleInstance) to ensure the thread-safety of the dictionary which contains these instances. The problem with the lock is that it's always done, also after initialization of the shared instance.

Probably the root cause of our problem isn't Autofac. But when we analyse mem dumps taken at a moment of 100% CPU peak, we always notice that the thread who's triggering GC is always stuck in the lock and blocking all other threads.

We hope with replacing the lock with the ReaderWriterLockSlim (as some components are still on .NET 3.5), we'll be able to locate the root cause of our problem which leads to a circumstance we're Autofac doesn't behave optimal.

Part of the .NET Call Stack of the GC triggering thread

Function

[[GCFrame]] 
[[HelperMethodFrame]] 
System_ni!System.Collections.Generic.Stack`1[[System.__Canon, mscorlib]].System.Collections.Generic.IEnumerable.GetEnumerator()+8d 
System_Core_ni!System.Linq.Enumerable.Any[[System.__Canon, mscorlib]](System.Collections.Generic.IEnumerable`1, System.Func`2)+5b 
Autofac.Core.Resolving.CircularDependencyDetector.CheckForCircularDependency(Autofac.Core.IComponentRegistration, System.Collections.Generic.Stack`1, Int32)+76 
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope, Autofac.Core.IComponentRegistration, System.Collections.Generic.IEnumerable`1)+5b 
Autofac.Core.Activators.Reflection.ConstructorParameterBinding.Instantiate()+9c 
Autofac.Core.Activators.Reflection.ReflectionActivator.ActivateInstance(Autofac.IComponentContext, System.Collections.Generic.IEnumerable`1)+109 
Autofac.Core.Resolving.InstanceLookup.Activate(System.Collections.Generic.IEnumerable`1)+59 
Autofac.Core.Resolving.InstanceLookup.Execute()+b5 
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope, Autofac.Core.IComponentRegistration, System.Collections.Generic.IEnumerable`1)+c8 
Autofac.Core.Activators.Reflection.ConstructorParameterBinding.Instantiate()+9c 
Autofac.Core.Activators.Reflection.ReflectionActivator.ActivateInstance(Autofac.IComponentContext, System.Collections.Generic.IEnumerable`1)+109 
Autofac.Core.Resolving.InstanceLookup.Activate(System.Collections.Generic.IEnumerable`1)+59 
Autofac.Core.Resolving.InstanceLookup.Execute()+b5 
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope, Autofac.Core.IComponentRegistration, System.Collections.Generic.IEnumerable`1)+c8 
Autofac.Core.Resolving.ResolveOperation.Execute(Autofac.Core.IComponentRegistration, System.Collections.Generic.IEnumerable`1)+39 
Autofac.ResolutionExtensions.ResolveOptionalService(Autofac.IComponentContext, Autofac.Core.Service, System.Collections.Generic.IEnumerable`1)+a8 
System.Web.Mvc.DefaultControllerFactory+DefaultControllerActivator.Create(System.Web.Routing.RequestContext, System.Type)+4e 
System.Web.Mvc.DefaultControllerFactory.CreateController(System.Web.Routing.RequestContext, System.String)+51 
System.Web.Mvc.MvcHandler.ProcessRequestInit(System.Web.HttpContextBase, System.Web.Mvc.IController ByRef, System.Web.Mvc.IControllerFactory ByRef)+125 

@alexmg
Copy link
Member

alexmg commented Apr 14, 2015

The code originally had a ConcurrentDictionary but it had to be removed when we migrated to PCL because it was not available in the chosen profile. We are currently updating the dev branch to work DNX and the good news is that we can now bring it back for the DNX451 and DNXCORE50 targets.

alexmg added a commit that referenced this issue Apr 14, 2015
@alexmg alexmg closed this as completed Apr 14, 2015
alexmg pushed a commit that referenced this issue Jan 13, 2019
Use lock only if instance not created yet

ConcurrentDictionary GetOrAdd AddOrUpdate with value factory is not thread safe, so lock still needed
https://github.com/dotnet/corefx/issues/25609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants