Mutex: barge instead of handing off the lock#573
Conversation
The old mutex handed the lock to a specific parked waiter, so every contended transfer was gated on the scheduler running that exact task, and everyone else queued up behind the gap. Woken waiters now compete for the lock with running tasks. Tasks still park in the mutex's own wait queue; foreign threads wait directly on the state word with a futex, so waking them needs no per-waiter bookkeeping (platforms without a futex fall back to the queue path). Contended counter, 4 workers x 100k lock/unlock, interleaved runs, medians: single-threaded runtime 8.5 -> 6.8 ms, multi-threaded 52 -> 41 ms (high variance either way, see the work stealing issue), OS threads 3562 -> 13 ms. The old foreign-thread path went through a per-waiter Notify handoff, which is where the last number comes from. FIFO ordering is gone; a task or thread that keeps re-acquiring can now starve waiters. If that ever shows up in practice, the fix is periodic forced handoff, which the wait queue still supports.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMutex and futex integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/sync/Mutex.zig (2)
111-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo spin-before-park in
lockThread.Every wake cycle here re-does an unconditional
swap, then parks again on a miss — correct, but it's the exact swap-based futex loop thatMutexFutex.lockSlow(os/thread.zig) precedes with a short spin to skip the syscall on brief contention. Worth mirroring that here for OS-thread contention, given the PR's stated goal of "reduced contention times... for OS threads."♻️ Optional spin-before-park
fn lockThread(self: *Mutex) void { + var spin: u32 = 0; + while (spin < 100) : (spin += 1) { + if (self.state.load(.monotonic) == unlocked) { + if (self.state.cmpxchgWeak(unlocked, thread_contended, .acquire, .monotonic) == null) return; + } + std.atomic.spinLoopHint(); + } while (self.state.swap(thread_contended, .acquire) != unlocked) { thread_futex.wait(&self.state, thread_contended); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync/Mutex.zig` around lines 111 - 115, Update Mutex.lockThread to add a short bounded spin-before-park phase, mirroring the established approach in MutexFutex.lockSlow. Retry the state acquisition during spinning, and call thread_futex.wait only after the spin fails, while preserving the existing contended/unlocked state transitions.
135-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancellation-vs-pop race path has no test coverage.
This branch — remove-from-queue fails because
unlockalready popped and signaled us, so we drain the pending wake and forward it to the next waiter — is the trickiest new logic in this rewrite (lost-signal-on-cancel is exactly the kind of bug that only shows up under real contention). None of the added tests (Mutex foreign threads,Mutex mixed tasks and threads) or the pre-existing ones exercise cancellation while parked inlockSlow.Want me to draft a test that spawns a task blocked on
lock(), cancels its group while another holder/waiter is churning the lock, and asserts no deadlock/lost wakeup?Also applies to: 224-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync/Mutex.zig` around lines 135 - 146, Add a concurrency test covering the cancellation-vs-pop race in Mutex.lockSlow: block a task in lock(), cancel its group while another holder or waiter unlocks and churns the mutex, and verify the cancellation completes without deadlock or lost wakeup. Ensure the test exercises the queue.remove failure path and confirms subsequent waiters can still acquire the mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/sync/Mutex.zig`:
- Around line 111-115: Update Mutex.lockThread to add a short bounded
spin-before-park phase, mirroring the established approach in
MutexFutex.lockSlow. Retry the state acquisition during spinning, and call
thread_futex.wait only after the spin fails, while preserving the existing
contended/unlocked state transitions.
- Around line 135-146: Add a concurrency test covering the cancellation-vs-pop
race in Mutex.lockSlow: block a task in lock(), cancel its group while another
holder or waiter unlocks and churns the mutex, and verify the cancellation
completes without deadlock or lost wakeup. Ensure the test exercises the
queue.remove failure path and confirms subsequent waiters can still acquire the
mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 643676e7-1368-494e-a27d-67e6508dd3ca
📒 Files selected for processing (2)
src/os/thread.zigsrc/sync/Mutex.zig
|
@coderabbitai Measured the spin-before-park suggestion before adopting it: 16 interleaved runs of the contended counter (4 OS threads, 100k locks each), spin done exactly like MutexFutex's 100-iteration pattern.
The spin does what it promises — parking drops by ~40% — and still loses by ~2.7x. With critical sections this short, spinners keep the state word's cache line bouncing between cores, which slows down the holder's unlock and the next acquire more than the saved syscalls are worth; parking takes waiters off the line. Same effect that led to spinning being removed from std.Thread.Mutex. So lockThread stays spin-free. The cancellation test suggestion was a good catch: added in a28e5f9. |
|
Tip For best results, initiate chat on the files or code changes.
Thanks for adding the cancellation test in a28e5f9. 🐇 ✏️ Learnings added
|
1 participant
The mutex handed the lock to a specific parked waiter on unlock, so every contended transfer waited for the scheduler to run that exact task — a convoy. This rewrites it as a barging lock: woken waiters compete with running tasks. Tasks still park in the mutex's own wait queue; foreign threads now wait directly on the state word with a futex instead of per-waiter Notify wakeups. Platforms without a raw futex (NetBSD) keep using the queue path.
Contended counter benchmark (4 workers × 100k lock/inc/unlock, old/new binaries interleaved per round, n=12):
The multi-threaded case has high variance in both versions; that turned out to be work-stealing churn rather than the mutex (separate issue). The OS-thread number is the per-waiter Notify handoff being replaced by a plain futex wait on the state word.
Trade-offs: FIFO ordering is gone (same as glibc/Go/parking_lot); a hot lock holder can starve waiters in principle. The wait queue still supports handing off directly, so eventual-fairness (periodic forced handoff) is a small follow-up if it's ever needed.
Adds foreign-thread and mixed tasks+threads tests; full suite passes (549/549).