Repository navigation
perf(editor): pace Windows playback on a high-resolution waitable timer instead of spinning 2 ms per frame - #3068
Merged
Merged
Conversation
…er instead of spinning the last 2 ms
…ead above normal priority
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| let timed = WAITABLE_TIMER.with(|timer| { | ||
| timer.as_ref().is_some_and(|timer| { | ||
| remaining <= WindowsWaitableTimer::SPIN | ||
| || timer.sleep(remaining - WindowsWaitableTimer::SPIN) |
Contributor
There was a problem hiding this comment.
Time subtraction breaks repository rules
AGENTS.md requires saturating_sub when subtracting Duration values. Use remaining.saturating_sub(WindowsWaitableTimer::SPIN) here, and replace thread_cpu() - cpu_start in the new pacing test too. The short-circuit check protects this call from underflow, but both expressions must follow the repository requirement before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/editor/src/playback.rs
Line: 191
Comment:
**Time subtraction breaks repository rules**
`AGENTS.md` requires `saturating_sub` when subtracting `Duration` values. Use `remaining.saturating_sub(WindowsWaitableTimer::SPIN)` here, and replace `thread_cpu() - cpu_start` in the new pacing test too. The short-circuit check protects this call from underflow, but both expressions must follow the repository requirement before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
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!
Member
Author
|
@greptileai review |
Member
Author
|
@greptileai review |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Windows,
precision_sleep_sync(crates/editor/src/playback.rs:107-130on main) sleeps until 2 ms before each frame deadline and then busy-spins. The playback loop calls it once per frame (playback.rs:1498), so the playback thread burns about 5% of a core at 30 fps and 10% at 60 fps for the whole of playback, and keeps that core from clocking down.Fix
CreateWaitableTimerExW(CREATE_WAITABLE_TIMER_HIGH_RESOLUTION)timer (one per playback thread, closed when the thread exits) until 500 us before the deadline, then spin the rest. The timer fires up to about 0.5 ms late on Blacksmith (a 250 us spin left the wake p95 at 255 us late, no spin at 505 us), so 500 us is the shortest spin that keeps the wake on time.THREAD_PRIORITY_ABOVE_NORMAL. Without it, a timer wake on a busy machine waits for a core behind decode and render threads and lands up to 10 ms late. The current spin has the same problem (wake p99 7.6 ms late with WARP rendering in the same process), so this improves pacing under load on both paths. The thread uses under 2% of a core, so it cannot starve anything.thread::sleepplus 2 ms spin runs unchanged. The 1 mstimeBeginPeriodguard stays.Measurements
Blacksmith
blacksmith-8vcpu-windows-2025. Scratch workflow builds base (main) and head on the same runner and interleaves them, 5 rounds, median [min, max]. Playback-thread CPU comes fromQueryThreadCycleTime, calibrated against a busy spin on the same machine. "Wake late" is how far after the frame deadline the playback thread resumed, the time the frame is submitted to the renderer.Real playback loop (
editor-playback-benchmark --headless --resolution quarter, 1280x720 60 fps fixture, 20 s per run):Same loop on a runner where WARP rendering saturated the CPU (earlier run, head without the priority vs head with it):
Sleep alone (
precision_sleep_syncback to back for 10 s; "loaded" adds 8 busy threads on the 8 vCPU runner):On an idle machine the wake is up to about 0.1 ms later than the spin gives, which is under 1% of a 60 fps frame, and the presented frame intervals don't change. Under load, main wakes up to 25 ms late (missing frames); head's p99 stays under 0.15 ms.
Tests
playback::tests::precision_sleep_returns_at_its_deadline(all platforms): the sleep never returns before its deadline and returns immediately when the deadline has passed.playback::tests::windows_has_a_high_resolution_waitable_timer(Windows): CI takes the timer path, and the timer sleeps for at least the requested time.playback::tests::precision_sleep_pacing(Windows, ignored): prints CPU and wake percentiles at 30 and 60 fps.

Confidence Score: 5/5
The PR appears safe to merge, with the previous finding fully addressed.Summary
Windows playback uses a high-resolution timer with a shorter final spin and raises the playback thread’s priority.
Reviews (3) · Last reviewed commit: "style: use saturating_sub for Duration s..." · Reviewed by Greptile