Repository navigation
perf(gpui): streaming images that reuse one texture per window - #2852
richiemcilroy wants to merge 4 commits into
Conversation
…eam, with backend tests and a CI job
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…long git paths for Windows patch tests
|
Atlas A/B micro-benchmark (scratch branch
Large allocs are Rust heap allocations of 64 KiB or more.
Notes:
|
This comment has been minimized.
This comment has been minimized.
|
The
wgpu-on-Metal on that VM is very noisy, but stream still wins at every size. Large allocs per frame drop from 2 to 0. |
…ploads instead of during paint
|
@greptileai review |
| + Some(position) => { | ||
| + let index = self.streams[position].index; | ||
| + self.streams[position] = self.new_stream(id, index, size, format, swizzle); |
There was a problem hiding this comment.
Stream resizing changes textures beneath existing sprite coordinates
Medium Severity
If a producer changes a shared stream from 64×64 to 128×128 between two paint_streaming_image calls in one draw, the first sprite retains its 64×64 tile bounds, but this replacement makes the same texture ID resolve to the 128×128 texture. The shaders divide those stored bounds by the current texture dimensions, so the first sprite stretches the top-left quarter instead of displaying the whole frame. GPUI's Window::reuse_paint can also preserve these stale bounds when another consumer updates the stream. Metal's update_stream and DirectX's update_stream reuse the index on resize as well. Keep frame dimensions consistent across a draw and invalidate cached consumers on resize, or represent stream source coordinates independently of the previous texture dimensions.
| + Err(error) => { | ||
| + eprintln!("skipping: {error}"); | ||
| + return None; |
There was a problem hiding this comment.
If adapter or device creation fails on a CI runner, stream_test returns None, and every stream test returns successfully. The new GPU job can therefore pass without checking uploads or draw ordering. Installing Mesa does not check that either requested backend works.
Add a CI-only requirement that fails when the requested device is unavailable, while keeping the intended skips for local runs.
Knowledge Base Used: Build and release automation
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/patches/zed-streaming-image.patch
Line: 2101-2103
Comment:
**GPU tests can silently skip**
If adapter or device creation fails on a CI runner, `stream_test` returns `None`, and every stream test returns successfully. The new GPU job can therefore pass without checking uploads or draw ordering. Installing Mesa does not check that either requested backend works.
Add a CI-only requirement that fails when the requested device is unavailable, while keeping the intended skips for local runs.
**Knowledge Base Used:** [Build and release automation](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/build-and-release-automation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Phase 1 of the gpui streaming-image design. This adds a new gpui patch,
zed-streaming-image.patch, plus tests and a CI job. No Cap call site is switched yet, so the app behaves exactly as before.What the patch adds
gpui::StreamingImage: aSend + Synchandle carrying a frame generation. Methods:set_frame(returns the previous buffer for reuse once nothing else holds it),copy_frame(double-buffers internally, so same-size frames don't allocate),current,clear. Strides are supported, and the formats areBgra8andRgba8. Bad frames (empty, a stride that is short or not 4-aligned, a short buffer, or overflow) returnErrand don't panic.AtlasTextureKind::Streamplus two required trait methods,PlatformAtlas::get_or_update_streamandremove_stream. They are implemented for Metal, wgpu, DirectX,TestAtlasandHeadlessAtlas.removeand the Windows unreferenced-texture cache never see them.Window::paint_streaming_image(_fitted):insert_polychrome_tilehelper withpaint_image_fitted. The helper was extracted mechanically, so crop UVs, radii, opacity and edge fade are identical.CommandBufferResources, so the lease is released when the GPU completes. There are at most 2 slots (1 for frames over 32 MB). The spare slot is freed after 120 uploads without contention, and uploads never wait on the GPU.COMET_GPU_STATSalso reports stream textures, MB and how many were created.Arcclone, no copy).before_framewrites it withwrite_texturestraight from the frame's buffer, at the same point the atlas flushes its uploads. This is a deliberate change from the design's "write during paint": on llvmpipe GL, awrite_textureissued during paint added 12-24% to draw time in the in-app A/B (perf(gpui): stream the editor preview into one texture (default on Linux) #2854). Bgra input is swizzled into a scratch buffer kept per stream, and only when the atlas format is Rgba8. Rgba input always uploads without conversion.UpdateSubresource(the stride is the row pitch). The stream is dropped on device loss, and the next paint uploads it again.AtlasTextureKindmatch was audited. Arms that can only ever see packed kinds now nameStreamasunreachable!(), and no_ =>arm covers the new kind.Quadhelper was missingfade.Tests (all skip cleanly when there is no device)
TestAtlasswapped in, a frame painted twice uploads once, a new generation uploads once more, and clearing or dropping the handle callsremove_streamexactly once.before_frame(only the newest pending frame is written), the bytes held match the atlas upload path for both atlas formats, Rgba/Bgra/stride parity, write-copy-write-copy ordering, recovery afterclear(), oversize rejection.CopyResource-into-staging ordering, Rgba/Bgra/stride parity, oversize rejection.CI
New job
GPUI patch tests(macOS, Windows WARP, Linux lavapipe + llvmpipe). It runs only whenapps/desktop-gpui/patches/**or the prepare script changes. It runs the tests above from the prepared../zed-capcheckout without--locked: the existing patch stack addswayland-clientto gpui without updating zed's lockfile, so--lockedfails.Merge order with #2776
Both PRs append a patch. The patches touch disjoint hunks and apply cleanly in either order, with identical results (verified). Whichever merges second gets a trivial textual conflict in
patch_pathsand the fixture test.The PR appears safe to merge, with non-blocking feedback about GPU tests silently skipping in CI.
Findings
Fix with agent prompt
Summary
Adds
StreamingImageand separate stream textures for Metal, wgpu, and DirectX without switching Cap callers.Reviews (1) · Last reviewed commit: "perf(gpui): write wgpu stream frames in ..." · Reviewed by Greptile