Repository navigation
fix(studio): the timeline viewport gate gives the same verdict on the same code - #5361
Merged
Merged
Conversation
… same code Steps are timed from the frame the jump is made in, so each is a whole number of frames and the 3.5-frame budget no longer cuts through one cluster of samples. When the head still misses a budget, it is measured against its base in alternating blocks on the same runner and fails only when it misses on more steps than the base beyond chance.
Contributor
Edit accuracy: accurate 2061 (base branch 2061), smooth 1601 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…s late steps later
…ort gate comparison
miguel-heygen
marked this pull request as ready for review
October 9, 2026 21:26
somanshreddy
approved these changes
Oct 9, 2026
somanshreddy
left a comment
Contributor
There was a problem hiding this comment.
Approving at def33eac. I found no blockers. This changes only CI and test code (no shipped code), and it merges cleanly with main.
Checked:
- Whole-frame timing: each step starts at the previous step's second rAF, so samples are whole frames and none can sit right on 58.3 ms. This is stricter, not looser.
- Base comparison: it runs only when both attempts miss and every run held its non-timing checks (
shouldCompareWithBase/runChecksHeld). Long tasks, DOM, direct scroll and memory are still judged on the head alone.timingPassedre-checksrunChecksHeld, so the comparison can't rescue a run that failed other checks. - Base commit:
git cat-file -p HEADfirst parent is the merge base onpull_request(merge ref) and the queue base onmerge_group, the same asbase-file.sh. The base is built from trusted code, so no fork code runs in the base worktree.
Tests: timeline-viewport-verdict.test.mjs passes 22/22. Mutants: 5/5 caught:
- σ 3→6;
- σ 3→2;
runChecksHelddropped fromtimingPassed;- frame intervals ignored;
- the 1% floor raised to 5%.
The gate job is green at this head. No required check is red. GCP BeginFrame image contract is failing, but it isn't required and this PR doesn't touch it.
Low / follow-up:
- The allowance is wide relative to the noise you measured. Head and base both run identical app code, yet the excess over base across the 13 settled comparisons was 3–20 steps. The allowance at those counts (3σ + 1%) is about 25–46, so a head can carry roughly twice the base's over-budget steps and pass (your own 38 → 76 example). 2σ + 1% would still pass all 13 identical-code runs, and the σ 3→2 mutant fails only one test. Worth tightening once the second-block bias is explained, since a moderate regression that shows only on slow runners passes silently today.
- A base failure fails the gate even when the head would pass outright. The base server is always started, and
run_armrequireswait_for_server base && wait_for_server head. If the base server doesn't come up, or the "Build the base" step fails, the arm fails even when the head meets the budget on its first attempt. That fails closed, but it's a new source of red for a PR meant to remove flakes. Waiting for the base only inside the comparison would avoid it. - The base build always runs (install plus 4 builds, about 1–2 min a job), even though the comparison is needed only on slow runners. That's a cost note only.
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.
What
The "Studio: timeline viewport gate" gives the same verdict on the same code. The 58.3 ms p95 budget is unchanged.
Two changes to how it measures:
second rAF timestamp - performance.now(), with the start taken partway into a frame. Samples then landed at "n frames minus however long that frame's earlier callbacks ran", and on slow runners one cluster sat right on the 58.3 ms (3.5 frame) budget. Now a step is a whole number of frames (33.3, 50, 66.7 ms), so no sample can sit at the budget, and a p95 under budget means 95% of jumps show within 3 frames, which is what the number was meant to say. This is stricter than before: every sample gets longer by the part of the frame that used to be left out.base-file.shreads) and serves it beside the head. If both timing attempts miss the budget, and only then, the gate measures head and base in alternating blocks (2 rounds; each block opens its build alone in a fresh Chrome, runs the usual 3 warm-up and 5 measured runs of 63 steps, then closes it) and fails when the head misses the budget on more steps than its base, or has more steps a frame past the budget, beyond 3 standard deviations of the difference of the two counts (interactions and frame intervals each). Long tasks, DOM size, direct scroll and memory are still judged on the head alone, and the comparison only runs when those all passed.Why
Same commit, two runs, opposite verdicts. On the head of #5158:
Both runs had 315 samples per attempt (5 runs x 63 steps, after 3 warm-up runs), so the sample count was not the problem. The per-step split shows why. The tail comes from one step, the 0.25 to 0.5 jump, which is 45 of the 315 samples. In attempt 2 of both runs, that step's 4-frame samples sat between 57.8 and 58.7 ms, right on the budget, and where in the frame the timer started decided the verdict.
On current main (after #5151) the gate still flips. Across 32 finished gate jobs since #5151, the on-arm p95 ranged from 33.1 to 61.3 ms (median 48.5). 3 first attempts failed, and 2 jobs failed both attempts, on a lint fix and an esbuild bump that cannot change timeline scrolling. Runners come in speed classes. Build steps that take 6 to 7 s on the fast ones take 9 to 11 s on the slow ones, and every failure was on a slow runner. Retrying on the same runner does not help: both failing jobs failed attempt 2 as well (61.0 and 60.4 ms).
Deeper sampling or more warm-up cannot fix a verdict that depends on which machine ran the job. Comparing with the base on the same machine can, and the budget still decides on any runner where the head meets it.
How
timeline-virtualization.mjs:measureScrollInteractionsstarts each step at the previous step's second rAF timestamp, which is the frame the next jump is made in. NewmeasureAgainstBase: head, base, base, head, each block alone in a freshly launched Chrome. The original page is closed after the memory check so nothing else renders during the comparison.openStudioandassertRowVirtualizationwere pulled out of the main flow so every page goes through the same checks. Two earlier versions were dropped after measuring them. Keeping the head's original page as the head side made the second Chrome read about 2 over-budget steps per run worse on identical code. Keeping both pages open made the idle one load the runner: on a slow CI runner the off arm's p95 doubled from 83 to 166 ms.timeline-viewport-verdict.mjs:judgeAgainstBase, andshouldCompareWithBase/timingPassed, which share one rule: the comparison can only settle timing, never runs that failed long-task or DOM checks.ci.yml: a step builds the base in a worktree. Each arm serves head and base, and the base page is loaded only when the head misses a budget. The step summary prints the comparison line when it ran.What changes for the unvirtualized arm (1000 rows, 75 ms budget): with whole-frame samples its steps read 66.7 or 83.3 ms, so 75 ms sits between two frame counts. On slow runners more than 5% of its steps take 5 frames, so that arm now usually misses the budget and is settled by the comparison, which adds about 2 minutes. That is the stricter measurement telling the truth about slow runners, and raising the budget to hide it is out of scope. The virtualized arm's 58.3 ms budget sits between 3 and 4 frames the same way.
Comparison rule, exactly: for each of interaction steps and frame gaps, count head and base samples over the budget and samples a frame past it. The head fails if any head count exceeds the base count by more than
3 * sqrt(head + base) + 1% of the steps. The 1% is there because a slow stretch of a shared runner makes a run of steps late together, so steps are not independent. On the unvirtualized arm with the base at 38 of 630 steps over budget, a head passes up to 76 and fails at 77.Known residual: the second block of the comparison (always a base block) reads about 5 to 14 over-budget steps lighter than the other three, including when both sides point at the same server. Its cause is not known. It counts against the head (the cautious direction) and stays well inside the allowance in every comparison measured. An earlier version also served the base fewer projects than the head; the base now gets the head's exact project set.
Tradeoff to read: if main itself drifts over the budget on a slow runner, a PR that matches main there passes with the comparison line in the summary instead of failing. The budget still fails a head that misses it on a runner where its base meets it, and any head clearly slower than its base (beyond the allowance above).
Decision on the unvirtualized arm (made here, reasoning for the record): keep its 75 ms budget and let the comparison settle it on slow runners. The alternatives were raising the budget to 83.3 ms or more, which would hide real tail growth, or going back to the old sampling, which is what made verdicts flip. A budget a slow runner cannot meet with main's own code should not decide a PR; the base on the same runner should. Fast runners still hold the arm to 75 ms outright.
Test plan
timeline-viewport-verdict.test.mjs, 22/22): head and base missing equally, identical builds either side of the budget, head slower even where the comparison puts it under budget, same count but late steps a frame later, a slow stretch's extra steps on identical builds, frame gaps alone, the rule that the comparison only settles timing, and the exact summary line. MakingjudgeAgainstBasealways pass turns 2 red; dropping the frame-later rule lets the 200 ms case pass.