-
Notifications
You must be signed in to change notification settings - Fork 7.7k
feat(tui): pace catch-up stream chunking with hysteresis #10461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Keep baseline streaming behavior unchanged: the 50ms commit tick still drains one line per tick in smooth mode. Extract adaptive chunking policy and commit-tick orchestration from ChatWidget into dedicated streaming modules: - codex-rs/tui/src/streaming/chunking.rs - codex-rs/tui/src/streaming/commit_tick.rs Chunking remains source-agnostic and queue-state driven. It enters catch-up on queue depth/age pressure, exits via hysteresis holds, and uses bounded batch drains to reduce lag without single-frame bursts. ChatWidget now delegates commit-tick execution through the streaming orchestrator while preserving lifecycle behavior (StartCommitAnimation/StopCommitAnimation and newline ordering). Add documentation for policy behavior, constant tuning, and debug flow in rustdoc and companion docs under docs/. Tested with: - just fmt - cargo test -p codex-tui
Preserve smooth-mode behavior while reducing backlog lag under bursty streaming. - keep smooth mode at one-line-per-tick draining - switch catch-up drain planning to batch the current queued backlog each catch-up tick - keep entry/exit hysteresis and re-entry hold behavior to prevent mode flapping - update chunking review/tuning docs to match the policy Validation: - just fmt - cargo test -p codex-tui
Reduce smooth-mode visual stepping by aligning commit tick cadence with frame-limited rendering. - add COMMIT_ANIMATION_TICK in app commit-animation scheduling - change baseline commit tick from 50ms to ~16.7ms (~60fps) - update chunking docs with the current baseline and tuning guidance - add validation history notes describing tested tuning passes and observed outcomes Validation: - just fmt - cargo test -p codex-tui
Increase the TUI frame-rate ceiling and align dependent scheduling with that cadence. - set frame limiter min interval to ~8.3ms (120fps) - expose a shared TARGET_FRAME_INTERVAL and reuse it for commit animation ticks and pager delayed redraw scheduling - update frame requester tests and docs to reflect the new cadence - update stream-chunking docs/validation notes with this tuning pass Validation: - just fmt - cargo test -p codex-tui
6284a0f to
db35d2a
Compare
wiltzius-openai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice :)
| //! | ||
| //! This is intentionally a small, pure helper so it can be unit-tested in isolation and used by | ||
| //! the async frame scheduler without adding complexity to the app/event loop. | ||
|
|
||
| use std::time::Duration; | ||
| use std::time::Instant; | ||
|
|
||
| /// A 60 FPS minimum frame interval (≈16.67ms). | ||
| pub(super) const MIN_FRAME_INTERVAL: Duration = Duration::from_nanos(16_666_667); | ||
| /// A 120 FPS minimum frame interval (≈8.33ms). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you think its worth checking the actual hardware refresh rate vs always using 120? just curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NNot simple to do in a way that makes sense for this. We can probably make it user configurable is this results in high cpu (though there's other things we could do to drive this down first)
Summary
streaming/chunking.rsandstreaming/commit_tick.rsTesting