samanyudas has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
fix(miri): pace a scrolling row at its slowest window#90
Why
Sam wants keyboard moves in Miri to feel smooth, with no desktop showing between columns while the row scrolls (miri#163). #87 removed most gaps between windows of the same app. Gaps between windows of different apps remained: 359 ms of desktop per burst of 12 presses on Sam's row.
Stacked on #87: the base is miri/163-row-gaps, and GitHub retargets this PR when #87 merges.
Root cause
Each window eased to its target on its own, open loop. Apps take a move at different speeds: Spotify at once, Helium 15 to 40 ms later, and up to a few hundred ms later right after a focus change. So a window ahead in the direction of motion ran away from a slower window behind it, and the desktop showed between them. A live trace on Sam's row: on the first press Spotify moved 72 pt left at once, while Helium behind it took the same move 26 to 45 ms later, which opened an 80 pt gap. Beeper and Helium swap roles on other presses, so the fix cannot depend on which app is slow.
What changed
src/ecs/pacing.rs:pace_rowsruns between the animation step and the frame write. While a row animates, it holds each moving window so that the window gets no closer to its target than the column behind it is to its own, as the window server shows that column. The slowest window sets the pace for every window ahead of it. A window that lags into the window ahead only overlaps it, which shows no desktop, so nothing waits on windows ahead of the slow one.- Holds only slow a window within its own step. A window never moves back, and every window still ends at its target.
- A window whose app took every move of a whole motion in time ("prompt") paces from where the engine puts it, so a row of quick apps moves together without a one-frame stagger per column.
- A window that shows no progress for
LANDING_TIMEOUT(250 ms) stops pacing others, so a hung app cannot stall the row. - Sliding photos at shared edges are held the same way, through
SlideHolds, which their ease steps respect. - The shown frames are read on a sampler thread (
ShownFrames), on a window server connection of its own (manager::shown_frames). A read at frame pace took up to 26 ms, too long for the engine frame. WithCGWindowListCreateDescriptionFromArrayon the main connection, the read waited for the engine's own pending drawing and the drawing waited for the read. That froze the engine for 500 ms at a time while a slice photo slid (seen in asampleof the engine). animate_entitiesno longer removesRepositionMarker.finish_repositionsremoves it after pacing, once the window is at its target, so a final ease snap cannot skip pacing.position_layout_windowsnow re-targets a window whose pending move points elsewhere, because pacing can hold a window exactly at its new target.
Verification
Live A/B on Sam's 27-inch row: 4 alternating rounds of 12 presses, fast-release engines on top of #162 (98e43d8) and #87. fast-final is 6564f03, and pace5 adds this branch at 8491af7.
| median per burst | fast-final | pace5 |
|---|---|---|
| desktop between different apps | 359 ms, 90 pt·s | 0 ms, 0 pt·s |
| desktop within one app | 305 ms | 93 ms |
| first / last row window starts moving after a press | 17 / 305 ms | 15 / 346 ms |
| time to place, p50 / max | 484 / 921 ms | 500 / 1171 ms |
| fast-app step, p95 / largest | 201 / 275 pt | 98 / 208 pt |
| fast-app pauses over 40 ms mid-motion | 7 | 12 |
- Cost to fast apps: Spotify and Beeper now wait for a slow Helium behind them, so they pause more often mid-motion (12 pauses per burst instead of 7). Their steps got smaller. The first window starts as fast as before, the last one 41 ms later at the median, and the slowest placement in a burst takes 250 ms longer.
- No overshoot, and every pace5 burst ended with every window back where it started.
- Every gap left on pace5 comes from a window that parks or un-parks. Those moves snap without an animation, so pacing cannot hold them. fast-final shows the same snap gaps. That is the next cause to fix for #163.
- Shared edge: a click on a tucked column's slice slides it back in 582 ms on pace5 (fast-final: 608 ms). Three Left presses bring two tucked columns back with their photos sliding on both builds.
- Before the connection fix (pace4), the 500 ms freezes delayed the row's first motion by about 130 ms per press and the slice click took 1.58 s. Those numbers are in the results file.
The recordings, frame captures and measurement files were removed for privacy.
Reproduce: on a row with Helium and a fast app (Spotify, Beeper) next to each other, press Control-Option-Right six times and Control-Option-Left six times, about 0.5 s apart. tools/abloop.sh alternates builds and tools/pacestats.py reports the gaps from the rowsample frames.
Tests:
- New in
src/tests/pacing.rs: a window ahead waits beside a slow window behind it, scrolling left and scrolling right. Both fail on the base on frame 0, with 43 pt and 145 pt of desktop. Both also check each frame through the slow app's last landing at real speed. - New in
src/tests/pacing.rs: a 6 pt move does not snap past a slow window. It fails without thefinish_repositionschange. - New in
src/tests/pacing.rs: a stuck app does not hold the row past 250 ms. - New in
src/tests/slice_photos.rs: a window leaving past the shared edge waits for a slow window behind it. It fails on the base and whenSlideHoldsis ignored. - New in
src/ecs/pacing.rs: a sampler unit test. - Changed:
test_focus_command_starts_animation_on_first_frame. Windows the engine has not yet seen take their moves in time now start one frame after the window behind them shows its move. cargo testpasses 1174 binary and 56 library tests.cargo fmt --checkandcargo clippy -- -D warningspass.cargo clippy --all-targetshas the base's 13 errors, none in changed lines.
Miri pin: miri#174.
Reviewed with sol-loop: 3 rounds with gpt-6.1-sol on high. Sol verdict: approved. Round 1 found the final ease snap skipping pacing (fixed in 02bad70, with the short-move test). Rounds 2 and 3 approved.
🤖 Generated with Claude Code
Checks
Added 5 commits
- fix(miri): end a move only once pacing lets the window reach its target02bad70
- test(miri): pace a scrolling row at its slowest window2fd3a20
- fix(miri): read shown frames on a window server connection of their own8491af7
- test(miri): hold a short move and the last landing for a slow window8ec9802
- fix(miri): pace a scrolling row at its slowest windowc1741ea
Important
Review skipped
Auto reviews are disabled on base/target branches other than the default branch.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
External image Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: defaults
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1f09ecd2-abc0-41d7-8438-ef717c7be889
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
- 🔍 Trigger review
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.