Metal: keep the row cache across scrolls - #636
Conversation
A terminal being written to rebuilt its whole screen every frame:
shaping the text, looking every glyph up in the atlas and laying out the
cells again for rows that had not changed at all. Three things caused
it, and all three had to go.
**The vertices were written in screen coordinates.** `lineOffset` came
from `row - yDisp`, so `yDisp` had to be part of `CacheSignature`, and
any scroll invalidated the whole cache. Rows are now built in their own
coordinates — bottom edge at zero — and where a row goes is handed to
the shader as a `float2` uniform per draw call. `yDisp` is out of the
signature.
**The cache was keyed by absolute row number.** A `CircularList` rotates
its references as the scrollback fills, so a row number stops meaning
the same line the moment output scrolls and every lookup missed. It is
keyed by the line's identity now, which is what the entry was already
checking with `lineRef === line`.
**The dirty range covers the whole screen on any scroll**, and that is
correct on its own terms: from the screen's point of view every row does
hold different text afterwards. But they are the same lines, moved, and
what actually says a line needs rebuilding is its own generation — which
the cache already checks. The range is therefore consulted only for rows
the cache has nothing for.
Measured by the test this adds — 48 rows of 120 columns, 200 scrolled
frames of coloured output:
before: 10.348 ms per frame, 48.0 rows rebuilt per frame
after: 0.596 ms per frame, 2.0 rows rebuilt per frame
And in an app, with a pane printing 250 lines a second, sampled for 15
seconds: `buildDrawData` falls from 25.8% of a core to 6.5%, and the
whole process from 41% to 20%.
The aggregated buffering mode has nowhere to put a per-row uniform, so
it adds the offset to the cells as it concatenates them — still only
arithmetic over cells that are already shaped and already found in the
atlas.
Two supporting changes. `buildDrawData` and the two debug row counters
become internal, so the cache can be measured from a test. And the
shader bundle is now also looked for beside the running binary: SwiftPM
puts the resource bundle next to the test bundle, and a test host has no
`.app` around it to look inside — without that the renderer cannot be
constructed in a test at all.
|
This looks great, I will take it for a spin on my performance branch ( In the meantime a couple of notes:
|
Addresses the review on migueldeicaza#636. A line's `generation` moves when its contents change and at no other time, but a row is also drawn differently when it is selected, when a link on it is highlighted, when the modifier for link activation goes down, and when the blink phase turns over. Those arrive through the dirty range, which this branch stopped taking at face value — so the blinking row kept its geometry and stopped blinking, and a selection could be left unpainted. Each cached row now also carries the presentation it was built under: the selected columns on that row, the highlighted link range, whether the modifier is down, and the blink phase. The blink phase is compared only for rows that have something blinking on them — otherwise a single blinking cell would rebuild the whole screen twice a second, which is the cost this cache exists to avoid. Whether a line blinks is decided once per rebuild and kept, since only a change of contents can change it, and that moves the generation. Appearance that belongs to the view rather than to a row goes into `CacheSignature` instead: a colour revision bumped in `colorsChanged`, the anti-aliasing setting for custom glyphs, and the link highlight mode. A new palette rebuilds everything, which is what it should do. Three tests cover the three paths, and each fails without its fix: selecting a row rebuilds that row and only that row, toggling the blink phase rebuilds exactly the blinking row, and replacing a colour rebuilds every visible row. The row counters also leave `#if DEBUG` and lose their `debug` prefix — they are what the tests assert on, and a test that only compiles against a debug build is not a test of what ships. That is what kept the suite from running in release. It runs there now: release, upstream: 3.391 ms per frame, 48.0 rows rebuilt per frame release, this: 0.222 ms per frame, 2.0 rows rebuilt per frame
|
Thank you — both were real, and the first one was a bug I had shipped into the branch rather than a risk I had weighed. Pushed a second commit. Presentation-only invalidations. You are right that The blink phase is compared only for rows that actually have a blinking cell. Otherwise one blinking cell anywhere rebuilds the whole screen twice a second, which is exactly the cost the cache is there to avoid. Whether a line has one is decided once per rebuild and kept: only a change of contents can change the answer, and that moves the generation. Appearance that belongs to the view rather than to a row went into Three tests, each of which fails without its fix: selecting a row rebuilds that row and only that row; toggling the blink phase rebuilds exactly the blinking row; replacing a colour rebuilds every visible row. Release. The reason the suite could not run there was mine too: the row counters lived under Numbers from release, same test, 48 rows of 120 columns over 200 scrolled frames of coloured output:
Still worth your eyes on the two things I cannot exercise properly: double-height/double-width rows and the kitty image paths both go through the coordinate transform this moves into a uniform. |
…tream The Metal renderer rebuilds every visible row on every frame while output is coming in: the vertices are written in screen coordinates, so `yDisp` is part of the cache signature and any scroll throws the cache away. That was the most expensive thing left in this app — an agent writes at the bottom of the screen, which is exactly what scrolls it. migueldeicaza/SwiftTerm#636 builds the rows in their own coordinates and hands their place to the shader as a uniform, keys the cache by the line rather than by the row number it happens to occupy, and rebuilds a row when its appearance changes rather than only when the dirty range says so. Release builds of this app on the same bench — a pane printing 250 lines a second, sampled for 15 seconds: buildDrawData 9.4% of a core -> 2.3% whole process 18.1% -> 7.8% Pinned by revision rather than by branch: a release has to build the same way twice, and a branch moves. When the pull request is merged this goes back to `from: "1.15.0"` and the fork can be deleted.
migueldeicaza
left a comment
migueldeicaza
left a comment
There was a problem hiding this comment.
Additionally, for an existing entry, needsRebuild does not use rebuildRange. This is also unsafe for Kitty graphics: replacing an existing image ID or updating an existing virtual placement can leave image counts, next IDs, and line generation unchanged. The renderer can then reuse old textures or placement geometry. Track Kitty state with a mutation revision, or distinguish scroll-only invalidation from redraws that must rebuild cached rows.
| data: rowData, buffers: cached.buffers) | ||
| rowCache[row] = entry | ||
| rowCache[lineKey] = entry | ||
| } |
There was a problem hiding this comment.
The cache signature does not include customBlockGlyphs or the selection foreground and background colors. Their setters request a full redraw without changing BufferLine.generation. Because cached rows now ignore the dirty range, the renderer keeps stale geometry. Temporary regression tests confirmed that each change rebuilt zero rows. Add these values to the signature, use a presentation revision, or preserve non-scroll invalidation.
The cache could answer "has this line's contents changed" and nothing else, so the previous commit had it ignore the dirty range for rows it already held — and with the range went every reason to redraw that is not a line's contents. Custom block glyphs, the selection colours and a kitty image replaced under an id already on screen all ask for a full redraw without moving any generation; each of them rebuilt zero rows. The distinction the cache needs already exists in the terminal. `updateRange(_:scrolling:)` keeps a scroll out of the scroll-invariant range on purpose, `getScrollInvariantUpdateRange` exposes it, and nothing in the framework had ever called it. The Metal path reads it now, in the absolute coordinates it is already in, and the cache takes the range at face value again for every row. Ignoring it costs stale pixels that nothing corrects; consulting it costs a rebuild, and the two are not comparable mistakes. That makes `RowPresentation` unnecessary and it goes: the blink timer already dirties only the rows that blink, and link highlighting only the rows a link is on. A selection change did mark the whole visible screen — every mouse movement during a drag rebuilt every row — and it now marks the union of what is selected and what just stopped being. Kitty state is tracked by a revision the state bumps itself, replacing a stamp made of counts and next ids: replacing an image under an existing id changes none of those, which is exactly the case that went unnoticed. Five tests, each failing on the commit before this one: toggling custom block glyphs, replacing either selection colour, replacing a placed kitty image, and a benchmark for the alternate screen — where a scroll cannot be a blit, so the region is genuinely dirty and the cache does not help. That case is unchanged from upstream: 3.5 ms and 48 rows per frame against 0.2 ms and 2 rows in the normal buffer.
|
Both were real, and the fix turned out to be subtraction rather than addition. Pushed as a third commit. The distinction you asked for already exists in the terminal. That makes One genuine performance bug turned up on the way. Kitty state is tracked by a revision the state bumps itself. TestsFive more, and each of the four regression tests fails on the previous commit with One correction to the earlier tests while I was there: they called NumbersRelease, 48 rows of 120 columns, 200 scrolled frames of coloured output: The alternate screen deserves the caveat. It has no scrollback, so a scroll there cannot be a blit, and
Still unexercised by hand: double-height/double-width rows and the kitty image paths, both of which go through the coordinate transform this moves into a per-row uniform. |
2 participants
While a terminal is being written to, the Metal renderer rebuilds every visible row on every frame — shaping the text, looking each glyph up in the atlas and laying out the cells again for rows whose content never changed. On a pane printing 250 lines a second,
buildDrawDatawas 25.8% of a core; the row cache next to it was never hit.Three separate things caused it, and all three had to go before any of it helped.
The vertices were written in screen coordinates.
lineOffsetcame fromrow - yDisp, soyDisphad to be part ofCacheSignature— and any scroll invalidated the entire cache. Rows are now built in their own coordinates, with the bottom edge at zero, and where a row goes is handed to the shader as afloat2uniform per draw call.yDispis out of the signature. Conveniently, position enteredbuildRowDrawDatain exactly one place, so everything downstream became local by construction.The cache was keyed by absolute row number. A
CircularListrotates its references as the scrollback fills, so a row number stops meaning the same line the moment output scrolls, and every lookup missed. It is keyed by the line's identity now — which is what the entry was already verifying withlineRef === line.The dirty range covers the whole screen on any scroll. That is correct on its own terms: from the screen's point of view, every row does hold different text afterwards. But they are the same lines, moved, and what actually says a line needs rebuilding is its own
generation, which the cache already checks. So the range is consulted only for rows the cache has nothing for. This is the part I would most like a second opinion on — it trades the range for the per-line generation, and if there is a case where a line's rendering changes without its generation moving, that case would now be missed.Numbers
From the test this adds — 48 rows of 120 columns, 200 scrolled frames of coloured output:
Sampled in a real app for 15 seconds, with a pane printing 250 lines a second:
MetalTerminalRenderer.draw(in:)buildDrawDatabuildRowDrawDataAlso here
The aggregated buffering mode has nowhere to put a per-row uniform, so it adds the offset to the cells as it concatenates them — still only arithmetic over cells that are already shaped and already found in the atlas.
buildDrawDataand the two debug row counters become internal, so the cache can be measured from a test at all. And the shader bundle is now also looked for beside the running binary: SwiftPM puts the resource bundle next to the test bundle, and a test host has no.apparound it to look inside — without that the renderer cannot be constructed in a test.Testing
swift testis green, including four new tests inMetalRowCacheTests: a one-line scroll rebuilds at most 3 rows of 24 (it rebuilt all 24 before), an idle frame rebuilds nothing, an edited row is still rebuilt, and the benchmark above. The scroll test fails onmain, which is what it is for.Beyond that it has been run in an app for a while — panes scrolling coloured output, box drawing, braille spinners — with no artefacts noticed. I have not exercised double-height/double-width rows or the kitty image paths by hand, and those touch the same coordinate transform, so they are worth a careful look from someone who knows them.