Coalesce duplicate BasicExecutor task wake-ups - #669
Conversation
|
Hey @Dylan-Gallagher, thank you for the PR! I looked into #666 and with the additional example provided by the reported I was able to reproduce and find out the cause. You can check out the issue for the explanation. As it stands, this PR would not solve the issue (I have tested), it's not really something we can solve easily in rclrs because it is a tokio behavior. |
espressolee
left a comment
There was a problem hiding this comment.
Reviewed exact head a0a85afb6159486b71450c130ef40c34fe53a707 against exact base 7ec55378e123df42f3671751161b9fbd046c4214. Current main is now 4606ed41a212fb6c65ab01d1e79ddabde4e7cb05, but basic_executor.rs is unchanged across that base drift and the merge tree is clean.
I agree with the existing discussion that this does not solve #666's Tokio cooperative-budget loop. I modeled the submitted Task/ArcWake/std::mpsc state machine independently under the project's Rust 1.85 MSRV and checked the narrower queue-coalescing claim:
- 1,000 baseline burst wakes produce 1,000 queue entries; this head produces 1
- 16 threads × 1,000 wakes still produce one outstanding entry
- a cross-thread wake delivered while
Future::pollis blocked schedules the required second poll - a synchronous wake immediately before
Poll::Readyleaves one harmless no-op entry, which drains cleanly - a stale waker remains coalesced after completion
- receiver disconnect resets
queuedafter the failed send - an 8-thread, 16,000-generation wake storm eventually observes the final generation without a lost wake
The seven hostile tests passed 100 consecutive runs (700/700 total executions), with Rust 1.85 fmt and clippy -D warnings clean for the isolated model. The exact-head CI is green on Humble, Jazzy, Kilted, Rolling, and Windows; the stable Linux log reports 121 unit tests and 51 doc tests passing on each ROS distribution.
The important high-level limit is also deterministic: a future that self-wakes once on every poll and returns Pending still executes 1,000/1,000 polls with the ready queue remaining non-empty. Coalescing cannot affect the one-wake-per-poll Tokio coop-budget behavior described in #666.
I found no correctness blocker in the narrow queue-entry invariant. The remaining decision is whether the extra atomic on every wake is justified by a real burst-wake workload independent of #666. A representative benchmark or queue-depth/poll-count measurement would establish that value; otherwise this is a correct but currently unquantified optimization. The upstream regressions are synchronous, so a cross-thread wake-during-poll test would also strengthen the concurrency contract.
Local limitation: the repository's documentation-only ROS shim does not provide the generated message types needed to compile the full rclrs test target, and the local Docker daemon was unavailable. I therefore rely on the exact-head remote ROS matrix for full-crate qualification and keep the local claim bounded to the changed task state machine.
Summary
BasicExecutorRuntimetask already has a ready-queue entryContext
The profile in #666 points at the task send/receive/waker path. The blocking
receiver does sleep when the ready queue is empty in a minimal idle executor,
but
Task::wake_by_refcurrently enqueues the same task on every notification.A burst of notifications can therefore keep redundant task entries in the
queue and cause repeated polls after the future has already observed the latest
state.
Rust's waker contract allows multiple wake-ups before the next poll to be
coalesced. This change uses an atomic queued flag to allow only one outstanding
ready entry per task. The flag is cleared before
Future::poll, which ensuresthat a wake delivered during the poll still schedules another poll.
I could not reproduce the issue's exact 28% CPU result without the reporter's
full async service workload, so this PR makes the narrower claim: it removes a
deterministically reproduced queue-amplification mechanism in the profiled hot
path. A two-second minimal idle spin was already near zero CPU before the
change.
Testing
cargo test -F default,serde executor::basic_executor::tests -- --test-threads=1cargo test -F default,serde -- --test-threads=1colcon build --packages-select rclrscargo fmt --all -- --checkrustfmt +nightly --edition 2021 --check rclrs/src/executor/basic_executor.rsgit diff --checkThe burst-wake regression demonstrates that 1,000 wake notifications result in
one outstanding ready entry. The second regression verifies that a synchronous
self-wake during the first poll still produces the required second poll.
Relates to #666.