Repository navigation
perf(gpui): stop the display link while a window has nothing to draw - #2725
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| + fn frame_finished(&self, busy: bool, now: Instant) { | ||
| + let Some(toggle) = &self.toggle else { | ||
| + return; | ||
| + }; | ||
| + if busy { | ||
| + self.busy_at.set(now); | ||
| + } else if now.duration_since(self.busy_at.get()) >= Self::IDLE_DELAY | ||
| + && !self.paused.replace(true) | ||
| + { | ||
| + toggle(false); |
There was a problem hiding this comment.
FramePacer behavior lacks tests
The added tests cover patch installation, not whether a window stops and resumes frame requests correctly. Add focused tests with a recording toggle for the 100 ms cutoff, busy frames resetting the delay, repeated idle ticks stopping only once, and wake() restarting only once. Without these checks, changes could leave idle windows ticking or prevent them from waking while the tests still pass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/patches/zed-idle-frames.patch
Line: 61-70
Comment:
**`FramePacer` behavior lacks tests**
The added tests cover patch installation, not whether a window stops and resumes frame requests correctly. Add focused tests with a recording toggle for the 100 ms cutoff, busy frames resetting the delay, repeated idle ticks stopping only once, and `wake()` restarting only once. Without these checks, changes could leave idle windows ticking or prevent them from waking while the tests still pass.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
There was a problem hiding this comment.
Added in 76f950a: frame_pacer_tests in the patch covers the 100 ms cutoff, a busy frame restarting the delay, repeated idle ticks stopping once, wake() restarting once and only when stopped, a woken window waiting a full delay before stopping again, and the no-toggle (non-macOS) path. Cap CI does not run gpui's own tests, so I ran them in the zed workspace: cargo test -p gpui --lib -F gpui_platform/runtime_shaders → 212 passed, including the 6 new ones.
![capy-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
|
@greptileai review |
What
An idle GPUI window on macOS still woke the main thread on every display refresh (121×/s on a 120 Hz display), because gpui keeps its
CVDisplayLinksubscription for as long as the window is visible, even when nothing is drawing.This adds
apps/desktop-gpui/patches/zed-idle-frames.patch, applied last on top of the existing gpui patch stack:gpui: aFramePacerper window. After 100 ms of frame ticks with nothing to do (window clean, no next-frame callbacks, nothing to present), it asks the platform to stop frame requests. Any invalidation (invalidate_view,set_dirty(true)/refresh) oron_next_frame/request_animation_framerestarts them. 100 ms is long enough that steady low-rate redraws (30 fps video, timers) never stop and restart the link between frames.PlatformWindow::frame_request_toggle()defaults toNone, so Windows, Linux and the test platform behave exactly as before.gpui_macos: the toggle unsubscribes the window from the display link and resubscribes it. On resume it also draws the waking frame right away on the main thread. Otherwise the restarted link only fires a refresh or two later (measured 17 ms wake→frame without this, 1.2 ms with it). While paused, occlusion, screen-change and activation handlers don't restart the link; the next invalidation does.scripts/prepare-gpui-dependency.shlists the new patch. Its tests gain a fourth fixture patch plus a case proving that a checkout prepared before a patch was added is moved onto the new stack automatically (local edits are still never overwritten).Measured (M4 Max, release, 120 Hz display, 15 s windows)
Idle main window and paused editor now both match the tray-only baseline (0.02%).
Checks:
node --test scripts/prepare-gpui-dependency.test.mjs: 8/8 pass.
The PR appears safe to merge, with the previous test-coverage concern addressed.Summary
Stops macOS frame requests after a window has nothing to draw for 100 ms, then restarts them when work arrives.
Reviews (2) · Last reviewed commit: "test(gpui): cover when the frame pacer s..." · Reviewed by Greptile