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

Improve ReaderWriterLockSlim scalability #13495

Merged
merged 2 commits into from Aug 23, 2017

Conversation

@kouvel
Member

kouvel commented Aug 20, 2017

Alternative to and subset of #13243, fixes #12780

  • Prevented waking more than one waiter when only one of them may acquire the lock
  • Limited spinning in some cases where it is very unlikely that spinning would help

The _myLock spin lock runs into some bad scalability issues. For example:

  1. Readers can starve writers for an unreasonable amount of time. Typically there would be more readers than writers, and it doesn't take many readers to starve a writer. On my machine with 6 cores (12 logical processors with hyperthreading), 6 to 16 reader threads attempting to acquire the spin lock to acquire or release a read lock can starve one writer thread from acquiring the spin lock for several or many seconds. The issue magnifies with more reader threads.
  2. Readers and especially writers that hold the RW lock can be starved from even releasing their lock. Releasing an RW lock requires acquiring the spin lock, so releasers are easliy starved by acquirers. How badly they are starved depends on how many acquirers there are, and it doesn't take many to show a very noticeable scalability issue. Often, these acquirers are those that would not be able to acquire the RW lock until one or more releasers release their lock, so the acquirers effectively starve themselves.

This PR does not solve (1), but solves (2) to a degree that could be considered sufficient. #13243 solves (1) and (2) and for (2) it is still better by order-of-magnitude compared with this PR in several cases that I believe are not extreme, but if the complexity of deprioritization is deemed excessive for the goals then of what I tried so far this is the perhaps simplest and most reasonable way to solve (2).

I believe this PR would also be a reasonably low-risk one to port back to .NET Framework.

Improve ReaderWriterLockSlim scalability
Alternative to and subset of #13243, fixes #12780

- Prevented waking more than one waiter when only one of them may acquire the lock
- Limited spinning in some cases where it is very unlikely that spinning would help

The _myLock spin lock runs into some bad scalability issues. For example:
1. Readers can starve writers for an unreasonable amount of time. Typically there would be more readers than writers, and it doesn't take many readers to starve a writer. On my machine with 6 cores (12 logical processors with hyperthreading), 6 to 16 reader threads attempting to acquire the spin lock to acquire or release a read lock can starve one writer thread from acquiring the spin lock for several or many seconds. The issue magnifies with more reader threads.
2. Readers and especially writers that hold the RW lock can be starved from even releasing their lock. Releasing an RW lock requires acquiring the spin lock, so releasers are easliy starved by acquirers. How badly they are starved depends on how many acquirers there are, and it doesn't take many to show a very noticeable scalability issue. Often, these acquirers are those that would not be able to acquire the RW lock until one or more releasers release their lock, so the acquirers effectively starve themselves.

This PR does not solve (1), but solves (2) to a degree that could be considered sufficient. #13243 solves (1) and (2) and for (2) it is still better by order-of-magnitude compared with this PR in several cases that I believe are not extreme, but if the complexity of deprioritization is deemed excessive for the goals then of what I tried so far this is the perhaps simplest and most reasonable way to solve (2).

I believe this PR would also be a reasonably low-risk one to port back to .NET Framework.
if (signaled == WaiterStates.None)
{
_writeEvent.Set(); // release one writer.

This comment has been minimized.

@stephentoub

stephentoub Aug 22, 2017

Member

With regards to backporting this to netfx, is there any reliability concern about just waking one, e.g. if that one is then aborted?

@stephentoub

stephentoub Aug 22, 2017

Member

With regards to backporting this to netfx, is there any reliability concern about just waking one, e.g. if that one is then aborted?

This comment has been minimized.

@kouvel

kouvel Aug 22, 2017

Member

The code to handle wakeup to unregister as a waiter and in this change to reset the woken state is inside a finally block, so it should be able to run that code before the thread aborts.

@kouvel

kouvel Aug 22, 2017

Member

The code to handle wakeup to unregister as a waiter and in this change to reset the woken state is inside a finally block, so it should be able to run that code before the thread aborts.

This comment has been minimized.

@kouvel

kouvel Aug 22, 2017

Member

But when porting, on the signaling side maybe I should put the state change and call to set together in a finally block. It would prevent abort during ExitMyLock but I'm thinking that should be ok. Or can just set the event before releasing the lock.

@kouvel

kouvel Aug 22, 2017

Member

But when porting, on the signaling side maybe I should put the state change and call to set together in a finally block. It would prevent abort during ExitMyLock but I'm thinking that should be ok. Or can just set the event before releasing the lock.

@kouvel

This comment has been minimized.

Show comment
Hide comment
@kouvel

kouvel Aug 22, 2017

Member

Missed a condition, it should only reset the woken state if the wait was successful. For unsuccessful waits (timeout/exception), the event would remain signaled and it's not necessary to signal it again. This could also trigger failure of the new assert.

Member

kouvel commented Aug 22, 2017

Missed a condition, it should only reset the woken state if the wait was successful. For unsuccessful waits (timeout/exception), the event would remain signaled and it's not necessary to signal it again. This could also trigger failure of the new assert.

@kouvel

This comment has been minimized.

Show comment
Hide comment
@kouvel

kouvel Aug 23, 2017

Member

I'll go ahead and merge this tomorrow

Member

kouvel commented Aug 23, 2017

I'll go ahead and merge this tomorrow

@@ -54,8 +53,10 @@ internal class ReaderWriterCount
/// </summary>
public class ReaderWriterLockSlim : IDisposable
{
private static readonly int ProcessorCount = Environment.ProcessorCount;

This comment has been minimized.

@liranc

liranc Aug 23, 2017

Assumes no processor affinity was set.
Is there any guideline regarding affinity? (ignore it? expect it?).

@liranc

liranc Aug 23, 2017

Assumes no processor affinity was set.
Is there any guideline regarding affinity? (ignore it? expect it?).

This comment has been minimized.

@kouvel

kouvel Aug 23, 2017

Member

It's being ignored in most places currently. Maybe PlatformHelper could be extended to check affinity as well. Or if it's not expected to change, could check it once on startup. I'll leave that for a separate issue.

@kouvel

kouvel Aug 23, 2017

Member

It's being ignored in most places currently. Maybe PlatformHelper could be extended to check affinity as well. Or if it's not expected to change, could check it once on startup. I'll leave that for a separate issue.

@kouvel kouvel merged commit e000977 into dotnet:master Aug 23, 2017

19 of 20 checks passed

Tizen armel Cross Debug Build Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
OSX10.12 x64 Checked Build and Test Build finished.
Details
Tizen armel Cross Release Build Build finished.
Details
Ubuntu arm Cross Release Build Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm Cross Debug Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details
Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt legacy_backend CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details

@kouvel kouvel deleted the kouvel:RwLockSlimWaiterFix branch Aug 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment