Skip to content

test(sidebar): the rebuild check waits on turns and the list's state, not fixed sleeps (47 s to 7 s, 122 s to 30 s) - #31206

Merged
lawrencecchen merged 1 commit into
feat-cmux-nextfrom
speed-sidebar-benches
Nov 1, 2026
Merged

lawrencecchen merged 1 commit into
feat-cmux-nextfrom
speed-sidebar-benches

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

SidebarIncrementalMatchesRebuildTests.expectMatchesRebuild (also used by SidebarIncrementalMatchesRebuildGapsTests) spent most of both suites' time in two fixed waits:

  1. Three 30 ms sleeps to let the list's completions run. On a fleet mini they took 90 to 440 ms per check.
  2. for _ in 0..<200 where <differs or animates or fades> { sleep 50 ms }. A for … where loop does not stop at the first match: it sleeps only while the list differs, but it looks 200 times. Nothing on the main thread changes between two looks with no await between them, so the 199 looks after a match were waste: 100 to 130 ms per check.

Diag job b5651a8eb364 (cmux7s, 329 checks): these two waits were 37 of 45 s in the base suite and 100 of 122 s in the Gaps suite.

Change (test-only, one file):

  • The completions run in three main-thread turns (turn): a yield for queued main-actor work, a run-loop pass that does not wait, then the window's layout, display and Core Animation commit (this runs the completion blocks of changes that did not animate).
  • The wait is a while loop with the same 50 ms sleep, the same three conditions (equal to the rebuild, no layer animation, no fading row) and the same 10 s cap. It ends at the first look that matches.

Every operation, check and compared field stays the same. In diag job 183c33f81446 (cmux10s), 46 of 223 checks still had to sleep (springs and fades with motion on), and the loop still waits for them.

Owner: a590 (sidebar-owner agent, plans/cmux-next/sidebar-owner.md). No CmuxNextSidebar source changes, so no Sidebar-Look trailer.

Performance

Suite time (✔ Suite X passed after N seconds):

suite host base e96e37a head e35bab8
SidebarIncrementalMatchesRebuildTests cmux14, each job alone 47.4 s (job 51af475dc4be) 6.7 s (job 40dbb4035de2)
SidebarIncrementalMatchesRebuildGapsTests cmux14, each job alone 122.0 s (same job) 29.9 s (same job)
both base8 shards at 61ad0ed (loaded hosts) 143.8 s / 130.2 s

Other runs at the head: cmux10s 5.8 s / 29.5 s (98a24281f3fb), diag 183c33f81446 on cmux10s 29.2 s for the Gaps suite.

Under load the Gaps suite is still slow at both ends, and this PR does not fix that:

  • cmux10s with a second job on the host: the base Gaps suite hung (theListMatchesAFreshRebuild(_:) made no progress for 180s, job 80bb54a3d540).
  • cmux12s, base and head jobs at the same time: base 433.7 s (c446ecbf2264), head 357.3 s (bb45546d4a71).
  • cmux14 while another job compiled: head 297.2 s (2795363c62ff).
    I did not find where that time goes: the diag runs were on quiet hosts. The base suite (reduce motion, no NSEvent path) stays at 6.5 to 6.8 s under the same load.

Testing

cmux-ci run --class light --display --target-worker <host> --script scripts/ci/package-test-lane.sh --ref <sha> --arg=suite --arg=Packages/macOS/CmuxNext --arg=SidebarIncrementalMatchesRebuildTests,SidebarIncrementalMatchesRebuildGapsTests --env CMUX_TEST_REQUIRE_GUI=1 --env CMUX_CI_XCODE_APP=/Applications/Xcode_26.6.app: all 25 cases pass at the head.

Observed / Not run

  • Observed: both suites pass at the head on cmux14 (40dbb4035de2, 2795363c62ff), cmux12s (bb45546d4a71) and cmux10s (98a24281f3fb). Phase timings from the diag branches speed-sidebar-diag and speed-sidebar-diag3 (not for merge).
  • Not run: the full CmuxNextAppTests target.

Changelog

