Repository navigation
Update: forbid sleeping on the dispatch path, and stop sleeping on it - #1499
Merged
Merged
Conversation
Codestyle rule 5 banned `yield()` in AICPU spin-waits but said nothing
about the host, so a sleep-based backoff on a mailbox poll read as
acceptable. It is not: a sleep quantum lands on every dispatch that
arrives mid-quantum, and no tuning recovers it, because a sleep interval
on a general-purpose OS overshoots what was asked for — a 50 us request
measures ~102 us on this box.
Generalize the rule. A wait that a task's latency passes through may only
spin or block on a wakeup primitive (semaphore, futex, condition
variable, pipe/eventfd read). Initialization and teardown are exempt and
may poll with a sleep, since a bring-up handshake or a shutdown drain
costs a one-off wait rather than per-task latency. The boundary is "does a
task wait behind this?" rather than which tier or file the wait lives in,
so a host completion poll lands on the same side as an AICPU ticket lock
while `close()` reaping lands on the other.
`LocalMailboxEndpoint::run` was the one place in the hierarchical worker
that broke this: it slept 50 us between `TASK_DONE` polls, on the
critical path of every dispatch. Its sibling `CONTROL_DONE` wait in the
same file already spins and samples child liveness against a
`kChildLivenessPollPeriod` wall clock, so the dispatch wait now matches
it. Liveness has to move off the iteration count for the same reason the
old comment gave for using one — "~10 ms at the 50 us sleep below" — that
mapping only held while the loop slept.
Measured, 3 sub workers with no-op callables:
before after
run() median 150.2 us 50.7 us
run() p90 152.3 us 51.6 us
run() max 175.9 us 94.1 us
burst, per task 106.0 us 11.0 us
`kChildLivenessPollInterval` loses its last caller and goes with it.
The trade is CPU: a parent thread now spins for as long as its child
holds a task, so W concurrently-dispatched workers keep W threads busy
(measured 2.98 cores for 3 outstanding dispatches). On the dispatch path
that is what the rule asks for, and it does not degrade under core
pressure — `tests/ut/py/test_worker` plus `test_hostsub_fork_shm.py` run
342 passed in 54.3 s pinned to 3 cores against 52.6 s unpinned. Removing
the CPU cost as well needs a blocking wakeup primitive on both ends of
the mailbox, which is tracked separately and is what the rule's
"spin *or* block" is pointing at.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
This was referenced Jul 26, 2026
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 27, 2026
After hw-native-sys#1499 took a single `Worker.run()` from 150 us to ~51 us, the obvious follow-up was whether another factor of three was available in what remained, and whether a blocking wakeup primitive (hw-native-sys#1498) was how to get it. Measured, and the answer to both is no. Recording it so the next person does not re-derive it. A bare fork+shm mailbox round trip with no simpler runtime in it costs 3.5 us against `Worker.run()`'s 53.8 us, which reads as "94% of dispatch latency is our code, not the IPC". That headline is an artifact of benchmarking a single task. Sweeping tasks per `run()` from 1 to 256 splits it into **43.3 us fixed per run() and 8.08 us marginal per task**: the cost is entering and leaving an orchestration, not dispatching work, and a production run submits a graph per `run()` rather than one task. At 256 tasks the fixed part is 2% of the invocation. The same numbers settle hw-native-sys#1498 against itself on latency grounds. A pipe wake measures 4.4 us one way on this box, so replacing the poll with a blocking wait adds more than the entire 3.5 us IPC floor to every dispatch, in exchange for CPU, while the latency it was meant to recover is not in the poll at all. What survives is narrower and unrelated: a child spinning while nothing is outstanding holds a core for as long as its parent lives — a CPU question for narrow-core hosts, already smaller since hw-native-sys#1495 made orphans exit with their parent. Also records the reconsideration triggers, since both conclusions are conditional: a workload that issues many small `run()` calls stops amortizing the fixed cost, and the CPU argument for a spin-then-block hybrid remains open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Jul 27, 2026
After #1499 took a single `Worker.run()` from 150 us to ~51 us, the obvious follow-up was whether another factor of three was available in what remained, and whether a blocking wakeup primitive (#1498) was how to get it. Measured, and the answer to both is no. Recording it so the next person does not re-derive it. A bare fork+shm mailbox round trip with no simpler runtime in it costs 3.5 us against `Worker.run()`'s 53.8 us, which reads as "94% of dispatch latency is our code, not the IPC". That headline is an artifact of benchmarking a single task. Sweeping tasks per `run()` from 1 to 256 splits it into **43.3 us fixed per run() and 8.08 us marginal per task**: the cost is entering and leaving an orchestration, not dispatching work, and a production run submits a graph per `run()` rather than one task. At 256 tasks the fixed part is 2% of the invocation. The same numbers settle #1498 against itself on latency grounds. A pipe wake measures 4.4 us one way on this box, so replacing the poll with a blocking wait adds more than the entire 3.5 us IPC floor to every dispatch, in exchange for CPU, while the latency it was meant to recover is not in the poll at all. What survives is narrower and unrelated: a child spinning while nothing is outstanding holds a core for as long as its parent lives — a CPU question for narrow-core hosts, already smaller since #1495 made orphans exit with their parent. Also records the reconsideration triggers, since both conclusions are conditional: a workload that issues many small `run()` calls stops amortizing the fixed cost, and the CPU argument for a spin-then-block hybrid remains open.
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
1 participant
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.
The rule
Codestyle rule 5 banned
yield()in AICPU spin-waits but said nothing about the host, so a sleep-based backoff on a mailbox poll read as acceptable. It is not: a sleep quantum lands on every dispatch that arrives mid-quantum, and no tuning recovers it, because a sleep interval on a general-purpose OS overshoots what was asked for — a 50 us request measures ~102 us on this box.Generalised:
The boundary is "does a task wait behind this?", not which tier or file the wait lives in: a host completion poll lands on the same side as an AICPU ticket lock, while
close()reaping lands on the other.The violation it names, fixed here
LocalMailboxEndpoint::runwas the one place in the hierarchical worker that broke this —sleep_for(50us)betweenTASK_DONEpolls, on the critical path of every dispatch.Its sibling
CONTROL_DONEwait in the same file already spins, sampling child liveness against akChildLivenessPollPeriodwall clock. The dispatch wait now matches it, so the file has one pattern instead of two.Liveness had to move off the iteration count for exactly the reason the old comment gave for using one — "~10 ms at the 50 us sleep below". That mapping only held while the loop slept; at spin speed 200 iterations is microseconds, and
waitpid()would be called thousands of times more often.kChildLivenessPollIntervalloses its last caller and goes with it.Measured
3 sub workers, no-op callables, aarch64 Linux:
run()medianrun()p90run()maxThe trade, stated
A parent thread now spins for as long as its child holds a task, so W concurrently-dispatched workers keep W threads busy — measured 2.98 cores for 3 outstanding dispatches.
On the dispatch path that is what the rule asks for, and I checked it does not degrade under core pressure, since a spinning parent plus spinning children is exactly the oversubscription case to worry about on a narrow CI runner:
Removing the CPU cost as well needs a blocking wakeup primitive on both ends of the mailbox — #1498, which is what the rule's "spin or block" is pointing at. Until then, spinning is the compliant choice and the latency win is available now.
Verification
pytest tests/ut/py -q→ 824 passed, 2 skipped.pytest examples tests/st --platform a2a3sim --device 0-7 -q→ 110 passed, 2 skipped, 0 failed, 60/60 groups. This one matters:worker_manager.cppis the L2/L3 dispatch path.pre-commitclean (clang-format, clang-tidy, cpplint, markdownlint).🤖 Generated with Claude Code