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

Fix lock contention #941 #906 #953

Closed
wants to merge 4 commits into from
Closed

Fix lock contention #941 #906 #953

wants to merge 4 commits into from

Conversation

avtc
Copy link
Contributor

@avtc avtc commented Dec 21, 2018

Nolock solution for TryGetRegistration, IsRegistered, RegistrationsFor. Issues: #941, #906

For case when ServiceRegistrationInfo is already exists and IsInitialized, i.e. after service was Resolved once.

There can be little memory overhead during initialization, but that is a small price for allowing it to work in multi-core high load environment.

@eugbaranov
Copy link
Contributor

It's possible to create some sort of benchmark to test both old and new implementations?

Do you have any screenshots from concurrency visualizer comparing the two?

@avtc
Copy link
Contributor Author

avtc commented Dec 23, 2018

Do you have any screenshots from concurrency visualizer comparing the two?

No.
I have is 85% CPU of 48 cores used by program without the fix, with profiler shows 99.6% of lock contention is inside TryGetRegistration and host become unresponsive. After fix the CPU is 5-10% and program works fine in the long run.

some sort of benchmark

Used https://github.com/danielpalme/IocPerformance

  1. Without the fix applied
Autofac 4.9.0-beta1                          Single      Multi
 Singleton                                      698        867
 Transient                                      988        940
 Combined                                      2811       2637
 Complex                                       8619       8366
 Property                                      8562       8175
 Generics                                      3381       2400
 IEnumerable                                  11996       6747
 Conditional                                   2026       1890
 Benchmark 'Child Container' (single thread) was stopped after 1,0 minutes. 266000 of 500000 instances have been resolved. Total execution would have taken: 1,9 minutes.
 Benchmark 'Child Container' (multiple threads) was stopped after 1,0 minutes. 476303 of 500000 instances have been resolved. Total execution would have taken: 1,0 minutes.
 Child Container                            112842*     62986*
 Asp Net Core                                 20338      13647
 Interception With Proxy                      28231      14754
 Prepare And Register                           338           
 Prepare And Register And Simple Resolve        395           
  1. With applied this pull and SingleInstance nolock pull Fix for #635 SingleInstance lock #952
Autofac 4.9.0-beta-2-nolock                  Single      Multi
 Singleton                                      671        426
 Transient                                      817        570
 Combined                                      2438       1531
 Complex                                       7408       4164
 Property                                      7420       4141
 Generics                                      3027       1795
 IEnumerable                                  11513       6406
 Conditional                                   1969       1238
 Benchmark 'Child Container' (single thread) was stopped after 1,0 minutes. 248000 of 500000 instances have been resolved. Total execution would have taken: 2,0 minutes.
 Benchmark 'Child Container' (multiple threads) was stopped after 1,0 minutes. 455460 of 500000 instances have been resolved. Total execution would have taken: 1,1 minutes.
 Child Container                            121120*     65868*
 Asp Net Core                                 19796      11432
 Interception With Proxy                      28335      14758
 Prepare And Register                           460           
 Prepare And Register And Simple Resolve        529          

Host:

OS=Windows 10.0.17134.472 (1803/April2018Update/Redstone4)
AMD FX(tm)-8320 Eight-Core Processor (Max: 3.50GHz), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.1.403

@alexmg
Copy link
Member

alexmg commented Jan 20, 2019

Thanks for putting together another good PR @avtc.

I wrote a quick multi threading test that could be used for some profiling and to visualize the concurrency improvements. It does 1,000,000 resolve operations as 10,000 resolves across 100 concurrent tasks.

In the screen capture below is the current code without the PR. I have highlighted Lock contention and there is plenty to be seen (the test was designed to really highlight the bottleneck). The total duration is around 347ms but this is with the profiler attached and should only be used as an indicator for comparison to the following run that also has the profiler attached.

concurrency locks

As expected the reduced locking definitely helps when concurrency comes into play. The screen capture below is with the changes in the PR. There is some lock contention up front as seen above, but after that locking is eliminated and the total time is around 160ms.

concurrency pr

I ran the same benchmarks and below is the 4.9.0 baseline on my machine.

Autofac 4.9.0                                Single      Multi
 Singleton                                      352        362
 Transient                                      451        348
 Combined                                      1272       1090
 Complex                                       3641       3511
 Property                                      3705       3415
 Generics                                      1259        997
 IEnumerable                                   4321       2737
 Conditional                                    868        848
 Child Container                              48406      26499
 Asp Net Core                                  7943       6236
 Interception With Proxy                      13240       6795
 Prepare And Register                           160
 Prepare And Register And Simple Resolve        191

