Repository navigation
renderer: one render target per gesture (ghostty_surface_set_host_capacity) - #82
lawrencecchen wants to merge 4 commits into
Conversation
…ain frame, and the landing gives them back (red) cmux FRAME OWNER S4 (contract m14): a held-key resize presents one frame per moving terminal per frame, and each new size made a new IOSurface and MTLTexture per frame (FrameState.resize -> initTarget). Time Profiler on the held keys-stress, switch on: IOSurfaceCreate 7 -> 57 ms, newTextureWithDescriptor:iosurface 15 -> 36 ms, __mmap 30 -> 104 ms per round. The API and the seams, with today's behavior, and the tests (red on behavior): - ghostty.h: ghostty_surface_set_host_capacity, ghostty_surface_shown_frame_size, and host_target_allocs, host_target_bytes (peak), host_target_live_bytes at the end of ghostty_surface_render_counters_s. The counters count (Metal initTarget, Target.deinit). - Stubs: Metal.setHostCapacity does nothing; ghostty_surface_shown_frame_size returns the shown IOSurface's size (today's); Metal.fitTarget keeps today's rule (a target of exactly the frame's size); Renderer.SwapChain.trim gives nothing back. IOSurfaceLayer takes frames with their drawn size (IOSurfaceLayer.Drawn) and still shows and compares the whole IOSurface. - generic: drawFrameLocked gives the frame its target through FrameState.fit and calls SwapChain.trim; the swap chain gets the API by pointer (Metal counts its targets); SwapChain is public for the tests. Tests (`host present: `, real Metal device, real swap chain without the GPU): - 24 frames at new sizes inside the capacity: one allocation per swap-chain target for the gesture (red: 24, one per frame); each frame drawn at its host size and shown exactly (contentsRect, top-left); past the capacity, the target grows once. - the landing: clearing the capacity leaves one target, at the pane's size (red: the two spares keep their sizes); - a frame in flight keeps its target until it completes (red: never given back); - the guard: without a capacity each new size makes one target of exactly that size and nothing is given back (green in red too); - a real render pass into a capacity target draws only the top-left drawn pixels (red: the whole target); - Core Animation renders the layer (CARenderer): a capacity frame shows only its drawn pixels, top-left, unscaled, the right way up (red: the whole surface); - the off-main and embedder size rules compare drawn sizes. Test helpers: Device.initForTest (the test binary does not link Metal; its entry point is looked up at run time) and the IOSurface lock binding, which ignored IOSurfaceLock's result and did not compile once used.
…acity) A held-key resize makes one present per moving terminal per frame (cmux contract m14), and each new host size made a new IOSurface and MTLTexture per frame (FrameState.resize -> initTarget). With ghostty_surface_set_host_capacity(w, h) (macOS; 0, 0 clears) every swap-chain target keeps one IOSurface at least that large (Metal.fitTarget, targetSize): a frame at a new size that fits allocates nothing. One allocation per swap-chain target per gesture (3 per surface): a target in flight or on screen cannot be redrawn, so it cannot be one per surface. A size past the capacity grows the target it is drawn into once, to that size. Without a capacity nothing changes. - Each frame is drawn at the host size from the target's top-left corner: Target.drawn_width/drawn_height; the render pass sets viewport and scissor to it (RenderPass.begin) for a target larger than its frame. - The layer shows only those pixels: IOSurfaceLayer.showLocked sets contents and contentsRect (drawnRect; y runs up for an IOSurface, as the CARenderer test proves) in one Core Animation lock span at every present. setSurfaceSync now takes the lock in renderer mode too. - Every size rule of the present modes compares drawn sizes, never a capacity surface's: frames carry their drawn size (IOSurfaceLayer.Drawn, Metal.fresh.width/height), and the shown size is read from the layer itself (shownSizeLocked: contentsRect x the IOSurface size). - The landing: clearing the capacity arms a trim and wakes the renderer thread (draw_now). Renderer.SwapChain.trim waits, at most 8 ms, until no frame is in flight, gives back every target larger than the host size (Target.release: empty until the next frame drawn into it makes one of its size), and a frame is then drawn at the host size, so the layer stops showing a capacity surface and memory returns to the pane's size even when nothing else draws. Never in the main thread's sync draws. - ghostty_surface_shown_frame_size reports the shown frame's drawn size.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Review (independent subagent): FIX at 4a4182c I read the full diff (base BlockingF1. A trim that times out never tries again, so a terminal that draws nothing more keeps its capacity-sized spare targets.
This is the case the PR says it covers. NEXT.md:127 says "memory returns to the pane's size even when nothing else draws", and the m14 condition says "a terminal that draws nothing after the landing kept two window-sized IOSurfaces". As written, it still can. The ghostty.h text (line 1637, "else it tries again at its next draw") is accurate but does not promise a next draw. F2. The trim waits (up to 8 ms) on the renderer thread before the landing frame. Suggested fix, which closes both: do not wait.
Then the release does not depend on a later draw and never delays the landing frame. Add a test: clear while a frame is in flight, complete it, and with no other draw the targets are given back. The current test "gives back no target while a frame is in flight" calls Checked, no problem found
Review (independent subagent): FIX at 4a4182c |
…back its capacity target when it completes (red) Review of #82 (comment 6652213651), two findings in the clear/trim path: - F1: when the trim's wait for frames in flight timed out, the targets stayed for "the next draw", but the clear wakes the renderer for one draw only: a terminal that draws nothing more kept its capacity-sized targets. - F2: the trim waited up to 8 ms under the draw mutex at the start of the landing draw. Seams (no behavior yet): Metal.frameCommitted (Frame.complete, before the commit) and Metal.frameDone (Frame.bufferCompleted, after the present and before the swap-chain slot is released). The test helpers commit and complete frames through them, in commit order. Tests: - the landing trim with a frame in flight returns at once (< 4 ms) and gives back the idle targets; the frame's completion gives back its own, with no draw (red: the trim waits 8 ms and gives nothing back); - a frame of the gesture in flight at the clear, then the landing frame, then both complete, no other draw: live target bytes are the landing target at the pane's size (red: the two capacity-sized targets stay). The test that called the trim again by hand is replaced.
…ts capacity target when it completes Review of #82 (comment 6652213651): - F2: SwapChain.trim no longer waits (the 8 ms waitIdle under the draw mutex on the landing frame is gone). It gives back at once every target larger than the host size that no frame is drawing (Target.in_flight, set by Metal.frameCommitted at the commit, cleared by Metal.frameDone at the completion). - F1: a target whose frame is in flight at the landing goes back when that frame completes: Metal.frameDone, on the GPU completion thread after the present (outside the Core Animation lock) and before the swap-chain slot is released, releases it while a cleared capacity's trim is armed and it is larger than the size the next frame draws at. No later draw is needed. The trim disarms only when no frame is in flight. - The landing frame is drawn because of Metal.landing_due (once per clear), no longer because the trim gave something back. The trim and the landing also run in the main thread's sync draws (nothing waits any more).
|
Review fixes for comment 6652213651 pushed as new commits (no force-push): red |
|
Heads-up from the upstream sync (#74): I am landing #74 into |
Replaces #81: the red commit now fails on behavior (it compiles), and the landing gives the capacity-sized targets back.
Problem
cmux contract m14 says that in every frame, a moving terminal presents a frame drawn at its pane's size. So a held-key resize of 14 terminals makes about one present per moving terminal per frame (805-1022 per round; 84 with the switch off). That count is the contract, so each present must cost less.
Today each new host size runs
FrameState.resize->api.initTarget, which makes a new IOSurface and a new MTLTexture for each swap-chain frame at each new size. Time Profiler on cmux's held keys-stress, per round:Each new IOSurface set as layer contents is also a new surface for WindowServer.
The bound: 1 allocation per swap-chain target per gesture
With a capacity, each swap-chain target allocates at most once per gesture: once, to reach the capacity. With
swap_chain_count3, that is 3 per terminal per gesture, not 1. It cannot be 1 per terminal: of the 3 targets, one can be in flight (the GPU draws it, and its completion presents it) and one is on screen. Neither can be redrawn, so a second size needs a second target, and so on. The Lead's red criterion said "at most 1 surface allocation per terminal per gesture". The test asserts exactly 1 per swap-chain target (3 in total for 24 frames at new sizes; today it is 24).Design
ghostty_surface_set_host_capacity(surface, width_px, height_px)(macOS;0, 0clears; default none, so behavior is unchanged without it):Metal.fitTarget(called byFrameState.fitfromdrawFrame) keeps each target at least the capacity (Metal.targetSize). A frame at a new size that fits allocates nothing. A size past the capacity grows the target it is drawn into once, to that size (a target never shrinks while the capacity is set).Target.drawn_width/drawn_heightis the frame's size. For a target larger than its frame,RenderPass.beginsets the viewport and the scissor to it; otherwise it makes no encoder call. The projection, the uniforms and the fragment positions already use the screen (host) size with the origin at the top-left. Custom shader textures keep the frame's size.IOSurfaceLayer.showLocked, which setscontentsandcontentsRect(drawnRect) in one[CATransaction lock]span. The four present paths aresetSurfaceSync,setSurface/setSurfaceCallback,setSurfaceOffMainandsetSurfacePresented.contentsGravitystays top-left, so bounds ahead of the frame still show it at the top-left, unscaled. The y flip: for an IOSurface, acontentsRecty of 0 is the surface's bottom row (probe run 41970106668, and the CARenderer test below), so the top-left sub-rect is(0, 1 - h/H, w/W, h/H).IOSurfaceLayer.Drawn,Metal.fresh.width/height). The shown frame's size is read from the layer itself (shownSizeLocked=contentsRectx the IOSurface size, under the CA lock), so it cannot disagree with what the layer shows, and a contents change by the embedder still counts.Metal.trim_armed), marks the landing frame due (Metal.landing_due) and wakes the renderer thread (draw_now, onerenderer_draw_now). In its next draw,Renderer.SwapChain.trimnever waits: it gives back at once every target larger than the host size that no frame is drawing (Target.in_flight, set byMetal.frameCommittedat the commit and cleared byMetal.frameDoneat the completion;Target.releaseleaves the target empty until the next frame drawn into it makes one of its size). That draw then draws the landing frame at the host size. A target whose frame is in flight goes back when that frame completes:Metal.frameDoneruns on the GPU completion thread after the present (outside the Core Animation lock) and before the swap-chain slot is released, and releases the target if it is larger than the size the next frame draws at. No later draw is needed, so a terminal that never draws again (no output, a hidden cursor) keeps no capacity-sized target. The trim disarms only when no frame is in flight. A new capacity before then disarms it (the next gesture reuses the targets).ghostty_surface_render_counters_s(C and Zig, same order):host_target_allocs(new render targets, all causes),host_target_bytes(the peak bytes of live target IOSurfaces, as agreed with frame-owner-s2), andhost_target_live_bytes(live target IOSurface bytes now).Target.live_byteslowers the live count when a target is freed. The swap chain now gets the API by pointer, soinitTargetcounts on the renderer's ownMetal, not on a copy.ghostty_surface_shown_frame_sizereports the shown frame's drawn size (the IOSurface can be larger). See "cmux follow-up".Every size comparison changed, and why
Each of these compared the IOSurface's size. With a capacity target that is the capacity, not the frame. Each now compares the drawn size:
IOSurfaceLayer.changesShownSizeIOSurfaceLayer.shownSize(used byMetal.hostFrame,displayWhileEmbedderPresents, the C API)shownSizeLocked(contentsRect x surface)IOSurfaceLayer.setSurfaceOffMainhost checkIOSurfaceLayer.offMainDecisionLockedIOSurfaceLayer.embedderDrops(andembedderShowsFrameLocked)IOSurfaceLayer.setSurfaceCallbackwrong-size discardMetal.takeFresh,Metal.hasFreshfresh.width/height)Metal.presentHostSizedWaitlayer.shownSize()(drawn, locked) vs hostMetal.embedderShowsKeptFramedrawFrametarget checkFrameState.fit->Metal.targetSize(fits, with a capacity)Unchanged:
Metal.setHostDrawableSizeonly checks whether contents is null.Overlay.zigand the overlay check ingeneric.zigread z2d surfaces, not IOSurfaces.Default-path changes (no capacity, cmux with the switch off)
setSurfaceSyncnow takes[CATransaction lock]in renderer mode too. Before, it took the lock only while the embedder presents host sizes. Now it always setscontentsandcontentsRecttogether under the lock, because an off-main present reads both under it. It never commits or flushes under the lock (the lock is recursive). It runs on the main thread (display pass,ghostty_surface_draw) and in the completion block of a sync draw.contentsRect, but only when it differs (showLockedcompares first). With no capacity it stays the unit rect, so no property is written.FrameState.fitreplaces the size check indrawFramewith the same rule as before when no capacity is set.SwapChain.trimreturns at once while no trim is armed.Tests (red on behavior, then green)
All new tests are named
host present:(the workflow's existing filter). TheMetal.zigones use a real Metal device (Device.initForTest) and the renderer's real swap chain (Renderer.SwapChain,nextFrame,FrameState.fit,trim,releaseFrame), without the GPU (testDrawpresents and completes each frame).expected 3, found 24expected 3404800, found 13529600the landing trim took 9043417 ns,TestUnexpectedResult12417 ns)expected 3404800, found 14694400bg_colorpipeline, IOSurface read back)surface pixel (6, 0): alpha 255CARenderer; proves the y flip)expected 6, found 8expected 120, found 160TestUnexpectedResultCommit 1 is the red: it adds the C API declarations, the entry points,
ghostty_surface_shown_frame_size(returning today's IOSurface size), the counters (they count), the seams with today's behavior (setHostCapacitydoes nothing,fitTargetuses the exact-size rule,SwapChain.trimgives nothing back, the layer still shows and compares the whole IOSurface), and the tests. Commit 2 adds the behavior.Runs (fork CI, workflow cut to the
host present:filter; scratch branches never merge)scratch/host-capacity-red-3= red commit2460a557d+ the cut: run 41977016390:Build Summary: 96/98 steps succeeded (1 failed); 112/119 tests passed (7 failed). It compiles; the 7 failures are the behavior lines in the table. All 33 existinghost present:tests pass in it.scratch/host-capacity-green-3= fix commit4a4182c22+ the cut: run 41977364169:Build Summary: 98/98 steps succeeded; 119/119 tests passed(the 33 existing and the 8 newhost present:tests) and lib-vt46/46 steps succeeded; 97/97 tests passed.contentsRecty = 0 selects the surface's bottom rows.Review fixes (comment 6652213651): runs
scratch/host-capacity-red-4= red commit47c05f27c(test only;Metal.frameCommitted/frameDoneare no-op seams called fromFrame.zig) + the cut: run 41987264371:118/120 tests passed (2 failed). The failures:the landing trim took 9043417 ns with a frame in flight(the 8 ms wait), andexpected 3404800, found 14694400(two capacity-sized targets kept after the landing with a frame in flight).scratch/host-capacity-green-4= fix commitf867bd2bf+ the cut: run 41987511810:98/98 steps succeeded; 120/120 tests passed, lib-vt97/97. The trim took12417 ns; live bytes after the landing with a frame in flight:3404800(the landing target at the pane's size), peak19801600.Live bytes (the landing test, 1000 x 700 px pane, capacity 2000 x 700, BGRA)
host_target_bytes)What the tests do not cover:
drawFrameitself, which needs a full renderer (font grid, terminal state). Its calls toFrameState.fitandSwapChain.trimwere read, not run. Also not covered: the renderer thread wake on clear (draw_now, read only); the terminal's own cell pipelines at a capacity size (onlybg_coloris drawn); custom shaders with a capacity; and the C entry points (compile-checked only). No cmux end-to-end run.cmux follow-up
cmux reads the shown frame from
layer.contents(TerminalSurfaceView.presentedFrame, an IOSurface) and takes itswidth/heightas the frame's pixel size. With a capacity, that is the capacity. cmux must:ghostty_surface_set_host_capacity(surface, w, h)at the gesture's start for each terminal that will move, with the largest pixel size the gesture can give it. Callghostty_surface_set_host_capacity(surface, 0, 0)at the landing, before the landing size'sghostty_surface_set_size, so the landing frame is drawn in a target of its own size and the spares are given back.ghostty_surface_shown_frame_size, notpresentedFrame.width/height: inTerminalSurfaceView+MotionSample.swift(shown?.width,shown?.height) and inTerminalSurfaceCommitSizeTests(sample()'sframe.width/heightandshownPixels).presentedFrameelsewhere must show only the drawn part.TerminalFrameHold.HeldFrameView.showcopiescontents, gravity and scale from the source layer; it must also copycontentsRect.TerminalLastFrame/showLastFrame(a kept IOSurface set as another layer's contents) must carry thecontentsRectwith it.TerminalSnapshotRenderer(CIImage(ioSurface:)) must crop to the drawn rect (CIImage's origin is bottom-left:(0, H - h, w, h)). After the landing frame is drawn, the shown frame is whole again.host_target_live_bytesandhost_target_allocsin the held-key bench to confirm 3 allocations per terminal per gesture and the landing's memory.layer.contents = nil(a purged frame) needs no change.Risks
host_target_bytesreports the peak. At the landing, the idle targets go back in the landing draw and a target in flight goes back when its frame completes.Metal.frameDonenow releases a target (an IOSurface and a texture) on the GPU completion thread after a clear, outside the CA lock. It callssurfaceSizethere; for a surface that is not host-sized that reads the layer's bounds off the main thread (the renderer thread already does).Target.in_flighthands the target between the threads (release/acquire); the renderer thread never touches a target in flight.renderer_draw_now), even when nothing else changed.setSurfaceSyncnow takes[CATransaction lock]in the default mode too. It never commits under it.layer.contentsitself while the layer'scontentsRectis a sub-rect would make the read size wrong (see follow-up 3).host_target_allocscounts all causes, including the 3 1x1 placeholder targets of every new swap chain.release/cmux-sync-20261104).Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Reduces held-key resize cost on macOS by giving each swap-chain render target one allocation per gesture instead of one per frame per new size. A new
ghostty_surface_set_host_capacity(surface, w, h)keeps every swap-chain target at least the gesture's capacity, so frames at new sizes that fit allocate nothing, and0, 0clears the capacity and gives the memory back at the landing. Without the call, behavior is unchanged, including cmux with its switch off.contentsandcontentsRecttogether under the Core Animation lock at every present, with the IOSurface y flip.ghostty_surface_shown_frame_sizereports the drawn frame's size (the surface can be larger), the render counters gainhost_target_allocs,host_target_bytes(peak), andhost_target_live_bytes, and all present-mode size comparisons now use drawn sizes instead of the surface's.Migration
ghostty_surface_set_size, and read the shown size viaghostty_surface_shown_frame_sizeinstead ofpresentedFrame.width/height.layer.contentsthemselves or copy contents properties into other layers must carrycontentsRectwhile a capacity is set.Written for commit 4a4182c. Summary will update on new commits.