Performance improvements on semaphore acquires - #1
Open
clemenslosbichler-cloud wants to merge 2 commits into
Open
Performance improvements on semaphore acquires#1clemenslosbichler-cloud wants to merge 2 commits into
clemenslosbichler-cloud wants to merge 2 commits into
Conversation
`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.
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
2 participants
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.
Replay of awslabs#321 for ContextGoblin review. Not for merge.