fix(miri): pace a scrolling row at its slowest window#90

merged5
miri/v0.5.2
miri/163-pacing
opened11d agobySSamanyu Das
View on GitHubTwo-way sync enabledPR last imported 10d ago

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_rows runs 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. With CGWindowListCreateDescriptionFromArray on 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 a sample of the engine).
  • animate_entities no longer removes RepositionMarker. finish_repositions removes it after pacing, once the window is at its target, so a final ease snap cannot skip pacing. position_layout_windows now 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 burstfast-finalpace5
desktop between different apps359 ms, 90 pt·s0 ms, 0 pt·s
desktop within one app305 ms93 ms
first / last row window starts moving after a press17 / 305 ms15 / 346 ms
time to place, p50 / max484 / 921 ms500 / 1171 ms
fast-app step, p95 / largest201 / 275 pt98 / 208 pt
fast-app pauses over 40 ms mid-motion712
  • 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 the finish_repositions change.
  • 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 when SlideHolds is 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 test passes 1174 binary and 56 library tests. cargo fmt --check and cargo clippy -- -D warnings pass. cargo clippy --all-targets has 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

1 skipped
All checks passed
8491af7

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
Synced from GitHub

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.