Repository navigation
docs: update replay cost comments after the viewport read change - #369
Conversation
- webm.ts and the export README gave a longest silent stretch of 8.4 s, under 3% of the stall window. A flush of 520 full-screen repaints at 600x150 has a stretch of 15.9 to 20.7 s on main. The examples are now measured examples with their host CPU pressure, not bounds. - The MAX_SPEED_FRAME_BUDGET comment gets per-flush costs and scan costs measured after #365, with the pressure of each group of runs. - The readViewportCells comment says why a read after a resize is fresh: Terminal.resize renders at once and the render calls getCursor(), which calls update(). A direct wasmTerm.resize() reads the old screen. - docs/USAGE.md: the replay time of 400 resizes at 600x150 is 592 s and 985 s, not 910 s.
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffe6155512
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- docs/USAGE.md: each number says what it times. The 400-resize export took 595 s and 2,582 s in all, of which the replay took 592 s and 985 s. The second export spent 1,597 s outside the replay. The 7,401-event export took 477 s, all of it in the replay. - webm.ts and the export README: a stretch depends on the output in one flush, the terminal size and the load of the host. The measurements do not isolate the terminal size alone. The examples drop the first person.
The fastest 80x24 run took 7,393 ms for 400 flushes, 18.48 ms per flush. The comment said 19 to 29 ms. It now says 18 to 29 ms.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
Three comments and one doc sentence about the ghostty-web replay no longer match the code or a measurement.
src/export/webm.tsandsrc/export/README.mdsaid the longest silent stretch of a healthy timed replay is 8.4 s, "under 3%" of the 5-minute stall window. That is false. A flush of 520 full-screen repaints at 600x150 had a stretch of 15.9 to 20.7 s in three runs on current main. The window is 300 s, so that is 5.3 to 6.9 %.MAX_SPEED_FRAME_BUDGETinbackend.tsgave per-flush costs from before perf: read the ghostty-web viewport once per decode #365. perf: read the ghostty-web viewport once per decode #365 made the in-page scan of the screen much cheaper. The comment also gave no measured scan cost.readViewportCellsinembeddedHarnessHtml.tssaid "a read in the task of a write or a resize sees the new screen". That holds for a resize only because of how the harness resizes. A later change that resizes withwasmTerm.resize()reads a stale screen.docs/USAGE.mdgave the time of 400 resizes at 600x150 as 910 s. That number came from before perf: read the ghostty-web viewport once per decode #365, and the doc did not say which stage it timed. The replay now takes 592 s to 985 s, and the whole export takes 595 s to 2,582 s, depending on host load.No code changes. Every changed line is a comment, a README line or a doc sentence.
What
src/export/webm.tsandsrc/export/README.md: the examples of silent stretches are now measured examples on current main, not bounds. They name the host CPU pressure of the runs. The comment says a stretch depends on the output in one flush, the terminal size and the host load. I removed "grows with the terminal size" because my runs of the stretch at 600x150 do not vary the size. The size claim rests on the per-flush costs below: one full-screen repaint costs 18 to 72 ms per flush at 80x24 and 157 to 191 ms at 200x60. The code comment no longer speaks in the first person. The "longest is 8.4 s, under 3 %" sentence is gone. The sentence "the window is the old fixed limit on the whole replay" stays in the code comment. It is still true.src/renderer/ghosttyWeb/backend.ts: theMAX_SPEED_FRAME_BUDGETcomment has the new per-flush numbers, the scan cost, the definition of "per flush" (replay time divided by the number of flushes) and the pressure of each group of runs. I removed the "400 outputs 40 ms apart" sentence. It had the same flush count as the 20 ms case, and the 20 ms runs now cover pressure from 7 % to 79 %.src/renderer/ghosttyWeb/embeddedHarnessHtml.ts: thereadViewportCellscomment now says a read after a write needs noupdate(). After a resize it holds only becauseTerminal.resizerenders at once, the render callsgetCursor(), andgetCursor()callsupdate(). After a directwasmTerm.resize(),getViewport()reads the old screen untilupdate()runs.docs/USAGE.md: each number now says what it times. "7,401 events took 473 s" is now "exporting ... took 477 s, of which the replay took 477 s" (my run of the same log). "400 resizes ... took 910 s" is now "exporting ... took 595 s and 2,582 s in two runs ... The replay took 592 s and 985 s of that. The second export spent the other 1,597 s outside the replay, in the browser start, the glyph check or the video finalize." The last new sentence says that the replay is the stage that the stall window watches.I checked
src/renderer/ghosttyWeb/README.md,src/export/README.md,docs/USAGE.mdandRELEASE.mdfor other numbers from before #365. The ghostty-web README and RELEASE.md have none.docs/USAGE.mdhad the two numbers above. A repo-wide grep for8.4 s,3%,11,000,4.4 sand1.3 to 2.0finds no other copy.Evidence
Every number comes from the real backend:
generateWebmExportwith a realGhosttyWebBackendand video, on main at74c22d74, Node 26.2.0. Host CPU pressure issome avg10of/proc/pressure/cpu, read by the scripts like this: the per-flush and stretch scripts read it at the start, every 2 s during the run and at the end, and the table shows the min to max of those readings. The scan script read it twice per run, at the start and at the end. The host was shared and busy (load average 10 to 165). Raw results and harness scripts are indocsync-evidence/next to this file (results*.jsonl,harness/).Per-flush cost, 80x24, 400 full-screen repaints 20 ms apart,
max-speed(replay time / 400 flushes)The median page call is 16.4 to 16.7 ms (one animation frame) in the quiet runs. Before #365 (
776ee5c9, interleaved with a1 to a3 on the same host): 44.5, 45.9 and 41.7 ms. At 80x24 #365 changes the cost per flush by less than the noise.Worst case, 200x60, 2,400 full-screen repaints 17 ms apart,
max-speed(2,400 flushes, 2,400 page calls)The runs alternated (main, before, main, before). In the two alternating pairs, main is 15 % and 26 % faster at 200x60 (459 s against 539 s, 377 s against 510 s), with overlapping pressure ranges. The old comment said 287 s to 405 s at pressure 15 to 45 %. My runs of the old code took 510 s and 539 s at 29 to 63 %, so the host is slower now. I did not change code to improve these numbers.
In-page scan (
notePaintedGraphemes) of a full screen, median of 25Pressure was 36 to 44 % (the two readings of each of the four scan runs). This reproduces "about 75 ms to about 6 ms" at 200x60. The scan is now about 3 % of a 200x60 flush. The rest is the frame and the canvas draw.
Silent stretches at 600x150,
recorded(the gaps that the stall watchdog sees)20.7 s is 6.9 % of the 300 s window. Replay time and whole export time. The Replay column is the time inside
replayWithTiming. The whole export also covers the browser start, the video finalize, the browser close and the temp-folder cleanup. The glyph check after the replay reads a result that the final hold already cached, so it costs about nothing. I measured both for three runs:acceleratedThe raw data cannot split the 1,597 s between the steps outside the replay. The harness timed the replay and the whole export, and nothing between them. I did not repeat the run. The docs name the steps outside the replay and do not say which one took the time. All of them run outside the stall watchdog. A verifier measured 13.9 s (4.6 %) and 9.5 s for the same kind of flush while reviewing #356. Both are below my 15.9 to 20.7 s. I did not measure them, so the comments do not use them. The longest stretch that I measured is 20.7 s.
Other runs
accelerated: replay 477 s and whole export 477 s at pressure 9 to 48 %. fix: stop a webm replay only when the renderer stalls #356 measured 473 s for the whole export.ffprobe): the 400-repaint run at 80x24 had a replay of 28.8 s and a video of 30.5 s. The video records the replay as it runs, so 2,400 flushes of 40 ms or more make 96 s or more.Claims of the
MAX_SPEED_FRAME_BUDGETcomment checked against the codeminSplitMs = max(1, hold, span / 2400)(backend.ts:759). The 17 ms log has span 40.8 s, a split of 17.0 ms and 2,400 flushes. Measured 2,400.case 'resize'callsflushOutputBatch(true, preResizeHoldMs)(backend.ts:904-907). That ends a flush only when output waits. It is outside the budget. True.replayWithStallWatchdog(webm.ts:156) restarts a 5-minute timer on every report.record-export.tsandsrc/export/have no other timer. Runs of 459 s, 539 s, 592 s and 985 s finished. True.VIDEO_FPS = 25(backend.ts:173) andrecordVideo.fps(backend.ts:1415) give 40 ms per frame. 2,400 x 40 ms = 96 s. True.writeBatchBridgewas called 400 times for 400 flushes and 2,400 times for 2,400. True.Item 3, the stale viewport read (ghostty-web 0.4.0,
node_modules/ghostty-web/dist/ghostty-web.js)wasmTerm, minifiedclass K):getViewport()(line 225) never callsupdate().getCursor()(line 176) callsupdate()(line 177).resize(line 147) does not callupdate().Terminal.writeInternal(line 2388) callswasmTerm.writeand noupdate().Terminal.resize(line 2422) callswasmTerm.resize(...)and thenthis.renderer.render(...).render(line 1397) starts withA.getCursor().embeddedHarnessHtml.ts:813(terminal.resize(cols, rows)).docsync-evidence/stale1.out,stale2.out), screen filled with one letter per row:getViewport()in the same task shows the new text. Same forterminal.write.terminal.resize(40, 10)and thengetViewport()equals the result afterupdate()(rowsTTTT...,UUUU...).wasmTerm.resize(40, 10)and thengetViewport()gives the old rows read with the new stride (AAAA...,BBBB...). Afterupdate()it givesTTTT....wasmTerm.resize(40, 10), thengetCursor(), thengetViewport()is fresh. Resizing back to 80x24 withoutupdate()gives rowsA, C, E, Gand notA, B, C, D.Validation
Run on the final head (the review rounds changed only
docs/USAGE.md,src/export/README.md, the comment insrc/export/webm.tsand one comment insrc/renderer/ghosttyWeb/backend.ts):npm run format:check: pass.npm run lint: 0 warnings, 0 errors.npm run typecheck: pass.npx vitest run test/unit: 149 files, 2,401 of 2,401 tests pass (pressure 44 to 49 %). An earlier full run at pressure 65 to 72 % failed 3 tests by their test timeouts (5 s, and 30 s forartifactManifestLock). The tests weredoctor.test.ts > returns unique passing checks ...,gc.test.ts > removes the session folder when a manifest write lands during its rmandartifactManifestLock.test.ts > lets one writer at a time in when many find a dead owner's lock. The three files passed together on a rerun at pressure 64 to 69 %. A full run onorigin/mainat74c22d74at pressure 64 % faileddoctor.test.tsagain and 3 other tests (lifecycle.test.tszombie tests). So these are load timeouts, not caused by a comment change.check.sh --drift-only: "Covered code matches the sync record."Earlier on the first head, I also ran
npx vitest run --maxWorkers=1 --retry=0 test/integration/renderer-backend.test.ts(36 of 36 pass), because the first head changed a comment inside the template string ofembeddedHarnessHtml.ts. The review round did not touch that file. On that first run,test/unit/commands/gc.test.ts > removes the session folder when a manifest write lands during its rmhit its 5 s timeout at pressure 78 % and passed on rerun alone. It passed in the run above. The change touches no code.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh