The Windows SRWLock primitive can be used for rw locking instead of a custom CRITICAL_SECTION+Semaphore implementation #3382

[dmachaj]

Closes #3382

Why is this change being made?

The primary motivation is to try and improve runtime performance. The OS primitive should be very efficient for the intended use case and will be modestly smaller than the existing implementation. This change also allows a dozen or so lines of code to be deleted in favor of just using the OS SRWLock implementation.

Briefly summarize what changed

Trim down the uv_rwlock_t struct to just the SRWLock field. There are comments about removing things in v2.x so I am unsure whether this is a breaking change, or not. I can modify it to be non-breaking if that is necessary.

Update the various uv_rwlock_ methods to call the Windows SRWLock API. This is mostly deleting a bunch of code and replacing it with one-liners.

How was the change tested?

I ran the existing test collateral (compiled as Debug for more asserts) and they passed:

 Build started...
 1>------ Build started: Project: RUN_TESTS, Configuration: Debug x64 ------
 1>Test project V:/T/libuv/build
 1>    Start 1: uv_test
 1>1/2 Test #1: uv_test ..........................   Passed  168.58 sec
 1>    Start 2: uv_test_a
 1>2/2 Test #2: uv_test_a ........................   Passed  166.91 sec
 1>
 1>100% tests passed, 0 tests failed out of 2
 1>
 1>Total Test time (real) = 335.50 sec
 ========== Build: 1 succeeded, 0 failed, 1 up-to-date, 0 skipped ==========

I also ran build\Release\uv_run_benchmarks_a.exe to try and benchmark a difference. However on closer inspection it seems that none of the benchmarks exercise this particular lock primitive. I verified this by adding debug information and running the benchmark under the debugger and observing that breakpoints in the modified code do not get hit.

I also applied this edit to a local clone of NodeJS (v16.x branch) and tried to benchmark running npm init react-app for create_react_app. Node does seem to use the rwlock primitive. The performance benefit was small but did seem to be there after a 10x run before and after. It improved the runtime by roughly 0.5 seconds out of 50 seconds (1% improvement).

(Disclaimer: I am a Microsoft employee)

[bnoordhuis]

Left some comments but thanks for the PR.

[bnoordhuis]

LGTM, thanks! This should probably get a look-over from at least one more maintainer, cc @libuv/windows.

[dmachaj]

I'm really glad I added that STATIC_ASSERT. I had a mistake where of course the sizes of uv_rwlock_t are different between 32-bit and 64-bit architectures. The PR build includes the 32-bit Windows flavor so that triggered a build break and made the problem clear. The latest push adjusts the sizes of the assert and the padding based on 32-bit or 64-bit architecture.

[dmachaj]

@bnoordhuis @vtjnash is there anything else that you'd like me to change before this is ready to merge? Thanks.

[bnoordhuis]

It's good to go from my perspective but we're close to making a release and it should probably be a conscious decision from multiple maintainers to add it to the release.

I think it's a risk free change but I'm not the resident Windows expert.

[dmachaj]

Understood. I was missing the bigger context about a release.

The only notable risk that I'm aware of is that SRWLock is unhappy about being acquired reentrantly in shared mode. That will appear to work as long as no exclusive writers are trying to acquire it. If there is a writer trying to acquire the lock then the reentrant reader acquire will block. I am not completely sure whether the previous read/write lock implementation in this project had the same concern or if it was happily acquired reentrantly.

I'm not aware of any risk besides that. The OS primitive is very widely used so it should be mature and reliable.

[bnoordhuis]

I'm going to land this. Thanks for the PR, David!

[vtjnash]

I am not completely sure whether the previous read/write lock implementation in this project had the same concern or if it was happily acquired reentrantly.

Just got bitten by this, as I was relying on this guarantee previously. On pthreads, reentrant acquisition is explicitly required by the standard (c.f. pthread_rwlockattr_setkind_np which lets you optionally disable the standards-compliant behavior). In C++ it is undefined for std::shared_mutex. I can find some workaround (I have extreme expectations, as I also needed it to be async-safe, which most implementations probably are, including the old implementation here, but not mingw-w64), it was just a little unexpected to realize that Windows has this issue.

We likely should at least try to document these better?

[bnoordhuis]

On pthreads, reentrant acquisition is explicitly required by the standard

Just so we're on the same page: only for read locks, not write locks, right?

Does #3383 (comment) mean libuv's behavior changed? If readers had precedence over writers before, and now it's the other way around, then that sounds like a recipe for deadlocks.

[vtjnash]

Correct

[bnoordhuis]

That's rather unfortunate. Should we revert this PR?

[dmachaj]

The Windows standard SRWLock does not explicitly give readers precedence over writers. Readers will line up behind a waiting writer to try and prevent lots of readers from permanently starving a writer. (This is neither more nor less correct than other strategies; there are different valid tradeoffs).

Some supporting reading:
Is it okay to acquire an SRWLOCK recursively? (And why not?)
If the slim reader/writer lock (SRWLOCK) doesn’t remember who the shared lock owner is, does that mean it’s okay to acquire it recursively?
How fair are SRW locks, particularly when there are both readers and writers?

For what it's worth AppVerifier can be used to flag a variety of issues at runtime on Windows. The most relevant to this discussion is that SRWLock recursive acquisition will generate a DebugBreak. (AppVerifier is a very useful tool; the heap checking is indispensable for debugging memory corruption).

[vtjnash]

@bnoordhuis Upon re-review, I realized the old implementation was also not async-safe, so it did not work for my needs anyways, just failed less often. So I need to replace that particular bit of code anyways with something custom-tailored to my requirements anyways.

[bnoordhuis]

@dmachaj That first oldnewthing blog post says SRWLocks aren't recursive, period.

Libuv's uv_rwlock_t is modeled after pthread_rwlock_t, which is recursive (for readers), and the old CRITICAL_SECTION implementation was, so that means this PR changes its behavior in an incompatible manner.

The documentation for uv_rwlock_t doesn't claim recursive locking is supported but, on the other hand, neither does it claim it's not (and it works on Unices but not any longer on Windows.)

@vtjnash WDYT? I'm inclined to revert because any issues will be hell for users to track down.

[vtjnash]

I am not sure. I am fixing my case now with a TLS counter to check for recursion prior to accessing the lock. For users, this shows up as a deadlock, which is pretty easy to track down (just interrupt the code and look at the stack).