Improve ReaderWriterLockSlim scalability #13495
Conversation
Alternative to and subset of dotnet#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. dotnet#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.
kouvel
added
area-System.Threading
bug
|
|
||
| if (signaled == WaiterStates.None) | ||
| { | ||
| _writeEvent.Set(); // release one writer. |
There was a problem hiding this comment.
With regards to backporting this to netfx, is there any reliability concern about just waking one, e.g. if that one is then aborted?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
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; | |||
There was a problem hiding this comment.
Assumes no processor affinity was set.
Is there any guideline regarding affinity? (ignore it? expect it?).
There was a problem hiding this comment.
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.
Alternative to and subset of #13243, fixes #12780
The _myLock spin lock runs into some bad scalability issues. For example:
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.