Repository navigation
perf(tui): cut typing latency when many sessions stream output - #241
Merged
Merged
Conversation
The TUI feeds EVERY session's PTY output into its per-session
history on the main event loop (`on_notification`) — `feed_pty`
(items + a shadow parser, per byte) plus a matrix-rain word
harvest (per byte). When several background sessions stream
heavily, that feed work competes with input handling on the same
loop, so held keys in the focused session queue up behind it and
the cursor hangs then jumps multiple chars.
`crates/e2e/tests/multi_session_latency.rs` (`#[ignore]`,
hardware-dependent timing): drives the real TUI in a PTY,
measures focused-session backspace-burst settle latency with no
background activity, then again while N background sessions
flood (started via IPC, bypassing the TUI which only types into
the focused session).
Measured:
- release, 4 background floods: 2.3ms -> 6.9ms (~3x).
- debug, same: baseline settles; under load the focused burst
does NOT settle within 30s (the loop is fully starved). The
benchmark reports "SATURATED" instead of panicking.
Investigation notes (see PR): the keystroke -> feed -> render
hot path in both `app.rs` and `pty_render.rs` is byte-identical
to v0.7.1 (the only post-0.7.1 change there is the #238
width-change-only zoom fix), so this is NOT a code regression
introduced after 0.7.1 — the mechanism existed in 0.7.1 too.
The ~10x debug amplification (saturation above) is the most
likely explanation for "worse than 0.7.1" if 0.7.1 was run as a
release build.
The matrix rain reveals at most one word per session per PTY_REVEAL_GAP and uses only the most-recent word, but observe_pty_activity scanned the entire PTY chunk on every notification. When a background session floods output, that O(chunk) scan ran on the main event loop for every chunk, competing with focused-session input handling. Scan only the last PTY_HARVEST_TAIL_BYTES (512) of each chunk. Earlier bytes cannot surface anyway (only the most-recent word is revealed), and the pool is already capped at PTY_WORD_POOL_MAX, so cross-chunk accumulation behavior is preserved. Repro (multi_session_latency, release): under-load typing penalty drops from ~3.0x to ~2.0x baseline.
The run_loop select! had no priority, so tokio picked a ready arm at random each iteration. When several sessions flood PTY output, notifications.recv() is almost always ready, so input handling was serviced only ~half the time it competed with the feed work — adding keystroke→render latency in the focused session (the 'hold arrow key, cursor hangs then jumps' symptom). Add `biased;` and move the input arm to the top so a ready keystroke is always handled before draining the background notification batch. Input is human-paced/bursty, so top priority can't starve the lower arms: between keystrokes input_stream is Pending and the notification + tick arms run exactly as before (behavior-preserving). Repro (multi_session_latency, release): under-load typing penalty drops from ~2.0x to ~1.0x baseline.
Add a deterministic gate for each optimization, since the e2e multi_session_latency benchmark can't reliably gate on them (the latency delta lives in a narrow, noisy, hardware-dependent load window — a sustained flood saturates even the fixed build, a bounded flood sometimes finishes before the measurement window). - matrix_rain: pty_activity_harvest_scans_only_the_chunk_tail — a word placed before the 512-byte tail window, with the tail carrying only digits (what a seq-style flood emits), must not be harvested. Fails if the harvest reverts to an O(chunk) scan. - app: run_loop_select_biases_input_ahead_of_notifications — the input-priority win is a tokio scheduling property with no stable runtime probe, so assert the structural invariant that produces it: the run_loop select! is `biased;` with the input arm ahead of the notification-drain arm. Fails if `biased;` is dropped or the arms are reordered.
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
With several large/active sessions loaded, the focused session's typing felt less snappy — holding an arrow key would hang ~0.1–0.2s then jump 3–5 characters. The TUI feeds every session's PTY output into its per-session history on the main event loop (
app.rs::on_notification), plus a shadow parser and a matrix-rain word harvest per chunk. When background sessions stream heavily, that feed work competes with input handling, so held keys queue up.Investigation
The hot keystroke→feed→render path is byte-identical to v0.7.1 (
git diff v0.7.1..HEADonapp.rs+pty_render.rs), and the contention mechanism existed in 0.7.1 too — so this is not a code regression; the release binary feels the same as 0.7.1 (debug amplifies it ~10x into visible saturation). These are opportunistic optimizations to the existing design, not a regression fix.Repro
crates/e2e/tests/multi_session_latency.rs(#[ignore], timing benchmark) measures focused-session backspace-burst settle latency with no background activity vs. with 4 background sessions flooding output. Release baseline: 2.3ms idle → 6.9ms under load (~3.0x).Changes
1. Bound the matrix-rain word harvest to the chunk tail (
matrix_rain.rs)observe_pty_activityscanned the entire PTY chunk on every notification, but the rain reveals at most one word per session perPTY_REVEAL_GAPand uses only the most-recent word. Now it scans only the lastPTY_HARVEST_TAIL_BYTES(512). Earlier bytes can't surface anyway and the pool is already capped, so cross-chunk accumulation behavior is preserved (existing tests stay green). Fleet-wide activity display is unchanged — every active session still reveals at its normal cadence.2. Bias the event loop toward input (
app.rs::run_loop)The
select!had no priority, so tokio picked a ready arm at random — under a floodnotifications.recv()is almost always ready, so input was serviced only ~half the time it competed. Addedbiased;with the input arm first, so a ready keystroke is handled before draining the background notification batch. Input is human-paced/bursty, so top priority can't starve the lower arms (between keystrokesinput_streamis Pending and the notification + tick arms run exactly as before). Behavior-preserving.Measurements (release, multi_session_latency)
Benchmark is hardware-dependent and noisy (values quantize ~2.3/4.6ms), but the under-load penalty consistently collapses from ~3x to ~1x.
Tests
matrix_rainunit tests pass (incl. the cross-chunk accumulation test).cargo test(debug) green, minus the 4 hardware-timing perf-budget tests CI already skips.