Conversation
jorajeev
previously approved these changes
Aug 18, 2026
`Acquire::poll` ran this in release builds on every poll:
assert_eq!(is_queued, self.waiter.waker.lock().unwrap().is_some());
That takes a `std::sync::Mutex` to check an internal invariant, on the
uncontended fast path, for every `Mutex::lock`, `RwLock::read`/`write`,
`Semaphore::acquire`, and every channel send and recv in the program under test.
Make it a `debug_assert_eq!`.
Two smaller cleanups alongside it: the `will_succeed` computation took two
separate `RefCell` borrows of the semaphore state (`is_closed`, then
`available_permits`) where one suffices and gives a single consistent snapshot,
and `release()` computed `ExecutionState::me()` eagerly for a `trace!` argument,
paying an `ExecutionState::with` on every release even with tracing disabled.
Measured on an M1 Pro, release, single task, no contention, against an identical
61ns per-scheduling-decision control:
operation steps/op before after
mutex lock/unlock 2 234ns 167ns -29%
rwlock write 2 240ns 163ns -32%
semaphore acquire 2 272ns 163ns -40%
mpsc send+recv 4 507ns 353ns -30%
Attribution: restoring only the `assert_eq!` while keeping the two cleanups
returns the numbers to baseline, so the assertion accounts for essentially all of
it. An isolated `std::sync::Mutex` lock/unlock measures 10.4ns, so the remainder
is presumably the barrier interacting with the surrounding `SeqCst` atomics.
Note this only helps release builds, since `debug_assert` stays active under a
default `cargo test`.
`BatchSemaphore::acquire` did `Arc::new(Waiter::new(num_permits))`
unconditionally, so every `Mutex::lock`, `RwLock::read`/`write`,
`Semaphore::acquire`, and every channel send and recv did a heap allocation.
The `Arc` exists because a blocked acquire is shared between two owners: the
`Acquire` future on the blocking task's stack, and the semaphore's
`waiters: VecDeque<Arc<Waiter>>`, which the releasing task reaches into to flip
`is_queued`/`has_permits`, read the clock, and take the waker. An acquire that
gets its permits immediately is never enqueued and never observed by another
task, so it needs none of that sharing.
`Acquire` now holds `num_permits`, `task_id`, `clock` and a plain `has_permits`
bool inline, with `waiter: Option<Arc<Waiter>>` allocated in the one branch that
enqueues. `has_permits()`, `is_queued()` and `grant_permits()` read the inline
copies while no waiter exists and the shared ones once it does, since from then
on the releasing task owns those writes.
`task_id` and `clock` are still snapshotted in `Acquire::new`, exactly as before,
and moved into the `Waiter` at enqueue. This is load bearing: `waiter.clock` feeds
the happens-before edge recorded in `unblock_waiters_from_front`, and a
`thread::switch()` sits between construction and blocking, so reading the clock at
enqueue time instead would silently change the happens-before graph that vector
clocks use to detect races.
A `Waiter` cannot instead live on the `Task` and be reused, because a task can
have several acquires outstanding at once: `future::batch_semaphore` already has a
test where one task drives two `lock()` futures for the same semaphore through
`FuturesUnordered`, and `upgrade()` polls an `Acquire` then releases while it is
still alive. Queue identity also matters, since `remove_waiter` locates entries by
`Arc::ptr_eq`.
Measured on an M1 Pro, release, single task, no contention, control 52-54ns:
operation steps/op before after primitive work
mutex lock/unlock 2 167ns 111ns 45ns -> 7ns
rwlock write 2 163ns 113ns 42ns -> 10ns
semaphore acquire 2 163ns 112ns 41ns -> 8ns
mpsc send+recv 4 353ns 250ns 110ns -> 43ns
"primitive work" is cost above what the same number of bare scheduling decisions
takes. This beat the 33ns the allocation alone accounts for, because the fast path
also stopped touching `SeqCst` atomics: `has_permits` and `is_queued` are plain
bool reads when no waiter exists.
Contributor
Author
|
Rebased now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Been looking a bit at performance, trying to get the hot paths down. Here's a commit that makes
BatchSemaphoreacquires faster always, and one commit that makes them faster when there is no contention (by not creating aWaiter)Acquire::pollran this in release builds on every poll:That takes a
std::sync::Mutexto check an internal invariant, on theuncontended fast path, for every
Mutex::lock,RwLock::read/write,Semaphore::acquire, and every channel send and recv in the program under test.Make it a
debug_assert_eq!.Two smaller cleanups alongside it: the
will_succeedcomputation took twoseparate
RefCellborrows of the semaphore state (is_closed, thenavailable_permits) where one suffices and gives a single consistent snapshot,and
release()computedExecutionState::me()eagerly for atrace!argument,paying an
ExecutionState::withon every release even with tracing disabled.Measured on an M1 Pro, release, single task, no contention, against an identical
61ns per-scheduling-decision control:
Attribution: restoring only the
assert_eq!while keeping the two cleanupsreturns the numbers to baseline, so the assertion accounts for essentially all of
it. An isolated
std::sync::Mutexlock/unlock measures 10.4ns, so the remainderis presumably the barrier interacting with the surrounding
SeqCstatomics.Note this only helps release builds, since
debug_assertstays active under adefault `cargo test
perf(semaphore): allocate the waiter only when an acquire blocks
BatchSemaphore::acquiredidArc::new(Waiter::new(num_permits))unconditionally, so every
Mutex::lock,RwLock::read/write,Semaphore::acquire, and every channel send and recv did a heap allocation.The
Arcexists because a blocked acquire is shared between two owners: theAcquirefuture on the blocking task's stack, and the semaphore'swaiters: VecDeque<Arc<Waiter>>, which the releasing task reaches into to flipis_queued/has_permits, read the clock, and take the waker. An acquire thatgets its permits immediately is never enqueued and never observed by another
task, so it needs none of that sharing.
Acquirenow holdsnum_permits,task_id,clockand a plainhas_permitsbool inline, with
waiter: Option<Arc<Waiter>>allocated in the one branch thatenqueues.
has_permits(),is_queued()andgrant_permits()read the inlinecopies while no waiter exists and the shared ones once it does, since from then
on the releasing task owns those writes.
task_idandclockare still snapshotted inAcquire::new, exactly as before,and moved into the
Waiterat enqueue. This is load bearing:waiter.clockfeedsthe happens-before edge recorded in
unblock_waiters_from_front, and athread::switch()sits between construction and blocking, so reading the clock atenqueue time instead would silently change the happens-before graph that vector
clocks use to detect races.
A
Waitercannot instead live on theTaskand be reused, because a task canhave several acquires outstanding at once:
future::batch_semaphorealready has atest where one task drives two
lock()futures for the same semaphore throughFuturesUnordered, andupgrade()polls anAcquirethen releases while it isstill alive. Queue identity also matters, since
remove_waiterlocates entries byArc::ptr_eq.Measured on an M1 Pro, release, single task, no contention, control 52-54ns:
"primitive work" is cost above what the same number of bare scheduling decisions
takes. This beat the 33ns the allocation alone accounts for, because the fast path
also stopped touching
SeqCstatomics:has_permitsandis_queuedare plainbool reads when no waiter exists.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.