Render terminals on the GPU, with a context budget and a DOM fallback - #63
Render terminals on the GPU, with a context budget and a DOM fallback#63Sadykhzadeh wants to merge 4 commits into
Conversation
xterm was left on its DOM renderer, which rebuilds one span per styled run per row on every frame. Loading @xterm/addon-webgl moves that to the GPU: measured on this machine, a render-bound stream costs 4.5 ms per frame on the DOM renderer and 0.73 ms on the GPU renderer at 120x40 (5.9x), and 1.7 MiB/s becomes 10.7 MiB/s. Output that arrives faster than the screen refreshes is parser-bound, not render-bound, and gains only ~1.4x. Two failure modes make this addon easy to ship badly, so both are handled rather than assumed away. A browser keeps 16 WebGL contexts alive per renderer process and silently kills the oldest past that; the 17th terminal blanks the first for the three seconds the addon waits for restoration. TerminalWorkspace mounts every pane of every tab at once by design, so the pane count is not bounded by what is on screen. A least-recently-used budget of eight panes bounds the GPU-backed subset to half the cap: a pane claims a slot when it comes to the front, and panes pushed out revert to the DOM renderer instead of losing a context. Holding the claim across tab switches avoids paying the 19 ms reattach on every switch. On context loss the addon is disposed, which restores xterm's DOM renderer with the buffer intact, so a GPU reset costs rendering quality and never the pane's contents. The addon is imported on demand and lands in its own 101 kB chunk, leaving the entry chunk 1.3 kB larger rather than 101 kB larger. The GPU renderer draws to a canvas, so a pane's transcript is no longer in the DOM. The browser regression scenarios read it from `.xterm-rows`, so the fixture now reports no WebGL2 — the same path a machine without WebGL2 takes — and terminal-gpu-renderer.mjs opts back in to cover the renderer the app ships with.
Greptile SummaryThe PR adds a budgeted WebGL terminal renderer with DOM fallback, context-loss recovery, lazy loading, and persisted screen-reader support. Existing terminals now apply screen-reader mode when the accessibility preference changes. Confidence Score: 5/5Safe to merge; the only remaining prior concern is non-blocking. The previous terminal-output accessibility finding is fixed: terminal creation enables xterm screen-reader mode from the persisted setting, and open terminals update when that setting changes. One previous non-blocking accessibility finding remains outstanding: the Screen reader support switch has no accessible name, so screen-reader users cannot identify what the control changes. Reviews (5): Last reviewed commit: "Say the default rather than who asks for..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| observer = new ResizeObserver(() => fit()); | ||
| observer.observe(containerRef.value); | ||
| fit(); | ||
| if (props.visible) void acquireWebglRenderer(props.pane.id, term); |
There was a problem hiding this comment.
Visible panes enable the WebGL renderer here, which replaces the terminal text rows with canvas output without enabling a screen-reader mode or maintaining an accessible transcript. Assistive technologies can access the input field but cannot read terminal output, preventing affected users from operating GPU-backed terminals. This must be fixed before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
- The exact Playwright/Chromium script executed the DOM and WebGL terminal paths, injected a unique output marker, and inspected terminal DOM and accessibility-tree state; it provides reproducible validation.
- The command log records the executed command, `/home/user/repo` working directory, and exit code 0; the rendered accessibility check completed successfully.
- The successful observed JSON records DOM rows and marker availability before WebGL and canvas-only output with no marker in DOM or Chromium accessibility tree after WebGL; it confirms the accessibility regression.
Terminal output with the DOM renderer baseline
- The rendered DOM-renderer baseline shows the connected terminal after fixture output was injected; its output is backed by DOM rows.
- The DOM-renderer recording shows the rendered terminal after output injection, providing the accessible-row baseline.
Terminal output after the WebGL renderer is enabled
- The rendered WebGL terminal shows the visible output area after the same output injection; it is drawn by canvas rather than DOM rows.
- The WebGL recording shows the changed canvas-rendered terminal state after output injection; terminal output has no validated accessible text alternative.
|
APPROVE WITH NITS I re-ran everything against a dev server I confirmed was serving
And I checked the two claims that would be quietly fatal if wrong:
Nits, worst first. 1. The budget's safety margin is measured on an engine the app ships on only one of three platforms. 2. 3. The pin is on the wrong package. 4. The WebGL2 opt-out is wider than the five assertions that need it. 5. One 6. On accessibility. I think you have laid the decision out fairly and I would not hold the PR for it. One framing I would add for whoever decides: with The honesty about the burst-drain table is what makes the rest of this reviewable — leaving 0.94x at 80x24 in the description rather than quietly reporting the 6.2x is the right call. |
new WebglAddon() sat outside the try. The addon runs its own webgl2 probe in the constructor, with attributes our cached probe never used, and throws before xterm ever sees it -- Safari below 16 being the case that always throws. Inside an async function that became a rejected promise rather than a fall back to the DOM renderer, and nothing covered it. Two tests now pin the behaviour; both fail against the previous code. Rewrite the budget comment to say what was actually measured. The cap of 16 and oldest-first eviction were observed on Chromium only; WKWebView and WebKitGTK were never measured here, so cite WebKit's own maxActiveContexts constant for those and mark them unverified rather than implying they were tested. The budget stays at 8, with the reasoning written down. Pin @xterm/xterm exactly to match @xterm/addon-webgl. The DOM fallback runs through xterm's private _core._createRenderer(), which no public typing covers and semver does not protect, and a broken dispose fails silently because the teardown catch swallows it. Note the coupling next to the catch.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
@greptile-apps, can you review this PR and report the found issues? |
A terminal draws its rows for the eye. xterm marks the DOM renderer's row container aria-hidden and the GPU renderer draws to a canvas, so neither is readable by assistive technology and terminal output has never been exposed to it here. What is readable is xterm's accessibility layer: it builds its own row elements and a live region from the buffer, and takes nothing from the renderer but its dimensions, so it works the same whichever one is drawing. A setting turns it on, applies to terminals that are already open, and is remembered. It costs a DOM tree per terminal, so it is off until asked for -- and it does not cost the GPU renderer, which is the point of keeping it independent of the drawing path.
|
Addressed in The DOM renderer is not accessible either. What is readable is xterm's accessibility layer, and it is independent of the renderer. The change. A Deliberately not auto-detected. There is no reliable way to detect a screen reader from a webview — the approaches that exist are heuristics that get it wrong in both directions, and a wrong guess either silently costs every terminal a DOM tree or silently withholds the layer from someone who needs it. Tests —
On the earlier nits from my own review — the engine-scope comment on @computerbox124 всё готово, можно смотреть. |
| <button | ||
| type="button" | ||
| role="switch" | ||
| :aria-checked="settings.screenReaderMode" | ||
| class="flex h-6 w-11 items-center rounded-full transition-colors duration-100 shrink-0" | ||
| :class="settings.screenReaderMode ? 'bg-primary' : 'bg-muted'" | ||
| @click="settings.setScreenReaderMode(!settings.screenReaderMode)" | ||
| > |
There was a problem hiding this comment.
The Screen reader support control is exposed as a switch without an accessible name. Screen readers announce only an unnamed switch and its state, so users cannot tell that it enables terminal output accessibility. Give the button an aria-label="Screen reader support" or reference the adjacent visible label with aria-labelledby. This is non-blocking, but makes the new accessibility preference difficult to identify for the people it is intended to support.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
- The authored Chromium Playwright script opens Settings, queries the target switch accessibility tree, records two captures, and writes the observed result; it is the executed source.
- The actual executed command output records the target’s role, accessible-name lookup count, DOM associations, and exit code 0; it shows the switch has no accessible name.
- The test’s machine-generated result records the accessibility snapshot `- switch`, zero name-matched switches, and missing naming attributes; it confirms the accessible name is empty.
- The initial rendered Settings view shows the visible Screen reader support text and its adjacent switch before focus; it establishes the tested UI scope.
Settings view before focusing the Screen reader support switch
- The poster frame shows the visible Screen reader support text beside the unlabeled target switch; it establishes the tested UI scope.
- The second rendered capture focuses the exact switch inspected by Playwright; it emphasizes the control whose accessibility tree exposes only `switch` without a name.
Settings view with the Screen reader support switch focused
- The poster frame shows the focused target switch adjacent to Screen reader support; it identifies the exact rendered control with an empty accessible name.
The comment hygiene check reads "unless asked for" as attribution. The sentence states the default instead.
|
Two follow-ups.
Second, this branch conflicts with nothing else in the set. I merged all nineteen PRs into one tree to check the whole batch composes: |
2 participants
Fixes #62.
Terminals ran on xterm's DOM renderer — the documented fallback, which rebuilds one
<span>per styled run per row whenever the screen changes. This loads@xterm/addon-webgland moves that work to the GPU.The addon is easy to ship badly, so the two failure modes that produce a blank terminal rather than a slow one are part of the change rather than hardening bolted on afterwards.
What changed
desktop/src/lib/terminalRenderer.ts(new)A process-wide, least-recently-used budget of eight GPU-backed panes.
The constraint is specific to this app. Chromium keeps 16 WebGL contexts alive per renderer process and kills the oldest without warning past that — measured directly: 64 contexts created, 16 alive, 48 lost, oldest first.
TerminalWorkspace.vue:101-102mounts every pane of every tab at once, which is correct and keeps xterm from remounting when panes move between tabs, but it means the number of live terminals is not bounded by what is on screen. Eight leaves half the cap as headroom, and a pane pushed out of the budget reverts to the DOM renderer — the renderer every pane uses today — rather than to a dead context.Scope of that measurement, corrected. The first revision of this PR wrote the invariant as a platform-general rule ("the cap must never be reached"). It is not one: 16-and-oldest-first was measured on Chromium only, which is the Windows WebView2 path. The app also ships on WKWebView (macOS) and WebKitGTK (Linux), and I have no machine for either, so neither was measured. Rather than invent numbers, the comment now cites what upstream declares: WebKit's
Source/WebCore/html/canvas/WebGLRenderingContextBase.cpphasstatic constexpr size_t maxActiveContexts = 16(andmaxActiveWorkerContexts = 4), andaddActiveContextrecycles the context with the lowestactiveOrdinal()— least-recently-active, not oldest-created, a subtly different policy from Chromium's. Both WebKit ports build that same WebCore file. That is a constant read out of upstream source, not a number observed in this app, and it is labelled as such in the code.Why the budget stays at eight rather than going more conservative. The argument for lowering it is that an unmeasured engine is an unknown lower bound, and WebKit's own worker-context limit of 4 shows the project does use smaller numbers somewhere. The argument against, which I find stronger: the two unmeasured engines declare the same ceiling, so this is not an unknown bound so much as an unconfirmed one; a smaller budget would start demoting panes that are simultaneously on screen in a multi-pane split, which is exactly the case the GPU renderer exists to serve; and the cost of being wrong is bounded. Overshooting the real cap costs the least-recently-used pane a stale frame for the addon's three-second restoration window and then drops it to the DOM renderer with its buffer intact — a quality regression on the pane furthest from the user's attention, never a correctness one. If either WebKit port turns out to enforce something lower in practice than the constant suggests, the budget is one constant to change and the tests already pin the eviction behaviour.
It is least-recently-used rather than visibility-gated on purpose. Disposing on hide and re-attaching on show is the obvious design and it is worse: attach costs 19 ms warm and 57 ms cold (shader compile plus texture atlas), so it would put a visible hitch on every tab switch. A pane claims a slot when it comes to the front and keeps it until eight other panes have been used more recently, so alternating between a few tabs never re-attaches at all.
Support is probed once per process, and the probe hands its own context straight back rather than holding one of the sixteen.
Repeat claims for the same pane join the pending one instead of racing it — a pane can be shown, hidden and shown again inside the dynamic import's window, and without that guard it ends up with two contexts.
Context loss
onContextLossdisposes the addon, which is what restores the DOM renderer. Verified against the installed version rather than assumed:WebglAddon.activate()registers a disposable that callsrenderService.setRenderer(terminal._core._createRenderer())thenhandleResize(cols, rows)._createRenderer()in@xterm/xterm5.5.0 returnsDomRenderer.dispose()the live renderer's constructor is identical to a never-accelerated terminal's, output written afterwards appears in the DOM, and nothing throws.Worth knowing: the addon does not report the loss immediately. It calls
preventDefault()and waits 3000 ms for restoration first — measured at 3075 ms end to end. The pane is frozen for those three seconds. Handling it is still mandatory; without the dispose the pane never comes back.The Safari-below-16 path was an unhandled rejection
new module.WebglAddon()sat outside thetrythat guards activation, so the constructor's own failure escaped the fallback entirely.attach()isasync, so on any platform where the constructor throws,acquireWebglRenderer()returned a rejected promise instead of resolvingfalse— an unhandled rejection at every call site, rather than the clean drop to the DOM renderer the rest of the module is built around. Nothing covered it.It is not a theoretical path. In
@xterm/addon-webgl0.18.0 the constructor runs its own probe before xterm ever sees the addon:Safari below 16 is the cited case, but note the probe uses different context attributes from
supportsWebgl2()above it, andsupportsWebgl2()caches its answer for the life of the process. So a platform that refusespreserveDrawingBuffer, or simply a moment when the engine is already at its context cap, fails here and not there — the two probes are not interchangeable.Fixed by constructing inside a
trythat returnsfalse, andonContextLossmoved into the existingtryalongsideloadAddonfor the same reason (same one-line failure class, same handler).Test result, before and after. Two tests were added first and run against the unmodified branch:
After the fix, same file:
Tests 15 passed (15). The second test also pins that a constructor failure leaves no stale entry ininFlight, so the pane can try again later rather than joining a dead claim.Lazy loading
import("@xterm/addon-webgl")on first use, matching the approach #37 introduces foraddon-search. It lands in its own chunk, so the entry chunk grows by 1.33 kB rather than 101 kB.Pinned to
0.18.0exactly — the addon published alongside@xterm/xterm5.5.0 — because addon/core mismatches cause subtle rendering bugs.@xterm/xtermis now pinned exactly too, at5.5.0instead of^5.5.0. Pinning one side of that pair and not the other did not describe the actual coupling. As the section above spells out, the DOM fallback this whole change rests on is the addon callingterminal._core._renderService.setRenderer(terminal._core._createRenderer()). None of_core,_renderServiceor_createRendererappears in@xterm/xterm's public typings, so semver says nothing about them and an xterm patch release is free to rename or drop any one of them.Pin both rather than loosen both, because of how it would fail.
Lease.detachswallows a throwingdispose()on purpose, so that a renderer which cannot unwind does not take the pane down with it. That same catch means a renamed private method produces no error anywhere: evicted panes would silently keep their GPU contexts, and the budget would quietly stop bounding anything — the exact failure this PR exists to prevent, reintroduced by a patch bump and invisible until a user hits the cap. A loud break would be tolerable on a range; a silent one is not. The coupling is now written down next to the catch, with instructions to re-run the eviction and context-loss tests against the real addon on any bump. Resolution is unchanged (@xterm/xtermwas already resolving to 5.5.0), so the lockfile delta is one line.(Unrelated, deliberately not touched here:
@xterm/addon-searchis on^0.16.0, which resolves to thexterm6.0.0-era addon against a 5.5.0 core, and@xterm/addon-fitreaches into_coreas well. Both predate this branch and belong in their own change.)@xterm/addon-canvaswas considered and left outIt would be a third renderer to test and a second chunk, and it only helps where WebGL2 is unavailable — where the DOM renderer already works and is already the fallback this change relies on. Not worth a dependency on the evidence available.
Measurements
Measured on this machine (Windows 11, RTX 5060 Ti), Playwright Chromium through ANGLE → D3D11, the same graphics path WebView2 uses on Windows, with vsync and the frame-rate limit disabled. 4 MiB of realistic output: a third heavily styled, a third plain bulk text, a third build-log lines.
Render-bound streaming — one 8 KiB chunk per animation frame, so the renderer draws every frame:
Burst drain — the whole corpus written as fast as
term.writeaccepts it:The second table is the honest limit of this change: when output outruns the refresh rate xterm coalesces and draws only the final state, so the work is in the VT parser and the renderer barely matters.
cat bigfileis close to that case. The 5-6x applies to sustained interactive-rate output.Entry chunk 653.47 kB → 654.80 kB raw (181.03 → 181.46 kB gzip); new
addon-webglchunk 101.22 kB / 26.06 kB gzip, fetched when a pane first comes on screen.The transcript leaves the DOM
The GPU renderer draws to a canvas, so
.xterm-rowsdoes not exist and a pane's text is not in the DOM. This is the part that deserves a decision rather than a default:.xterm-rows, and they time out rather than failing loudly.tauri-fixture.jsnow reports no WebGL2 so those scenarios run on the DOM renderer — which is a genuinely supported configuration, the same path a machine without WebGL2 takes — and they keep proving exactly what they proved before, since routing, focus, lifecycle and fault isolation are all renderer-independent. A newterminal-gpu-renderer.mjsopts back in and covers the renderer the app actually ships with, so the shipped path is not left unverified.screenReaderMode: false, so xterm builds no accessibility layer under either renderer. Today a screen reader traverses the DOM rows incidentally; under WebGL there is nothing to traverse. That is not xterm's intended accessibility path —screenReaderModeis, and it has its own cost — but it is a real reduction in what assistive technology can reach, and it is not something this PR can measure away. Happy to follow up withscreenReaderModewired to a setting if you want that closed before this lands.Rendering is otherwise equivalent. Same fixture session, same output, dark theme:
DOM renderer:
GPU renderer:
Conflict with #49
#49 rewrites the data path in
TerminalPane.vue(its hunks are around lines 147-260 — listener setup andconnectSession). This change touches lines 13, 350 and 358 — the import, terminal construction and teardown — soTerminalPane.vueshould merge cleanly.The overlap is
desktop/e2e/tauri-fixture.js: #49 edits around lines 8-21 and this inserts eleven lines at line 3, so the hunks share context and git will likely flag it. The resolution is mechanical — keep both, the WebGL2 guard at the top of the IIFE and #49's channel changes below it. Happy to rebase whichever way round they land.Rust and
core/src/session.rsare untouched; this is frontend-only.Test plan
cd desktop && npx vue-tsc --noEmit— clean, exit 0cd desktop && npm test— 329 passed across 38 files (308 before, +21: 15 for the budget module, 6 for the component wiring)cd desktop/e2e && UI_RECORD=0 npm test— 72 scenarios pass, exit 0 (68 before, +4 GPU renderer scenarios)node scripts/verify/check-comments.mjs—{"ok":true,"count":0,"files":128}node scripts/verify/control-clavyn.mjs doctor --pretty— all checks okcd desktop && npm run build— entry chunk +1.33 kB, addon in its own chunkNew coverage:
One caveat on how this was verified, because it nearly produced a false pass: the browser suites target a dev server on
127.0.0.1:1420, and another checkout on this machine already held that port, so the first run drove the wrong build and reported 68/68 against code that did not contain this change. Every result above is from a dev server on a port confirmed to be serving this branch — for the follow-up run,UI_URL=http://127.0.0.1:5199with the served/src/lib/terminalRenderer.tsfetched and checked to containnew module.WebglAddon()inside atrybefore the suite was started.