none


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Speeds up the sidebar rebuild test helper by waiting on main-thread turns and the list's state instead of fixed sleeps.

SidebarIncrementalMatchesRebuildTests.expectMatchesRebuild now runs the completions in three main-thread turns (a yield, a non-blocking run-loop pass, then layout, display and commit) instead of three 30 ms sleeps, and the match wait is a while loop with the same conditions and 10 s cap that stops at the first match instead of a for … where loop that kept looking 199 times. All checks, operations and compared fields are unchanged; all 25 cases pass. Suite time drops from 47 s to 7 s (SidebarIncrementalMatchesRebuildTests) and 122 s to 30 s (SidebarIncrementalMatchesRebuildGapsTests). Under load the Gaps suite is still slow, but fixing that is out of scope here.

Written for commit e35bab8. Summary will update on new commits.

View guided diff Turn on auto-fix

… not on fixed sleeps

SidebarIncrementalMatchesRebuildTests.expectMatchesRebuild let the
list's completions run with three 30 ms sleeps, then looked 200 times
for a match: a for-where loop that slept only while the list differed
but kept looking after it matched. Nothing on the main thread changes
between two looks with no await between them, so the 199 looks after a
match were waste (100 to 130 ms a check), and the sleeps took 90 to
440 ms a check on a fleet mini. Together they were 37 of the suite's
45 s and 100 of SidebarIncrementalMatchesRebuildGapsTests' 122 s (diag
job b5651a8eb364 on cmux7s, 329 checks).

The completions now run in three main-thread turns (a yield for queued
main-actor work, a run loop pass that does not wait, then the window's
layout, display and commit), and the wait is a while loop with the
same 50 ms sleeps, the same three conditions (equal to the rebuild, no
layer animation, no fading row) and the same 10 s cap. Every operation,
every check and every compared field is unchanged. Of the 329 checks
in the diag run, 5 needed the wait to sleep (motion on), and they still
get it.
@lawrencecchen lawrencecchen added the risk:low Risk tier (docs/ci/risk-tiers.md) label Nov 1, 2026
@blacksmith-sh

blacksmith-sh Bot commented Nov 1, 2026

Copy link
Copy Markdown

Found 1 test failure on Blacksmith runners:

Failure

Test View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent, Opus 5.5 claude-opus-5-5, read-only): SHIP at e35bab8

The reviewer confirmed:

  • The old loop was wasted work. for _ in 0..<200 where cond checked all 200 times with no await, so the end state is the same as with the new while loop.
  • The settled-list wait is unchanged and enough. It is the loop at SidebarIncrementalMatchesRebuildTests.swift:255. It waits for the list to equal the rebuild (including the drag, settling and landing flags, line 221), no layer animation, and empty fadingRows. The sidebar has no DispatchQueue.main.async. Its delayed work (animation completions, the spring-load task, the observation task) is covered by those states.
  • No assertion is lost.
  • Times on cmux14: 47.4 → 6.7 s and 122.0 → 29.9 s (jobs 51af475dc4be → 40dbb4035de2).

Low, not blocking, existed before this PR:

  • The end-of-motion work in SidebarRowMotion.swift:70-79 runs in a CA completion block, so the loop can stop between the end of the keys and that block. The old code had the same gap.
  • The two 50 ms sleeps before "after the folded group springs open" (Tests:331, Gaps:261) use time as correctness. Wait on model.group(folded)?.isCollapsed == false instead.

Owner a590 (sidebar).

@lawrencecchen
lawrencecchen marked this pull request as ready for review November 1, 2026 18:31
@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review e35bab8: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr31206 -f ref=speed-sidebar-benches -f platform=macos -f backend_mode=local

Shipped in:

@lawrencecchen
lawrencecchen merged commit 58046ca into feat-cmux-next Nov 1, 2026
254 of 273 checks passed
@lawrencecchen
lawrencecchen deleted the speed-sidebar-benches branch November 1, 2026 18:50

This branch was previously deployed

1 inactive deployment
ci-routing-agent — e35bab83 Deployed Nov 1, 2026 by lawrencecchen via namespace autofix #11533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:low Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant