From 822b7d5862e56bf9ef91258ac92ce1d9c3c0123b Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 08:51:43 -0500 Subject: [PATCH 01/11] docs: add engine track (E) + M6 macOS Iced to the migration roadmap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-osc/src/lib.rs | 30 ++- docs/development/iced-migration-roadmap.md | 259 ++++++++++++++++++--- docs/development/iced-parity-inventory.md | 1 + 3 files changed, 246 insertions(+), 44 deletions(-) diff --git a/crates/roost-osc/src/lib.rs b/crates/roost-osc/src/lib.rs index 9664afbf..b7335f21 100644 --- a/crates/roost-osc/src/lib.rs +++ b/crates/roost-osc/src/lib.rs @@ -1,18 +1,26 @@ //! Streaming OSC scanner — Phase 6a P4. //! -//! OSC scanner tailored to the daemon's architecture: +//! Originally written for the daemon, which was intentionally +//! libghostty-free: the UI reported raw OSC bytes via `ReportOsc` and +//! the daemon parsed everything it needed to route, so this scanner +//! emits `Title` for OSC 0/1/2 on top of the notification/cwd/color +//! classes even though the UI's libghostty handles titles itself. //! -//! * The UI's libghostty handles window-title OSC (0/1/2) itself, -//! so a scanner sitting next to it would only need to cover the -//! OSC classes libghostty doesn't surface (notifications, cwd, -//! color queries). +//! **That premise is obsolete — the daemon is gone and both Rust UIs +//! embed libghostty — but the conclusion still holds, for a different +//! reason.** libghostty's OSC parser cannot replace this one: +//! `GhosttyOscCommandData` exposes exactly one payload accessor +//! (`CHANGE_WINDOW_TITLE_STR`), identical in our pinned header and at +//! Ghostty tip. It discriminates 22 command *types* but hands back no +//! data for OSC 7 / 9 / 10-12 / 4 / 133 / 52 / 22 — seven of the eight +//! [`OscEvent`] variants. The policy below (percent-decode + +//! `file://` extraction, ConEmu OSC 9 sub-command filtering, OSC 52 +//! base64 decode with refuse-on-truncation, `MAX_BODY`, reply +//! synthesis) has no C-API counterpart and would survive regardless. //! -//! * This scanner instead sits in the daemon, which is -//! intentionally libghostty-free (see goal-rust-port-polish DL -//! choices). The UI reports raw OSC bytes via `ReportOsc` and the -//! daemon parses everything it needs to route. So this scanner -//! also emits `Title` for OSC 0/1/2, on top of the -//! notification/cwd/color classes. +//! Re-evaluate only if libghostty-vt grows `GHOSTTY_OSC_DATA_*` +//! accessors beyond window title — tracked as a watch item in +//! `docs/development/iced-migration-roadmap.md`. //! //! Architecture: //! diff --git a/docs/development/iced-migration-roadmap.md b/docs/development/iced-migration-roadmap.md index 555674db..f0127a29 100644 --- a/docs/development/iced-migration-roadmap.md +++ b/docs/development/iced-migration-roadmap.md @@ -248,6 +248,84 @@ architecture cleanup that everything real-time depends on; 3f is done and (plan 016) — only 3h (polish parity II, user-directed) remains open on this track. +### Engine track (E) — shared renderer + robustness + +Deliberately **not** M3 slices. These span `roost-vt` / `roost-ui-model` +and land in all three UIs, so they are not part of "Iced functional +parity"; they run alongside M3/M4 rather than gating either. Lettered `E` +to avoid colliding with the parity inventory's P0/P1/P2 *priority* labels. + +Recorded 2026-08-06 from a Ghostty-comparison discovery pass. The headline +finding: **libghostty-vt's render-state dirty tracking is exposed in the +pinned header and wrapped by nobody** — not Swift, not GTK, not Iced. All +three walk the full grid on every update. + +* **E1. Renderer baseline measurement.** Frame time under a full-screen + scrolling TUI at max window size, both renderers (wgpu + tiny-skia), + both OSes. Runs first: its number is what decides whether E4 is worth + doing. Context for why this is a measurement and not an assumption — + the per-cell `renderer.fill_text` in `terminal_widget.rs` looked like + an Iced-specific regression and is not one: `iced_wgpu`'s text pipeline + caches shaping in a keyed atlas, and the Swift renderer is *more* naive + (`TerminalView.swift` per-cell `NSAttributedString.draw`, whose own + comment names a glyph atlas as the next-tier optimization). Treat the + renderer as uniformly naive across all three UIs, not as an Iced risk. +* **E2. Render-state dirty coverage.** `crates/roost-vt/src/render_state.rs` + wraps exactly `update` / `colors` / `cursor` / `walk`. The pinned + `third_party/ghostty/out/include/ghostty/vt/render.h` also offers + global dirty (`GHOSTTY_RENDER_STATE_DATA_DIRTY` → false/partial/full), + per-row dirty via the row iterator, the matching setters for resetting + both layers, and a two-phase `begin_update` / `end_update` split (only + the first needs terminal access, so the lock can be released before the + deferred work). Wrap all of it. `../libghostty-rs`'s + `crates/libghostty-vt/src/render.rs` is MIT and a good reference + implementation — its typestate around begin/end is worth copying. +* **E3. Dirty-row snapshot rebuild.** Consumes E2. `refresh_snapshot` + (`app/terminal_tab.rs`) currently rebuilds the whole grid per PTY + update, allocating `vec![vec![String::new(); cols]; rows]` — a `String` + per cell including blanks — plus a `String` per `DrawCell`. Rebuild only + dirty rows. GTK's `terminal_view.rs` gets the same treatment. +* **E4. Run coalescing** — **conditional on E1.** Merge adjacent cells + sharing fg/bg/style into one `fill_text` run. A `roost-ui-model` change, + so GTK benefits too. Do not start this without E1's number. +* **E5. Sprite parity in Iced.** `mac/Sources/Roost/Sprite.swift` and + `crates/roost-linux/src/sprite.rs` draw U+2500–U+259F (box-drawing, + block elements) geometrically because font glyphs don't tile + pixel-perfectly across adjacent cells. **`roost-iced` has no sprite + path at all** — so TUI chrome shows hairline seams there and not in + either shipped UI. This is a regression against our own shipped Linux + UI, not a macOS concern. The Linux sibling is already Rust: move it to + a shared crate and add the draw call. Add the matching row to the + parity inventory (it has none today, which is why this went unnoticed). +* **E6. IME input.** `crates/roost-iced/src/input.rs` handles no `Ime` + events, so dead keys, CJK input, and the emoji picker are broken. + winit surfaces IME on both platforms — this is a Linux-shipping gap + too, not a macOS-only one. +* **E7. Crash robustness.** No `panic::set_hook` anywhere in `roost-iced` + or `roost-engine`; GTK ships to Linux users today with none. Add a + panic hook that logs + writes a crash file, and close [#299] (verified + release-build infinite loop on a malformed font) — promote it out of + the maintenance backlog. Both are entry gates for M6. +* **E8. Ghostty pin + zig bump** — *sequenced after the M6 direction + resolves.* `third_party/ghostty/build.sh` pins `c74f6d5` (2026-04-25) + against zig 0.15.2 (`.mise.toml`); `../libghostty-rs` pins `ab0b9da` + (2026-07-22, **603 commits ahead**) and needs zig 0.16.x. Wanted + eventually regardless. A large share of the cost is revalidating the + **Swift** build — `mac/Package.swift` links the same static archive — + so the price drops sharply if Swift is retiring. That is why this sits + after the M6 decision, not before it. +* **E9. `libghostty-rs` integration spike + library audit.** Depends on + E8. Checked out at `../libghostty-rs` (tip `72ac98f`, 2026-07-29, MIT, + not on crates.io — fine, `publish = false`). It is broader than + `roost-vt` (osc, kitty graphics, paste, sgr, screen, unicode, focus) + and more rigorous (borrow-checked lifetimes, typestate updates). Two + integration notes: `GHOSTTY_SOURCE_DIR` can point its `build.rs` at our + existing checkout, and a `pkg-config` feature can discover a + pre-built archive — worth a spike so we don't run zig twice. Adopting + it dissolves the `render_state.rs` ↔ `RenderState.swift` 1:1 parity + correspondence, which is a *cost* while Swift lives and a non-issue + after. Audit for further wins once integrated. + ### Maintenance backlog (filed, not scheduled) Work this migration surfaced that should not block a slice. Pull one in @@ -269,6 +347,7 @@ when it touches the code you are already in: | `roost-engine::facade` has no consumer; prove it or delete it (blocks M5) | [#286] | | `app/interactions.rs` at 2,960 lines — finer split when fixtures allow | [#288] | | swift-testing runner SIGABRT on fast value-check swarms (XCTest workaround) | [#289] | +| **OSC consolidation — watch item, do not act.** `roost-osc` cannot fold into libghostty's OSC parser: `GhosttyOscCommandData` exposes exactly one payload accessor (`CHANGE_WINDOW_TITLE_STR`), identical in our pinned header and in `../ghostty` tip, so a pin bump does not help. libghostty discriminates 22 command *types* but hands back no data for OSC 7 / 9 / 10-12 / 4 / 133 / 52 / 22 — 7 of the 8 events `OscEvent` needs. The custom parts (percent-decode + `file://` extraction, ConEmu OSC 9 sub-command filtering, OSC 52 base64 decode + refuse-on-truncation, `MAX_BODY`, reply synthesis) are policy with no C-API counterpart and would survive anyway. **Exit condition:** libghostty-vt adds `GHOSTTY_OSC_DATA_*` accessors beyond window title — then re-evaluate. | — | [#281]: https://github.com/charliek/roost/issues/281 [#282]: https://github.com/charliek/roost/issues/282 @@ -306,42 +385,20 @@ should re-verify every row against current behavior, close what shipped, and decide [#284] (visual-parity CI gate: required for M4 or waived). After plan 015, the open P1 set is expected to be exactly the 3e polish scope plus the upstream-blocked drops row ([#302]) — the audit confirms -that expectation rather than assuming it. - -### Possible direction — macOS side-by-side evaluation (not committed) - -Recorded 2026-08-05: the Iced work has landed better than expected, and -replacing the Swift app is now a *possible* direction rather than a -non-goal — but it is not guaranteed, a lot of testing stands between here -and any commitment, and **Swift remains the production daily driver -regardless** (guardrail #1 unchanged). Two consequences today: - -1. **Design Iced platform-clean now.** New roost-iced capability with a - native surface gets a per-OS backend seam rather than a Linux-only - shape — notifications (slice 3f) are the first instance: Linux D-Bus - ships; the macOS backend (`UNUserNotificationCenter`, which needs a - real .app bundle identity + code signature) is deferred, not - designed out. No macOS backend gets half-shipped from an unbundled - binary. -2. **M5 is frozen pending this direction** — see below. - -If the direction firms up, the evaluation vehicle is a parallel-install -signed+notarized DMG (the `ai.stridelabs.Roost.iced` profile is already -fully isolated for side-by-side running), gated on a robustness pass: -release-profile CI coverage for Iced, the [#299] swash release-hang fix -(a Mac daily driver meets the full macOS font universe), a panic-hook -crash/feedback story, and a mac-UX gap audit (menu bar, cmd-key -conventions, dock badge — plus 3e's vibrancy spike). Guardrail #3's -"absent from release artifacts" would be amended consciously at that -point (separate opt-in artifact, never bundled into the Swift release). +that expectation rather than assuming it. Amended 2026-08-06: add the new +box-drawing/sprite row (engine-track slice E5) to that expected set. It is +a genuine P1 gap against *both* references, and the fact that it went +unnoticed until a Ghostty-comparison pass — because the inventory had no +row for it — is exactly the drift the audit exists to catch. Treat +"is there a row for this at all?" as an audit question, not just "is this +row current?". ### M5 — Rust under Swift (exploration, frozen) -Frozen 2026-08-05 pending the possible-direction note above: if a -full Iced replacement is evaluated, a Swift-facing FFI boundary is -likely wasted investment — [#286] holds at "don't invest, don't -delete" until the direction resolves. Original exploration plan kept -below for when/if it resumes. +Frozen 2026-08-05 pending M6 below: if a full Iced replacement is +evaluated, a Swift-facing FFI boundary is likely wasted investment — +[#286] holds at "don't invest, don't delete" until the direction +resolves. Original exploration plan kept below for when/if it resumes. The Swift app's polish and daily use make replacement remote; the question is where shared Rust reduces duplication *without* slowdowns. @@ -363,6 +420,142 @@ is where shared Rust reduces duplication *without* slowdowns. 4. Decision gate with data; until then `IPCHandlerImpl.swift` / `Workspace.swift` remain authoritative on Mac. +### M6 — macOS Iced (evaluation → parity) + +**Runs parallel to M4, not after it.** The number follows M5 for document +order only; nothing here waits on shipping Iced to Linux users. + +Status (2026-08-06): replacing the Swift app is now **likely** rather than +merely possible — but it is not committed, it is a ways off, and +**Swift remains the production daily driver throughout** (guardrail #1 +unchanged). This section supersedes the earlier "Possible direction — +macOS side-by-side evaluation" note. Two standing consequences: + +1. **Design Iced platform-clean now.** New roost-iced capability with a + native surface gets a per-OS backend seam rather than a Linux-only + shape — notifications (slice 3f) are the first instance: Linux D-Bus + ships; the macOS backend is deferred, not designed out. No macOS + backend gets half-shipped from an unbundled binary. +2. **M5 stays frozen** while this is live. M6 is the *opposite* direction + from M5 (Iced replaces Swift vs. Rust under Swift); keep them separate + so the frozen thing stays frozen. + +**Entry gate:** E5 and E7 complete; release-profile CI coverage for Iced; +parity-inventory audit clean. Note the reference bar here is the **Swift +app**, which is consistently stricter than M4's GTK bar — the inventory +tracks both, and M6 is the superset. + +**Accepted regressions**, decided rather than discovered: accessibility. +AppKit gives the Swift sidebar and menus VoiceOver support for free; an +Iced canvas gives essentially none. Named here so it is a conscious trade. + +Guardrail #3's "absent from release artifacts" is amended consciously at +6a: a separate opt-in artifact, never bundled into the Swift release. + +Slices: + +* **6a. Bundling + parallel install.** The foundation — Sparkle, + notifications, and TCC testing all need real bundle identity, so + nothing below starts until this lands. `mac/scripts/bundle.sh` is + toolkit-agnostic apart from `swift build --show-bin-path`; fork or + parameterize it over (binary, bundle id, plist). `make-dmg.sh` needs no + change. `roost-iced` must also resolve its profile from its own bundle + id rather than calling `BundleProfile::iced()` unconditionally. + Decisions taken 2026-08-06: + * **Display name `Roost-Iced`** (`CFBundleName` / + `CFBundleDisplayName`; window title should match) — two apps called + "Roost" in the Dock and Cmd-Tab is genuinely confusing. + **Display-only.** `app_label` stays `Roost-iced` (lowercase `i`): + it drives the socket dir, the log dir, and the `identify` wire + response (`roost-ipc/src/messages.rs`), which roosttest asserts on, + and case-changing it is a no-op on macOS but a breaking path change + on Linux, where `roost-iced` also runs. Do not "fix" the + inconsistency. + * **Fresh, separate `state.json`** — no import, no migration. This is + already what `BundleProfile::iced()` does, so it is *zero* work. + Scoped to macOS side-by-side; it does **not** settle M4's Linux + adopt-or-migrate question, where Iced eventually replaces GTK for + the same users. + * **Sparkle/appcast deferred to 6c.** 6a ships with no auto-update + (`SUEnableAutomaticChecks` is already `false`). + Side-by-side is mostly already solved and needs no new machinery: + distinct bundle ids, per-profile socket/state/log paths with tests + asserting distinctness (`roost-ipc/src/paths.rs`), per-profile + single-instance locks. **Claude hooks need no per-app configuration** — + `ROOST_SOCKET` is injected into every PTY child (`roost-engine/src/pty.rs`, + `PtySupervisor.swift`) and sits at precedence #2 in target resolution, + above `--target` and auto-detect, so a Claude session in an Iced tab + dials the Iced socket automatically; `claude_settings_document()` + (`roost-cli/src/main.rs`) bakes no socket or profile into + `claude-settings.json`. One operational wrinkle to note in the install + output, not fix in code: `claude install` writes `self_exe()`, so with + two bundles the hook file points at whichever `roostctl` ran it last. + Two bundle ids also mean two entries in System Settings › Notifications. +* **6b. Native shim seam.** One decision with three consumers (6c/6d/6e): + call AppKit via `objc2` (already in the lockfile through winit) or + build a small Swift static library behind a C ABI. Leaning Swift lib — + it amortizes across Sparkle, menus, and notifications, and the Swift + toolchain is already a build dependency. Decide with a spike, not in + the abstract. +* **6c. Sparkle auto-update.** `mac/Package.swift` + `App.swift` use + ~two API calls (`SPUStandardUpdaterController`, `checkForUpdates(_:)`); + `bundle.sh` already embeds and inside-out-signs the framework, and + `release.yml` already EdDSA-signs and publishes the appcast. The + evaluation build needs a **separate feed or none** so the two apps do + not offer each other's updates. Design note for the eventual cutover: + Sparkle does not care what language wrote the app, so an Iced build + shipping under the *same* bundle id with the same `SUPublicEDKey` and a + higher `CFBundleVersion` upgrades existing Swift installs in place. +* **6d. Menu bar.** winit installs none, so Iced on macOS currently has + no menus at all. `App.swift` builds ~35 items across App/File/View/ + Edit/Window plus a dynamic Window menu of tabs and projects. Options: + `muda` (designed to sit alongside winit) or hand-rolled NSMenu via + `objc2-app-kit`. The keybind story is *better* in Rust — the table + already lives in `roost-ui-model`, so menu equivalents and the terminal + key encoder read one source instead of Swift re-deriving them. Highest + volume, lowest risk; also where "custom options later" becomes cheap + once menu items are just `Message` variants. +* **6e. Desktop notifications, macOS backend.** Closes [#303]. The seam + from slice 3f is backend-agnostic already (`notifications.rs`: worker, + per-tab replace semantics, click routing); only `mod backend` is + missing. `UNUserNotificationCenter` requires a bundled, signed app — + hence the 6a dependency, and it cannot be validated from `make + run-iced`. Match `mac/Sources/Roost/DesktopNotifications.swift` + semantics. Replace-by-server-id becomes UN's stable per-tab identifier, + which is simpler than the D-Bus version. +* **6f. Window vibrancy.** May partly land in 3h, which already owns the + spike. Worth knowing before starting: there is **no `NSVisualEffectView` + anywhere in the Swift source** — the sidebar's translucency is AppKit's + implicit source-list material (`outline.style = .sourceList` plus + `scrollView.drawsBackground = false` and no pane fill), and the Swift + code only ever works *around* it. So there is nothing to port 1:1; + Iced must build it explicitly. The seam is + `iced::window::run(id, |w: &dyn Window| …)` — a main-thread + `HasWindowHandle`, with `raw-window-handle` at 0.6.2, the version + `window-vibrancy` wants. Same call is the hook for Sparkle init, NSMenu + install, and dock badge. Do **not** use `window::Settings { blur }`: + winit's macOS impl calls the private `CGSSetWindowBackgroundBlurRadius` + SPI and blurs the whole window, not a region. The effect view sits + behind the wgpu surface, so the terminal region must stay opaque. +* **6g. macOS platform hygiene.** Individually small, collectively a + pass. Entitlements + purpose strings first: Roost is the TCC + *responsible app* for every child process in a tab, so without + `device.audio-input` / `device.camera` / `automation.apple-events` a + `/voice` or `osascript` in a tab fails **silently** — no prompt, no + error (see the rationale comment in `mac/Resources/Roost.entitlements`, + including which entitlements we deliberately omit). Then: Dock badge + + dock menu, `NSApplicationDelegate` lifecycle (reopen-from-Dock, + open-file/URL, graceful terminate), activation policy, and Secure + Keyboard Entry (`EnableSecureEventInput` — a terminal convention + neither app has today). +* **6h. macOS verification tier.** What makes the parity claim honest + instead of hand-verified: a CGEvent real-input harness ([#285] — the + uinput tier is Linux-only, a gap that matters far more once Mac is a + target), `e2e-iced-mac` as a required gate, and release-profile CI + coverage for Iced. Remember the enumerated-list trap: new roosttest + modules need `ICED_E2E_TESTS` in the `Makefile` *and* the `ci.yml` + lists, or they never run. + ## Gauntlet operating notes * One milestone slice per pass; every pass ends in PR(s) watched to green diff --git a/docs/development/iced-parity-inventory.md b/docs/development/iced-parity-inventory.md index fbf578f9..d0ad321e 100644 --- a/docs/development/iced-parity-inventory.md +++ b/docs/development/iced-parity-inventory.md @@ -148,6 +148,7 @@ product polish, and P2 is an optional native/toolkit refinement. | Terminal scrollback | Wheel/page navigation scrolls retained history locally when mouse reporting is off; alternate-screen behavior follows terminal modes | Closed: wheel and bare PageUp/PageDown page navigation both route through the shared GTK/Iced `roost-vt` policy — retained history, exact bottom state, next-terminal-key snap, mouse-report precedence, alternate-screen arrows/forwarding, and a full-viewport local page move that preserves selection and bypasses snap only on the local route. Swift Mac has no PageUp/PageDown scrollback route (deliberate Rust-UI-first divergence; no prior reference behavior existed) | closed | Physical X11 wheel under both renderers; `roost-vt::route_page` unit/fixture coverage plus both-UI (GTK/Iced) adapter fixtures (selection preserved, zero local PTY bytes, byte-identical Forward path); physical PageUp/PageDown segment in `iced_clipboard_check.py` | | Terminal typography | Configured family/size, baseline and cell metrics stable across styles and graphemes | Renderer-measured size and installed-family selection now reflow every live tab atomically, persist through the shared config policy, and reach new/restored tabs; focused glyph baseline/style comparison remains | P1 | Latin/wide/combined/style fixture under wgpu and tiny-skia; shared GTK/Iced font selection E2E | | Terminal cursor/selection/link | Shared colors and modes with reference-like cursor, selection, and link feedback | Functional coverage exists; geometry/color comparison remains incomplete | P1 | Focused cursor/selection/link screenshots plus existing real-input gates | +| Terminal box-drawing/block glyphs | Both shipped UIs draw U+2500–U+257F and U+2580–U+259F geometrically rather than from the font, because font glyphs do not tile pixel-perfectly across adjacent cells — `mac/Sources/Roost/Sprite.swift` and `crates/roost-linux/src/sprite.rs`, both ports of Ghostty's `font/sprite/draw/{block,box}.zig` and kept in lockstep per the CLAUDE.md parity rule | **No sprite path in `roost-iced` at all** — box-drawing and block elements fall through to font glyphs, so TUI chrome shows hairline seams (most visible in wordmark/logo art) that neither shipped UI has. Row added 2026-08-06; the gap existed unnoticed because this inventory had no box-drawing row. Tracked as engine-track slice E5: the Linux sibling is already Rust, so the work is move-to-shared-crate plus a draw call | P1 | Seam-free capture of a box-drawing/block fixture under both renderers, cross-checked against the GTK and Mac references; codepoint-dispatch unit coverage mirroring the existing Swift/Rust sprite tests | | Palette placement | Centered, elevated compact panel over an undimmed terminal, styled semantic rows, keyboard focus, and scrolling | Closed for the visual-feasibility slice: exact reference neutrals, border/shadow, content-sized 660 pt card capped at 500 pt, compact command/agent/notification/provider rows, shortcut hints, fuzzy-match accents, disabled state, and a narrow neutral scrollbar | closed | Five named GTK/Iced captures; focus/scroll tests plus real row/card/outside pointer routing | | Empty/loading/error states | Deliberate shell placeholders without changing hierarchy | Plain `Starting terminal…`; status errors are appended to the sidebar | P1 | Seeded empty/loading/error snapshots and recovery tests | | Hover/focus/disabled states | Subtle per-control hover and visible focus without global blue fills | Mostly inherited stock theme states | P1 | Renderer-neutral state-style unit tests plus real pointer/keyboard capture | From 8bc8b91bfa7e0878706b618ef66dc987e09febd8 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 08:53:51 -0500 Subject: [PATCH 02/11] =?UTF-8?q?docs:=20correct=20E2=20=E2=80=94=20two-ph?= =?UTF-8?q?ase=20render-state=20update=20is=20not=20at=20our=20Ghostty=20p?= =?UTF-8?q?in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- docs/development/iced-migration-roadmap.md | 25 ++++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/docs/development/iced-migration-roadmap.md b/docs/development/iced-migration-roadmap.md index f0127a29..16a8b440 100644 --- a/docs/development/iced-migration-roadmap.md +++ b/docs/development/iced-migration-roadmap.md @@ -274,12 +274,20 @@ three walk the full grid on every update. wraps exactly `update` / `colors` / `cursor` / `walk`. The pinned `third_party/ghostty/out/include/ghostty/vt/render.h` also offers global dirty (`GHOSTTY_RENDER_STATE_DATA_DIRTY` → false/partial/full), - per-row dirty via the row iterator, the matching setters for resetting - both layers, and a two-phase `begin_update` / `end_update` split (only - the first needs terminal access, so the lock can be released before the - deferred work). Wrap all of it. `../libghostty-rs`'s - `crates/libghostty-vt/src/render.rs` is MIT and a good reference - implementation — its typestate around begin/end is worth copying. + per-row dirty (`ROW_DATA_DIRTY` via the row iterator), and the matching + `OPTION_DIRTY` / `ROW_OPTION_DIRTY` setters for resetting both layers. + Wrap those. No bindgen work: all four constants are already in the + generated `sys` module, so this is pure safe-wrapper surface. + **Footgun to handle explicitly** (render.h calls it "an extremely + important detail"): the two dirty layers are independent — clearing the + global state does *not* clear per-row flags, and `update` only ever + sets, never clears. The caller owns both resets. + **Two-phase `begin_update` / `end_update` is NOT available at our pin** — + it landed upstream after `c74f6d5` (present at `../ghostty` tip and in + `../libghostty-rs`, absent from our generated bindings). It is an E8 + follow-on, not part of E2; don't plan around it. + `../libghostty-rs`'s `crates/libghostty-vt/src/render.rs` is MIT and a + useful reference for the dirty accessors regardless. * **E3. Dirty-row snapshot rebuild.** Consumes E2. `refresh_snapshot` (`app/terminal_tab.rs`) currently rebuilds the whole grid per PTY update, allocating `vec![vec![String::new(); cols]; rows]` — a `String` @@ -313,7 +321,10 @@ three walk the full grid on every update. eventually regardless. A large share of the cost is revalidating the **Swift** build — `mac/Package.swift` links the same static archive — so the price drops sharply if Swift is retiring. That is why this sits - after the M6 decision, not before it. + after the M6 decision, not before it. Carries E2's deferred half: the + two-phase `begin_update` / `end_update` split arrives with the newer + pin, letting the renderer drop the terminal lock before the deferred + work — a latency win under heavy PTY output. * **E9. `libghostty-rs` integration spike + library audit.** Depends on E8. Checked out at `../libghostty-rs` (tip `72ac98f`, 2026-07-29, MIT, not on crates.io — fine, `publish = false`). It is broader than From 97550a05846c6a6fa46ae958c5488f7dd24f7f9a Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 09:57:09 -0500 Subject: [PATCH 03/11] feat(vt): wrap libghostty's render-state dirty tracking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- Makefile | 7 +- crates/roost-vt/src/lib.rs | 2 +- crates/roost-vt/src/render_state.rs | 270 +++++++++++++-- crates/roost-vt/tests/render_dirty_test.rs | 385 +++++++++++++++++++++ 4 files changed, 624 insertions(+), 40 deletions(-) create mode 100644 crates/roost-vt/tests/render_dirty_test.rs diff --git a/Makefile b/Makefile index 60e802fa..ed96605c 100644 --- a/Makefile +++ b/Makefile @@ -73,8 +73,13 @@ ICED_E2E_TESTS := tools/roosttest/test_smoke.py tools/roosttest/test_iced_walkin ICED_CLIPBOARD_TESTS := tools/roosttest/test_selection.py tools/roosttest/test_osc52.py test: test-rust test-mac test-harness ## All unit/integration tests (Rust + Swift + harness) -test-rust: ## cargo test --workspace +# roost-vt's tests/*.rs all start with `#![cfg(feature = "ffi")]`, so the +# `--workspace` run compiles and then silently skips every one of them. The +# second line mirrors CI's separate `cargo test -p roost-vt --features ffi` +# step (.github/workflows/ci.yml, rust job) so `make test` runs them too. +test-rust: ## cargo test --workspace (+ roost-vt ffi tests, cfg-gated out of the default run) cargo test --workspace + cargo test -p roost-vt --features ffi test-iced: ## Iced unit tests (renderer + input + adapter) cargo test -p roost-iced diff --git a/crates/roost-vt/src/lib.rs b/crates/roost-vt/src/lib.rs index 0ed95813..78e31903 100644 --- a/crates/roost-vt/src/lib.rs +++ b/crates/roost-vt/src/lib.rs @@ -124,7 +124,7 @@ pub use mouse_encoder::{ mouse_action, mouse_button, MouseAction, MouseButton, MouseEncoder, MouseEvent, }; #[cfg(feature = "ffi")] -pub use render_state::{Cell, Colors, CursorInfo, CursorVisualStyle, RenderState, Style}; +pub use render_state::{Cell, Colors, CursorInfo, CursorVisualStyle, Dirty, RenderState, Style}; #[cfg(feature = "ffi")] pub use scroll::{PageDirection, PageRoute, ScrollDirection, ScrollRoute, TerminalScroll}; #[cfg(feature = "ffi")] diff --git a/crates/roost-vt/src/render_state.rs b/crates/roost-vt/src/render_state.rs index 3ecad119..5d23cf12 100644 --- a/crates/roost-vt/src/render_state.rs +++ b/crates/roost-vt/src/render_state.rs @@ -5,11 +5,18 @@ //! and row-cells handles once. They're reused across frames. //! 2. `update(&terminal)` snapshots the current screen. //! 3. `walk(|cell| ...)` iterates rows × cells, calling the closure -//! once per cell. +//! once per cell; `walk_dirty` iterates only the rows that +//! changed and consumes the frame's damage. //! 4. `cursor()` / `colors()` extract additional per-frame data. //! -//! Matches `mac/Sources/Roost/RenderState.swift` 1:1 in shape — same -//! constructor pattern, same walk surface, same cursor info layout. +//! `mac/Sources/Roost/RenderState.swift` mirrors the constructor, +//! walk, and cursor-info shape — but the two have **intentionally +//! diverged** as of the dirty-tracking API below (`Dirty`, `dirty`, +//! `mark_full`, `dirty_rows`, `walk_dirty`). Swift deliberately does +//! not get dirty tracking: it is the daily driver, its full-grid +//! renderer is adequate, and the macOS-Iced evaluation may retire it, +//! so investing in its render path is potentially wasted work. Do not +//! "restore parity" here without that decision changing. use std::ptr; @@ -115,6 +122,28 @@ pub struct Cell { pub style: Style, } +/// Global dirty state after `update`. Maps `GhosttyRenderStateDirty`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Dirty { + Clean, + Partial, + Full, +} + +impl Dirty { + fn from_raw(v: sys::GhosttyRenderStateDirty) -> Self { + match v { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE => Self::Clean, + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_PARTIAL => Self::Partial, + // Anything we don't recognize — including a variant a future + // Ghostty adds — maps to `Full`, not `Clean`. Guessing "clean" + // for an unknown state would skip the rebuild and freeze the + // screen; guessing "full" only costs a redraw. + _ => Self::Full, + } + } +} + pub struct RenderState { handle: sys::GhosttyRenderState, row_iter: sys::GhosttyRenderStateRowIterator, @@ -269,46 +298,19 @@ impl RenderState { /// libghostty's contract says they're safe to re-bind via the next /// `_next` call without reallocation. pub fn walk(&mut self, terminal: &Terminal, mut f: impl FnMut(u32, Cell)) -> Result<()> { - // Rebind the row iterator to this frame's state. The C signature - // expects `GhosttyRenderStateRowIterator*` (pointer-to-handle slot), - // not the handle's value — the function writes into the slot to - // re-anchor the pre-allocated iterator at the new frame. Passing - // `self.row_iter as *mut _` would point at the iterator's IMPL - // and corrupt its internal state, leaving `..._next` returning - // false on every row (silent: no error, just zero cells walked). - // Mirrors `mac/Sources/Roost/RenderState.swift::walk`'s - // `withUnsafeMutablePointer(to: &self.rowIter)` pattern. - // SAFETY: state + iter handles non-null per constructor. - let rc = unsafe { - sys::ghostty_render_state_get( - self.handle, - sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_ROW_ITERATOR, - (&mut self.row_iter) as *mut _ as *mut _, - ) - }; - Error::from_result(rc)?; + self.bind_row_iterator()?; // Keep `terminal` alive across the walk so libghostty doesn't // drop allocations that back the iterators. Borrowing & makes // the lifetime explicit; the variable itself is intentionally // unused. let _ = terminal; - let mut row_idx: u32 = 0; - // SAFETY: iter handle non-null. - while unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { - // Bind this row's cells to row_cells. Same pointer-to-slot - // semantics as the row iterator above — pass `&mut`, not the - // handle value. - // SAFETY: iter + cells handles non-null. - let rc = unsafe { - sys::ghostty_render_state_row_get( - self.row_iter, - sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_CELLS, - (&mut self.row_cells) as *mut _ as *mut _, - ) - }; - if Error::from_result(rc).is_err() { - row_idx += 1; + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + if self.bind_row_cells().is_err() { continue; } @@ -319,11 +321,203 @@ impl RenderState { f(row_idx, cell); col = col.saturating_add(1); } - row_idx += 1; } Ok(()) } + /// Global dirty state. Pure read — clears nothing. + pub fn dirty(&self) -> Result { + let raw = self.read_u32(sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_DIRTY)?; + Ok(Dirty::from_raw(raw)) + } + + /// Raise the global dirty state to `Full`, forcing the next + /// `walk_dirty` to visit every row. Monotonic: this is the only + /// public way to move the dirty state at all, and it only ever + /// raises. Lowering lives solely inside `walk_dirty`, where both + /// layers are cleared together — which is what keeps libghostty's + /// two-layer footgun (clearing one layer does not clear the other) + /// structurally unreachable from safe code. + pub fn mark_full(&mut self) -> Result<()> { + self.set_global_dirty(sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FULL) + } + + /// Viewport row indices libghostty currently marks dirty. Pure with + /// respect to dirty state — clears nothing, on either layer. It does + /// rebind the cached row-iterator handle (`self.row_iter`) to walk the + /// rows, so it must not be called from inside a `walk` / `walk_dirty` + /// callback: that would re-anchor the iterator mid-iteration and + /// corrupt the caller's in-flight walk. Diagnostic / test accessor; + /// renderers want `walk_dirty`. + pub fn dirty_rows(&mut self, terminal: &Terminal) -> Result> { + self.bind_row_iterator()?; + // Keep `terminal` alive across the walk (see `walk`). + let _ = terminal; + + let mut rows = Vec::new(); + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + if self.read_row_dirty() { + rows.push(row_idx); + } + } + Ok(rows) + } + + /// Walk only the rows that changed, handing each row's complete cell + /// list to `f`. Visits every row when the global state is `Full`. + /// + /// **Consumes the frame's damage: clears BOTH dirty layers.** A + /// `Clean` frame visits nothing but still clears both, so the + /// contract is uniform regardless of what came back. + /// + /// Contract: `f` is called exactly once per visited row, with that + /// row's COMPLETE cell slice. It is never called mid-row, and never + /// for a row whose read failed — so a caller may replace its cached + /// row wholesale on each callback without a half-built row ever + /// landing in the cache. A row whose cells could not be read keeps + /// its dirty flag set, so the next frame retries it. + /// + /// Returns the global state as it was on ENTRY. + /// + /// On an early `Err` return some rows may already be cleared while + /// the global layer is not. That is safe by construction: residual + /// damage means the next frame redraws MORE than necessary, never + /// less. + pub fn walk_dirty( + &mut self, + terminal: &Terminal, + mut f: impl FnMut(u32, &[Cell]), + ) -> Result { + let global = self.dirty()?; + self.bind_row_iterator()?; + // Keep `terminal` alive across the walk so libghostty doesn't + // drop allocations that back the iterators (see `walk`). + let _ = terminal; + + let mut cells: Vec = Vec::new(); + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + + let row_dirty = self.read_row_dirty(); + let visit = match global { + Dirty::Clean => false, + // `render.h` does not promise that a `Full` frame also + // flags every row, so don't depend on it. + Dirty::Full => true, + Dirty::Partial => row_dirty, + }; + + if visit { + if self.bind_row_cells().is_err() { + // Leave this row's flag SET so the next walk retries + // it, and don't call `f` with a partial row. + continue; + } + + cells.clear(); + let mut col: u16 = 0; + // SAFETY: row_cells handle non-null. + while unsafe { sys::ghostty_render_state_row_cells_next(self.row_cells) } { + cells.push(self.read_current_cell(col)); + col = col.saturating_add(1); + } + f(row_idx, &cells); + } + + if row_dirty { + self.clear_row_dirty()?; + } + } + + self.set_global_dirty(sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE)?; + Ok(global) + } + + /// Rebind `self.row_iter` to the current frame's state. + /// + /// The C signature expects `GhosttyRenderStateRowIterator*` (a + /// pointer-to-handle slot), not the handle's value — the call writes + /// into the slot to re-anchor the pre-allocated iterator at the new + /// frame. Passing `self.row_iter as *mut _` would point at the + /// iterator's IMPL and corrupt its internal state, leaving + /// `..._next` returning false on every row (silent: no error, just + /// zero cells walked). Mirrors + /// `mac/Sources/Roost/RenderState.swift::walk`'s + /// `withUnsafeMutablePointer(to: &self.rowIter)` pattern. + fn bind_row_iterator(&mut self) -> Result<()> { + // SAFETY: state + iter handles non-null per constructor. + let rc = unsafe { + sys::ghostty_render_state_get( + self.handle, + sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_ROW_ITERATOR, + (&mut self.row_iter) as *mut _ as *mut _, + ) + }; + Error::from_result(rc) + } + + /// Rebind `self.row_cells` to the row the iterator currently sits + /// on. Same pointer-to-slot semantics as `bind_row_iterator`. + fn bind_row_cells(&mut self) -> Result<()> { + // SAFETY: iter + cells handles non-null per constructor. + let rc = unsafe { + sys::ghostty_render_state_row_get( + self.row_iter, + sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_CELLS, + (&mut self.row_cells) as *mut _ as *mut _, + ) + }; + Error::from_result(rc) + } + + /// Dirty flag of the row the iterator currently sits on. A failed + /// read reports `false`, which leaves the flag alone rather than + /// clearing damage we could not confirm. + fn read_row_dirty(&self) -> bool { + let mut out: bool = false; + // SAFETY: iter handle non-null; out is a real local. + let rc = unsafe { + sys::ghostty_render_state_row_get( + self.row_iter, + sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_DIRTY, + (&mut out) as *mut bool as *mut _, + ) + }; + Error::from_result(rc).is_ok() && out + } + + fn clear_row_dirty(&mut self) -> Result<()> { + let value = false; + // SAFETY: iter handle non-null; value is a real local. + let rc = unsafe { + sys::ghostty_render_state_row_set( + self.row_iter, + sys::GhosttyRenderStateRowOption_GHOSTTY_RENDER_STATE_ROW_OPTION_DIRTY, + (&value) as *const bool as *const _, + ) + }; + Error::from_result(rc) + } + + fn set_global_dirty(&mut self, value: sys::GhosttyRenderStateDirty) -> Result<()> { + // SAFETY: handle non-null; value is a real local. + let rc = unsafe { + sys::ghostty_render_state_set( + self.handle, + sys::GhosttyRenderStateOption_GHOSTTY_RENDER_STATE_OPTION_DIRTY, + (&value) as *const _ as *const _, + ) + }; + Error::from_result(rc) + } + fn read_current_cell(&self, col: u16) -> Cell { let bg = self.read_cells_color( sys::GhosttyRenderStateRowCellsData_GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_BG_COLOR, diff --git a/crates/roost-vt/tests/render_dirty_test.rs b/crates/roost-vt/tests/render_dirty_test.rs new file mode 100644 index 00000000..15bdab21 --- /dev/null +++ b/crates/roost-vt/tests/render_dirty_test.rs @@ -0,0 +1,385 @@ +#![cfg(feature = "ffi")] +//! Pins the dirty-tracking semantics of libghostty's render state at our +//! pinned Ghostty SHA. Every expectation here was measured against the +//! real `libghostty-vt.a` before the wrapper was written (plan 017 §2.2); +//! a failure means libghostty's behavior moved, not that the wrapper is +//! merely mis-specified. + +use roost_vt::{ColorRgb, Dirty, RenderState, ScrollViewport, Terminal, TerminalOptions}; + +/// A terminal + render state that have never been updated: the render +/// state still reports `Full` on its first `update`. +fn fresh(cols: u16, rows: u16) -> (Terminal, RenderState) { + let t = Terminal::new(TerminalOptions { + cols, + rows, + max_scrollback: 500, + }) + .expect("Terminal::new"); + (t, RenderState::new().expect("RenderState::new")) +} + +/// A terminal + render state already drained to `Clean`, so the next +/// change is the only damage the test sees. +fn settled(cols: u16, rows: u16) -> (Terminal, RenderState) { + let (t, mut rs) = fresh(cols, rows); + settle(&mut rs, &t); + (t, rs) +} + +/// Drain the current damage and confirm the terminal has settled to +/// `Clean` — i.e. both dirty layers really were cleared. +fn settle(rs: &mut RenderState, t: &Terminal) { + rs.update(t).expect("update"); + rs.walk_dirty(t, |_, _| {}).expect("walk_dirty"); + rs.update(t).expect("update"); + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "terminal should settle to Clean after a walk_dirty + no-change update" + ); +} + +/// Row indices visited by one `walk_dirty`, plus the state it reported. +fn visit(rs: &mut RenderState, t: &Terminal) -> (Dirty, Vec) { + let mut rows = Vec::new(); + let state = rs + .walk_dirty(t, |row, _| rows.push(row)) + .expect("walk_dirty"); + (state, rows) +} + +fn row_text(cells: &[roost_vt::Cell]) -> String { + cells + .iter() + .map(|c| if c.text.is_empty() { " " } else { &c.text }) + .collect::() + .trim_end() + .to_string() +} + +#[test] +fn fresh_update_reports_full_and_visits_every_row() { + let (t, mut rs) = fresh(80, 24); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full, "walk_dirty returns the state on entry"); + assert_eq!(rows, (0..24).collect::>()); +} + +#[test] +fn settled_terminal_reports_clean_and_visits_nothing() { + let (t, mut rs) = settled(80, 24); + + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Clean); + // Zero rows visited is the proof that walk_dirty cleared BOTH layers: + // had it cleared only the global one, every row flag would survive + // and this walk would visit all 24. + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn single_row_write_reports_partial_on_that_row_only() { + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + + let mut visited: Vec<(u32, String)> = Vec::new(); + let state = rs + .walk_dirty(&t, |row, cells| visited.push((row, row_text(cells)))) + .expect("walk_dirty"); + + assert_eq!(state, Dirty::Partial); + assert_eq!(visited.len(), 1, "expected only row 0, got {visited:?}"); + assert_eq!(visited[0].0, 0); + assert_eq!(visited[0].1, "hello"); +} + +#[test] +fn row_flags_are_cleared_alongside_the_global_layer() { + // render.h's "extremely important detail": the global and per-row + // dirty layers are independent, so clearing one does not clear the + // other. If `walk_dirty` cleared only the global layer, row 0's flag + // would survive its first walk and row 0 would be redrawn on every + // subsequent frame forever. This test is what catches that. + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + let (_, first) = visit(&mut rs, &t); + assert_eq!(first, vec![0]); + + // Park the cursor off row 0 and settle, so the next write's cursor + // movement can't re-dirty row 0 for a reason unrelated to the + // footgun (§2.2 finding 4: a cursor move dirties both the row it + // leaves and the row it lands on). + t.vt_write(b"\x1b[11;1H"); + rs.update(&t).expect("update"); + let _ = visit(&mut rs, &t); + settle(&mut rs, &t); + + t.vt_write(b"\x1b[5;1HmarkerX"); + rs.update(&t).expect("update"); + let (state, second) = visit(&mut rs, &t); + + assert_eq!(state, Dirty::Partial); + assert!( + !second.contains(&0), + "row 0 must not be revisited — its flag was consumed by the first \ + walk_dirty; got {second:?}" + ); + // Row 4 is the write; row 10 is the cursor row it vacated. + assert_eq!(second, vec![4, 10]); +} + +#[test] +fn full_dirty_state_is_cleared_by_walk_dirty() { + // Full-layer counterpart to `row_flags_are_cleared_alongside_the_global_layer` + // above, which pins that the ROW layer would survive a walk that + // cleared only the global one. This test pins the other half: + // forcing the GLOBAL layer to Full and confirming `walk_dirty` + // clears it too, so a subsequent no-change frame settles to Clean + // instead of reporting Full forever. + let (t, mut rs) = settled(80, 24); + + rs.mark_full().expect("mark_full"); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..24).collect::>()); + + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Clean); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Clean); + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn dirty_rows_is_a_pure_read() { + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + + let first = rs.dirty_rows(&t).expect("dirty_rows"); + let second = rs.dirty_rows(&t).expect("dirty_rows"); + assert_eq!(first, vec![0]); + assert_eq!(first, second, "dirty_rows must not consume anything"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + + let (_, rows) = visit(&mut rs, &t); + assert_eq!( + rows, first, + "walk_dirty still sees the rows dirty_rows read" + ); +} + +#[test] +fn mark_full_forces_a_full_visit() { + let (t, mut rs) = settled(80, 24); + + rs.mark_full().expect("mark_full"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..24).collect::>()); +} + +#[test] +fn theme_color_changes_report_full() { + let (mut t, mut rs) = settled(80, 24); + + // The Rust theme path — no vt bytes at all. + t.set_color_foreground(ColorRgb::new(1, 2, 3)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(4, 5, 6)) + .expect("set_color_background"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (_, rows) = visit(&mut rs, &t); + assert_eq!(rows.len(), 24); + + settle(&mut rs, &t); + t.set_color_palette(&[ColorRgb::new(9, 9, 9); 256]) + .expect("set_color_palette"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (_, rows) = visit(&mut rs, &t); + assert_eq!(rows.len(), 24); +} + +#[test] +fn resize_reports_full_over_the_new_row_count() { + let (mut t, mut rs) = settled(80, 24); + + t.resize(100, 30, 8, 16).expect("resize"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..30).collect::>()); +} + +#[test] +fn viewport_scroll_reports_full() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..40 { + t.vt_write(format!("line-{i:03}\r\n").as_bytes()); + } + settle(&mut rs, &t); + + // A pure viewport move writes no cells, yet every visible row now + // shows different content — libghostty reports FULL, which is what + // lets a row cache trust it (§2.2 probe [S1]). + t.scroll_viewport(ScrollViewport::Delta(-10)); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..6).collect::>()); +} + +#[test] +fn output_driven_scroll_reports_full() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..6 { + t.vt_write(format!("\x1b[{};1Hrow-{i}", i + 1).as_bytes()); + } + // The marker loop's last write already leaves the cursor on row 6 + // (0-indexed 5), the last viewport row. + settle(&mut rs, &t); + + // Writing past the last row scrolls the viewport by one line via + // normal PTY output — no explicit `scroll_viewport` call. Per + // third_party/ghostty/src/src/terminal/render.zig:299-302 ("If our + // viewport pin changed, we do a full rebuild"), libghostty reports + // this the same way as `viewport_scroll_reports_full` above: a full + // rebuild, not an incremental per-row change. Streaming output that + // scrolls the viewport therefore gets no incremental benefit from + // dirty tracking. This test pins that expectation so a future + // Ghostty bump that changes it is noticed. + t.vt_write(b"\r\nSCROLLED"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..6).collect::>()); +} + +#[test] +fn in_place_write_without_scrolling_stays_partial() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..6 { + t.vt_write(format!("\x1b[{};1Hrow-{i}", i + 1).as_bytes()); + } + settle(&mut rs, &t); + + // No viewport-pin change here — the write lands in place on row 2, + // moving the cursor off row 6 (where the marker loop parked it). + t.vt_write(b"\x1b[2;1HINPLACE"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Partial); + // Row 1 (0-indexed) is the write; row 5 is the cursor row it + // vacated. This is the case where E3 actually saves work: only the + // touched rows are marked, not a full rebuild. + assert_eq!(rows, vec![1, 5]); +} + +#[test] +fn osc_default_color_change_reports_clean_known_limitation() { + // KNOWN LIMITATION, pinned deliberately. At our Ghostty pin, changing + // the default background via OSC 11 as PTY bytes marks NOTHING dirty + // — not the global layer, not a single row — even though every cell + // that inherits the default now renders differently. (The same change + // made through the Rust API, `set_color_background`, does report + // FULL; only the OSC-bytes path is silent.) + // + // This is why `roost-iced` must compare the default fg/bg itself and + // call `mark_full` when they move (plan 017 D3b) instead of trusting + // dirty tracking alone. + // + // If a future Ghostty bump starts reporting Full here, this test + // fails loudly — that is the point. At that time it can be relaxed + // and D3b's guard reconsidered. + // + // Both defaults are set before settling, mirroring what roost always + // does at attach — libghostty's colors reporting (render.zig's + // `orelse break :bg_fg`) only fully engages once both fg and bg + // defaults are set, so a test that left one unset would not pin the + // behavior roost actually experiences. + let (mut t, mut rs) = fresh(80, 24); + t.set_color_foreground(ColorRgb::new(0xAA, 0xAA, 0xAA)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(0x11, 0x11, 0x11)) + .expect("set_color_background"); + settle(&mut rs, &t); + + t.vt_write(b"\x1b]11;#123456\x07"); + rs.update(&t).expect("update"); + + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "OSC 11 reporting dirty would be an IMPROVEMENT — see the comment" + ); + assert!(rs.dirty_rows(&t).expect("dirty_rows").is_empty()); + let (_, rows) = visit(&mut rs, &t); + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn decscnm_swaps_reported_colors_without_marking_dirty() { + // Same class of limitation as the OSC 10/11 test above: libghostty's + // colors() swap on DECSCNM (reverse video) only applies when BOTH + // defaults are set — render.zig:326-339 does `orelse break :bg_fg` — + // and even then the swap itself marks nothing dirty. Since roost + // always sets both defaults at attach, this is the transition the + // iced consumer actually hits: it must compare the default fg/bg + // pair itself on every frame rather than trusting dirty state for + // this case. + let (mut t, mut rs) = fresh(80, 24); + t.set_color_foreground(ColorRgb::new(0xAA, 0xAA, 0xAA)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(0x11, 0x11, 0x11)) + .expect("set_color_background"); + t.vt_write(b"hello"); + settle(&mut rs, &t); + + let before = rs.colors().expect("colors"); + assert_eq!(before.foreground, ColorRgb::new(0xAA, 0xAA, 0xAA)); + assert_eq!(before.background, ColorRgb::new(0x11, 0x11, 0x11)); + + t.vt_write(b"\x1b[?5h"); + rs.update(&t).expect("update"); + + let after = rs.colors().expect("colors"); + assert_eq!( + after.foreground, + ColorRgb::new(0x11, 0x11, 0x11), + "DECSCNM should swap the reported foreground to the old background" + ); + assert_eq!( + after.background, + ColorRgb::new(0xAA, 0xAA, 0xAA), + "DECSCNM should swap the reported background to the old foreground" + ); + + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "the color swap itself must not mark anything dirty — see comment" + ); + assert!(rs.dirty_rows(&t).expect("dirty_rows").is_empty()); +} From 899758bb78c86d3bb2931382aade338996ab530c Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 10:06:32 -0500 Subject: [PATCH 04/11] feat(iced): instrument render refresh and draw with perf counters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-iced/src/app/servicing.rs | 37 +++++++ crates/roost-iced/src/app/terminal_tab.rs | 14 +++ crates/roost-iced/src/main.rs | 1 + crates/roost-iced/src/perf.rs | 126 ++++++++++++++++++++++ crates/roost-iced/src/terminal_widget.rs | 5 + 5 files changed, 183 insertions(+) create mode 100644 crates/roost-iced/src/perf.rs diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index ab4a0a81..ef5b792b 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -1509,4 +1509,41 @@ mod tests { ); assert_eq!(workspace.active().1, other.id, "and moves nothing"); } + + /// Pins the per-tab counters `refresh_snapshot` maintains. These are + /// asserted on the tab's own `TabRenderStats`, not the process-global + /// aggregate in `perf` — `cargo test -p roost-iced` runs concurrently + /// with other tests that spawn their own PTY and refresh their own + /// tab, and a global counter would pick up their activity too. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn refresh_snapshot_updates_the_tabs_own_render_stats() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(75, feed_tx); + assert_eq!(tab.render_stats, crate::perf::TabRenderStats::default()); + + tab.refresh_snapshot().expect("refresh"); + + assert_eq!(tab.render_stats.refresh_calls, 1); + assert_eq!( + tab.render_stats.rows_rebuilt, + u64::from(DEFAULT_ROWS), + "the current refresh always rebuilds the whole grid" + ); + assert_eq!( + tab.render_stats.cells_walked, + u64::from(DEFAULT_COLS) * u64::from(DEFAULT_ROWS) + ); + assert!( + tab.render_stats.refresh_nanos > 0, + "refresh does real work, so elapsed time should be nonzero" + ); + + tab.refresh_snapshot().expect("second refresh"); + assert_eq!( + tab.render_stats.refresh_calls, 2, + "counters accumulate across calls rather than resetting" + ); + + supervisor.close(75); + } } diff --git a/crates/roost-iced/src/app/terminal_tab.rs b/crates/roost-iced/src/app/terminal_tab.rs index c239e0bd..7a278e29 100644 --- a/crates/roost-iced/src/app/terminal_tab.rs +++ b/crates/roost-iced/src/app/terminal_tab.rs @@ -206,6 +206,7 @@ pub(super) struct TerminalTab { rows: u16, pub(super) applied_metrics: Option, pub(super) metric_generation: u64, + pub(super) render_stats: crate::perf::TabRenderStats, } impl Drop for TerminalTab { @@ -286,6 +287,7 @@ impl TerminalTab { rows: DEFAULT_ROWS, applied_metrics: None, metric_generation: 0, + render_stats: crate::perf::TabRenderStats::default(), }) } @@ -877,13 +879,16 @@ impl TerminalTab { } pub(super) fn refresh_snapshot(&mut self) -> Result<()> { + let refresh_started_at = Instant::now(); self.recompute_hover()?; self.render_state.update(&self.terminal)?; let colors = self.render_state.colors()?; let cursor = self.render_state.cursor(); let mut cells = Vec::new(); let mut rows = vec![vec![String::new(); usize::from(self.cols)]; usize::from(self.rows)]; + let mut cells_walked: u64 = 0; self.render_state.walk(&self.terminal, |row, cell| { + cells_walked += 1; if row >= u32::from(self.rows) || cell.col >= self.cols { return; } @@ -913,6 +918,11 @@ impl TerminalTab { }); } })?; + // The full-grid rebuild below always touches every row of `rows`, + // so its length is the truthful "rows rebuilt" count today. Once + // the rebuild goes incremental (C5), `rows` will hold only the + // rebuilt rows and this stays correct without changing. + let rows_rebuilt = rows.len() as u64; let rows_text = rows .into_iter() .map(|row| row.concat().trim_end().to_string()) @@ -939,6 +949,10 @@ impl TerminalTab { }), pointer_shape: self.effective_pointer_shape().into(), }; + let elapsed = refresh_started_at.elapsed(); + self.render_stats + .record_refresh(elapsed, rows_rebuilt, cells_walked); + crate::perf::record_refresh(elapsed, rows_rebuilt, cells_walked); Ok(()) } diff --git a/crates/roost-iced/src/main.rs b/crates/roost-iced/src/main.rs index 1e9d25a8..8cd49f4a 100644 --- a/crates/roost-iced/src/main.rs +++ b/crates/roost-iced/src/main.rs @@ -6,6 +6,7 @@ mod input; mod notifications; mod palette_scroll; mod paste_image; +mod perf; mod png_encode; mod screenshot; mod sidebar_resize; diff --git a/crates/roost-iced/src/perf.rs b/crates/roost-iced/src/perf.rs new file mode 100644 index 00000000..df4963a6 --- /dev/null +++ b/crates/roost-iced/src/perf.rs @@ -0,0 +1,126 @@ +//! Render performance instrumentation. Counters and timings only — this +//! commit changes no rendering behavior, it just measures the current one +//! so a later commit has a baseline to optimize against. +//! +//! Two scopes, deliberately: +//! +//! - [`TabRenderStats`] is a plain (non-atomic) per-`TerminalTab` struct. +//! `cargo test -p roost-iced` runs tests in parallel and the test fixture +//! spawns a real PTY per test, so several tests refresh concurrently; a +//! `after - before` delta read off a shared global counter would be +//! polluted by whatever other tests are doing at the same moment. Per-tab +//! counters are what unit tests assert on, and they sidestep that flake +//! class entirely by construction. +//! - [`snapshot`] reads a process-global `AtomicU64` aggregate that every +//! tab folds its work into. This is what a later IPC op reads out of a +//! running app; no test asserts on it. +//! +//! Two traps to know about before trusting a number out of this module: +//! +//! - `iced::window::screenshot` re-renders the window, so `roostctl +//! screenshot` (and everything in `tools/screenshot/`) inflates +//! `draw_calls` / `draw_nanos` / `fill_text_calls` just by having run. +//! Read the counters before taking a screenshot, or [`reset`] afterward. +//! - The `draw_*` and `fill_text_calls` counters only exist in a running +//! app: `TerminalWidget::draw` needs a live iced `Renderer`, which unit +//! tests don't construct. There is no per-tab equivalent of them for the +//! same reason — see [`TabRenderStats`]. + +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Duration; + +static REFRESH_CALLS: AtomicU64 = AtomicU64::new(0); +static REFRESH_NANOS: AtomicU64 = AtomicU64::new(0); +static ROWS_REBUILT: AtomicU64 = AtomicU64::new(0); +static CELLS_WALKED: AtomicU64 = AtomicU64::new(0); +static DRAW_CALLS: AtomicU64 = AtomicU64::new(0); +static DRAW_NANOS: AtomicU64 = AtomicU64::new(0); +static FILL_TEXT_CALLS: AtomicU64 = AtomicU64::new(0); + +/// A read of the process-global aggregate at one instant. Every field is a +/// running total since process start (or the last [`reset`]). +/// +/// `snapshot`/`reset` have no caller yet — the IPC op that exposes this to +/// `roostctl` lands in a later commit — so this and they are allowed dead +/// for now rather than stubbed with a fake caller. +#[allow(dead_code)] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct RenderStats { + pub refresh_calls: u64, + pub refresh_nanos: u64, + pub rows_rebuilt: u64, + pub cells_walked: u64, + pub draw_calls: u64, + pub draw_nanos: u64, + pub fill_text_calls: u64, +} + +/// Per-tab counters, folded into the global aggregate on every refresh. +/// There is deliberately no per-tab equivalent of `draw_calls` / +/// `draw_nanos` / `fill_text_calls`: `TerminalWidget::draw` renders from a +/// `TerminalSnapshot` clone handed to it by iced and has no way back to the +/// `TerminalTab` that produced it, so those three counters only exist in +/// the global aggregate. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct TabRenderStats { + pub refresh_calls: u64, + pub refresh_nanos: u64, + pub rows_rebuilt: u64, + pub cells_walked: u64, +} + +impl TabRenderStats { + pub(crate) fn record_refresh( + &mut self, + elapsed: Duration, + rows_rebuilt: u64, + cells_walked: u64, + ) { + self.refresh_calls += 1; + self.refresh_nanos += elapsed.as_nanos() as u64; + self.rows_rebuilt += rows_rebuilt; + self.cells_walked += cells_walked; + } +} + +/// Read the global aggregate. +#[allow(dead_code)] +pub fn snapshot() -> RenderStats { + RenderStats { + refresh_calls: REFRESH_CALLS.load(Ordering::Relaxed), + refresh_nanos: REFRESH_NANOS.load(Ordering::Relaxed), + rows_rebuilt: ROWS_REBUILT.load(Ordering::Relaxed), + cells_walked: CELLS_WALKED.load(Ordering::Relaxed), + draw_calls: DRAW_CALLS.load(Ordering::Relaxed), + draw_nanos: DRAW_NANOS.load(Ordering::Relaxed), + fill_text_calls: FILL_TEXT_CALLS.load(Ordering::Relaxed), + } +} + +/// Zero the global aggregate. Useful before an operation known to skew it +/// (see the screenshot trap above) so the next read is uncontaminated. +#[allow(dead_code)] +pub fn reset() { + REFRESH_CALLS.store(0, Ordering::Relaxed); + REFRESH_NANOS.store(0, Ordering::Relaxed); + ROWS_REBUILT.store(0, Ordering::Relaxed); + CELLS_WALKED.store(0, Ordering::Relaxed); + DRAW_CALLS.store(0, Ordering::Relaxed); + DRAW_NANOS.store(0, Ordering::Relaxed); + FILL_TEXT_CALLS.store(0, Ordering::Relaxed); +} + +/// Fold one `refresh_snapshot` call into the global aggregate. +pub(crate) fn record_refresh(elapsed: Duration, rows_rebuilt: u64, cells_walked: u64) { + REFRESH_CALLS.fetch_add(1, Ordering::Relaxed); + REFRESH_NANOS.fetch_add(elapsed.as_nanos() as u64, Ordering::Relaxed); + ROWS_REBUILT.fetch_add(rows_rebuilt, Ordering::Relaxed); + CELLS_WALKED.fetch_add(cells_walked, Ordering::Relaxed); +} + +/// Fold one `TerminalWidget::draw` call into the global aggregate. +pub(crate) fn record_draw(elapsed: Duration, fill_text_calls: u64) { + DRAW_CALLS.fetch_add(1, Ordering::Relaxed); + DRAW_NANOS.fetch_add(elapsed.as_nanos() as u64, Ordering::Relaxed); + FILL_TEXT_CALLS.fetch_add(fill_text_calls, Ordering::Relaxed); +} diff --git a/crates/roost-iced/src/terminal_widget.rs b/crates/roost-iced/src/terminal_widget.rs index a7fc8b12..2071a6cb 100644 --- a/crates/roost-iced/src/terminal_widget.rs +++ b/crates/roost-iced/src/terminal_widget.rs @@ -569,11 +569,14 @@ impl Widget for TerminalWidget { _cursor: mouse::Cursor, viewport: &Rectangle, ) { + let draw_started_at = Instant::now(); let bounds = layout.bounds(); let Some(clip) = bounds.intersection(viewport) else { + crate::perf::record_draw(draw_started_at.elapsed(), 0); return; }; + let mut fill_text_calls: u64 = 0; renderer.with_layer(clip, |renderer| { fill_quad(renderer, bounds, color(self.snapshot.background)); let metrics = self.metrics; @@ -611,6 +614,7 @@ impl Widget for TerminalWidget { color(cell.foreground), clip, ); + fill_text_calls += 1; } } @@ -692,6 +696,7 @@ impl Widget for TerminalWidget { } } }); + crate::perf::record_draw(draw_started_at.elapsed(), fill_text_calls); } } From c313d374e02898d3b5aa224005f65cc5cadfa7a2 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 10:18:47 -0500 Subject: [PATCH 05/11] feat(ipc): add app.render_stats op to read render counters from a live UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-cli/src/main.rs | 53 +++++++-- crates/roost-engine/src/ipc.rs | 45 ++++++-- crates/roost-iced/src/app.rs | 4 +- crates/roost-iced/src/app/servicing.rs | 15 +++ crates/roost-iced/src/perf.rs | 14 +-- crates/roost-ipc/src/messages.rs | 103 ++++++++++++++++++ crates/roost-ipc/tests/vectors.rs | 34 ++++++ crates/roost-linux/src/app.rs | 14 ++- docs/reference/cli.md | 13 +++ docs/reference/ipc.md | 42 +++++++ .../ipc-vectors/app.render_stats.request.json | 7 ++ .../app.render_stats.response.json | 13 +++ 12 files changed, 326 insertions(+), 31 deletions(-) create mode 100644 tests/ipc-vectors/app.render_stats.request.json create mode 100644 tests/ipc-vectors/app.render_stats.response.json diff --git a/crates/roost-cli/src/main.rs b/crates/roost-cli/src/main.rs index a98f313d..afc127e8 100644 --- a/crates/roost-cli/src/main.rs +++ b/crates/roost-cli/src/main.rs @@ -21,6 +21,7 @@ //! roostctl project {list,create,rename,delete,reorder} //! roostctl palette {open,state,query,activate,dismiss} //! roostctl screenshot [--out PATH] [--scale 1|2] +//! roostctl render-stats [--reset] //! roostctl claude-hook EVENT //! roostctl claude install [--force] //! @@ -45,13 +46,14 @@ use clap::{Parser, Subcommand, ValueEnum}; use roost_agent::claude::{canonical_hook_event, claude_event_to_reports, CLAUDE_HOOK_EVENTS}; use roost_ipc::messages::ops; use roost_ipc::messages::{ - IdentifyParams, IdentifyResult, NotificationCreateParams, PaletteActivateParams, - PaletteItemView, PaletteOpenParams, PalettePresentParams, PalettePresentResult, - PaletteQueryParams, PaletteStateResult, ProjectCreateParams, ProjectCreateResult, - ProjectDeleteParams, ProjectRenameParams, ProjectReorderParams, ScreenshotParams, - ScreenshotResult, TabClearNotificationParams, TabCloseParams, TabDumpParams, TabDumpResult, - TabFocusParams, TabListResult, TabOpenParams, TabOpenResult, TabReorderParams, TabResizeParams, - TabSetStateParams, TabSetTitleParams, TabState, TabWriteParams, + AppRenderStatsParams, AppRenderStatsResult, IdentifyParams, IdentifyResult, + NotificationCreateParams, PaletteActivateParams, PaletteItemView, PaletteOpenParams, + PalettePresentParams, PalettePresentResult, PaletteQueryParams, PaletteStateResult, + ProjectCreateParams, ProjectCreateResult, ProjectDeleteParams, ProjectRenameParams, + ProjectReorderParams, ScreenshotParams, ScreenshotResult, TabClearNotificationParams, + TabCloseParams, TabDumpParams, TabDumpResult, TabFocusParams, TabListResult, TabOpenParams, + TabOpenResult, TabReorderParams, TabResizeParams, TabSetStateParams, TabSetTitleParams, + TabState, TabWriteParams, }; use roost_ipc::paths::BundleProfileKind; use roost_ipc::target::{ResolvedTarget, TargetError, TargetSelector}; @@ -169,6 +171,16 @@ enum Cmd { #[arg(long, default_value_t = 1, value_parser = clap::value_parser!(u32).range(1..=2))] scale: u32, }, + /// Read the running UI's render-path counters — refresh/draw call + /// counts, elapsed nanos, rows and cells walked, `fill_text` calls. + /// The only way to measure the real draw path: it needs a live + /// renderer no unit test can construct. + RenderStats { + /// Zero the counters after reading them, so the next read is a + /// clean delta over whatever ran in between. + #[arg(long)] + reset: bool, + }, /// Claude Code hook entry point. Reads the JSON event payload /// from stdin (Claude's contract), dispatches state + /// notification ops to the running UI, and ALWAYS exits 0 with @@ -881,6 +893,33 @@ async fn main() -> Result<()> { } } } + Cmd::RenderStats { reset } => { + let stats: AppRenderStatsResult = client + .call(ops::APP_RENDER_STATS, AppRenderStatsParams { reset }) + .await?; + let per = |total: i64, calls: i64| { + if calls > 0 { + (total / calls).to_string() + } else { + "-".to_string() + } + }; + println!("refresh_calls {}", stats.refresh_calls); + println!("refresh_nanos {}", stats.refresh_nanos); + println!("rows_rebuilt {}", stats.rows_rebuilt); + println!("cells_walked {}", stats.cells_walked); + println!("draw_calls {}", stats.draw_calls); + println!("draw_nanos {}", stats.draw_nanos); + println!("fill_text_calls {}", stats.fill_text_calls); + println!( + "ns_per_refresh {}", + per(stats.refresh_nanos, stats.refresh_calls) + ); + println!( + "ns_per_draw {}", + per(stats.draw_nanos, stats.draw_calls) + ); + } Cmd::Palette(PaletteCmd::Open { kind, json }) => { let state: PaletteStateResult = client .call(ops::PALETTE_OPEN, PaletteOpenParams { kind }) diff --git a/crates/roost-engine/src/ipc.rs b/crates/roost-engine/src/ipc.rs index 336e09d8..a12fd8be 100644 --- a/crates/roost-engine/src/ipc.rs +++ b/crates/roost-engine/src/ipc.rs @@ -21,16 +21,16 @@ use std::sync::Arc; use roost_ipc::agent::{self, TabAgentReportParams}; use roost_ipc::messages::{ ops, AppActivateParams, AppActiveTerminalFocusedParams, AppActiveTerminalFocusedResult, - AppCursorShapeParams, AppCursorShapeResult, AppSelectedTabIdParams, AppSelectedTabIdResult, - AppSetWindowFocusParams, ClipboardDumpParams, ClipboardDumpResult, ClipboardWriteParams, - IdentifyParams, IdentifyResult, NotificationCreateParams, PaletteActivateParams, - PaletteDismissParams, PaletteOpenParams, PalettePresentParams, PalettePresentResult, - PaletteQueryParams, PaletteStateParams, PaletteStateResult, ProjectCreateParams, - ProjectCreateResult, ProjectDeleteParams, ProjectRenameParams, ProjectReorderParams, - ResolvedCell, ScreenshotParams, ScreenshotResult, SelectionClearParams, SelectionDumpParams, - SelectionDumpResult, SelectionSetParams, SidebarDumpParams, SidebarDumpResult, - SidebarSetWidthParams, TabAgentReportResult, TabCapturePtyInputParams, - TabCapturePtyInputResult, TabClearNotificationParams, TabCloseParams, + AppCursorShapeParams, AppCursorShapeResult, AppRenderStatsParams, AppRenderStatsResult, + AppSelectedTabIdParams, AppSelectedTabIdResult, AppSetWindowFocusParams, ClipboardDumpParams, + ClipboardDumpResult, ClipboardWriteParams, IdentifyParams, IdentifyResult, + NotificationCreateParams, PaletteActivateParams, PaletteDismissParams, PaletteOpenParams, + PalettePresentParams, PalettePresentResult, PaletteQueryParams, PaletteStateParams, + PaletteStateResult, ProjectCreateParams, ProjectCreateResult, ProjectDeleteParams, + ProjectRenameParams, ProjectReorderParams, ResolvedCell, ScreenshotParams, ScreenshotResult, + SelectionClearParams, SelectionDumpParams, SelectionDumpResult, SelectionSetParams, + SidebarDumpParams, SidebarDumpResult, SidebarSetWidthParams, TabAgentReportResult, + TabCapturePtyInputParams, TabCapturePtyInputResult, TabClearNotificationParams, TabCloseParams, TabDispatchMouseEventParams, TabDumpCursor, TabDumpParams, TabDumpResolvedParams, TabDumpResolvedResult, TabDumpResult, TabExpandSelectionAtParams, TabExpandSelectionAtResult, TabFeedPtyBytesParams, TabFocusParams, TabFocusResult, TabListResult, TabOpenParams, @@ -66,6 +66,12 @@ type WindowMetricsReply = tokio::sync::oneshot::Sender>; +/// Reply for [`UiRequest::AppRenderStats`]: the UI's render-path +/// counters. Read-only; always answers `Ok`, matching +/// `WindowMetricsReply`. A UI with no instrumentation answers with a +/// zeroed struct rather than an error — see the GTK arm. +type RenderStatsReply = tokio::sync::oneshot::Sender>; + /// Reply for a [`UiRequest::Dump`]: the viewport text on success, an /// error message (e.g. tab not found / no live terminal) on failure. type DumpReply = tokio::sync::oneshot::Sender>; @@ -268,6 +274,14 @@ pub enum UiRequest { /// collapsed flag (logical points). Backs the sidebar-holds-width /// regression suite. Ungated (read-only). WindowMetrics { reply: WindowMetricsReply }, + /// `app.render_stats` — read the UI's render-path counters, and + /// zero them afterward when `reset`. Ungated (read-only, and the + /// counters are the only way to measure the real draw path: it + /// needs a live renderer no unit test can construct). + AppRenderStats { + reset: bool, + reply: RenderStatsReply, + }, /// `app.sidebar_dump` — read the sidebar's last-rendered agent rows /// per project, plus the agents-visible toggle. Ungated (read-only); /// reads `ProjectUi::rendered_agents`, the same cache the sidebar @@ -681,6 +695,17 @@ async fn dispatch( .map_err(|m| HandlerError::new("internal", m))?; encode(&result) } + ops::APP_RENDER_STATS => { + let p: AppRenderStatsParams = decode(params)?; + let result = h + .ui_call(|reply| UiRequest::AppRenderStats { + reset: p.reset, + reply, + }) + .await? + .map_err(|m| HandlerError::new("internal", m))?; + encode(&result) + } ops::SIDEBAR_DUMP => { let _p: SidebarDumpParams = decode(params)?; let result = h diff --git a/crates/roost-iced/src/app.rs b/crates/roost-iced/src/app.rs index df1574dd..0fa1c6c4 100644 --- a/crates/roost-iced/src/app.rs +++ b/crates/roost-iced/src/app.rs @@ -29,8 +29,8 @@ use roost_engine::{ }; use roost_ipc::agent; use roost_ipc::messages::{ - PaletteItemView, PalettePresentResult, PaletteStateResult, Project, SidebarDumpAgentRow, - SidebarDumpProject, SidebarDumpResult, WindowMetricsResult, + AppRenderStatsResult, PaletteItemView, PalettePresentResult, PaletteStateResult, Project, + SidebarDumpAgentRow, SidebarDumpProject, SidebarDumpResult, WindowMetricsResult, }; use roost_ipc::paths::BundleProfile; use roost_ipc::IpcServer; diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index ef5b792b..1f2c70c0 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -759,6 +759,21 @@ impl App { .ok_or_else(|| format!("tab {tab_id} has no live terminal")); let _ = reply.send(result); } + UiRequest::AppRenderStats { reset, reply } => { + let stats = crate::perf::snapshot(); + if reset { + crate::perf::reset(); + } + let _ = reply.send(Ok(AppRenderStatsResult { + refresh_calls: stats.refresh_calls as i64, + refresh_nanos: stats.refresh_nanos as i64, + rows_rebuilt: stats.rows_rebuilt as i64, + cells_walked: stats.cells_walked as i64, + draw_calls: stats.draw_calls as i64, + draw_nanos: stats.draw_nanos as i64, + fill_text_calls: stats.fill_text_calls as i64, + })); + } UiRequest::WindowMetrics { reply } => { let collapsed = self.workspace.sidebar_collapsed(); let resolved_family = self diff --git a/crates/roost-iced/src/perf.rs b/crates/roost-iced/src/perf.rs index df4963a6..35fa3abd 100644 --- a/crates/roost-iced/src/perf.rs +++ b/crates/roost-iced/src/perf.rs @@ -12,8 +12,8 @@ //! counters are what unit tests assert on, and they sidestep that flake //! class entirely by construction. //! - [`snapshot`] reads a process-global `AtomicU64` aggregate that every -//! tab folds its work into. This is what a later IPC op reads out of a -//! running app; no test asserts on it. +//! tab folds its work into. This is what the `app.render_stats` IPC op +//! reads out of a running app; no test asserts on it. //! //! Two traps to know about before trusting a number out of this module: //! @@ -39,11 +39,6 @@ static FILL_TEXT_CALLS: AtomicU64 = AtomicU64::new(0); /// A read of the process-global aggregate at one instant. Every field is a /// running total since process start (or the last [`reset`]). -/// -/// `snapshot`/`reset` have no caller yet — the IPC op that exposes this to -/// `roostctl` lands in a later commit — so this and they are allowed dead -/// for now rather than stubbed with a fake caller. -#[allow(dead_code)] #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct RenderStats { pub refresh_calls: u64, @@ -83,8 +78,7 @@ impl TabRenderStats { } } -/// Read the global aggregate. -#[allow(dead_code)] +/// Read the global aggregate. Backs the `app.render_stats` IPC op. pub fn snapshot() -> RenderStats { RenderStats { refresh_calls: REFRESH_CALLS.load(Ordering::Relaxed), @@ -99,7 +93,7 @@ pub fn snapshot() -> RenderStats { /// Zero the global aggregate. Useful before an operation known to skew it /// (see the screenshot trap above) so the next read is uncontaminated. -#[allow(dead_code)] +/// Exposed over IPC as `app.render_stats` with `reset: true`. pub fn reset() { REFRESH_CALLS.store(0, Ordering::Relaxed); REFRESH_NANOS.store(0, Ordering::Relaxed); diff --git a/crates/roost-ipc/src/messages.rs b/crates/roost-ipc/src/messages.rs index 1157338f..2f72574d 100644 --- a/crates/roost-ipc/src/messages.rs +++ b/crates/roost-ipc/src/messages.rs @@ -1028,6 +1028,46 @@ pub struct SidebarDumpResult { pub projects: Vec, } +/// `app.render_stats` request — read the running UI's render-path +/// counters. `reset` zeroes them *after* the read, so a caller can +/// read-reset, run a workload, then read the delta directly. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct AppRenderStatsParams { + #[serde(default)] + pub reset: bool, +} + +/// `app.render_stats` response — running totals since process start +/// (or the last `reset: true`). +/// +/// Every counter is string-wrapped: the nanosecond accumulators pass +/// 2^53 after roughly 104 days of measured render time, and a JSON +/// number would silently lose precision in any JS client. The +/// remaining counters ride the same convention so the shape is +/// uniform rather than half-and-half. +/// +/// Permissive (no `deny_unknown_fields`) like every other result +/// struct, so a newer UI can add counters without breaking older +/// clients. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct AppRenderStatsResult { + #[serde(with = "string_int64")] + pub refresh_calls: i64, + #[serde(with = "string_int64")] + pub refresh_nanos: i64, + #[serde(with = "string_int64")] + pub rows_rebuilt: i64, + #[serde(with = "string_int64")] + pub cells_walked: i64, + #[serde(with = "string_int64")] + pub draw_calls: i64, + #[serde(with = "string_int64")] + pub draw_nanos: i64, + #[serde(with = "string_int64")] + pub fill_text_calls: i64, +} + /// `window.resize` request — programmatically set the window's logical /// size. Test-mode only (gated by `ROOST_TEST_MODE=1`); see the op /// const comment block below for the rationale. @@ -1204,6 +1244,15 @@ pub mod ops { /// is a wire-visible test failure rather than an invisible one /// (plan 007 §3.8). pub const SIDEBAR_DUMP: &str = "app.sidebar_dump"; + /// Read the running UI's render-path counters (refresh + draw call + /// counts, elapsed nanos, rows/cells walked, `fill_text` calls), + /// optionally zeroing them after the read. Ungated for the same + /// reason as `tab.dump_resolved`: reading counters mutates nothing + /// a user can see. The draw-side numbers only exist in a running + /// app — `TerminalWidget::draw` needs a live renderer, which unit + /// tests can't construct — so this op is the only way to measure + /// the real draw path. + pub const APP_RENDER_STATS: &str = "app.render_stats"; /// Command-palette overlay: open a root frame, read the current /// frame's rows, set the filter, activate a row (same dispatch as its @@ -1901,6 +1950,60 @@ mod tests { assert!(serde_json::from_str::(bad).is_err()); } + /// `reset` defaults to false so `{"op":"app.render_stats"}` with no + /// params at all is a plain read — the common case. Same shape as + /// `TabCapturePtyInputParams::drain`. + #[test] + fn app_render_stats_params_default_reset_is_false() { + let p: AppRenderStatsParams = serde_json::from_str("{}").unwrap(); + assert!(!p.reset); + assert!(!AppRenderStatsParams::default().reset); + round_trip(&AppRenderStatsParams { reset: true }); + let bad = r#"{"reset":true,"extra":"x"}"#; + assert!(serde_json::from_str::(bad).is_err()); + } + + /// Every counter is string-wrapped int64: nanosecond accumulators + /// exceed JS's 2^53 safe range, and a mixed number/string shape + /// would be the worst of both. Assert the *encoding*, not just the + /// round-trip, so dropping `string_int64` from a field fails here. + #[test] + fn app_render_stats_result_counters_are_string_wrapped() { + let r = AppRenderStatsResult { + refresh_calls: 12, + refresh_nanos: 9_007_199_254_740_993, + rows_rebuilt: 288, + cells_walked: 23_040, + draw_calls: 30, + draw_nanos: 4_500_000, + fill_text_calls: 720, + }; + let json = serde_json::to_string(&r).unwrap(); + assert!(json.contains(r#""refresh_calls":"12""#), "got: {json}"); + assert!( + json.contains(r#""refresh_nanos":"9007199254740993""#), + "got: {json}" + ); + assert!(json.contains(r#""rows_rebuilt":"288""#), "got: {json}"); + assert!(json.contains(r#""cells_walked":"23040""#), "got: {json}"); + assert!(json.contains(r#""draw_calls":"30""#), "got: {json}"); + assert!(json.contains(r#""draw_nanos":"4500000""#), "got: {json}"); + assert!(json.contains(r#""fill_text_calls":"720""#), "got: {json}"); + round_trip(&r); + } + + /// Result structs stay permissive so a newer UI can add a counter + /// without breaking older clients — the same contract + /// `TabDumpResolvedResult` documents. + #[test] + fn app_render_stats_result_accepts_extra_fields() { + let json = r#"{"refresh_calls":"1","refresh_nanos":"2","rows_rebuilt":"3", + "cells_walked":"4","draw_calls":"5","draw_nanos":"6", + "fill_text_calls":"7","gpu_nanos":"8"}"#; + let r: AppRenderStatsResult = serde_json::from_str(json).expect("permissive decode"); + assert_eq!(r.fill_text_calls, 7); + } + #[test] fn window_resize_params_reject_unknown_field() { round_trip(&WindowResizeParams { diff --git a/crates/roost-ipc/tests/vectors.rs b/crates/roost-ipc/tests/vectors.rs index 7a513d50..2fa0e751 100644 --- a/crates/roost-ipc/tests/vectors.rs +++ b/crates/roost-ipc/tests/vectors.rs @@ -210,6 +210,40 @@ fn sidebar_dump_vector_decodes_into_its_typed_params() { assert!(result.projects[1].agents.is_empty()); } +/// `app.render_stats` is the one op whose *every* result field is a +/// string-wrapped int64. Generic round-tripping would happily accept a +/// vector that wrote them as JSON numbers, which is exactly the drift +/// this convention exists to prevent — so decode it into the typed +/// struct. +#[test] +fn render_stats_vector_decodes_into_its_typed_params() { + use roost_ipc::messages::{ops, AppRenderStatsParams, AppRenderStatsResult, RawRequest}; + + let mut path = vectors_dir(); + path.push("app.render_stats.request.json"); + let raw = fs::read_to_string(&path).expect("read request vector"); + let req: RawRequest = serde_json::from_str(&raw).expect("decode envelope"); + assert_eq!(req.op, ops::APP_RENDER_STATS); + let params: AppRenderStatsParams = + serde_json::from_value(req.params).expect("decode render_stats params"); + assert!(!params.reset); + + let mut path = vectors_dir(); + path.push("app.render_stats.response.json"); + let raw = fs::read_to_string(&path).expect("read response vector"); + let resp: roost_ipc::messages::Response = + serde_json::from_str(&raw).expect("decode response envelope"); + let result: AppRenderStatsResult = + serde_json::from_value(resp.result.expect("result body")).expect("decode result"); + assert_eq!(result.refresh_calls, 412); + assert_eq!(result.refresh_nanos, 51_500_000); + assert_eq!(result.rows_rebuilt, 9_888); + assert_eq!(result.cells_walked, 790_400); + assert_eq!(result.draw_calls, 377); + assert_eq!(result.draw_nanos, 94_250_000); + assert_eq!(result.fill_text_calls, 9_048); +} + #[test] fn event_vectors_have_required_envelope_shape() { let dir = vectors_dir(); diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 80df539b..7359f5c5 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -26,8 +26,8 @@ use libadwaita::prelude::*; use libadwaita::{ApplicationWindow, TabView, WindowTitle}; use roost_ipc::agent::{self, AgentLifecycle, AgentTabState}; use roost_ipc::messages::{ - PaletteItemView, PaletteStateResult, Project, SidebarDumpAgentRow, SidebarDumpProject, - SidebarDumpResult, Tab, WindowMetricsResult, + AppRenderStatsResult, PaletteItemView, PaletteStateResult, Project, SidebarDumpAgentRow, + SidebarDumpProject, SidebarDumpResult, Tab, WindowMetricsResult, }; use tokio::runtime::Handle; @@ -1099,6 +1099,16 @@ impl App { UiRequest::WindowMetrics { reply } => { let _ = reply.send(app.ipc_window_metrics()); } + UiRequest::AppRenderStats { reset: _, reply } => { + // Deliberate placeholder: the GTK renderer has no + // perf instrumentation yet (roadmap slice E3b). + // Zeros keep the op's contract identical on both + // Rust UIs — refusing here would make this the + // first op one Rust UI answers and the other + // doesn't, and there is no `unsupported` error + // code to refuse with. + let _ = reply.send(Ok(AppRenderStatsResult::default())); + } UiRequest::SidebarDump { reply } => { let _ = reply.send(app.ipc_sidebar_dump()); } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index b4a585ec..7d78083b 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -196,6 +196,19 @@ roostctl screenshot > shot.png # raw PNG bytes to stdout `--scale` is `1` (default, logical window size) or `2`. With `--out` the CLI writes the file and prints the dimensions + byte count to stderr; without it, the raw PNG bytes go to stdout (nothing else is printed, so the stream stays binary-clean). Backed by the `app.screenshot` IPC op — see [ipc.md](ipc.md). +## `render-stats` + +Read the running UI's render-path counters — the only way to measure the real draw path, since `TerminalWidget::draw` needs a live renderer no unit test can construct. + +```bash +roostctl render-stats # running totals since process start +roostctl render-stats --reset # read, then zero for a clean next delta +``` + +Prints one counter per line plus two derived averages (`ns_per_refresh`, `ns_per_draw`, shown as `-` when the matching call count is zero). `--reset` zeroes the counters **after** the read, so the read-reset / run workload / read pattern gives you a delta directly. + +`refresh_*` covers the snapshot rebuild that walks libghostty's render state (with `rows_rebuilt` / `cells_walked` measuring what it touched); `draw_*` and `fill_text_calls` cover the widget draw pass. Note that `roostctl screenshot` re-renders the window and so inflates the draw counters — read before capturing, or reset after. The GTK UI reports all zeros (no instrumentation yet). Backed by the `app.render_stats` IPC op — see [ipc.md](ipc.md). + ## `palette` subcommands Drive the command-palette overlay: open it, inspect its rows, filter, activate a row, dismiss. Activating a row runs the **same** command its keybind would (a command row's id is its keybind action), so this is a command-dispatch surface, not just a UI poke. Each subcommand prints the resulting palette state (a `>` marks the highlighted row); `--json` emits the structured result. diff --git a/docs/reference/ipc.md b/docs/reference/ipc.md index 70b24cac..291c5e1d 100644 --- a/docs/reference/ipc.md +++ b/docs/reference/ipc.md @@ -604,6 +604,48 @@ during a drag are transient UI state, not part of this contract. Ungated, read-only — always available, matching `app.window_metrics`. +### `app.render_stats` + +Read the running UI's render-path counters. This is the only way to +measure the real draw path: `TerminalWidget::draw` needs a live +renderer, which unit tests cannot construct. + +Request: `{"params": {"reset": false}}`. `reset` defaults to `false`, +so `{"params": {}}` — or no `params` at all — is a plain read. + +Response: +```json +{"refresh_calls": "412", "refresh_nanos": "51500000", + "rows_rebuilt": "9888", "cells_walked": "790400", + "draw_calls": "377", "draw_nanos": "94250000", + "fill_text_calls": "9048"} +``` + +Every counter is a **string-wrapped int64** (the same +`string_int64` convention the envelope `id` uses, above): +the nanosecond accumulators pass 2^53 after roughly 104 days of +measured render time, and the rest ride the same convention so the +shape is uniform. All are running totals since process start, or since +the last `reset: true`. + +`refresh_calls` / `refresh_nanos` cover the snapshot rebuild that walks +libghostty's render state; `rows_rebuilt` and `cells_walked` are what +that walk touched. `draw_calls` / `draw_nanos` / `fill_text_calls` +cover the widget draw pass. + +`reset: true` zeroes the counters **after** the read, so a caller can +read-reset, run a workload, then read the delta directly. + +Caveat: `app.screenshot` re-renders the window, so taking a screenshot +inflates the three draw counters. Read before capturing, or reset +after. + +Ungated, read-only — always available, matching `tab.dump_resolved`. +The GTK UI answers with the same shape, all counters zero: its +renderer has no instrumentation yet, and a uniform contract across +both Rust UIs beats an op one of them refuses. CLI: `roostctl +render-stats [--reset]`. + ### `sidebar.set_width` *(test-only — gated)* **Requires `ROOST_TEST_MODE=1` set in the UI's launch environment.** diff --git a/tests/ipc-vectors/app.render_stats.request.json b/tests/ipc-vectors/app.render_stats.request.json new file mode 100644 index 00000000..17d61756 --- /dev/null +++ b/tests/ipc-vectors/app.render_stats.request.json @@ -0,0 +1,7 @@ +{ + "id": "21", + "op": "app.render_stats", + "params": { + "reset": false + } +} diff --git a/tests/ipc-vectors/app.render_stats.response.json b/tests/ipc-vectors/app.render_stats.response.json new file mode 100644 index 00000000..480b272b --- /dev/null +++ b/tests/ipc-vectors/app.render_stats.response.json @@ -0,0 +1,13 @@ +{ + "id": "21", + "ok": true, + "result": { + "refresh_calls": "412", + "refresh_nanos": "51500000", + "rows_rebuilt": "9888", + "cells_walked": "790400", + "draw_calls": "377", + "draw_nanos": "94250000", + "fill_text_calls": "9048" + } +} From f3e265795bdc1a14f1797067545ef8c9d9b97743 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 10:30:06 -0500 Subject: [PATCH 06/11] test(perf): add the render-path measurement harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- Makefile | 8 +- crates/roost-iced/src/app.rs | 6 + crates/roost-iced/src/app/perf_bench.rs | 154 ++++++++++++++++++++++++ tools/README.md | 8 +- tools/perf/README.md | 134 +++++++++++++++++++++ tools/perf/render-stats.sh | 43 +++++++ 6 files changed, 351 insertions(+), 2 deletions(-) create mode 100644 crates/roost-iced/src/app/perf_bench.rs create mode 100644 tools/perf/README.md create mode 100755 tools/perf/render-stats.sh diff --git a/Makefile b/Makefile index ed96605c..b5d03267 100644 --- a/Makefile +++ b/Makefile @@ -67,7 +67,7 @@ run-mac: bundle ## Launch the bundled Mac app # ---- test ------------------------------------------------------------- -.PHONY: test test-rust test-iced test-mac test-harness e2e e2e-gtk e2e-iced e2e-iced-clipboard e2e-mac e2e-gtk-ci e2e-iced-ci e2e-mac-ci smoke-gtk smoke-iced smoke-mac visual-parity smoke-mac-launch test-real-input test-iced-real-input test-iced-wayland-input check-iced +.PHONY: test test-rust test-iced test-mac test-harness e2e e2e-gtk e2e-iced e2e-iced-clipboard e2e-mac e2e-gtk-ci e2e-iced-ci e2e-mac-ci smoke-gtk smoke-iced smoke-mac visual-parity smoke-mac-launch test-real-input test-iced-real-input test-iced-wayland-input check-iced perf-refresh perf-render-stats ICED_E2E_TESTS := tools/roosttest/test_smoke.py tools/roosttest/test_iced_walking_skeleton.py tools/roosttest/test_notifications.py tools/roosttest/test_provider.py tools/roosttest/test_sidebar_pixels.py tools/roosttest/test_tab_strip_pixels.py tools/roosttest/test_focus.py tools/roosttest/test_palette.py tools/roosttest/test_z_typography.py tools/roosttest/test_project_lifecycle.py tools/roosttest/test_sidebar_resize.py ICED_CLIPBOARD_TESTS := tools/roosttest/test_selection.py tools/roosttest/test_osc52.py @@ -145,6 +145,12 @@ test-iced-real-input: build-iced ## Iced real clipboard input (self-contained L test-iced-wayland-input: build-iced ## Iced system clipboard with cage + a real uinput seat ROOST_REQUIRE_REAL_INPUT=1 uv run --group test python tools/input/linux/iced_wayland_clipboard_check.py +perf-refresh: ## In-crate refresh_snapshot perf harness (release, --ignored; see tools/perf/README.md) + cargo test -p roost-iced --release -- --ignored --nocapture + +perf-render-stats: ## Render-path counters for a running Iced UI (tools/perf/render-stats.sh iced ; args for other targets) + tools/perf/render-stats.sh iced + # ---- code quality ----------------------------------------------------- .PHONY: fmt fmt-check clippy themes-check check diff --git a/crates/roost-iced/src/app.rs b/crates/roost-iced/src/app.rs index 0fa1c6c4..459cd889 100644 --- a/crates/roost-iced/src/app.rs +++ b/crates/roost-iced/src/app.rs @@ -73,6 +73,12 @@ mod interactions; mod palettes; mod servicing; mod terminal_tab; +// The in-crate `#[ignore]`d perf harness — see `tools/perf/README.md` for +// how to run it. Gated on `cfg(test)` like `terminal_tab`'s test-only +// `attach_test_terminal` fixture it depends on; it carries no production +// code, so the whole module (not just an inner `mod tests`) is test-only. +#[cfg(test)] +mod perf_bench; pub(crate) use self::interactions::RenameTarget; use self::interactions::{ diff --git a/crates/roost-iced/src/app/perf_bench.rs b/crates/roost-iced/src/app/perf_bench.rs new file mode 100644 index 00000000..73b2dd9a --- /dev/null +++ b/crates/roost-iced/src/app/perf_bench.rs @@ -0,0 +1,154 @@ +//! In-crate performance harness for `TerminalTab::refresh_snapshot`. +//! +//! `roost-iced` has no `[lib]` target (`crates/roost-iced/Cargo.toml`), so +//! nothing outside the crate can import `TerminalTab` — an external +//! Criterion-style bench binary is not an option here. This lives +//! in-crate instead, `#[ignore]`d so it never runs under a normal `cargo +//! test -p roost-iced` or in CI. Invoke it explicitly: +//! +//! cargo test -p roost-iced --release -- --ignored --nocapture +//! +//! It reuses the `attach_test_terminal` fixture the rest of the app's +//! unit tests use and reads its numbers off each tab's own +//! `TabRenderStats`, not the process-global aggregate in `crate::perf` — +//! `cargo test` runs tests concurrently and a shared counter would be +//! polluted by whatever other test is refreshing its own tab at the same +//! moment (see `crate::perf`'s module doc for the same reasoning). +//! +//! This measures `refresh_snapshot` only. The `draw_*` / `fill_text_calls` +//! counters in `crate::perf::RenderStats` need a live iced `Renderer` that +//! only the windowing system hands `TerminalWidget::draw` — no unit test +//! constructs one, and there is no per-tab equivalent of them for the same +//! reason (see `TabRenderStats`'s doc comment). Read those with `roostctl +//! render-stats` against a running app instead — see `tools/perf/`. +use super::*; + +const ITERATIONS: usize = 200; + +struct WorkloadResult { + name: &'static str, + iterations: usize, + wall: Duration, + stats: crate::perf::TabRenderStats, +} + +impl WorkloadResult { + /// Stable, greppable `key value` lines — commit C7 diffs two runs of + /// this output (this commit vs. HEAD, via a `git worktree`) to build + /// the before/after table, so the format here must not drift. + fn print(&self) { + let ns_per_refresh = self + .stats + .refresh_nanos + .checked_div(self.stats.refresh_calls) + .map_or_else(|| "-".to_string(), |ns| ns.to_string()); + let rows_per_refresh = if self.stats.refresh_calls > 0 { + self.stats.rows_rebuilt as f64 / self.stats.refresh_calls as f64 + } else { + 0.0 + }; + println!("=== {} ===", self.name); + println!("iterations {}", self.iterations); + println!("wall_ns {}", self.wall.as_nanos()); + println!("refresh_calls {}", self.stats.refresh_calls); + println!("refresh_nanos {}", self.stats.refresh_nanos); + println!("ns_per_refresh {ns_per_refresh}"); + println!("rows_rebuilt {}", self.stats.rows_rebuilt); + println!("cells_walked {}", self.stats.cells_walked); + println!("rows_per_refresh {rows_per_refresh:.2}"); + println!(); + } +} + +/// Attach a fresh test terminal, run `iterations` steps of `mutate` + +/// `refresh_snapshot`, and return the per-tab counter delta. A fresh tab +/// per workload (rather than one shared tab) keeps each workload's +/// counters at exactly its own numbers with no baseline subtraction to +/// get wrong. +fn run_workload( + tab_id: i64, + name: &'static str, + iterations: usize, + mut mutate: impl FnMut(&mut TerminalTab, usize), +) -> WorkloadResult { + let (feed_tx, _feed_rx) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(tab_id, feed_tx); + assert_eq!( + tab.render_stats, + crate::perf::TabRenderStats::default(), + "a freshly attached tab has untouched counters" + ); + + let wall_started = Instant::now(); + for i in 0..iterations { + mutate(&mut tab, i); + tab.refresh_snapshot().expect("refresh_snapshot"); + } + let wall = wall_started.elapsed(); + + let stats = tab.render_stats; + supervisor.close(tab_id); + + WorkloadResult { + name, + iterations, + wall, + stats, + } +} + +/// Runs three workloads back to back and prints each one's per-tab +/// `refresh_snapshot` counters. No numbers are asserted on or persisted +/// here — this commit only builds the harness; C7 runs it against this +/// commit and against `HEAD` back to back via a `git worktree` (a +/// same-machine, same-moment A/B) and diffs the two printed tables. +/// +/// The three workloads are deliberately distinct shapes, not three sizes +/// of the same thing: +/// +/// - **W1 — pointer-motion storm.** No terminal mutation between +/// refreshes at all. This is what `interactions.rs`'s pointer-motion +/// handler produces: `refresh_or_warn` (wrapping `refresh_snapshot`) +/// runs on *every* mouse-motion event over a terminal +/// (`interactions.rs:1341`), even though nothing in the grid changed. +/// This is the headline case for dirty tracking — today it rebuilds +/// the entire grid for zero-content-change motion. +/// - **W2 — in-place TUI redraw.** Each iteration writes to a couple of +/// fixed rows via absolute cursor positioning (`\x1b[{row};1H...`), so +/// the viewport never scrolls. This is the vim/htop shape: a full +/// redraw of a bounded region, not a scroll. +/// - **W3 — scrolling stream (CONTROL).** Plain `line\r\n` output that +/// scrolls the viewport, same as a chatty build log. **This workload is +/// expected to show NO improvement, ever**: libghostty full-rebuilds +/// the render state whenever the viewport's scroll pin changes +/// (`third_party/ghostty/src/src/terminal/render.zig:299-302`). It is +/// in this harness precisely so a before/after table has a control +/// that stays flat — a future reader seeing W3 unchanged should read +/// that as the harness working correctly, not as something to "fix". +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "perf harness — run explicitly: cargo test -p roost-iced --release -- --ignored --nocapture"] +async fn refresh_snapshot_perf_harness() { + let w1 = run_workload(9_101, "W1 pointer-motion-storm", ITERATIONS, |_tab, _i| { + // Deliberately empty: the whole point of W1 is a refresh with no + // preceding terminal mutation. + }); + + let w2 = run_workload(9_102, "W2 in-place-tui-redraw", ITERATIONS, |tab, i| { + let row = 3 + (i % 2); // alternate two fixed rows; never scrolls + tab.write_vt(format!("\x1b[{row};1Hframe {i:04}").as_bytes()); + }); + + let w3 = run_workload( + 9_103, + "W3 scrolling-stream-CONTROL", + ITERATIONS, + |tab, i| { + tab.write_vt(format!("line-{i:04}\r\n").as_bytes()); + }, + ); + + println!(); + w1.print(); + w2.print(); + w3.print(); +} diff --git a/tools/README.md b/tools/README.md index 7adfd7ed..4d46c9ff 100644 --- a/tools/README.md +++ b/tools/README.md @@ -2,7 +2,9 @@ Three layers, by *what they can verify* and *how they drive the app*. Reach for the highest layer that can answer your question — it's faster, -more deterministic, and more portable. +more deterministic, and more portable. Layers 1-3 verify *correctness*; +[`perf/`](perf/README.md) measures *cost* instead and is not part of this +ladder — see its own README. ``` tools/ @@ -11,6 +13,10 @@ tools/ input/ Layer 3 — real OS input injection, platform-specific. linux/ uinput key/pointer + clipboard + single-monitor (COSMIC/Wayland). (mac/) CGEvent equivalent — planned. + perf/ Render-path cost — a sibling axis, not a layer (see intro above). + roosttest_unit/ (non-tier — fast unit tests for the harness wiring itself) + shed/ (non-tier — Apple VZ Linux microVM driver for Linux testing from a Mac) + wayland/ (non-tier — Wayland-specific test support) ``` | Layer | Dir | Drives via | Verifies | Platforms | CI | diff --git a/tools/perf/README.md b/tools/perf/README.md new file mode 100644 index 00000000..b45e14bc --- /dev/null +++ b/tools/perf/README.md @@ -0,0 +1,134 @@ +# Roost render performance harness (`tools/perf/`) + +Measures the **cost** of the Iced UI's render path — not correctness. +`tools/perf/` is a sibling of `tools/roosttest/`, `tools/screenshot/`, and +`tools/input/` (see [`../README.md`](../README.md)), not a fourth tier of +that ladder: those three verify *behavior*; this measures *how much CPU +work it took*. + +Two readouts, backed by the counters `crates/roost-iced/src/perf.rs` +instruments: + +| Readout | What it reads | Needs a running UI? | +|---|---|---| +| `cargo test -p roost-iced --release -- --ignored --nocapture` | Per-tab `TabRenderStats` (`refresh_calls`, `refresh_nanos`, `rows_rebuilt`, `cells_walked`) | No — an in-crate `#[ignore]`d test | +| `tools/perf/render-stats.sh ` (→ `roostctl render-stats`) | The process-global aggregate, same fields plus `draw_calls` / `draw_nanos` / `fill_text_calls` | Yes | + +## The in-crate test: `cargo test -p roost-iced --release -- --ignored --nocapture` + +`roost-iced` has no `[lib]` target +(`crates/roost-iced/Cargo.toml`), so nothing outside the crate can import +`TerminalTab` — an external bench binary isn't an option. The harness is +instead an `#[ignore]`d test, +`crates/roost-iced/src/app/perf_bench.rs::refresh_snapshot_perf_harness`, +reusing the same `attach_test_terminal` fixture the rest of the app's +unit tests use. It is genuinely `#[ignore]`d: a normal `cargo test -p +roost-iced` (what CI runs) never executes it. Always run it `--release` +— debug-profile timings aren't representative and aren't comparable +across runs. + +It runs three workloads, N=200 refreshes each, and prints one block of +`key value` lines per workload. **The format is stable and greppable +on purpose** — a before/after comparison (see below) diffs two runs of +this output. + +### The three workloads + +- **W1 — pointer-motion storm.** 200 refreshes with **no terminal + mutation** between them. This is the shape + `crates/roost-iced/src/app/interactions.rs:1341` produces: + `refresh_snapshot` runs on every single mouse-motion event over a + terminal, even though nothing in the grid changed. **This is the + headline case** — today it rebuilds the entire grid for zero-content + motion. +- **W2 — in-place TUI redraw.** 200 iterations, each writing to a couple + of fixed rows via absolute cursor positioning (`\x1b[{row};1H...`) so + the viewport never scrolls, then refreshing. The vim/htop shape. +- **W3 — scrolling stream (CONTROL).** 200 iterations of plain + `line\r\n` output that scrolls the viewport, then refreshing. + **This workload is expected to show NO improvement, ever.** libghostty + full-rebuilds its render state whenever the viewport's scroll pin + changes (`third_party/ghostty/src/src/terminal/render.zig:299-302`), + so no amount of dirty-tracking on our side changes what it hands back. + It's in the harness precisely so a before/after table has a control + that stays flat — **a flat W3 is a PASS**, not a disappointment. Do not + "fix" it. + +### What it can't measure + +This harness only exercises `refresh_snapshot` — the snapshot rebuild +that walks libghostty's render state. It cannot produce the `draw_*` / +`fill_text_calls` counters: `TerminalWidget::draw` needs a live iced +`Renderer`, which only the windowing system hands it, and no unit test +constructs one. For those, use `render-stats.sh` against a running app. + +### Before/after comparison + +A single run's numbers mean little on their own; what matters is the +delta between two runs of the *same machine, same moment* — CPU +frequency scaling, thermal throttling, and background load all move the +absolute numbers around run to run. Compare two runs taken back to back +(e.g. via a `git worktree` checking out two commits side by side, so +each build is fresh and neither run waits on the other's cache) rather +than trusting a number captured hours or days apart. + +```bash +cargo test -p roost-iced --release -- --ignored --nocapture > before.txt +# ...checkout the candidate change... +cargo test -p roost-iced --release -- --ignored --nocapture > after.txt +diff before.txt after.txt +``` + +Read `rows_per_refresh` first — it's the direct measure of "how much of +the grid did this refresh actually touch." W1 today reports the full 32 +rows/refresh; the point of dirty tracking is driving that toward 0 +without moving W3. + +## The running-app readout: `tools/perf/render-stats.sh` + +```bash +tools/perf/render-stats.sh iced # interactive: prompt before reading +tools/perf/render-stats.sh iced 10 # reset, sleep 10s, read +tools/perf/render-stats.sh mac +tools/perf/render-stats.sh gtk # GTK has no instrumentation yet — reports all zeros +``` + +Resets the counters, waits for you to exercise the running UI (either +interactively or for a fixed duration), then prints `roostctl +render-stats`'s delta since the reset. `--target` follows the rest of +`tools/`: `mac|gtk|iced` (see [`../screenshot/README.md`](../screenshot/README.md) +for the per-target launch/socket details this script reuses via +`../screenshot/lib.sh`). + +This is the *only* way to read `draw_calls` / `draw_nanos` / +`fill_text_calls` — they require a live iced `Renderer`, which only a +running UI has. `refresh_*` / `rows_rebuilt` / `cells_walked` are also +available here (same fields the in-crate test prints), aggregated across +every tab in the process rather than per-tab. + +## Two traps to know about before trusting a number + +- **The locked-Mac caveat.** External/presented frame rate is + meaningless on a locked or occluded Mac — macOS throttles + presentation regardless of how much CPU work the app is doing, so a + wall-clock or FPS-style measurement taken against a locked/backgrounded + window tells you about the OS's compositor policy, not about Roost. + This is exactly why this harness measures CPU-side spans + (`refresh_nanos`, `draw_nanos`) and deterministic counters + (`rows_rebuilt`, `cells_walked`, `draw_calls`) instead of frame rate. + There is no workaround for this — presented-frame timing on a + locked/occluded window is not a signal this harness can produce, so it + doesn't try. +- **The screenshot trap.** `iced::window::screenshot` re-renders the + window, so `roostctl screenshot` and everything in + [`../screenshot/`](../screenshot/README.md) inflate `draw_calls` / + `draw_nanos` / `fill_text_calls` just by having run. If a screenshot + happens inside your measurement window, either read the counters + *before* taking it, or `roostctl render-stats --reset` (or + `render-stats.sh`'s reset step) *after* it, before the window you + actually care about. + +See `crates/roost-iced/src/perf.rs`'s module doc for the underlying +counters, and [`docs/reference/cli.md`](../../docs/reference/cli.md) / +[`docs/reference/ipc.md`](../../docs/reference/ipc.md) for the `roostctl +render-stats` / `app.render_stats` wire contract. diff --git a/tools/perf/render-stats.sh b/tools/perf/render-stats.sh new file mode 100755 index 00000000..92b704ae --- /dev/null +++ b/tools/perf/render-stats.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# Read a running Iced UI's render-path counters (`roostctl render-stats`) +# around a measurement window, so the printed numbers are a clean delta +# instead of a running total since process start. +# +# tools/perf/render-stats.sh iced # interactive: prompt before reading +# tools/perf/render-stats.sh iced 10 # reset, sleep 10s, read +# +# Only the *running-app* half of the perf harness: `draw_calls` / +# `draw_nanos` / `fill_text_calls` need a live iced `Renderer`, which only +# a running UI has (see tools/perf/README.md). The `refresh_*` / +# `rows_rebuilt` / `cells_walked` counters this also prints are the same +# ones the in-crate `cargo test -p roost-iced --release -- --ignored +# --nocapture` harness measures without a UI — reach for that instead +# when you don't need the draw-path numbers. +# +# --target follows the rest of tools/: mac|gtk|iced (see ../screenshot/). +# The GTK UI has no render-path instrumentation yet, so it reports all +# zeros — that is expected, not a bug in this script. +set -euo pipefail +source "$(cd "$(dirname "${BASH_SOURCE[0]}")/../screenshot" && pwd)/lib.sh" + +TARGET="${1:?usage: render-stats.sh [duration_seconds]}" +DURATION="${2:-}" + +ut_init "${TARGET}" +ut_alive || { + echo "error: ${TARGET} UI is not running — launch it first (tools/screenshot/launch.sh ${TARGET})" >&2 + exit 1 +} + +echo "==> resetting ${TARGET} render-stats counters" >&2 +rc render-stats --reset >/dev/null + +if [[ -n "${DURATION}" ]]; then + echo "==> waiting ${DURATION}s — exercise the app now (scroll, type, resize...)" >&2 + sleep "${DURATION}" +else + read -rp "Exercise the app now, then press Enter to read the counters... " _unused +fi + +echo "==> ${TARGET} render-stats since reset:" >&2 +rc render-stats From 6afc38d9464c43aae432f6624bdc5084aac83da0 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 10:47:43 -0500 Subject: [PATCH 07/11] perf(iced): rebuild only dirty rows in the terminal snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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>`, 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-iced/src/app.rs | 8 +- crates/roost-iced/src/app/servicing.rs | 173 +++++++++++++++++++++- crates/roost-iced/src/app/terminal_tab.rs | 143 ++++++++++++------ crates/roost-iced/src/terminal_widget.rs | 160 +++++++++++++++----- 4 files changed, 390 insertions(+), 94 deletions(-) diff --git a/crates/roost-iced/src/app.rs b/crates/roost-iced/src/app.rs index 459cd889..66b14b2c 100644 --- a/crates/roost-iced/src/app.rs +++ b/crates/roost-iced/src/app.rs @@ -47,9 +47,9 @@ use roost_ui_model::{ }; use roost_url::HoverUrl; use roost_vt::{ - key_action, mouse_action, mouse_button, KeyEncoder, KeyEvent, MouseEncoder, MouseEvent, - PageDirection, PageRoute, RenderState, ScrollDirection, ScrollRoute, Terminal, TerminalOptions, - TerminalScroll, TerminalSelection, + key_action, mouse_action, mouse_button, ColorRgb, KeyEncoder, KeyEvent, MouseEncoder, + MouseEvent, PageDirection, PageRoute, RenderState, ScrollDirection, ScrollRoute, Terminal, + TerminalOptions, TerminalScroll, TerminalSelection, }; use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; @@ -60,7 +60,7 @@ use crate::palette_scroll::Visibility; use crate::sidebar_resize::SidebarResizeGrip; use crate::strip_reorder::{ReorderStrip, StripEvent}; use crate::terminal_widget::{ - resolve_colors, DrawCell, TerminalMetrics, TerminalPointerEvent, TerminalSnapshot, + DrawCell, RenderedRow, TerminalMetrics, TerminalPointerEvent, TerminalSnapshot, TerminalWheelEvent, TerminalWidget, TERMINAL_PADDING, }; use crate::Message; diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index 1f2c70c0..65d5a7d0 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -1319,7 +1319,7 @@ mod tests { assert!(collected.error.is_none()); let tab = tabs.get_mut(&70).expect("the tab is still attached"); tab.refresh_snapshot().expect("refresh the touched tab"); - assert_eq!(tab.snapshot.rows_text[0], "hello"); + assert_eq!(tab.snapshot.grid[0].text, "hello"); supervisor.close(70); } @@ -1542,7 +1542,7 @@ mod tests { assert_eq!( tab.render_stats.rows_rebuilt, u64::from(DEFAULT_ROWS), - "the current refresh always rebuilds the whole grid" + "the first refresh has no cached grid, so it rebuilds every row" ); assert_eq!( tab.render_stats.cells_walked, @@ -1558,7 +1558,176 @@ mod tests { tab.render_stats.refresh_calls, 2, "counters accumulate across calls rather than resetting" ); + assert_eq!( + tab.render_stats.rows_rebuilt, + u64::from(DEFAULT_ROWS), + "nothing touched the terminal, so the second refresh rebuilds \ + zero rows and the total does not move" + ); + assert_eq!( + tab.render_stats.cells_walked, + u64::from(DEFAULT_COLS) * u64::from(DEFAULT_ROWS), + "and walks no cells either" + ); supervisor.close(75); } + + /// The failure a per-row cache can silently produce is "right cells, + /// wrong row" — content landing one row off, or a stale row surviving + /// a rebuild. A substring search over the joined dump would not catch + /// either, so this writes a distinct marker to one row at a time and + /// checks the WHOLE row vector element-for-element after every write. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn incremental_rebuild_keeps_every_row_at_its_own_index() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(76, feed_tx); + // Absolute positioning only: a scroll would move every row and + // turn this into a full-rebuild test by accident. + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("refresh the cleared grid"); + + let rows = usize::from(DEFAULT_ROWS); + let mut expected = vec![String::new(); rows]; + // Out of order on purpose, and not every row: a cache that keys on + // walk order rather than the reported row index passes an + // in-order fill. + for (step, row) in [4usize, 1, rows - 1, 0, 9, 4].into_iter().enumerate() { + let marker = format!("marker-{step}-row-{row}"); + tab.write_vt(format!("\x1b[{};1H{marker}", row + 1).as_bytes()); + tab.refresh_snapshot().expect("refresh after the write"); + expected[row] = marker; + assert_eq!( + tab.dump().rows_text, + expected, + "after step {step} (row {row}) every row must hold exactly its own content" + ); + } + + supervisor.close(76); + } + + /// `TerminalSnapshot::blank` fills its rows with an empty string while + /// `refresh_snapshot` builds `" "`-filled rows and trims them. Both + /// must land on `""`, because `tab.dump` — and the whole e2e suite + /// through it — reads one before the first refresh and the other + /// after. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_blank_snapshot_and_a_refreshed_empty_grid_dump_the_same_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(77, feed_tx); + let blank = tab.dump().rows_text; + assert_eq!(blank, vec![String::new(); usize::from(DEFAULT_ROWS)]); + + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("refresh the cleared grid"); + assert_eq!( + tab.dump().rows_text, + blank, + "a refreshed empty grid trims down to the same rows blank starts at" + ); + + supervisor.close(77); + } + + /// `OSC 11` changes the terminal's default background with libghostty + /// reporting nothing dirty, so only `refresh_snapshot`'s cached-default + /// guard keeps cached rows from freezing at the old color. Without it + /// the untouched row below would keep rendering the old background. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn changing_the_default_background_rebuilds_cached_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(78, feed_tx); + tab.write_vt(b"\x1b[2J\x1bH\x1b[1;1Hcolored"); + tab.refresh_snapshot() + .expect("refresh with the row written"); + let before = tab.snapshot.background; + + let rebuilt_before = tab.render_stats.rows_rebuilt; + tab.write_vt(b"\x1b]11;rgb:00/00/ff\x07"); + tab.refresh_snapshot().expect("refresh after OSC 11"); + + assert_ne!( + tab.snapshot.background, before, + "OSC 11 must reach the render state's default background" + ); + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a default-color change invalidates every cached row" + ); + let resolved = tab.resolved_cells(); + let cell = resolved + .cells + .iter() + .find(|cell| cell.row == 0 && cell.col == 0) + .expect("row 0 col 0 is in the resolved grid"); + assert_eq!( + cell.bg, + ( + tab.snapshot.background.r, + tab.snapshot.background.g, + tab.snapshot.background.b + ), + "the rebuilt row resolves against the new default, not the cached one" + ); + + supervisor.close(78); + } + + /// `tab.dump_resolved` densifies the sparse per-row cells back into a + /// full grid. It is the one consumer that has to re-derive a cell's row + /// from its grid position now that `DrawCell` no longer carries one, so + /// it gets its own coverage: dense, row-major, and each cell resolved + /// against the row it actually came from. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn resolved_cells_densifies_the_grid_row_major_from_the_row_index() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(79, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[H"); + // Row 3 (1-based) col 2 (1-based) — off both axes' origin, so a + // transposed or off-by-one index cannot coincide with the truth. + tab.write_vt(b"\x1b[3;2H\x1b[1;41mX\x1b[0m"); + tab.refresh_snapshot().expect("refresh"); + + let resolved = tab.resolved_cells(); + assert_eq!(resolved.cols, DEFAULT_COLS); + assert_eq!(resolved.rows, DEFAULT_ROWS); + assert_eq!( + resolved.cells.len(), + usize::from(DEFAULT_COLS) * usize::from(DEFAULT_ROWS), + "the resolved grid is dense" + ); + for (index, cell) in resolved.cells.iter().enumerate() { + assert_eq!(cell.row, (index / usize::from(DEFAULT_COLS)) as u32); + assert_eq!(cell.col, (index % usize::from(DEFAULT_COLS)) as u16); + } + + let marked = &resolved.cells[2 * usize::from(DEFAULT_COLS) + 1]; + assert_eq!(marked.text, "X"); + assert!(marked.bold); + assert!(marked.has_explicit_bg); + let red = tab.theme.palette[1]; + assert_eq!( + marked.bg, + (red.r, red.g, red.b), + "SGR 41 resolves through the theme palette's red" + ); + + let neighbor = &resolved.cells[2 * usize::from(DEFAULT_COLS)]; + assert_eq!(neighbor.text, " "); + assert!(!neighbor.has_explicit_bg); + assert!(!neighbor.bold); + assert_eq!( + neighbor.bg, + ( + tab.snapshot.background.r, + tab.snapshot.background.g, + tab.snapshot.background.b + ), + "an untouched cell falls back to the terminal default" + ); + + supervisor.close(79); + } } diff --git a/crates/roost-iced/src/app/terminal_tab.rs b/crates/roost-iced/src/app/terminal_tab.rs index 7a278e29..48cfe9c8 100644 --- a/crates/roost-iced/src/app/terminal_tab.rs +++ b/crates/roost-iced/src/app/terminal_tab.rs @@ -202,6 +202,20 @@ pub(super) struct TerminalTab { pub(super) pointer_shape: String, pub(super) theme: Theme, pub(super) snapshot: TerminalSnapshot, + /// The per-row render cache `refresh_snapshot` maintains; the snapshot + /// gets a clone of it (O(rows) refcount bumps). The three `cached_*` + /// fields are the keys this cache is valid under — see + /// `refresh_snapshot`'s caching invariant. + grid: Vec>, + cached_grid_size: Option<(u16, u16)>, + cached_defaults: Option<(ColorRgb, ColorRgb)>, + cached_theme_generation: Option, + /// Bumped whenever a theme lands on this tab. Nothing theme-derived + /// besides the default fg/bg pair enters `RenderedRow::build` today, + /// but GTK's twin resolver already pulls the theme's `bold_color`, so + /// this fails the cache safe toward over-rebuilding if that override + /// ever lands here. + theme_generation: u64, cols: u16, rows: u16, pub(super) applied_metrics: Option, @@ -283,6 +297,13 @@ impl TerminalTab { pointer_shape: "default".into(), theme, snapshot: TerminalSnapshot::blank(DEFAULT_COLS, DEFAULT_ROWS), + // Left empty on purpose: the first `refresh_snapshot` finds no + // cached grid size, sizes the grid and forces a full rebuild. + grid: Vec::new(), + cached_grid_size: None, + cached_defaults: None, + cached_theme_generation: None, + theme_generation: 0, cols: DEFAULT_COLS, rows: DEFAULT_ROWS, applied_metrics: None, @@ -871,6 +892,9 @@ impl TerminalTab { fn apply_theme_candidate(&mut self, theme: &Theme) -> Result<()> { self.theme = theme.clone(); + // Every theme application — including `set_theme`'s rollback — + // lands here, so this is the one place the generation must move. + self.theme_generation = self.theme_generation.wrapping_add(1); self.terminal.set_color_foreground(theme.foreground)?; self.terminal.set_color_background(theme.background)?; self.terminal.set_color_cursor(theme.cursor)?; @@ -878,63 +902,79 @@ impl TerminalTab { self.refresh_snapshot() } + /// Rebuild the snapshot from the terminal, reusing every cached row + /// libghostty reports as unchanged. + /// + /// **Caching invariant.** A cached `RenderedRow` is valid exactly + /// while (a) libghostty reports its row undirty and (b) the inputs + /// `RenderedRow::build` reads besides the row's own vt cells — the + /// default fg/bg pair and the grid width — are unchanged. Everything + /// that alters what a row should render must therefore either mark + /// that row dirty inside libghostty or move one of the cache keys + /// guarded below. Anyone adding a third input to `RenderedRow::build` + /// must add a guard for it here. + /// + /// The default-color guard is not belt-and-braces: `OSC 10`/`OSC 11` + /// and DECSCNM (`CSI ?5h`) change the terminal's default fg/bg with + /// libghostty reporting `Clean` and no row flagged (measured; pinned + /// by `crates/roost-vt/tests/render_dirty_test.rs`). Since + /// `resolve_colors` folds those defaults into every cell that does not + /// set its own, a cached row would otherwise freeze at the old color. pub(super) fn refresh_snapshot(&mut self) -> Result<()> { let refresh_started_at = Instant::now(); self.recompute_hover()?; self.render_state.update(&self.terminal)?; let colors = self.render_state.colors()?; let cursor = self.render_state.cursor(); - let mut cells = Vec::new(); - let mut rows = vec![vec![String::new(); usize::from(self.cols)]; usize::from(self.rows)]; + + // Each guard raises the dirty state BEFORE recording its new key, + // so a failed `mark_full` leaves the key stale and the next + // refresh retries the invalidation rather than skipping it. + let size = (self.cols, self.rows); + if self.cached_grid_size != Some(size) { + // Both axes: a width-only resize leaves the row count alone + // while invalidating every cached row's column content. + self.render_state.mark_full()?; + // Every slot shares one empty row — rows are replaced + // wholesale by the walk below, never mutated in place. + let blank_row = Arc::new(RenderedRow::default()); + self.grid = vec![blank_row; usize::from(self.rows)]; + self.cached_grid_size = Some(size); + } + let defaults = (colors.foreground, colors.background); + if self.cached_defaults != Some(defaults) { + self.render_state.mark_full()?; + self.cached_defaults = Some(defaults); + } + if self.cached_theme_generation != Some(self.theme_generation) { + self.render_state.mark_full()?; + self.cached_theme_generation = Some(self.theme_generation); + } + + let cols = self.cols; + let grid = &mut self.grid; + let mut rows_rebuilt: u64 = 0; let mut cells_walked: u64 = 0; - self.render_state.walk(&self.terminal, |row, cell| { - cells_walked += 1; - if row >= u32::from(self.rows) || cell.col >= self.cols { + self.render_state.walk_dirty(&self.terminal, |row, cells| { + cells_walked += cells.len() as u64; + // Clamped against the cache's own length, not `self.rows`: + // the guard above keeps the two equal, and reading the length + // here means a row index past the end can never index out of + // bounds even if that ever stopped holding. + if row as usize >= grid.len() { return; } - let text = if cell.text.is_empty() { - " ".to_string() - } else { - cell.text - }; - rows[row as usize][usize::from(cell.col)] = text.clone(); - let (foreground, background) = resolve_colors( - cell.fg, - cell.bg, - (colors.foreground, colors.background), - cell.style.inverse, - ); - if text != " " || cell.bg.is_some() || cell.style.inverse { - cells.push(DrawCell { - row, - col: cell.col, - text, - foreground, - background, - explicit_background: cell.bg.is_some() || cell.style.inverse, - bold: cell.style.bold, - italic: cell.style.italic, - inverse: cell.style.inverse, - }); - } + grid[row as usize] = Arc::new(RenderedRow::build(cells, defaults, cols)); + rows_rebuilt += 1; })?; - // The full-grid rebuild below always touches every row of `rows`, - // so its length is the truthful "rows rebuilt" count today. Once - // the rebuild goes incremental (C5), `rows` will hold only the - // rebuilt rows and this stays correct without changing. - let rows_rebuilt = rows.len() as u64; - let rows_text = rows - .into_iter() - .map(|row| row.concat().trim_end().to_string()) - .collect(); + self.snapshot = TerminalSnapshot { cols: self.cols, rows: self.rows, foreground: colors.foreground, background: colors.background, cursor, - cells, - rows_text, + grid: self.grid.clone(), selection_background: self.theme.selection_background, selection_spans: self .selection @@ -965,21 +1005,26 @@ impl TerminalTab { .cursor .filter(|cursor| cursor.visible) .map(|cursor| (cursor.row, cursor.col, cursor.visible)), - rows_text: self.snapshot.rows_text.clone(), + rows_text: self + .snapshot + .grid + .iter() + .map(|row| row.text.clone()) + .collect(), } } pub(super) fn resolved_cells(&self) -> ResolvedCellsData { - let mut by_position: HashMap<(u32, u16), &DrawCell> = self - .snapshot - .cells - .iter() - .map(|cell| ((cell.row, cell.col), cell)) - .collect(); let mut cells = Vec::with_capacity(usize::from(self.cols) * usize::from(self.rows)); for row in 0..u32::from(self.rows) { + let mut by_col: HashMap = self + .snapshot + .grid + .get(row as usize) + .map(|rendered| rendered.cells.iter().map(|cell| (cell.col, cell)).collect()) + .unwrap_or_default(); for col in 0..self.cols { - let cell = by_position.remove(&(row, col)); + let cell = by_col.remove(&col); let foreground = cell.map_or(self.snapshot.foreground, |cell| cell.foreground); let background = cell.map_or(self.snapshot.background, |cell| cell.background); cells.push(ResolvedCellData { diff --git a/crates/roost-iced/src/terminal_widget.rs b/crates/roost-iced/src/terminal_widget.rs index 2071a6cb..f198bb73 100644 --- a/crates/roost-iced/src/terminal_widget.rs +++ b/crates/roost-iced/src/terminal_widget.rs @@ -11,6 +11,7 @@ use iced::{ }; use roost_engine::pointer::{PointerAction, PointerButton}; use roost_vt::{ColorRgb, CursorInfo, CursorVisualStyle, SelectionSpan}; +use std::sync::Arc; use std::time::{Duration, Instant}; use unicode_width::UnicodeWidthStr; @@ -124,9 +125,12 @@ fn draw_font(base: Font, text: &str, bold: bool, italic: bool) -> Font { } } +/// One resolved cell. Deliberately carries no row index: its row is the +/// index of the [`RenderedRow`] that owns it. Storing the row here as well +/// would let the two disagree, which is exactly the "right cells, wrong +/// row" failure the per-row cache could otherwise hide. #[derive(Debug, Clone)] pub struct DrawCell { - pub row: u32, pub col: u16, pub text: String, pub foreground: ColorRgb, @@ -137,6 +141,74 @@ pub struct DrawCell { pub inverse: bool, } +/// One viewport row's render output, shared behind an `Arc` so cloning a +/// snapshot (which `App::view` does every frame) is O(rows) refcount bumps +/// rather than O(cells) `String` clones, and so a row libghostty reports +/// undirty survives a refresh without being rebuilt. +/// +/// **Overlay invariant — a `RenderedRow` holds terminal content ONLY.** +/// Selection tint, link-hover underline and the cursor are snapshot-level +/// fields drawn in separate passes after the cell loop in +/// [`TerminalWidget::draw`], and must NEVER be baked into a [`DrawCell`]'s +/// colors. A row is cached across refreshes; folding selection into a +/// cell's background would freeze the tint in the cache, which surfaces as +/// "the selection sometimes doesn't clear". +#[derive(Debug, Default)] +pub struct RenderedRow { + /// Sparse: only the cells that draw something, ascending by column. + pub cells: Vec, + /// The row's text, joined and `trim_end`ed — what `tab.dump` returns. + pub text: String, +} + +impl RenderedRow { + /// Resolve one viewport row from libghostty's cells for that row. + /// + /// Everything this reads is a parameter — the row's vt cells, the + /// terminal's default fg/bg pair, and the grid width. That list IS the + /// cache key `TerminalTab::refresh_snapshot` guards on; adding a + /// fourth input here means extending those guards (see that function's + /// caching invariant). + pub fn build(cells: &[roost_vt::Cell], defaults: (ColorRgb, ColorRgb), cols: u16) -> Self { + let mut row = RenderedRow { + cells: Vec::new(), + text: String::with_capacity(usize::from(cols)), + }; + for cell in cells { + // libghostty yields a row's cells in ascending, gapless column + // order, so appending is the same string the old + // index-into-a-dense-`Vec`-then-`concat` build produced + // — including for a short row, whose missing tail contributed + // empty strings there and contributes nothing here. + if cell.col >= cols { + continue; + } + let text = if cell.text.is_empty() { + " " + } else { + cell.text.as_str() + }; + row.text.push_str(text); + let (foreground, background) = + resolve_colors(cell.fg, cell.bg, defaults, cell.style.inverse); + if text != " " || cell.bg.is_some() || cell.style.inverse { + row.cells.push(DrawCell { + col: cell.col, + text: text.to_string(), + foreground, + background, + explicit_background: cell.bg.is_some() || cell.style.inverse, + bold: cell.style.bold, + italic: cell.style.italic, + inverse: cell.style.inverse, + }); + } + } + row.text.truncate(row.text.trim_end().len()); + row + } +} + #[derive(Debug, Clone)] pub struct TerminalSnapshot { pub cols: u16, @@ -144,8 +216,8 @@ pub struct TerminalSnapshot { pub foreground: ColorRgb, pub background: ColorRgb, pub cursor: Option, - pub cells: Vec, - pub rows_text: Vec, + /// Indexed by viewport row; `grid.len() == rows`. + pub grid: Vec>, pub selection_background: ColorRgb, pub selection_spans: Vec, pub link_hover: Option, @@ -154,6 +226,11 @@ pub struct TerminalSnapshot { impl TerminalSnapshot { pub fn blank(cols: u16, rows: u16) -> Self { + // Every row shares one empty `RenderedRow`: rows are replaced + // wholesale, never mutated in place. `RenderedRow::text` is `""` + // here, which is what a `refresh_snapshot`-built blank row also + // trims down to — `tab.dump` depends on the two agreeing. + let blank_row = Arc::new(RenderedRow::default()); Self { cols, rows, @@ -168,8 +245,7 @@ impl TerminalSnapshot { b: 24, }, cursor: None, - cells: Vec::new(), - rows_text: vec![String::new(); usize::from(rows)], + grid: vec![blank_row; usize::from(rows)], selection_background: ColorRgb { r: 72, g: 83, @@ -581,40 +657,46 @@ impl Widget for TerminalWidget { fill_quad(renderer, bounds, color(self.snapshot.background)); let metrics = self.metrics; - for cell in &self.snapshot.cells { - let position = cell_position(bounds.position(), cell.col, cell.row, metrics); - if cell.explicit_background { - fill_quad( - renderer, - // Preserve the Canvas path's overdraw policy. Adjacent - // antialiased quads can otherwise expose hairlines of - // the default background under tiny-skia. - Rectangle::new( - position, - Size::new(metrics.cell_width, metrics.cell_height), - ), - color(cell.background), - ); - } - if !cell.text.is_empty() && cell.text != " " { - let font = draw_font(metrics.font, cell.text.as_str(), cell.bold, cell.italic); - renderer.fill_text( - text::Text { - content: cell.text.clone(), - bounds: Size::new(f32::INFINITY, metrics.cell_height), - size: Pixels(metrics.font_pixels), - line_height: text::LineHeight::Relative(TERMINAL_LINE_HEIGHT), - font, - align_x: text::Alignment::Default, - align_y: iced::alignment::Vertical::Top, - shaping: text::Shaping::Auto, - wrapping: text::Wrapping::None, - }, - Point::new(position.x, position.y + 1.0), - color(cell.foreground), - clip, - ); - fill_text_calls += 1; + for (row_idx, row) in self.snapshot.grid.iter().enumerate() { + // The row index comes from the grid position, never from + // the cell — see `DrawCell`. + let row_y = row_idx as u32; + for cell in &row.cells { + let position = cell_position(bounds.position(), cell.col, row_y, metrics); + if cell.explicit_background { + fill_quad( + renderer, + // Preserve the Canvas path's overdraw policy. Adjacent + // antialiased quads can otherwise expose hairlines of + // the default background under tiny-skia. + Rectangle::new( + position, + Size::new(metrics.cell_width, metrics.cell_height), + ), + color(cell.background), + ); + } + if !cell.text.is_empty() && cell.text != " " { + let font = + draw_font(metrics.font, cell.text.as_str(), cell.bold, cell.italic); + renderer.fill_text( + text::Text { + content: cell.text.clone(), + bounds: Size::new(f32::INFINITY, metrics.cell_height), + size: Pixels(metrics.font_pixels), + line_height: text::LineHeight::Relative(TERMINAL_LINE_HEIGHT), + font, + align_x: text::Alignment::Default, + align_y: iced::alignment::Vertical::Top, + shaping: text::Shaping::Auto, + wrapping: text::Wrapping::None, + }, + Point::new(position.x, position.y + 1.0), + color(cell.foreground), + clip, + ); + fill_text_calls += 1; + } } } From 5a9849a7107a8bfa7c4a999b319342114b9041f0 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 11:04:12 -0500 Subject: [PATCH 08/11] test(iced): pin the dirty-row rebuild behavior against regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-iced/src/app/servicing.rs | 223 +++++++++++++++++++++++++ 1 file changed, 223 insertions(+) diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index 65d5a7d0..164013eb 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -1730,4 +1730,227 @@ mod tests { supervisor.close(79); } + + /// A single-row write with the cursor already parked on that row must + /// rebuild exactly one row — the headline claim `refresh_snapshot`'s + /// per-row cache makes. The cursor is parked and settled *before* the + /// write under test because libghostty dirties both the row the cursor + /// leaves and the row it lands on (pinned by + /// `crates/roost-vt/tests/render_dirty_test.rs`'s + /// `row_flags_are_cleared_alongside_the_global_layer`); moving and + /// writing in the same step would fold that cursor-motion row into the + /// count this test is trying to isolate. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_single_row_write_with_the_cursor_already_parked_rebuilds_exactly_that_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(80, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("settle the cleared grid"); + + tab.write_vt(b"\x1b[3;1H"); + tab.refresh_snapshot() + .expect("settle with the cursor parked on row 2"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let cells_before = tab.render_stats.cells_walked; + tab.write_vt(b"X"); + tab.refresh_snapshot() + .expect("refresh after the single-row write"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + 1, + "the cursor was already on row 2, so writing to it dirties only that row" + ); + assert_eq!( + tab.render_stats.cells_walked - cells_before, + u64::from(DEFAULT_COLS), + "walk_dirty hands the whole row's cells to the one rebuilt row" + ); + + supervisor.close(80); + } + + /// `set_theme` bumps `theme_generation`, and `refresh_snapshot`'s + /// `cached_theme_generation` guard exists precisely to force a full + /// rebuild off that bump — today nothing but the default fg/bg pair + /// (already covered by the default-color guard) is theme-derived, but + /// the guard is there so a future theme-derived input (e.g. GTK's + /// `bold_color` override) fails safe toward over-rebuilding rather than + /// silently keeping stale rows. + /// + /// Measured while writing this test: `apply_theme_candidate`'s color + /// FFI calls (`set_color_foreground`/`background`/`cursor`/`palette`) + /// already report `Dirty::Full` on their own at our pinned Ghostty SHA + /// — pinned separately by `theme_color_changes_report_full` in + /// `crates/roost-vt/tests/render_dirty_test.rs` — so with a real theme + /// apply neither `cached_defaults` nor `cached_theme_generation` is + /// individually load-bearing for this test (confirmed: disabling both + /// at once still left it passing). What *does* make it fail is the + /// same class of bug as the resize guard above — the FFI calls + /// silently not reaching libghostty while `theme_generation` still + /// bumps: stubbing those calls out with the generation guard in place + /// still passed (`DEFAULT_ROWS`), and disabling the guard on top of + /// that stub dropped the rebuild to 0. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn applying_a_theme_rebuilds_every_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(81, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let dracula = Theme::load_bundled("Dracula"); + tab.set_theme(&dracula).expect("theme applies"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "set_theme's own refresh must rebuild every row, not just the changed ones" + ); + + supervisor.close(81); + } + + /// Pins the `(cols, rows)` cache-size guard against a narrower + /// row-count-only guard: a width-only resize leaves `self.rows` + /// unchanged, so a guard keyed on row count alone would miss it and + /// every cached row would keep rendering at the old column width. + /// + /// Measured while writing this test: at our pinned Ghostty SHA, + /// `Terminal::resize` itself always reports `Dirty::Full` regardless of + /// which axis moved (pinned separately by + /// `resize_reports_full_over_the_new_row_count` in + /// `crates/roost-vt/tests/render_dirty_test.rs`), so on the real + /// `apply_geometry` path this guard's own `mark_full` is currently a + /// redundant second line of defense, not the sole reason this test + /// passes. It stops being redundant, and this test starts actually + /// depending on it, the moment `apply_geometry`'s call into libghostty + /// silently no-ops while `self.cols`/`self.rows` still move — verified + /// by temporarily stubbing that call out during review: with the + /// `(cols, rows)` guard intact the rebuild count held at + /// `DEFAULT_ROWS`, and narrowing the guard to rows-only on top of that + /// stub dropped it to 0. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_width_only_resize_rebuilds_every_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(82, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let metrics = tab.applied_metrics.expect("installed metrics"); + let rebuilt_before = tab.render_stats.rows_rebuilt; + let change = tab + .apply_geometry( + DEFAULT_COLS + 10, + DEFAULT_ROWS, + metrics, + tab.metric_generation + 1, + ) + .expect("apply a width-only geometry change") + .expect("cols moved, so this is a real geometry change"); + assert!( + change.grid_changed, + "cols moved, so the grid-changed flag must fire even though rows did not" + ); + tab.commit_geometry(change); + tab.refresh_snapshot() + .expect("refresh after the width-only resize"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a width-only resize invalidates every cached row even though the row count is unchanged" + ); + + supervisor.close(82); + } + + /// Pins that a scrolled-back viewport is never served from the stale + /// row cache. None of `refresh_snapshot`'s three cache-key guards fire + /// here — grid size, defaults, and theme generation are all unchanged + /// by a page up — so this pins libghostty's own dirty reporting for a + /// viewport move (it reports every row dirty) rather than one of this + /// module's guards. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn scrolling_back_into_history_rebuilds_every_row_and_changes_the_text() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(83, feed_tx); + for line in 0..(usize::from(DEFAULT_ROWS) * 3) { + tab.write_vt(format!("history-{line:04}\r\n").as_bytes()); + } + tab.refresh_snapshot().expect("settle at the live bottom"); + let before_text = tab.dump().rows_text; + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let route = tab + .handle_page(PageDirection::Up) + .expect("page up into history"); + assert!( + matches!( + route, + PageRoute::LocalViewport { + scrolled_back: true + } + ), + "enough history exists that page up must move the local viewport: {route:?}" + ); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a viewport move rebuilds every row rather than reusing the live-bottom cache" + ); + assert_ne!( + tab.dump().rows_text, + before_text, + "the scrolled-back viewport must show different rows than the live bottom" + ); + + supervisor.close(83); + } + + /// The headline win of the dirty-tracking change: a hover-only motion + /// event — no button, no terminal mouse tracking — never writes to the + /// terminal, so `refresh_snapshot` must rebuild nothing even though it + /// still republishes the snapshot (pointer shape / hover overlay can + /// change independently of content). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_pointer_motion_refresh_with_no_terminal_change_rebuilds_zero_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(84, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let cells_before = tab.render_stats.cells_walked; + // What `App::pointer` does for a hover-only motion: dispatch through + // `handle_native_pointer`, then refresh. + tab.handle_native_pointer(NativePointerDispatch { + action: PointerAction::Motion, + button: None, + col: 2, + row: 0, + mods: 0, + click_count: 0, + inside: true, + link_modifier_held: false, + }) + .expect("hover motion dispatch"); + tab.refresh_snapshot() + .expect("refresh after the motion event"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + 0, + "a motion event with no mouse tracking touches only overlay state, not content" + ); + assert_eq!( + tab.render_stats.cells_walked - cells_before, + 0, + "and walks no cells either" + ); + + supervisor.close(84); + } } From de26505fafee5d7ec678dfe8bb4c319065f4606b Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 11:11:07 -0500 Subject: [PATCH 09/11] docs: true up the engine track against what E1-E3 actually shipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- docs/development/iced-migration-roadmap.md | 122 ++++++++++++++++----- 1 file changed, 92 insertions(+), 30 deletions(-) diff --git a/docs/development/iced-migration-roadmap.md b/docs/development/iced-migration-roadmap.md index 16a8b440..784bf9a8 100644 --- a/docs/development/iced-migration-roadmap.md +++ b/docs/development/iced-migration-roadmap.md @@ -260,42 +260,104 @@ finding: **libghostty-vt's render-state dirty tracking is exposed in the pinned header and wrapped by nobody** — not Swift, not GTK, not Iced. All three walk the full grid on every update. -* **E1. Renderer baseline measurement.** Frame time under a full-screen - scrolling TUI at max window size, both renderers (wgpu + tiny-skia), - both OSes. Runs first: its number is what decides whether E4 is worth - doing. Context for why this is a measurement and not an assumption — - the per-cell `renderer.fill_text` in `terminal_widget.rs` looked like - an Iced-specific regression and is not one: `iced_wgpu`'s text pipeline - caches shaping in a keyed atlas, and the Swift renderer is *more* naive - (`TerminalView.swift` per-cell `NSAttributedString.draw`, whose own - comment names a glyph atlas as the next-tier optimization). Treat the - renderer as uniformly naive across all three UIs, not as an Iced risk. -* **E2. Render-state dirty coverage.** `crates/roost-vt/src/render_state.rs` - wraps exactly `update` / `colors` / `cursor` / `walk`. The pinned - `third_party/ghostty/out/include/ghostty/vt/render.h` also offers - global dirty (`GHOSTTY_RENDER_STATE_DATA_DIRTY` → false/partial/full), - per-row dirty (`ROW_DATA_DIRTY` via the row iterator), and the matching - `OPTION_DIRTY` / `ROW_OPTION_DIRTY` setters for resetting both layers. - Wrap those. No bindgen work: all four constants are already in the - generated `sys` module, so this is pure safe-wrapper surface. - **Footgun to handle explicitly** (render.h calls it "an extremely - important detail"): the two dirty layers are independent — clearing the - global state does *not* clear per-row flags, and `update` only ever - sets, never clears. The caller owns both resets. +The E-entries below were written as predictions before any of this +shipped; where the measured result corrected one, that correction is +recorded alongside it rather than quietly edited away. + +* **E1. Renderer baseline measurement — done, prediction corrected.** + The original plan was frame time under a full-screen scrolling TUI at + max window size. That workload is precisely the one E3 cannot move: + libghostty full-rebuilds its render state whenever the viewport pin + changes ("If our viewport pin changed, we do a full rebuild" — + `third_party/ghostty/src/src/terminal/render.zig:299-302`), so any + output that scrolls is a full rebuild by construction, independent of + our dirty-tracking wrapper. Measuring only that would have produced a + near-flat number and fed a false "no win here" into E4's go/no-go. + What actually shipped: deterministic per-tab counters + (`refresh_calls` / `refresh_nanos` / `rows_rebuilt` / `cells_walked`, + plus `draw_calls` / `draw_nanos` / `fill_text_calls` for the + draw path) and CPU-side span timings, exercised by three workloads — + W1 pointer-motion storm, W2 in-place TUI redraw, W3 scrolling stream + as an explicit control expected to stay flat — readable three ways: + CI unit tests, an `#[ignore]`d in-crate harness (`make perf-refresh`), + and the `app.render_stats` IPC op via `roostctl render-stats` + (`make perf-render-stats`), which is the only way to get draw-path + numbers at all (no unit test constructs a live `iced::Renderer`). + Locked-Mac caveat: presented frame rate is meaningless on a locked or + occluded Mac (macOS throttles presentation), which is why this + measures CPU spans and counters instead. See `tools/perf/README.md` + for the harness. +* **E2. Render-state dirty coverage — done.** Shipped in + `crates/roost-vt/src/render_state.rs`: `Dirty { Clean, Partial, Full }`, + `dirty()`, `mark_full()`, `dirty_rows()`, `walk_dirty()`. The footgun + (render.h's "extremely important detail" that clearing one dirty layer + does not clear the other) is handled by making consumption imply + reset — `walk_dirty` clears BOTH layers together, and there is + deliberately no public way to lower the dirty state otherwise: a + general `set_dirty` would have made `set_dirty(Clean)` the very + footgun this wrapper exists to prevent. `walk` is unchanged, so E2 is + purely additive and GTK is untouched by it. 14 tests in + `crates/roost-vt/tests/render_dirty_test.rs` pin the measured + semantics. **Two-phase `begin_update` / `end_update` is NOT available at our pin** — it landed upstream after `c74f6d5` (present at `../ghostty` tip and in `../libghostty-rs`, absent from our generated bindings). It is an E8 follow-on, not part of E2; don't plan around it. `../libghostty-rs`'s `crates/libghostty-vt/src/render.rs` is MIT and a useful reference for the dirty accessors regardless. -* **E3. Dirty-row snapshot rebuild.** Consumes E2. `refresh_snapshot` - (`app/terminal_tab.rs`) currently rebuilds the whole grid per PTY - update, allocating `vec![vec![String::new(); cols]; rows]` — a `String` - per cell including blanks — plus a `String` per `DrawCell`. Rebuild only - dirty rows. GTK's `terminal_view.rs` gets the same treatment. -* **E4. Run coalescing** — **conditional on E1.** Merge adjacent cells - sharing fg/bg/style into one `fill_text` run. A `roost-ui-model` change, - so GTK benefits too. Do not start this without E1's number. +* **E3. Dirty-row snapshot rebuild — done for `roost-iced`.** Consumes + E2. `refresh_snapshot` (`app/terminal_tab.rs`) used to rebuild the + whole grid per PTY update, allocating `vec![vec![String::new(); cols]; + rows]` — a `String` per cell including blanks — plus a `String` per + `DrawCell`. Shipped shape: `TerminalSnapshot`'s `cells` + `rows_text` + became `grid: Vec>`, `DrawCell` lost its `row` field + so the row index has exactly one source of truth (the owning + `RenderedRow`'s position), and three invalidation guards force a full + rebuild outside `walk_dirty`'s own signal: grid size on both axes, + the default fg/bg pair, and a theme generation counter. + Measured (`refresh_snapshot`, `--release`, N=200/workload, before = + `f3e2657` pre-E3, after = this branch's HEAD, both built fresh in a + `git worktree` back-to-back): + + | workload | before ns/refresh | after ns/refresh | speedup | rows rebuilt/refresh | + |---|---|---|---|---| + | W1 pointer-motion storm | 107,605 | 956 | 113x | 32.00 → 0.16 | + | W2 in-place TUI redraw | 110,025 | 6,163 | 17.9x | 32.00 → 2.15 | + | W3 scrolling stream (control) | 116,609 | 57,473 | 2.0x | 32.00 → 27.50 | + + **Important limitation:** streaming/scrolling output is unimproved by + design (W3, per E1's correction above) — the wins are concentrated in + in-place TUI redraws and the non-PTY refresh callers, above all mouse + motion (W1), which previously rebuilt the entire grid on every pointer + event for zero content change. + **libghostty limitation worked around:** `OSC 10`/`OSC 11` and DECSCNM + change the reported default colors while libghostty reports + `dirty=Clean` with zero rows flagged — the dirty API alone can't see + it — so the consumer must compare the default fg/bg pair itself + (the `cached_defaults` guard above). Tripwire tests in + `crates/roost-vt/tests/render_dirty_test.rs` will fail loudly if a + future Ghostty bump changes this. +* **E3b. Dirty-row rebuild for GTK + real `render_stats` — not started.** + Split out of E3, which originally read "GTK's `terminal_view.rs` gets + the same treatment." Two parts: `crates/roost-linux/src/terminal_view.rs` + adopts `walk_dirty` the same way `roost-iced` did, and + `app.render_stats` gets a real implementation — today it answers with + zeroed counters as a deliberate parity placeholder + (`crates/roost-linux/src/app.rs`'s `UiRequest::AppRenderStats` arm), + so the op's contract is identical on both Rust UIs while only one + collects real numbers. GTK is unaffected by the E1–E3 pass beyond that + placeholder, since `walk` itself was left unchanged. +* **E4. Run coalescing — future work, GO per E1's number.** Merge + adjacent cells sharing fg/bg/style into one `fill_text` run. A + `roost-ui-model` change, so GTK benefits too. Was "conditional on E1 + — do not start this without E1's number"; that number now exists: + a running (debug-build) app doing a 300-line scrolling burst on a full + screen issues ~2,410 `fill_text` calls per draw (one per visible + cell), and draw now dominates a refresh at ~1.37 ms vs. ~1 µs for an + idle refresh post-E3 (696 µs for a full refresh). `fill_text_calls` is + a deterministic counter (host- and build-independent); the nanosecond + figures are debug-build and indicative only. Not being done in this + pass. * **E5. Sprite parity in Iced.** `mac/Sources/Roost/Sprite.swift` and `crates/roost-linux/src/sprite.rs` draw U+2500–U+259F (box-drawing, block elements) geometrically because font glyphs don't tile From 496b1b01d752972c3e828e01c2dc832dc0fe6b8c Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 11:24:49 -0500 Subject: [PATCH 10/11] fix(vt): don't strand a failed row by clearing the global dirty layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- crates/roost-engine/src/ipc.rs | 7 +++--- crates/roost-iced/src/app/servicing.rs | 2 +- crates/roost-vt/src/render_state.rs | 13 +++++++++- docs/development/iced-migration-roadmap.md | 16 +++++++++---- docs/reference/ipc.md | 3 ++- tools/perf/README.md | 28 ++++++++++++++-------- 6 files changed, 48 insertions(+), 21 deletions(-) diff --git a/crates/roost-engine/src/ipc.rs b/crates/roost-engine/src/ipc.rs index a12fd8be..db82f9f2 100644 --- a/crates/roost-engine/src/ipc.rs +++ b/crates/roost-engine/src/ipc.rs @@ -275,9 +275,10 @@ pub enum UiRequest { /// regression suite. Ungated (read-only). WindowMetrics { reply: WindowMetricsReply }, /// `app.render_stats` — read the UI's render-path counters, and - /// zero them afterward when `reset`. Ungated (read-only, and the - /// counters are the only way to measure the real draw path: it - /// needs a live renderer no unit test can construct). + /// zero them afterward when `reset`. Ungated. Not read-only: with + /// `reset` it reads and then clears. The counters are the only way + /// to measure the real draw path, which needs a live renderer no + /// unit test can construct. AppRenderStats { reset: bool, reply: RenderStatsReply, diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index 164013eb..f7c2ffd2 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -1638,7 +1638,7 @@ mod tests { async fn changing_the_default_background_rebuilds_cached_rows() { let (feed_tx, _) = engine_feed::channel(); let (mut tab, supervisor) = attach_test_terminal(78, feed_tx); - tab.write_vt(b"\x1b[2J\x1bH\x1b[1;1Hcolored"); + tab.write_vt(b"\x1b[2J\x1b[1;1Hcolored"); tab.refresh_snapshot() .expect("refresh with the row written"); let before = tab.snapshot.background; diff --git a/crates/roost-vt/src/render_state.rs b/crates/roost-vt/src/render_state.rs index 5d23cf12..9b82a482 100644 --- a/crates/roost-vt/src/render_state.rs +++ b/crates/roost-vt/src/render_state.rs @@ -399,6 +399,7 @@ impl RenderState { let _ = terminal; let mut cells: Vec = Vec::new(); + let mut retry_pending = false; for row_idx in 0u32.. { // SAFETY: iter handle non-null. if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { @@ -418,6 +419,7 @@ impl RenderState { if self.bind_row_cells().is_err() { // Leave this row's flag SET so the next walk retries // it, and don't call `f` with a partial row. + retry_pending = true; continue; } @@ -436,7 +438,16 @@ impl RenderState { } } - self.set_global_dirty(sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE)?; + // A row we could not read kept its flag, but clearing the global + // layer to FALSE would strand it: the next `walk_dirty` reads + // `Clean` and visits nothing at all, so the retry the contract + // promises would never happen. Leave the frame `Partial` instead, + // which is exactly what the surviving row flags mean. + self.set_global_dirty(if retry_pending { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_PARTIAL + } else { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE + })?; Ok(global) } diff --git a/docs/development/iced-migration-roadmap.md b/docs/development/iced-migration-roadmap.md index 784bf9a8..b37fa1c1 100644 --- a/docs/development/iced-migration-roadmap.md +++ b/docs/development/iced-migration-roadmap.md @@ -325,11 +325,17 @@ recorded alongside it rather than quietly edited away. | W2 in-place TUI redraw | 110,025 | 6,163 | 17.9x | 32.00 → 2.15 | | W3 scrolling stream (control) | 116,609 | 57,473 | 2.0x | 32.00 → 27.50 | - **Important limitation:** streaming/scrolling output is unimproved by - design (W3, per E1's correction above) — the wins are concentrated in - in-place TUI redraws and the non-PTY refresh callers, above all mouse - motion (W1), which previously rebuilt the entire grid on every pointer - event for zero content change. + **Important limitation:** streaming/scrolling output gets **no + dirty-tracking benefit** — W3 barely moves in rows rebuilt (32.00 → + 27.50) because libghostty full-rebuilds on any viewport-pin change, per + E1's correction above. Its 2.0x clock gain is a *separate* effect: E3 + also deleted the dense `vec![vec![String::new(); cols]; rows]` + allocation, which cost a `String` per cell including blanks, so even + full rebuilds got cheaper. Judge W3 on rows/refresh staying high, not + on its timing. The dirty-tracking wins are concentrated in in-place TUI + redraws and the non-PTY refresh callers — above all mouse motion (W1), + which previously rebuilt the entire grid on every pointer event for + zero content change. **libghostty limitation worked around:** `OSC 10`/`OSC 11` and DECSCNM change the reported default colors while libghostty reports `dirty=Clean` with zero rows flagged — the dirty API alone can't see diff --git a/docs/reference/ipc.md b/docs/reference/ipc.md index 291c5e1d..cd0b4a7f 100644 --- a/docs/reference/ipc.md +++ b/docs/reference/ipc.md @@ -640,7 +640,8 @@ Caveat: `app.screenshot` re-renders the window, so taking a screenshot inflates the three draw counters. Read before capturing, or reset after. -Ungated, read-only — always available, matching `tab.dump_resolved`. +Ungated — always available, matching `tab.dump_resolved`. Not +read-only: `reset: true` reads the counters and then zeroes them. The GTK UI answers with the same shape, all counters zero: its renderer has no instrumentation yet, and a uniform contract across both Rust UIs beats an op one of them refuses. CLI: `roostctl diff --git a/tools/perf/README.md b/tools/perf/README.md index b45e14bc..739840b6 100644 --- a/tools/perf/README.md +++ b/tools/perf/README.md @@ -46,13 +46,21 @@ this output. the viewport never scrolls, then refreshing. The vim/htop shape. - **W3 — scrolling stream (CONTROL).** 200 iterations of plain `line\r\n` output that scrolls the viewport, then refreshing. - **This workload is expected to show NO improvement, ever.** libghostty - full-rebuilds its render state whenever the viewport's scroll pin - changes (`third_party/ghostty/src/src/terminal/render.zig:299-302`), - so no amount of dirty-tracking on our side changes what it hands back. - It's in the harness precisely so a before/after table has a control - that stays flat — **a flat W3 is a PASS**, not a disappointment. Do not - "fix" it. + **This workload is the control: expect little or no gain in + rows-rebuilt, ever.** libghostty full-rebuilds its render state + whenever the viewport's scroll pin changes + (`third_party/ghostty/src/src/terminal/render.zig:299-302`), so no + amount of dirty-tracking on our side reduces the rows it hands back — + E3 measured 32.00 -> 27.50 rows/refresh here, against 32.00 -> 0.16 + for W1. + + Its *timing* can still improve, and did: 2.0x (116,609 -> 57,473 + ns/refresh) in the E3 measurement, because removing the dense + `vec![vec![String::new(); cols]; rows]` allocation made even a full + rebuild cheaper. So judge W3 on **rows_per_refresh staying high**, not + on its clock. A W3 whose rows/refresh collapses means rows are being + served from a stale cache across a viewport move — that is a bug, not + a win. ### What it can't measure @@ -80,9 +88,9 @@ diff before.txt after.txt ``` Read `rows_per_refresh` first — it's the direct measure of "how much of -the grid did this refresh actually touch." W1 today reports the full 32 -rows/refresh; the point of dirty tracking is driving that toward 0 -without moving W3. +the grid did this refresh actually touch." Pre-E3 every workload +reported the full 32 rows/refresh; the point of dirty tracking is +driving W1 and W2 toward 0 while W3 stays high. ## The running-app readout: `tools/perf/render-stats.sh` From 1e8dde0ac8033d85f9a11daaf1415f2d1c48ca38 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Thu, 6 Aug 2026 11:50:34 -0500 Subject: [PATCH 11/11] ci: fail ci-success on any non-passing job status, not just failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d --- .github/workflows/ci.yml | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee789cf4..a0a586e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1104,10 +1104,19 @@ jobs: run: | results="${{ needs.rust-lint.result }} ${{ needs.harness-unit.result }} ${{ needs.themes-parity.result }} ${{ needs.rust-build.result }} ${{ needs.swift-mac.result }} ${{ needs.gtk-build.result }} ${{ needs.iced-build-e2e.result }} ${{ needs.e2e-gtk.result }} ${{ needs.e2e-mac.result }}" echo "job results: ${results}" + # Allowlist, not a denylist. A denylist of failure/cancelled let + # `abandoned` through — the status GitHub assigns when its own + # infrastructure kills a job — so a run where 7 of 9 jobs never + # executed reported this gate GREEN. Observed on PR #306 during a + # GitHub "Failed to resolve action download info" incident. + # Anything that is not an actual pass now fails the gate. for r in ${results}; do - if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then - echo "::error::a CI job failed (${results})" - exit 1 - fi + case "${r}" in + success|skipped) ;; + *) + echo "::error::a required CI job did not pass: '${r}' (${results})" + exit 1 + ;; + esac done echo "all good"