Here is the run with the changes and most things look better (the multi tests in particular).

Autofac 4.9.0                                Single      Multi
 Singleton                                      329        198
 Transient                                      431        260
 Combined                                      1129        626
 Complex                                       3637       1873
 Property                                      3463       1845
 Generics                                      1257        699
 IEnumerable                                   4363       2349
 Conditional                                    840        470
 Child Container                              52734      27608
 Asp Net Core                                  7877       4323
 Interception With Proxy                      13827       7088
 Prepare And Register                           224
 Prepare And Register And Simple Resolve        254

Without the ServiceRegistrationInfo change the number are slightly better so if not needed I would like to leave them out. Did you come across a particular scenario that required the ServiceRegistrationInfo changes? I ran the same test without them and didn't hit a problem but also didn't create any additional and more advanced tests either.

Autofac 4.9.0                                Single      Multi
 Singleton                                      332        196
 Transient                                      410        249
 Combined                                      1110        612
 Complex                                       3433       1812
 Property                                      3546       1817
 Generics                                      1206        665
 IEnumerable                                   4304       2293
 Conditional                                    810        468
 Child Container                              51897      27174
 Asp Net Core                                  7881       4276
 Interception With Proxy                      13124       6793
 Prepare And Register                           226
 Prepare And Register And Simple Resolve        255

I was going to leave this PR for after the 4.9.0 release but am now considering including them.

@eugbaranov
Copy link
Contributor

Looks like a great change. Just wondering - instead of calling Volatile.Read/Write explicitly, would it be move concise to mark fields as volatile?

}

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

@tillig
Copy link
Member

tillig commented Jan 30, 2019

I got a chance to look at this and, just eyeballing it, I think the ComponentRegistry changes are good. I made a note on ServiceRegistrationInfo where the things done to optimize lock contention may help there but may also work against memory traffic optimizations from PR #919. If we can take just the ComponentRegistry changes as an incremental improvement, I think that'd be good.

alexmg added a commit that referenced this pull request Feb 10, 2019
…e resolution. Change extracted from PR #953. Added some benchmarks for concurrent resolution.
@alexmg
Copy link
Member

alexmg commented Feb 10, 2019

I have taken the ComponentRegistry changes and included them in the 4.9.0 release. Thank you for the PR @avtc.

@alexmg alexmg closed this Feb 10, 2019
@avtc
Copy link
Contributor Author

avtc commented Feb 10, 2019

@alexmg you can not just take changes in ComponentRegistry without taking changes from ServiceRegistrationInfo. Because changes in ComponentRegistry uses ServiceRegistrationInfo non thread safe properties IsRegistered, Implementations, IsInitialized and method TryGetRegistration.

Please consider reverting your partial changes or take full changes.

@alexmg
Copy link
Member

alexmg commented Feb 10, 2019

Hi @avtc. Your response does come a little late as the release has been pushed to NuGet, but not too late for another release if required. I guess you must have missed the comment from @tillig and I took the lack of response to mean you had no concerns.

IsRegistered, Implementations, and TryGetRegistration all require that IsInitialized be true before they are accessed as checked by the RequiresInitialization method in ServiceRegistrationInfo. That property is checked in GetInitializedServiceInfoOrDefault so if a ServiceRegistrationInfo is picked up before it has been initialized the code in the ComponentRegistry should drop into the lock statement that then uses GetInitializedServiceInfo.

In the concurrency tests I created I did not see any InvalidOperationException being thrown from the RequiresInitialization check indicating the IsInitialized check in GetInitializedServiceInfoOrDefault seemed to be doing its job. I might be missing something else that you noticed during your testing though, and if truly required can add the changes and push out a 4.9.1 release.

@avtc
Copy link
Contributor Author

avtc commented Feb 10, 2019

@alexmg you are right, i missed that point. Maybe only in case registrations will be added after ServiceRegistrationInfo is initialized it will not work as expected.

Also, i would suggest to make IsInitialized volatile.

@alexmg
Copy link
Member

alexmg commented Feb 11, 2019

@avtc I actually went and had another look at the ServiceRegistrationInfo changes last night and found the volatile field for IsInitialized and brought it across for the next release. Because the value only ever transitions from false to true, it seems the worst case for reordering is that a thread that could have avoided the lock, would end up taking one because false was returned when it had actually already transitioned to true. That should be alright to wait for the next bug fix release.

@alexmg
Copy link
Member

alexmg commented Feb 18, 2019

I have released 4.9.1 which includes the volatile change for IsInitialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants