perf(render): reuse clean rows across pure vertical scroll in FrameSnapshot - #54
Merged
Conversation
… reuse FrameSnapshotReuseKey included row_base/abs_row_base, so a scrolling viewport (a `cat` flood, build logs) changed the key every frame and clean-row reuse was fully defeated, even though almost every visible row was unchanged content that merely moved. `from_screen_recycle` also had `reuse_clean_rows && scroll_shift == 0`, which independently disabled reuse on any scroll regardless of key match. Fix: when the recycle key mismatches only by row_base, and the viewport was `viewport_offset == 0` in both frames (i.e. an auto-following viewport, never touching scrollback), rotate the recycled row buffer by the row_base delta before the ordinary clean/dirty pass. `viewport_offset == 0` in both frames guarantees the entire visible window is always the live grid in both frames — so every realigned slot stays protected by its own per-row dirty bit, regardless of scrollback growth or eviction happening underneath. This is intentionally *not* generalized to a pinned or history-scrolled viewport: a row can migrate from the live grid (dirty-bit protected) into scrollback (trusted unconditionally, no dirty check available) between two snapshots, and a naive row_base-delta realignment can't tell whether that stale slot was ever re-read fresh before the migration. Those cases still safely fall back to a full rebuild (covered by new differential tests: pin-then-scroll-underneath, history navigation, scrollback-clamped viewport, and delta exceeding the viewport height). Measured on crates/noa-render/tests/scroll_reuse_bench.rs (release, 120x40, 4000 snapshots/workload, interleaved git-stash A/B, 5 pairs): per-call `FrameSnapshot::from_terminal_recycle` cost on a pure-scroll flood drops from a ~3.3-5.3us median to a ~0.5-0.9us median (roughly 6-8x), and similarly on a scroll+in-place-rewrite mixed workload (~3.3-4.7us to ~0.5-0.75us). The static (no-scroll) control is unaffected, as expected. bench/'s bench_throughput measures the parser/grid path only and can't see this render-path change, hence the dedicated harness. Whether this per-snapshot win is visible end-to-end depends on what share of a redraw's CPU is snapshot construction versus segmentation/shaping/rasterization/GPU submission on a given frame — not independently re-profiled here.
Independent verification of a28df93 found that realign_recycle_for_scroll inferred a uniform whole-viewport translation from row_base's delta alone. That inference only holds for Screen::scroll_up_region's full-height fast path (the one that calls RingGrid::advance_base). A DECSTBM scroll region that starts at row 0 but excludes the bottom rows (e.g. a fixed status line) also records scrollback growth (records_scrollback_for_region only checks top == 0) and so still moves row_base by n, but never touches the rows below the region — a blind rotate_left(n) handed those untouched clean rows a neighbor's stale content instead of leaving them alone. Gate the realignment on Screen::scroll_shift (incremented only by the full-height path) matching the row_base delta exactly, via a new non-consuming peek_scroll_shift() read taken before the ordinary clean/dirty pass consumes the counter. Any region-scoped scroll now correctly falls back to a full rebuild instead of corrupting rows outside the scrolled region. Added a differential regression test reproducing the corruption (scroll_reuse_does_not_corrupt_fixed_rows_below_a_partial_scroll_region), confirmed it fails without the new guard and passes with it, and reran the scroll_reuse_bench A/B (release, interleaved git-checkout, 5 pairs): the fix does not regress the realignment fast path (scroll median stays sub-microsecond, ~0.3-0.8us across runs) while now safely bailing out on the region-scoped case.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf78969328
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+462
to
+466
| let delta_row_base = new_key.row_base as i64 - old_key.row_base as i64; | ||
| if delta_row_base <= 0 || delta_row_base as usize >= rows_n { | ||
| return false; // hazard 3: no forward overlap survives | ||
| } | ||
| if scroll_shift as i64 != delta_row_base { |
There was a problem hiding this comment.
Once the byte-limited scrollback begins evicting rows (including immediately when scrollback-limit is 0), visible_row_base() stays constant or advances by less than scroll_shift, even though the live grid still undergoes the same whole-viewport translation. Computing the delta from row_base therefore makes this check reject every subsequent flood frame and restores the full-row cloning cost that this commit is intended to eliminate; validate/rotate using the session-absolute base delta (or otherwise account for rows_evicted) so reuse survives normal scrollback eviction.
Useful? React with 👍 / 👎.
Merged
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.
Summary
A scrolling viewport (
catflood, build logs) changedFrameSnapshotReuseKeyevery frame via
row_base, so clean-row reuse was fully defeated even thoughalmost every visible row was unchanged content that had merely moved. When the
key differs only by new output pushing an auto-following viewport forward, the
recycled row buffer is now realigned instead of discarded, cutting
FrameSnapshot::from_terminal_recyclefrom a ~3.3-5.3us median to ~0.5-0.9uson a pure-scroll flood (~6-8x).
Changes
FrameSnapshot::realign_recycle_for_scroll: rotates the recycled buffer bythe
row_basedelta, gated on four provable-safety checks (shape match,viewport_offset == 0in both frames, delta < viewport height, andscroll_shiftequal to the delta). Anything unprovable falls back to a fullrebuild — never to wrong content.
Screen::peek_scroll_shift(): non-consuming read so the gate can inspect thefull-height scroll count before the ordinary clean/dirty pass consumes it.
Requiring it to equal the delta rejects DECSTBM region scrolls, which grow
row_basewithout moving the rows below the region.crates/noa-render/tests/scroll_reuse_bench.rs: dedicated snapshot-pathharness (scroll / static / mixed).
bench/'sbench_throughputmeasures theparser/grid path and cannot observe this change.
Risk
Medium. Touches the render reuse-key invalidation path, whose failure mode is
silent visual corruption. Mitigated by fail-safe gates and seven differential
tests that compare recycled output against a fresh
FrameSnapshot::peekof thesame terminal moment; the partial-scroll-region regression test was confirmed
failing without the guard. Trivially revertable.
Test plan
cargo test --workspacegreen (unsandboxed; noa-ipc/noa-pty included)cargo clippy --workspace --all-targets/cargo fmt --checkcleanmedian, mixed 3.3-4.7us -> 0.5-0.75us, static control unaffected