perf: stop reading the environment on every block and every poll - #322
Open
sarsko wants to merge 1 commit into
Open
perf: stop reading the environment on every block and every poll#322sarsko wants to merge 1 commit into
sarsko wants to merge 1 commit into
Conversation
`backtrace_enabled()` was `std::env::var(CAPTURE_BACKTRACE).is_ok()`, evaluated on
every call. It is called from `Task::block` and `Task::sleep`, so on every time a
task blocks and every time a future returns `Poll::Pending`. `std::env::var` takes
a lock on the process environment and allocates a `String` to return.
Read it once into a `OnceLock`. The variable is documented as a debugging aid
("should only be set when debugging a failing test"), nothing sets it mid-process,
and the sibling `SHUTTLE_RANDOM_SEED` is likewise read once per run.
Found by sampling profile rather than by reasoning: `std::sys::env::unix::getenv`
showed up at 6.8-9.0% of self time on lock-heavy workloads, which is also why a
chunk of `alloc::alloc` was there.
Measured on an M1 Pro, release, warmed, best of three runs each themselves a
best-of-three over 200 schedules:
workload before after
work_queue 1.009ms 0.922ms -8.6%
semaphore 2.504ms 2.206ms -11.9%
transfers 1.252ms 1.234ms -1.4%
The spread matches what the profile predicted per workload (6.8%, 9.0%, 2.0%
respectively): the more a workload blocks, the more it was paying.
Follow-up not included here: `RandomScheduler::new_execution` reads
`SHUTTLE_ALWAYS_PERSIST_SEED` once per execution. That is far less hot than this
was, but it is the same pattern.
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.
Problem
backtrace_enabled()was evaluated fresh on every call:It is called from
Task::blockandTask::sleep, so on every time a task blocksand every time a future returns
Poll::Pending.std::env::vartakes a lock onthe process environment and allocates a
Stringto return, so a program under testthat blocks frequently was paying for both on nearly every scheduling step.
Change
Read it once into a
OnceLock.The variable is documented as a debugging aid — "Capturing backtraces is quite
expensive, so this should only be set when debugging a failing test" — nothing in
the tree sets it mid-process, and the sibling
SHUTTLE_RANDOM_SEEDis likewiseconsulted once per run. So reading it once matches the intent.
Measurements
Apple M1 Pro,
--release, warmed, minimum of three runs each of which is itself abest-of-three over 200 schedules:
How it was found
A sampling profile, not reasoning.
std::sys::env::unix::getenvshowed up at 6.8%of self time on the shared-queue workload and 9.0% on the semaphore one, and it also
accounted for a chunk of the
alloc::allocsamples sitting next to it.The per-workload spread is a useful sanity check on the attribution: the profile
predicted 6.8% / 9.0% / 2.0% for the three workloads above, and the measured
improvements came out at 8.6% / 11.9% / 1.4%. The more a workload blocks, the more
it was paying, which is what you would expect from something charged per block and
per pending poll.
Worth noting that several rounds of ablation on this hot path never found it,
because ablation can only test hypotheses you think to have, and "we read an
environment variable per block" was not one of mine.
Testing
cargo nextest run --release --workspace— 644 passed, 24 skippedcargo test --release --doc --workspace— passescargo fmt --all -- --checkandcargo clippy --all-targets -- -D clippy::all— cleancargo doc --no-deps— no new warningsFollow-up, not in this PR
RandomScheduler::new_executionreadsSHUTTLE_ALWAYS_PERSIST_SEEDonce perexecution. That is far less hot than this was — per execution rather than per step,
so it did not register in the profile — but it is the same pattern and could get the
same treatment.