proposal: sync: Add UnlockToRLock() to RWMutex

[pjebs]

There is a scenario that can't be done efficiently (or at all) from outside sync package.
This is with regards to RWMutex

If you have an operation that has a well delineated "write" portion and "read" portion:

1. Aquire a Lock
2. Do something with the lock.
3. Release the Lock.
4. Immediately acquire RLock.
5. Do something with read lock.
6. Release the RUnlock.

The objective is:

1. Increase throughput by allowing more goroutines wishing to Read to be able to do so. AND
2. Prevent another goroutine that wants to Lock from interjecting between steps 3 and 4 above.

My proposal is a function called UnlockToRLock() that is simple to implement from inside sync package that covers the scenario above.

The alternative is:

1. Reduce throughput by keeping Step 1-6 inside a Lock OR
2. Accept a goroutine that wants to acquire lock interjecting between Step 3&4 (the scenario assumes an operation where the "read" portion must follow the "write" portion, and it is not correct for another goroutine to potentially interfere before the "read")

[ianlancetaylor]

I think this is a dup of #4026 and #23513.

[navytux]

For the reference: pygolang settled on UnlockToRLock name:

https://lab.nexedi.com/nexedi/pygolang/commit/a9345a98

[rsc]

Over on #23513 (comment) I said:

I'm not convinced this is helpful in a real program, and there is a real cost to adding complexity to the API. Please show us benchmarks improving a real workload, not a microbenchmark.

Optimizing the "holding for writing and dropping to holding for reading" makes it sound like "holding for writing" is something that can be done efficiently, but my understanding is that on today's machines, if an RWMutex is held for writing with any appreciable frequency, you don't see performance wins beyond having a plain Mutex. The contention from the writers dominates everything else.

If my understanding is correct, then this new API surface would add complexity without any performance benefit, and so we shouldn't add it.

Does anyone have numbers to address this concern, one way or the other?

[networkimprov]

@pjebs, you could get some of the semantic benefit of UnlockAndRLock() with TryRLock(). Any thoughts?

[navytux]

@rsc, on my side it is not the performance in the first place, but semantic guarantee of not releasing the lock while downgrading it. If there is no such guarantee, the writer that done modification part of its job and no longer needs the lock to be exclusively held, needs to recheck the data for consistency after Unlock->Rlock because the data might have been changed by other mutators while the lock was not held. With atomic downgrade there is no such need.

And contrary to upgrade, which is not possible to implement without deadlocks, atomic downgrade is well-defined operation with well-defined semantic.

Leaving performance topic for others.

[ianlancetaylor]

@navytux The question that @rsc is asking is: if you need to downgrade the lock, would you get better performance overall by always using a sync.Mutex rather than a sync.RWMutex. sync.RWMutex is more complex and harder to understand than sync.Mutex, so the only reason to ever use a sync.RWMutex is performance. If you need to acquire a write lock and downgrade the lock, @rsc is suggesting that you will get a better performance by using sync.Mutex. There is no reason to make sync.RWMutex still more complex if it doesn't give a performance advantage.

[networkimprov]

There are other reasons to use a RWMutex, including to wait for all potentially-concurrent tasks to cease (and block new ones), e.g. before shutdown/upgrade. Such tasks acquire a long-lived read lock.

If a situation implies a RWMutex, you should use that, even if there's no apparent performance issue. As the app evolves, or its workloads in the field increase, you avoid the need to replace a Mutex with a RWMutex. And you have more meaningful code.

You might also be able to replace a collection of Mutexes covering separate resources with a single RWMutex covering them all, if most of the access to them is read-only.

[ianlancetaylor]

@networkimprov Those are nice examples, but do any of them require a UnlockThenRLock method?

[as]

I experimented with this concept here https://github.com/as/lock/blob/master/lock.go and it was called Downgrade.

In practice, it had very little benefit for similar reasons that @rsc mentioned.

[networkimprov]

My point is that performance is really not the correct focus of this discussion.

@navytux makes a good point re the need to recheck the data for consistency after Unlock->RLock. That operation may not be lightweight.

[as]

My point is that performance is really not the correct focus of this discussion.

@networkimprov I disagree, the consistency can only be violated if you release the lock. Why do you release the lock and re-acquire the read lock?

[ianlancetaylor]

Performance really does come in, because the code can simply replace the sync.RWMutex with a sync.Mutex. The resulting code will be simpler, and there will be no need to use UnlockThenRLock because you will always hold a full lock. And it seems that for cases where it is useful to call UnlockThenRLock, using a sync.Mutex may well be faster. So the question is: if it would be both faster and simpler to use a sync.Mutex, then why should we make the implementation of sync.RWMutex more complex?