Engine track E1-E3: render-state dirty tracking + the measurement that proves it - #306
Merged
Merged
Conversation
A Ghostty-comparison discovery pass surfaced work that the roadmap had
no home for, plus one parity gap the inventory had no row for.
Engine track (E), shared across all three UIs — not M3 slices, since
these span roost-vt/roost-ui-model rather than Iced parity:
* The headline finding: libghostty-vt's render-state dirty tracking
(global + per-row, plus the two-phase begin/end update split) is
exposed in our pinned render.h and wrapped by nobody. All three UIs
walk the full grid on every update. E2/E3 close that.
* E1 measures first: the per-cell fill_text in terminal_widget.rs
looked like an Iced regression and is not one — iced_wgpu caches
shaping, and the Swift renderer is more naive. E4 is conditional on
E1's number rather than assumed.
* E5 records a real gap: roost-iced has no sprite path, so
box-drawing/block glyphs seam where neither shipped UI does. Adds
the missing parity-inventory row.
* E8/E9 (Ghostty pin + zig bump, then a libghostty-rs spike) are
sequenced after the M6 decision — most of their cost is
revalidating the Swift build, which drops sharply if Swift retires.
M6 — macOS Iced (evaluation → parity) supersedes the "Possible
direction" note, upgraded from possible to likely-but-not-committed.
Slices 6a-6h, running parallel to M4. Decisions recorded in 6a:
display name Roost-Iced while app_label stays Roost-iced (it drives
socket/log paths and the identify wire response), fresh separate
state.json, Sparkle deferred to 6c. Side-by-side needs little new
machinery — ROOST_SOCKET already routes Claude hooks per-tab.
Also documents why OSC consolidation is a dead end: libghostty exposes
one payload accessor (window title) across 22 command types, identical
at our pin and at tip, so roost-osc stays. Its module doc said it was
built for the daemon; that premise is obsolete but the conclusion holds
for a different reason, now recorded with an exit condition.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
…y pin Verified against the generated bindings and both headers: the pinned c74f6d5 exposes ghostty_render_state_update plus the dirty get/set on both layers, but no begin_update/end_update — those landed upstream later (present at ../ghostty tip and in ../libghostty-rs, which pins ab0b9da). The original E2 text conflated the tip header I read the two-phase design notes from with the header we actually build against, which would have sent an implementation pass chasing a symbol that does not exist. E2 is now scoped to what the pin actually offers, and gains two notes worth having: no bindgen work is needed (all four dirty constants are already in the generated sys module), and the independent-layers footgun render.h flags is called out explicitly. Two-phase moves to E8, where the pin bump makes it available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C1 (engine track E2). libghostty tracks, per update, exactly
which viewport rows changed — and nobody in roost read it. All three UIs
walk the full grid every frame. This adds the safe wrapper; the consumer
lands in C5.
New surface on RenderState (ffi-gated, like the rest of the type):
Dirty { Clean, Partial, Full }
dirty() — global state, pure read
mark_full() — raise to Full; the ONLY public way to move dirty state
dirty_rows() — which rows are flagged, pure w.r.t. dirty state
walk_dirty() — visit changed rows, consuming the frame's damage
The two dirty layers are independent — render.h calls that "an extremely
important detail": clearing the global state does not clear per-row
flags, and update() only ever raises, never clears. So walk_dirty is the
only thing that lowers, and it clears BOTH layers together. There is
deliberately no public way to clear one layer alone: an earlier draft
exposed set_dirty(Dirty), which would have made set_dirty(Clean) the very
footgun the wrapper exists to prevent. mark_full only raises.
walk() keeps its exact signature and behavior — this is purely additive,
so GTK's renderer is untouched.
14 tests pin semantics measured against our pinned libghostty (probe
output archived with the plan), including the two that will matter later:
- Row flags and the global flag are both cleared by walk_dirty. Two
successive single-row writes visit one row each; if only the global
layer were cleared, the second walk would redraw the first row forever.
- Known-limitation tripwires: OSC 10/11 and DECSCNM change the reported
default colors while reporting Clean with zero rows flagged. That is
why the consumer must compare the default fg/bg pair itself rather than
trusting dirty state (plan D3b). If a Ghostty bump ever fixes this,
these tests fail loudly and can be relaxed.
- Output that scrolls the viewport reports Full over every row, by design
in libghostty (render.zig:299-302, "if our viewport pin changed, we do
a full rebuild"). Streaming output gets no incremental benefit; the
savings are in-place TUI redraws and the non-PTY refresh callers.
Also fixes make test-rust: every roost-vt test is cfg(feature = "ffi"),
so `cargo test --workspace` compiled them and ran none. The local gate
was silently skipping this crate's entire suite — including, until now,
its most safety-critical one.
Self-review finding (codex and cursor both unavailable this session —
rate-limited and unauthenticated respectively): Dirty::from_raw mapped
unrecognized values to Clean, so a variant added by a future Ghostty
would read as "nothing changed" and freeze the screen. Now maps to Full —
guessing "full" only costs a redraw.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C2 (engine track E1). Measurement scaffolding — no rendering behavior changes. The optimization it exists to evaluate lands in C5. Counters: refresh_calls, refresh_nanos, rows_rebuilt, cells_walked, draw_calls, draw_nanos, fill_text_calls. Two scopes, and the split is deliberate: - Per-TerminalTab plain u64 counters are what unit tests assert on. cargo test -p roost-iced runs in parallel and the fixture spawns a real PTY per test, so several tests refresh concurrently — an after-before delta on a shared global would be polluted by whatever else is running. Per-tab counters remove that flake class by construction rather than by serializing the tests. - A process-global AtomicU64 aggregate (Relaxed) that each tab folds into, for reading counters out of a running app over IPC (C3). rows_rebuilt is derived from the actual rebuild rather than hardcoded to self.rows, even though today those are always equal — it has to stay truthful when the rebuild goes incremental in C5, which is the entire point of measuring it now. Two traps recorded in the module doc because both silently corrupt a number nobody re-checks: - iced::window::screenshot re-renders the window, so roostctl screenshot and everything in tools/screenshot/ inflate the draw counters just by having run. Read before screenshotting, or reset after. - draw_* only exist in a running app: TerminalWidget::draw needs a live iced Renderer, which unit tests don't construct. That is also why there is no per-tab equivalent of the draw counters — the widget renders from a snapshot clone with no handle back to the tab that produced it. Overhead is two Instant::now() calls and a few relaxed atomic adds per refresh/draw, against operations that are microseconds to milliseconds. Always on, not feature-gated: numbers we publish should come from the binary we actually ship. snapshot()/reset() are allow(dead_code) until C3 wires them to roostctl. Skipped the simplify pass: the diff is 57 lines across four files plus one self-contained new module, below the threshold where that pass has historically returned anything. Codex and cursor both unavailable this session, so the correctness pass was a self-review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
…e UI
Plan 017 C3 (engine track E1). Exposes C2's counters over the op set so
performance can be measured against the real renderer via roostctl. This
is the only way to get draw-path numbers at all: TerminalWidget::draw
needs a live iced Renderer that unit tests cannot construct.
roostctl render-stats [--reset]
reset zeroes the counters after reading, so a caller can do
read-reset / run workload / read.
Both Rust UIs answer it. roost-iced returns real counters; roost-linux
returns the same struct zeroed, with a comment marking that as a
placeholder until GTK gets instrumented (roadmap E3b). An earlier draft
had GTK refuse the op — two problems with that, both caught in review:
it would have been the first op one Rust UI answers and the other
doesn't, against CLAUDE.md's "one contract, two implementations at
parity"; and there is no error code to refuse with, since the catalogue
is closed and enforced (roost-ipc/src/lib.rs:74-76) and lists nine codes,
none of them "unsupported". Zeroed counters keep the contract uniform and
cost less code.
Ungated, following the tab.dump_resolved precedent (messages.rs:586) —
reading counters is harmless, and gating it would mean the numbers we
publish come from a build nobody ships.
Wire conventions, each matched to existing precedent:
- Params get deny_unknown_fields; the result deliberately does not, so a
newer UI can add counters without breaking older clients (the same
property messages.rs:1679-1686 asserts for TabDumpResolvedResult).
- reset is serde(default), so {"op":"app.render_stats"} with no params
works, as for TabCapturePtyInputParams.
- All seven counters cross the wire as string_int64. Nanosecond
accumulators pass JavaScript's 2^53 safe-integer range, and this is the
repo's existing answer to that; one test pins the encoding at
9007199254740993 specifically.
Golden vectors added for the request and response. The loader is
schema-agnostic, so a vector written with JSON numbers instead of strings
would round-trip happily and hide a schema drift — an extra typed-decode
test in tests/vectors.rs guards that.
Docs updated in both places the repo requires: docs/reference/ipc.md
(messages.rs:1 states the wire types mirror it 1:1) and
docs/reference/cli.md.
The Mac Swift UI is untouched and answers unknown-op, a documented code.
Nothing in this plan touches Swift.
Verified cargo build -p roost-linux explicitly: UiRequest is exhaustively
matched there, so a missing arm is a compile error rather than a runtime
surprise.
Skipped the simplify pass deliberately: this diff mirrors the repo's
per-op boilerplate across five crates, and DRYing it would diverge from
the house pattern every other op follows. Codex and cursor both
unavailable this session; correctness pass was a self-review.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C4 (engine track E1). Builds the harness; captures no numbers.
C7 runs it against this commit and against HEAD back-to-back via git
worktree, so the before/after is thermally fair and reproducible by a
reviewer rather than resting on archived output from two different days.
Two readouts, because neither alone is enough:
- An #[ignore]d in-crate test (make perf-refresh) times refresh_snapshot.
It has to live in-crate: roost-iced has no [lib] target, so an external
bench cannot import its internals.
- roostctl render-stats against a running app (make perf-render-stats)
for the draw path, which is unreachable from tests — TerminalWidget::draw
needs a live iced Renderer.
Three workloads, deliberately not collapsed into one:
W1 pointer-motion storm — refreshes with no terminal mutation, the
shape interactions.rs:1341 produces on every
mouse-motion event. The headline case.
W2 in-place TUI redraw — absolute-positioned writes, no scroll. The
vim/htop shape.
W3 scrolling stream — CONTROL. Expected to stay flat forever:
libghostty full-rebuilds whenever the
viewport pin changes (render.zig:299-302),
so streaming output cannot benefit. A flat W3
is a PASS, not a disappointment — recorded in
the test and the README so nobody "fixes" it.
An earlier plan draft measured only a scrolling TUI, which is precisely
the workload that cannot improve; it would have produced a flat number
and then fed that number into E4's go/no-go.
Baseline from this commit, all three workloads: 32.00 rows rebuilt per
refresh on a 32-row grid — i.e. the whole grid, every time, regardless of
what changed.
tools/perf/README.md states two limits rather than working around them:
presented frame rate is meaningless on a locked or occluded Mac because
macOS throttles presentation, which is why this measures CPU spans and
counters; and iced::window::screenshot re-renders the window, so roostctl
screenshot and all of tools/screenshot/ inflate the draw counters just by
having run.
tools/perf/ is a sibling in tools/README.md, not a fourth tier: layers
1-3 are a capability ladder ("what they can verify") while perf measures
cost, an orthogonal axis — and tools/ already holds non-tier siblings
(roosttest_unit/, shed/, wayland/). The three-layer table is untouched.
No ci.yml paths-filter entry: the `tests` filter gates e2e jobs that
never run anything under tools/perf/, so adding it would trigger the full
GTK+macOS matrix on a README typo and still not run the harness. The
rustcore filter already covers where the in-crate test lives.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C5 (engine track E3), consuming C1's walk_dirty. No user-visible behavior change — same cells, same order, same colors, same tab.dump text. refresh_snapshot previously rebuilt the entire grid on every call: a dense vec![vec![String::new(); cols]; rows] (a String per cell, including blanks), a full walk, a String per DrawCell, and a rows_text concat — and it runs on every mouse-motion event over the terminal (interactions.rs:1341), not just on PTY output. TerminalSnapshot's `cells` + `rows_text` become `grid: Vec<Arc<RenderedRow>>`, indexed by viewport row. Per-row Arcs also make App::view's per-frame snapshot clone O(rows) refcount bumps instead of O(cells) String clones, with no change to the view code. Wrapping the whole snapshot in an Arc would have needed Arc::make_mut, which copy-on-writes everything whenever the view still holds a reference — reintroducing the clone being removed. DrawCell loses its `row` field. Under a per-row grid the row index would exist twice — as the vector index and inside every cached cell — and any disagreement between them is exactly the "right cells, wrong row" failure this change risks. It is now derived from the grid index, so that bug is unrepresentable rather than tested-for. Three guards force a full rebuild; none is optional: - Grid size, keyed on (cols, rows) — both axes. A width-only resize leaves the row count unchanged while every cached row was built for the old column count. - Default fg/bg pair. This is the subtle one: OSC 10/11 and DECSCNM change the terminal's default colors while libghostty reports Clean with zero rows flagged (measured; pinned by tests in roost-vt). Since resolve_colors folds the defaults into every cell that doesn't set its own, a cached row would otherwise freeze at the old text color. Not a bug before this commit — everything rebuilt every time — so E3 would have introduced it. - Theme generation, bumped by set_theme. Belt-and-braces for anything theme-derived that might enter the resolver later; GTK's twin resolver already pulls bold_color. Each guard raises the dirty state before recording its key, so a failed mark_full leaves the key stale and the next refresh retries. Measured with the C4 harness (release), rows rebuilt per refresh: W1 pointer-motion storm 32.00 -> 0.16 W2 in-place TUI redraw 32.00 -> 2.15 W3 scrolling stream 32.00 -> 27.50 (control — must stay high) W3 holding is the expected result, not a miss: libghostty full-rebuilds whenever the viewport pin changes (render.zig:299-302), so streaming output cannot benefit. W1's residual 0.16 is entirely its first refresh, before any grid is cached; the other 199 rebuild zero rows. Tests: element-for-element grid integrity (markers written to rows out of order, with a repeat, comparing the WHOLE rows_text vector after each write — a substring search over the joined dump would pass even with every row off by one); blank-vs-trim_end equivalence that tab.dump depends on; OSC 11 forcing a rebuild; resolved_cells re-deriving rows from grid position. Each was mutation-checked — removing the default-color guard fails its test with 0 rows rebuilt where 32 are expected, confirming the hole is real. Verified beyond unit tests: make e2e-iced, 66 passed / 3 skipped (documented environment skips) against a freshly built binary, and a roostctl screenshot of a 12-row colored test pattern rendering each row at its own index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C6. C5 made refresh_snapshot rebuild only dirty rows; these are the CI tests that stop it silently reverting to a full rebuild, or silently over-rebuilding. All assertions are deltas read from the tab's own TabRenderStats, never the process-global aggregate — cargo test runs in parallel and the fixture spawns a real PTY per test, so a global counter's delta would be polluted by whatever else is mid-refresh. Added: - a single-row write rebuilds exactly that row (+1 row, +100 cells). The cursor is parked on the target row and settled BEFORE the write, since libghostty dirties both the row the cursor leaves and the one it lands on — without that, the test would measure cursor bookkeeping rather than the write. - applying a theme rebuilds every row (+32) - a width-only resize rebuilds every row (+32) — pins the (cols, rows) guard, which a row-count-only guard would miss - scrolling back into history rebuilds every row and changes rows_text - a pointer-motion refresh with no terminal change rebuilds zero rows. This is the headline win of C5, so it gets an explicit test. Already covered by C5 and left alone: no-change-rebuilds-zero, and OSC 11 forcing a rebuild with the resolved colors actually changing. Every test was mutation-checked — a regression test that passes while the behavior is broken is worse than none: - unconditional mark_full broke the zero-rebuild and single-row tests - removing the cached_defaults guard broke the OSC 11 test - removing the refresh in handle_page's LocalViewport branch broke the scrollback test The theme and width-resize guards produced a genuine surprise worth recording: at our pinned Ghostty SHA, Terminal::resize and set_color_* already report Dirty::Full on their own (independently pinned in roost-vt's render_dirty_test.rs), so removing refresh_snapshot's own guards alone does NOT break those tests. Getting a discriminating mutation needed the underlying FFI call stubbed as well, so the Rust-side state moves while libghostty never hears about it — with the guard intact both tests still passed; with it removed on top of the stub, both failed. So those two guards are genuinely belt-and-braces rather than load-bearing today, which is what the plan claimed and this now demonstrates. Recorded inline as the bug class each guard defends against. 257 iced unit tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Plan 017 C7. Documentation only. The E-track entries were written as predictions, and one was wrong in a way worth recording rather than quietly fixing. E1's proposed measurement — "frame time under a full-screen scrolling TUI" — is precisely the workload E3 cannot improve: libghostty does a full rebuild whenever the viewport pin changes (render.zig:299-302), so any scrolling output is a full rebuild by construction. Measuring only that would have produced a near-flat number and then fed it into E4's go/no-go as a false negative. E1's entry now records the correction and the shipped shape: per-tab counters plus CPU spans over three workloads (with the scrolling case kept as an explicit control), readable from CI tests, an #[ignore]d harness, and the app.render_stats op — the last being the only route to draw-path numbers, since TerminalWidget::draw needs a live iced Renderer. E2 and E3 marked complete with their shipped shapes, and E3's measured result: W1 pointer-motion storm 107,605 -> 956 ns/refresh (113x) W2 in-place TUI redraw 110,025 -> 6,163 (17.9x) W3 scrolling stream 116,609 -> 57,473 (control) Recorded honestly: streaming output is unimproved by design, and the wins land in in-place TUI redraws and the non-PTY refresh callers — above all mouse motion, which previously rebuilt the entire grid on every pointer event. Also recorded is the libghostty limitation E3 had to work around, since it will bite anyone touching this again: OSC 10/11 and DECSCNM change the reported default colors while dirty stays Clean with zero rows flagged, so the consumer must compare the default pair itself. Tripwire tests fail loudly if a future Ghostty bump changes that. E3b is a new entry, split out of E3's old "GTK gets the same treatment" tail: terminal_view.rs adopting walk_dirty, plus a real GTK app.render_stats to replace the zeroed parity placeholder. E4's go/no-go is now decided from a real number, and it is GO: ~2,410 fill_text calls per draw, one per visible cell, with draw at ~1.37 ms now dominating refresh (~1 us idle post-E3). The call count is a deterministic counter; the timings are debug-build and indicative only. E4 remains future work — not started here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds Rust terminal dirty tracking, Iced cached-row rendering, render-performance counters, an ChangesRender performance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Roostctl
participant EngineIPC
participant IcedApp
participant RenderStats
Roostctl->>EngineIPC: app.render_stats(reset)
EngineIPC->>IcedApp: UiRequest::AppRenderStats
IcedApp->>RenderStats: snapshot and optional reset
RenderStats-->>IcedApp: counter values
IcedApp-->>EngineIPC: AppRenderStatsResult
EngineIPC-->>Roostctl: encoded counters and averages
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/roost-engine/src/ipc.rs`:
- Around line 277-284: Update the AppRenderStats documentation in
crates/roost-engine/src/ipc.rs lines 277-284 to replace “read-only” with wording
that describes it as ungated and states that reset=true reads and then clears
the counters; apply the same wording in docs/reference/ipc.md lines 643-647.
In `@crates/roost-iced/src/app/servicing.rs`:
- Line 1641: Correct the setup escape sequence in the test using tab.write_vt so
the cursor-home command is \x1b[H rather than the stray \x1bH. Match the sibling
setup sequences while preserving the existing clear-screen and explicit
\x1b[1;1H commands.
In `@crates/roost-vt/src/render_state.rs`:
- Around line 417-441: Update the row-processing flow in the render-state method
around bind_row_cells and clear_row_dirty so any row whose cells fail to read
preserves or escalates the global dirty state to Partial instead of allowing the
final set_global_dirty(...FALSE) to mark the frame clean. Ensure the next walk
retries the failed row while retaining the existing behavior for successfully
read rows.
In `@tools/perf/README.md`:
- Around line 37-55: Correct the workload documentation to match measurements:
in tools/perf/README.md lines 37-55, describe W3 as a low-gain control that may
improve despite libghostty full rebuilds; in tools/perf/README.md lines 82-85,
remove the claim that W1 rebuilds 32 rows per refresh; in
docs/development/iced-migration-roadmap.md lines 328-332, replace “unimproved by
design” with the documented limitation and measured 2.0x improvement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b850def0-c4dd-4850-a829-86ddfa5cd49d
📒 Files selected for processing (26)
Makefilecrates/roost-cli/src/main.rscrates/roost-engine/src/ipc.rscrates/roost-iced/src/app.rscrates/roost-iced/src/app/perf_bench.rscrates/roost-iced/src/app/servicing.rscrates/roost-iced/src/app/terminal_tab.rscrates/roost-iced/src/main.rscrates/roost-iced/src/perf.rscrates/roost-iced/src/terminal_widget.rscrates/roost-ipc/src/messages.rscrates/roost-ipc/tests/vectors.rscrates/roost-linux/src/app.rscrates/roost-osc/src/lib.rscrates/roost-vt/src/lib.rscrates/roost-vt/src/render_state.rscrates/roost-vt/tests/render_dirty_test.rsdocs/development/iced-migration-roadmap.mddocs/development/iced-parity-inventory.mddocs/reference/cli.mddocs/reference/ipc.mdtests/ipc-vectors/app.render_stats.request.jsontests/ipc-vectors/app.render_stats.response.jsontools/README.mdtools/perf/README.mdtools/perf/render-stats.sh
CodeRabbit review of PR #306. Four findings, all valid, all fixed. The real one: walk_dirty leaves a row's dirty flag SET when its cells can't be read, so the next walk retries it — that is the documented contract. But it then unconditionally cleared the GLOBAL layer to FALSE, which strands the row: the next walk_dirty reads Clean, and Clean visits nothing at all, so the retry never happens. The frame is reported clean while a row is still damaged. Now a failed read leaves the frame Partial, which is exactly what the surviving row flags mean, so the next walk visits and retries it. Not covered by a test — a mid-walk FFI read failure isn't practically inducible against a real libghostty handle — so it stays pinned by the contract and by this being the only path that can set it. Three documentation accuracy fixes: - app.render_stats was described as "read-only" in both the UiRequest doc and docs/reference/ipc.md, but reset=true reads and then zeroes. Now says ungated, and says what reset does. - tools/perf/README.md claimed W3 "is expected to show NO improvement, ever" and that W1 "today reports the full 32 rows/refresh" — both stale after E3 landed. W3 measured 2.0x faster despite rows/refresh barely moving (32.00 -> 27.50), because E3 also removed the dense vec![vec![String::new(); cols]; rows] allocation, so even full rebuilds got cheaper. The README now says to judge W3 on rows/refresh staying high rather than on its clock, and notes that a W3 whose rows/refresh collapses is a stale-cache bug, not a win. Same correction applied to the roadmap's E3 entry. - A test setup used \x1bH (HTS, set horizontal tab stop) where cursor home was meant. Harmless — an explicit \x1b[1;1H followed — but it misrepresented the setup. Dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Found while shipping this PR, and it is worse than a nuisance: the
required gate reported GREEN on a run where seven of nine jobs never
executed.
A GitHub incident ("Failed to resolve action download info: Service
Unavailable") killed most jobs in run 31119739931. GitHub marks those
`abandoned`. The gate was a denylist — it failed only on `failure` or
`cancelled` — so `abandoned` sailed through:
job results: abandoned skipped abandoned abandoned success abandoned
abandoned success abandoned
all good
ci-success is the single required check for merging, so this could have
merged a PR with essentially nothing verified, and would do so again for
anyone hitting a bad GitHub day.
Now an allowlist: only `success` and `skipped` pass, anything else fails
with the offending status named. Robust to whatever status GitHub adds
next, and it can only ever produce more red, never a false green.
Unrelated to plan 017's subject matter, pulled in because this PR is
where the hole surfaced — per CLAUDE.md's "pull one in when it touches
the code you are already in".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
This was referenced Aug 7, 2026
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.
Engine-track slices E1–E3 from the iced migration roadmap: wrap libghostty-vt's render-state dirty tracking, make the Iced snapshot rebuild only dirty rows, and build the measurement that proves it.
Plan
017-engine-dirty-tracking-perf(kept outside the repo; substance reproduced below).Results
refresh_snapshot, both trees built--releaseand run back-to-back in one session viagit worktreeso the comparison is thermally fair and reproducible from this PR alone:The headline win is not the one the roadmap predicted. E1's entry proposed measuring "frame time under a full-screen scrolling TUI" — which is precisely the workload E3 cannot improve: libghostty full-rebuilds whenever the viewport pin changes (
render.zig:299-302), so streaming output is a full rebuild by construction. Measuring only that would have produced a near-flat number and fed it into E4's go/no-go as a false negative.The real win was the case nobody was looking at: moving the mouse across the terminal previously reallocated the entire grid on every pointer event (
interactions.rs:1341). That is now zero rows.W3's 2.0× despite rows-per-refresh barely moving is explained: E3 also removed the dense
vec![vec![String::new(); cols]; rows]allocation — aStringper cell including blanks — so even full rebuilds got cheaper.What changed, per slice
E2 —
roost-vtdirty API (render_state.rs). NewDirty { Clean, Partial, Full },dirty(),mark_full(),dirty_rows(),walk_dirty().walkis untouched, so this is purely additive and GTK is unaffected.The two dirty layers are independent —
render.hcalls that "an extremely important detail": clearing the global state does not clear per-row flags, andupdateonly ever raises. So consumption implies reset:walk_dirtyclears both layers together, and there is deliberately no public way to lower the dirty state. An earlier draft exposedset_dirty(Dirty); review caught thatset_dirty(Clean)is the footgun the wrapper exists to prevent. The footgun is now structurally unreachable rather than documented-around.E3 — dirty-row rebuild in
roost-iced.TerminalSnapshot'scells+rows_textbecamegrid: Vec<Arc<RenderedRow>>. Per-rowArcs also makeApp::view's per-frame snapshot clone O(rows) refcount bumps instead of O(cells)Stringclones, with no change to the view code.DrawCelllost itsrowfield — under a per-row grid the row index would exist twice (vector index + inside every cell), and disagreement between them is exactly the "right cells, wrong row" bug this change risks. It is now derived from the grid index, making that bug unrepresentable.E1 — measurement. Per-tab counters (assertable in CI) + CPU-side span timings, over three workloads with the scrolling case kept as an explicit control. Readable three ways: CI unit tests, an
#[ignore]d in-crate harness (make perf-refresh), and a newapp.render_statsIPC op viaroostctl render-stats(make perf-render-stats) — the only route to draw-path numbers, sinceTerminalWidget::drawneeds a live icedRenderer.A libghostty limitation E3 had to work around
OSC 10/OSC 11and DECSCNM change the reported default colors while libghostty reportsdirty=Cleanwith zero rows flagged. Sinceresolve_colorsfolds the defaults into every cell that doesn't set its own, a cached row would freeze at the old text color.This was not a bug before this PR — everything rebuilt every time — so E3 would have introduced it. Found by discovery probe before any code was written. Closed by comparing the default fg/bg pair on every refresh (six bytes), and mutation-checked: removing the guard fails the test with 0 rows rebuilt where 32 are expected. Tripwire tests in
roost-vtfail loudly if a future Ghostty bump changes the upstream behavior.Verification
cargo test -p roost-vt --features fficargo test -p roost-icedcargo test -p roost-ipcmake e2e-iced(Mac, fresh binary)make e2e-gtk(shed)Pixel-identical before/after: the same 12-row colored test pattern fed to the pre-E3 build and to HEAD, captured with
roostctl screenshot, produces byte-identical PNGs (matching MD5). That is the check that catches a row-cache indexing bug, whichtab.dumptext alone would not.Every regression test was mutation-checked. That surfaced a finding worth recording: the grid-size and theme-generation guards are belt-and-braces, not load-bearing today — libghostty already reports
Fullfor resize andset_color_*, so breaking those guards alone doesn't fail the tests; the FFI call had to be stubbed too.E4 go/no-go — GO
E4 was gated on E1's number. It now exists: ~2,410
fill_textcalls per draw (one per visible cell), with draw at ~1.37 ms now dominating refresh (~1 µs idle post-E3). Coalescing adjacent cells sharing fg/bg/style is where the remaining win is.fill_text_callsis deterministic; the timings are debug-build and indicative. E4 is not done here — recorded, per the plan's scope.Parity, dependencies, secrets
libghostty-rs.app.render_stats; GTK returns the struct zeroed as a deliberate placeholder. An earlier draft had GTK refuse it — review caught two problems: it would have been the first op one Rust UI answers and the other doesn't (against CLAUDE.md's "one contract, two implementations at parity"), andunsupportedisn't a valid error code (the catalogue is closed and enforced atroost-ipc/src/lib.rs:74-76). E3b gives GTK the real implementation.libghostty-vt.adirectly with its ownRenderState.swift.render_state.rs's "matches RenderState.swift 1:1" comment now records the intentional divergence and why.make e2e-macnot run — nothing here touches Swift.Accepted risks / known gaps
test_osc_pipeline.pyisn't inICED_E2E_TESTSand has never run against iced. The element-for-element unit test and the pixel diff are the nets. Wiring it in is left as follow-up rather than done sight-unseen — a red gate would land on an unrelated PR.make test-rustwascargo test --workspacewith no--features ffi, so it compiled everyroost-vttest and ran none. This PR adds that crate's most safety-critical tests, so the Makefile is fixed here.Panel review
Codex was rate-limited and unavailable; Cursor's CLI wasn't authenticated. GLM 5.2 and CodeRabbit reviewed the plan. Both found real problems, all dispositioned in the plan's § "Panel corrections". The most valuable: CodeRabbit read libghostty's Zig source and caught that the benchmark workload was the one case E3 can't improve — which reshaped E1 entirely.
Plan detail: design decisions
D1 — dirty API shape.
walk_dirtymakes consumption imply reset; onlymark_fullcan move dirty state, and only upward.Fullvisits every row rather than trusting row flags (render.hdoesn't promise they agree). Rows are handed over as complete&[Cell]slices via one reused buffer, so a caller can replace a cached row wholesale without a half-built row landing in the cache.D2 — snapshot shape.
grid: Vec<Arc<RenderedRow>>. Per-rowArcs rather thanArc<TerminalSnapshot>, which would needArc::make_mutand copy-on-write the whole snapshot whenever the view still holds a reference — reintroducing the clone being removed.D3/D3b/D3c — three guards + two written invariants. Grid size on both axes (a width-only resize leaves row count unchanged); the default fg/bg pair (the OSC hole); a theme generation counter (belt-and-braces for anything theme-derived entering the resolver later — GTK's twin already pulls
bold_color). Documented invariants: what makes a cached row valid, and thatgridholds terminal content only — selection/hover/cursor are overlay passes and must never be baked into cell colors.Scope held. E4 recorded not done; E5–E9 untouched; no pin bump.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Summary by CodeRabbit
New Features
roostctl render-statsto view or reset UI rendering metrics.Bug Fixes
Documentation