Skip to content

fix(kio): split waiters by condition so writes don't churn closed/consumer waiters#1739

Merged
kixelated merged 1 commit into
mainfrom
claude/tender-ellis-ca7120
Jun 14, 2026
Merged

fix(kio): split waiters by condition so writes don't churn closed/consumer waiters#1739
kixelated merged 1 commit into
mainfrom
claude/tender-ellis-ca7120

Conversation

@kixelated

@kixelated kixelated commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

kio used a single waiter list, so every event woke every parked waiter. The expensive case is a per-modification write (the hot path): it woke closed(), used(), and unused() waiters that don't care about value changes, and conversely consumer create/drop woke the value waiters. Those spurious wakes just re-register and ping-pong, which is the root cause behind the per-poll consume() churn.

This splits State into three lists, keyed by what each waiter actually waits on:

List Waiters Woken by
waiters_value poll/wait every modification (hot path) + close
waiters_closed closed() close only
waiters_consumer used()/unused() consumer 0↔1 + close

Close still wakes all three, since every waiter resolves on close. No other event over-wakes.

Why these groupings

  • used/unused share one list. Every call site uses them sequentially (used().await, then a loop selecting on unused()), so they're never parked on the same channel simultaneously and a combined list never cross-wakes in practice. A separate unused list would add cost for no benefit.
  • closed() gets its own list. It's heavily used (87 call sites) and parked long-term in select! blocks. Sharing the value list meant it ate a spurious wake on every frame write — a bigger churn source than used/unused.

Also

  • Added a [Unreleased] CHANGELOG entry.
  • Added waker-counting regression tests asserting each event wakes only the waiters that care: value writes don't wake closed()/unused(), consumer churn doesn't wake value/closed() waiters, and close wakes all three.

Relation to #1735

Rebased on top of #1735 (read-only-predicate poll/wait rework). That PR fixed the "a no-op mutation during a pending poll wakes the poller's own waiter" footgun by making the predicate read-only; this PR fixes the orthogonal "every event wakes every waiter" problem by splitting the list. The two compose: poll/wait now register in waiters_value, and a write through the returned Mut wakes only that list (plus closed/consumer on close).

Public API

No public API change — State and its waiter lists are pub(crate). Internal behavior-preserving fix, hence targeting main.

Tradeoff

State now holds three WaiterLists (each ~256 B inline via INLINE_WAITERS = 32), so it grows ~512 B vs. the single-list original. waiters_value/waiters_closed both scale with subscriber count, but waiters_consumer is almost always 0–1 waiters, where inline-32 is waste. A clean follow-up is making WaiterList generic over its inline size and giving the consumer list a tiny one; left out here to keep the change focused.

Test plan

(Written by Claude)

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 323e0a5d-f00b-4225-9d2e-f34c41e0f943

📥 Commits

Reviewing files that changed from the base of the PR and between 520e79c and f7d4fa8.

📒 Files selected for processing (6)
  • rs/kio/CHANGELOG.md
  • rs/kio/src/consumer.rs
  • rs/kio/src/lib.rs
  • rs/kio/src/producer.rs
  • rs/kio/src/tests.rs
  • rs/kio/src/weak.rs

Walkthrough

The change replaces a single unified WaiterList on the shared State<T> struct with three category-specific lists: waiters_value, waiters_closed, and waiters_consumer. A new take_close_waiters() method drains all three lists at once for close-event dispatch. Every poll and drop site across Producer, Consumer, and Weak is updated to register and wake only the matching category list. Mut::drop unconditionally wakes waiters_value and additionally wakes waiters_closed and waiters_consumer when a mutation closes the channel. Four regression tests using a CountWaker helper verify that events in one category do not spuriously wake waiters in another.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The linked issue #39 concerns license change to MIT, which is completely unrelated to this PR's waiter-splitting performance optimization. Verify the correct linked issue; this PR should reference performance or waiter-related issues, not a license change issue.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately summarizes the main change: splitting waiters by condition to prevent spurious wakes from writes affecting irrelevant waiter lists.
Description check ✅ Passed Description is comprehensive and directly related to the changeset, explaining the problem, solution, design rationale, and testing approach.
Out of Scope Changes check ✅ Passed All changes (waiter list splitting, test additions, CHANGELOG updates) are directly related to the stated objective of optimizing waiter notification behavior.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/tender-ellis-ca7120

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated
kixelated enabled auto-merge (squash) June 14, 2026 21:33
…sumer waiters

A single waiter list meant every event woke every parked waiter. A
per-modification write (the hot path) woke `closed()`, `used()`, and
`unused()` waiters that don't care about value changes, and consumer
create/drop woke the value waiters. Those spurious wakes re-register and
ping-pong, which is the root cause behind the per-poll consume() churn.

Split `State` into three lists keyed by what each waiter waits on:

- waiters_value    -> poll/wait, woken on every modification (+ close)
- waiters_closed   -> closed(), woken only on close
- waiters_consumer -> used()/unused(), woken on consumer 0<->1 (+ close)

Close still wakes all three, since every waiter resolves on close.

`used`/`unused` share one list: every call site uses them sequentially
(used().await, then a loop selecting unused()), so they're never parked
on the same channel at once and a combined list never cross-wakes.

`closed()` gets its own list because it's heavily used (87 call sites)
and parked long-term, so sharing the value list churned it on every
frame write.

Also drop `[lib] test = false`, which silently skipped the inline tests
on a plain `cargo test -p kio` (CI's --all-targets ran them, so this was
a local footgun only), and add waker-counting regression tests that
assert each event wakes only the waiters that care.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kixelated
kixelated force-pushed the claude/tender-ellis-ca7120 branch from a66a3a9 to f7d4fa8 Compare June 14, 2026 21:49
@kixelated
kixelated merged commit 277c239 into main Jun 14, 2026
1 check passed
@kixelated
kixelated deleted the claude/tender-ellis-ca7120 branch June 14, 2026 21:59
@moq-bot moq-bot Bot mentioned this pull request Jun 14, 2026
kixelated added a commit that referenced this pull request Jun 14, 2026
…sumer waiters (#1739)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kixelated added a commit that referenced this pull request Jun 15, 2026
Reconciles main's new features with dev's reshaped APIs:

- moq-mux codec imports: combined main's `TrackProvider` (import onto an
  existing track, #1684) with dev's per-track timescale. The timescale is
  baked into `TrackProvider::create()` so unique tracks publish at the
  native container timescale; fixed tracks keep the caller's info. Added a
  `FixedTrackReconfigured` error variant per codec to replace the
  anyhow::bail in the crate::Result paths.
- kio: dev's new `Weak::poll_closed` now registers on main's split
  `waiters_closed` list (#1739) instead of the removed single `waiters`.
- js/watch: re-implemented main's distance-based `visible` download control
  (#1744) on top of dev's input/output signal architecture. The renderer
  takes `visible: Visible` as an input and emits the boolean `output.visible`;
  the backend keeps owning the paused/enabled gating.
- noq/websocket: bumped web-transport-noq to 0.2 (#1741, #1743) and qmux to
  0.1.3, then mapped qmux's new `Error::Http(status)` to
  `ConnectError::Unauthorized` so the WebSocket fallback surfaces auth
  rejections like the QUIC backend.
- Cargo: took the newer of each shared dep (web-async 0.1.4, iroh 0.6,
  noq 0.2); Cargo.lock based on dev's to avoid gratuitous transitive bumps.
- Brought the test code main introduced (and dev's stale integration tests)
  up to dev's current API (create_track(name, info), announced().next(),
  track.name(), with_publisher/with_subscriber, moq_net::Timestamp).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@moq-bot moq-bot Bot mentioned this pull request Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant