Repository navigation
perf(rendering): cut Metal command buffers 70% and block on frame submissions - #2634
Conversation
|
@harshav167 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks @harshav167, these numbers are great and we would like to get this in. It now conflicts with main in
Your |
wgpu-core opens a HAL command buffer for every render and compute pass, plus one for the barriers it records ahead of it. Barriers are no-ops on Metal, so about half of the MTLCommandBuffers each rendered frame created and committed carried no work, and every commit is a kernel submission (gfx-rs/wgpu#5738). The vendored Metal backend now creates the MTLCommandBuffer when the first encoder starts, never commits empty ones, and signals the submission fence on the last non-empty buffer.
RendererLayers::render opened a render pass for every layer, even when consecutive layers loaded and drew into the same texture. Each boundary stores and reloads the whole attachment on Apple's tile-based GPUs, and wgpu-core turns each pass into its own native command buffer. Layers now share an open pass while they target the same texture with a load op. A pass still ends at a target switch, a clear, a mask, a blur-cache copy or a texture swap, so draw order and rendered pixels are unchanged. Text, caption and keyboard renderers take any RenderPass lifetime so they can draw into the shared pass.
…d exports The render farm's blurred-background cache now runs everywhere: while the background, blur amount and sizes are unchanged, the blurred backdrop is copied instead of re-blurred with two full-resolution gaussian passes. The process-wide opt-in flag is removed. The cached texture is released on the first frame without blur. The blur layer refreshed its strength only while blur was above zero, so after blur was set to 0 the previous strength stayed in effect and the frame still had a cache key; the strength is now refreshed every frame.
…adback waits PendingReadback::wait and the macOS PendingSurface::wait polled, yielded nine times, then slept 100 µs, which tokio rounds up to a whole millisecond, so a frame whose GPU work had finished could still wait for the next timer tick. cap-test's performance suite showed it as render times split into two bands one tick apart. Both waits now block on the frame's own submission with device.poll(WaitForSubmissionIndex) on a blocking thread. They keep the readback deadlines: each wait gets only the time left under the playback or export deadline, measured with readback_waited, and wgpu's own 60 s wait is retried until the export deadline runs out. PendingSurface::wait takes the submission index from its caller's submit. In the suite, against main: median render time -57.1%, suite CPU -7.1%. Yielding instead of sleeping, as the NV12 export readback does, removed the bands too but used 10.7% more suite CPU than main. The vendored DX12 Device::wait waited on one auto-reset event per device, so concurrent waiters on the shared device could lose a wakeup and hold wgpu-core's fence lock until the 60 s timeout. This backports gfx-rs/wgpu#8273, an event per wait, byte-identical to the hunk in CapSoftware#2541.
…ait change EXPORT-FINDINGS and PLAYBACK-FINDINGS sessions for this branch: encoder-bound exports, command buffers and render passes per frame by change, the CPU profile, the GPUI app against main, the GIF and MOV checks, and the repository suite results with the preview and readback wait change. They correct the earlier note that 4K export is GPU-bound and drop GPU figures that did not reproduce.
b1d0842 to
a1ef731
Compare
|
Thanks @richiemcilroy — rebased onto main at Export deadlines are preserved.
#2648 is untouched. It changed On the rebased head, fmt and the exact CI clippy command are clean on Two overlap corrections to the original description: #2464 currently conflicts with main itself after #2636/#2648, independent of this branch. #2541 merges cleanly with main and conflicts with this PR only in The five workflow runs for @greptileai please re-review |
|
Pushed f152c12 to fix the sync-tests build: |
…th edits change pixels on every adapter
|
@greptileai review |
richiemcilroy
left a comment
There was a problem hiding this comment.
Verified: frame-identical output vs main, CI and engine/real-capture suites green, Greptile 5/5. Thanks!
Summary
RendererLayers::renderdraws every editor preview, playback, and export frame. Before this branch, macOS frames paid for empty Metal command buffers, one render pass per layer, repeated static-background blurs, and frame waits that slept in whole timer ticks.This PR removes the empty buffers, lets consecutive same-target layers share a render pass, enables the existing blurred-background cache outside the render farm, and waits on the frame's actual submission instead of sleep-polling. Output stays identical in the checks below.
Headline results on an M4 Max:
Review route
Start with the four production changes in this order:
vendor/wgpu-hal/src/metal/{command,mod,device}.rs: lazyMTLCommandBuffercreation and the fence signal move. Thenvendor/wgpu-hal/src/dx12/device.rs: one event per fence wait.crates/rendering/src/lib.rs:LayerPasses, the layer order inRendererLayers::render, and the blur-cache key. The three layer files only relax render-pass lifetimes.crates/rendering/src/frame_pipeline.rs: session textureCOPY_DST,wait_for_submission,PendingReadback, andPendingSurface.apps/desktop-gpui/src/camera_blur.rsandcrates/rendering/examples/preview-surface-benchmark.rs: callers now pass the submission that produced the surface.Tests are concentrated in
crates/rendering/src/overlay_layers/native_tests.rs. The two findings files carry the raw benchmark tables and session history, so this description only repeats the numbers needed to judge the change.What changed
Stop committing empty Metal command buffers
wgpu-core opens one HAL command buffer for each pass and another for barriers ahead of it. Barriers do nothing in the Metal backend, so many of those buffers were created and committed empty. The backend now creates the
MTLCommandBufferwhen the first encoder starts, skips empty buffers at submit, and signals the fence on the last encoded buffer.begin_debug_markerstill creates the buffer so a debug group pushed before a pass is popped from the same buffer.This follows the direction suggested in gfx-rs/wgpu#5738. It is also the first local edit to the vendored
wgpu-hal; a comment at the patch site says to reapply it when that copy is replaced.Share one render pass across same-target layers
LayerPasseskeeps a pass open while consecutive layers load from and draw into the same texture. It closes the pass for a target switch, a clear, a mask, a blur-cache copy, an encoder-level operation, or a texture swap. Draw order does not change.Each removed boundary saves one wgpu-core command buffer and encoder. It also avoids a full-attachment store and reload on tile-based GPUs, although that part was not measurable in GPU busy time on this machine. The measured gain is CPU and driver submission work.
Sharing a pass adds one rule for layer authors: a layer that changes scissor, viewport, blend constant, or stencil state must restore it before returning. Captions and keyboard are the only current scissor users, both already reset it, and a regression test now covers the contract.
Reuse a static blurred background
The render farm already had a
BlurResultKeycache for image, wallpaper, colour, and gradient backgrounds. The editor and desktop exports now use it too, and the process-wide opt-in flag is deleted. The key covers background generation, blur amount, output size, and session texture size. Animated backgrounds never use the cache. Session textures therefore always carryCOPY_DST.The cache holds one texture the size of the renderer's output while a static background is blurred (about 33 MB for a 4K export renderer; preview-sized in the editor) and releases it on the first frame without blur. That memory buys skipping two full-resolution gaussian passes on every frame with a static blurred backdrop.
Turning the cache on in the editor also exposed a pre-existing bug, fixed here: the blur layer only refreshed its strength while blur was above zero, so setting blur to 0 on a live renderer left the previous strength in effect. Strength now refreshes every frame.
Block on the frame's submission
PendingReadback::waitand the macOS preview'sPendingSurface::waitused to poll, yield nine times, sleep 100 µs, then sleep 1 ms. tokio's timer rounds the 100 µs sleep to about 1 ms, so completion could wait for the next tick even when the GPU had finished.Both waits now use
wait_for_submission, which runsdevice.poll(WaitForSubmissionIndex)inspawn_blockingunder the existing GPU wait timeout.PendingReadbackstores the index returned by its readback submit.PendingSurface::waitnow takes the index for the encoder that wrote its surface; its in-tree callers in the editor, camera-blur preview, tests, and surface benchmark pass the index from their own submit. This follows the shape #2541 uses for readbacks.The MP4 NV12 surface waits are untouched. #2464 owns those.
Results
Unless a section says otherwise, measurements are release builds on an M4 Max running macOS 27.2. The two Studio recordings are 2542x1226 with background blur, shadow, zoom, and motion blur, and 1260x860 with captions and keyboard overlays.
Submission work
Metal System Trace, 10-second windows,
capprocess only, blur recording at 1080p:Buffer and encoder counts repeat exactly. GPU busy varies by roughly a quarter between identical runs at about 20% load, so the table gives ranges. At 4K, pass merging cuts buffers from 10.2 to 7.2 and encoders from 16.9 to 11.0, with GPU busy unchanged within noise. Render passes fall from 6 to 1 per frame with blur and from 4 to 1 without it.
Export CPU and profile
Five interleaved rounds per recording, each round compared with itself because absolute CPU drifted between rounds:
The branch used less CPU in all 10 rounds. MP4 wall time did not move on this machine. A standalone
h264_videotoolboxencode of the same frames takes as long as the 1080p and 4K exports, so VideoToolbox is the limit.A 10-second Time Profiler sample of the 1080p blur export puts numbers to the mechanism. Total sampled CPU fell from 3,589 ms to 2,937 ms (-18%). Command-buffer submission fell from 905 ms to 630 ms, other Metal/AGX/IOGPU driver work from 658 ms to 468 ms, and wgpu from 231 ms to 198 ms. VideoToolbox/CoreMedia (712 to 705 ms) and FFmpeg (247 to 228 ms) stayed flat. The changed waits are not on the MP4 NV12 path.
GIF uses the RGBA readback wait, so it was measured again with the blocking wait: 5 of 5 paired rounds used less CPU than main (median -1.5%), instructions fell 0.5%, and all ten outputs were byte-identical. GIF remains encoder-bound. Fifteen earlier rounds without the wait fix gave no defensible wall-time claim, so none is made here.
Repository performance suite
cap-test suite performance --duration 8oncap-performance-fixtures, seven interleaved triplets (main, branch without the wait change, branch with the blocking wait), all 21 runs passing:This suite is also how the wait problem showed up. Before the wait change, the branch's render p50 landed in a 3.4-3.7 ms band in 5 of 7 pairs (median +24.8%) while mean, p95, and throughput were flat. A diagnostic build using the existing NV12 spin rule in both builds removed the bands, which identified the timer tick as the cause. That spin is not shipped: it used 10.7% more suite CPU than main. The blocking wait keeps the latency result and uses 3.7% less CPU than the no-fix branch in the same session.
The blocking build's p95 still splits around 4.2 and 5.3 ms. That looks like the vendored Metal HAL's own 1 ms sleep inside
Device::wait; it has not been isolated. An event-driven HAL wait is possible follow-up work. It is not in this PR.GPUI app
apps/desktop-gpuihas its own workspace and resolves the same wgpu-core 25.0.2 and vendoredwgpu-hal. GPUI's UI path uses wgpu 29 and is separate from these changes.Debug
cap-gpuibuilds of main and this branch ran the app'sCAP_GPUI_AUTO_*harness on a copy of the blur recording:gtimeoutsat between the app andtime -l.No warnings, validation errors, or panics were logged in those runs. Release GPUI builds were not measured.
Correctness
ffmpeg -f framemd5): 5,616/5,616 on the blur recording, 2,712/2,712 on the captions/keyboard recording, and 5,616/5,616 for each blurred image, wallpaper, gradient, colour, and mid-export style-segment variant. GIF outputs after the wait change were byte-identical in all five pairs.layers_sharing_a_pass_render_like_one_pass_per_layerrequires byte-identical output from merged and split rendering across overlays, blurred backgrounds, masks, camera3d gaussian and bokeh focus blur, colour grade, device frame and notch, camera overlay, camera-only transition, and all of them together. Its one-pass-per-layer reference uses a#[cfg(test)]split switch that is compiled out of release builds. That test-only state is the main structural wart in the diff; it earns its keep by making the reference exact, but reviewers should look at it directly.scissored_overlays_leave_the_full_target_to_the_next_layer_in_their_passplaces an image after keyboard or captions in one pass and checks that it is not clipped by a stale scissor. On the WARP family only, the separate "frame changed" sanity check is skipped when that adapter leaves the frame unchanged, matching the existing notch test. The merged-versus-split equality check always runs.cached_background_blur_matches_a_fresh_blur_and_follows_editscovers blur changes, background changes, an output resize and back, and the cache releasing when blur turns off.cargo test --lib: cap-rendering 308, cap-editor 191, cap-export 42, cap-camera-effects 27. The ignored GPU tests (text-layer ordering, both animated-gradient tests) pass on the final tree, as doesexport_sequence_render_matches_editor_reference_for_real_recordingat 1080p and 4K on copies of both real recordings.cargo fmt --all --check;cargo clippy --workspace --exclude cap-desktop --all-features --locked -- -D warningson Rust 1.88.0;cap-renderingclippy with--all-targets. Inapps/desktop-gpui,cap-gpuitests pass (1,155 passed, 10 ignored) andcap-gpui-kernelspasses (30), on Rust 1.96.1 where CI uses 1.95.0.API changes and platform risk
cap_rendering::enable_blur_result_cacheis removed. The render farm's call is removed with it.&selfand anyRenderPasslifetime, instead of tying the layer and pass lifetimes together.PendingSurface::waittakes aSubmissionIndexinstead of&wgpu::Queue. Callers must pass the submission that wrote the surface. All in-tree callers were updated and searched again after the GPUI build exposed two outside the first search.PendingReadback::waitis cross-platform and reaches DX12Device::wait. The vendored DX12 wait originally shared one auto-reset event, which loses wakeups with concurrent waiters; this branch backports the per-wait event fix from Create a seperate event for each fence wait. gfx-rs/wgpu#8273, byte-identical to perf(windows): fence-waited readbacks, overlapped MF export decode, cheaper screen capture #2541's vendored file. It does not carry upstream's present-queue idle change or the shared event's removal, which sit on the surface present/resize path rather than these offscreen waits. The Windows compile is CI-only evidence: no Windows target is installed on the test machine. Linux Vulkan uses a timeline semaphore and is safe with concurrent waiters; the GLES wait takes the GL context lock, and which Linux backend ships was not verified.PendingSurfacepath. It has its own device and thread, so it has no shared-device stall exposure, but its timing change was not measured.PendingReadback::waitbecause it makes the same design change for Windows readbacks; the DX12 files are identical, so whichever lands second has a mechanical rebase.Reproduce
LIBCLANG_PATHmatters only when Homebrew LLVM is first onPATH; bindgen 0.70 mis-parses FFmpeg's headers with libclang 23.Not in this PR
MTLSharedEventlistener plus a condvar) would remove the residual p95 split caused by the HAL's own 1 ms sleep. That is a second vendored-HAL patch and is left as follow-up work.

Confidence Score: 5/5
The PR appears safe to merge; no actionable new defect was established.Summary
This PR reduces frame submission work and changes how rendered frames wait for the GPU.
No actionable new findings were established. Builds, GPU tests, and visual checks were not run during this review.
harshav167explicitly acknowledges the loss of CPU-side submit overlap on shared devices and the longer lock hold after a hung GPU. Those trade-offs are not reported again.Reviews (4) · Last reviewed commit: "test(rendering): blur a checkerboard in ..." · Reviewed by Greptile