perf(grid): ring-buffer row indexing — O(1) full-screen scroll#53
perf(grid): ring-buffer row indexing — O(1) full-screen scroll#53tonyfettes wants to merge 1 commit into
Conversation
The active screen stored rows in parallel `FixedArray`s indexed directly by logical row, so a full-screen scroll (the line-feed hot path) physically slid every surviving row up one slot: `descs`/`cells`/`link_ids` `unsafe_blit`s of O(rows), with the two reference arrays paying a per-row incref/decref that nets to zero — a move treated as a copy. Profiling put `shift_full_width_up` at ~46% of the `scroll_full` workload (blit/memmove 13.5%, incref 6.1%, decref 2.1%). Add a `base : Int` ring offset to the grid view; logical row `r` lives at `phys(r) = base + r` (single conditional subtract, no `%`). A whole-screen scroll now bumps `base` and blanks only the freed rows — O(count) instead of O(rows), with no ref-array traffic at all. Sub-region (margin) scrolls keep a per-row reference move (physical slots may wrap, so no single blit); the generic column-range path is unchanged. The cleanup/blank/dirty passes are preserved verbatim, so every scroll produces a logically identical screen — only the physical storage layout rotates. `base` lives on the grid view, not the shared page storage: the active page's arrays rotate in place, but only a grid view ever indexes them, and `pagelist` always does so through a passed view's new public `phys_row`. The one long-lived view is mutated in place by scrolls; `active_grid` mints a fresh `base: 0` view only where the page storage is fresh or fully blank (construction, post-reset re-fetch, post-resize rebuild), so a zero offset is always correct there. `full_reset` normalizes `base` to 0. All logical→physical translation is centralized in `phys` and the desc/cells/link accessor helpers. scroll_full 4.25ms → 3.84ms (−10%), scroll_storm 714µs → 621µs (−13%), cjk −5%, colored_log −3.5%; plain_lines / mixed_realistic flat; all 569 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1814f61b0f
ℹ️ 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".
| // A fresh view starts unrotated. Sound only at the call sites that re-fetch | ||
| // (construction / post-`full_reset` / post-resize), where the page storage is | ||
| // fresh or fully blank — see the INVARIANT on `ScreenGridState.base`. | ||
| base: 0, |
There was a problem hiding this comment.
Preserve the current ring offset when rebuilding a grid view
When active_grid() is called after a full-screen scroll, the active page arrays may already be ring-rotated by ScreenGridState.base, but this public method always returns a new view with base: 0. Any caller that re-fetches the active grid outside the narrow construction/reset/resize paths will then read physical row 0 as logical row 0, so snapshots, formatting, or subsequent writes can observe rows in the wrong order after scrolling. The offset needs to be stored/restored with the page or this API should not be able to mint a fresh zero-base view over rotated non-blank storage.
Useful? React with 👍 / 👎.
What
Make a full-screen scroll O(1) instead of O(rows). The active screen stored
rows in parallel
FixedArrays indexed directly by logical row, so everyline-feed physically slid all surviving rows up one slot: three
unsafe_blits,and the two reference arrays (
cells,link_ids) paid a per-rowincref/decref that nets to zero — a move treated as a copy. Profiling put
shift_full_width_upat ~46% of thescroll_fullworkload (memmove 13.5%,incref 6.1%, decref 2.1%).
How
Add a ring-buffer offset
base : Intto the grid view; logical rowrlives atphysical slot
phys(r) = base + r(single conditional subtract, no%).base, blank only the freedrows. O(count), zero ref-array traffic.
around the ring, so a single blit won't do).
The cleanup / blank / dirty passes are preserved verbatim, so every scroll
produces a logically identical screen — only the physical storage layout rotates.
All logical→physical translation is centralized in
phys(#inline) and thedesc/cells/link accessor helpers.
Where
baselivesOn the grid view, not the shared page storage. The active page's arrays
rotate in place, but only a grid view ever indexes them, and
pagelistalwaysdoes so through a passed view's new public
phys_row. The one long-lived view ismutated in place by scrolls;
active_gridmints a freshbase: 0view only wherethe page storage is fresh or fully blank (construction, post-
full_resetre-fetch, post-resize rebuild), so a zero offset is always correct there —
documented as a loud INVARIANT on the field.
full_resetnormalizesbaseto 0.(An earlier attempt shared
baseas aRef[Int]between page and view forrobustness, but the per-cell heap-deref regressed
plain_lines+3% /wrapped_blob+6%. A plain field — a struct-field load — keepsplain_linesflat and nearly doubles the scroll win, since the O(rows) cleanup/dirty loops
also resolve
base.)Benchmarks
Interleaved, thermally controlled (
moon bench --release --target native):The residual +2–3% on
wrapped_blob/tui_redrawis the intrinsic cost of theoffset arithmetic on per-row-access-heavy wrap/clear paths — profiling confirmed
it is the translation work itself, not call overhead (inlining
physcleared itfrom the profile without moving wall-clock). Not worth removing the translation
(that is the whole optimization), and the representative
mixed_realisticandthe common
plain_linesare both flat.Tests
All 569 tests pass. White-box grid helpers and
pagelistreaders route throughphys_row; the cleanup/blank/dirty semantics are unchanged.🤖 Generated with Claude Code