perf(ci): give the heavier test lane the larger runner - #6080

Open

Han5991 wants to merge 1 commit into

facebook:main from

Han5991:ci/swap-test-lane-runners

Open

perf(ci): give the heavier test lane the larger runner #6080
Han5991 wants to merge 1 commit into
facebook:main from
Han5991:ci/swap-test-lane-runners

Conversation

@Han5991

@Han5991 Han5991 commented Sep 6, 2026

Copy link

Copy Markdown

Contributor

Why

test-ui and test-node hold their runners the wrong way round relative to the work they carry.

lane	runner	job	suite step
`test-ui`	`2-core-ubuntu-arm`	14m52s	834s
`test-node`	`4-core-ubuntu`	3m18s	165s

Run 34000689684 (main, success). Reproduced on 33994177354 (926s/189s) and 33980770638 (974s/194s), and on deploy runs 33980455596 and 33952374356.

The 4-core box is under the three-minute lane. The fifteen-minute lane — the one nearest the ~20 minute wall the lane comment itself describes — is on the 2-core arm box.

The number that put it there

The comment introduced with the lane split says:

node is the heavier lane — locally 570s against the ui project's 244s — so it takes the larger supported runner while ui stays on the 2-core arm box.

That comparison does not appear to be like-for-like. #6051's own validation notes record the node suite as run "with bounded workers"; the ui line carries no such note. And node is precisely the project that a bounded pool penalises — vitest.config.ts documents that it runs on the forks pool because CLI tests call process.chdir(), and that "several CLI suites spawn a fresh node bin/astryx.mjs per assertion (real process boundary)".

The CI numbers are hard to reconcile with 570s any other way: unbounded, the node lane finishes in 165s while also paying the cold ensureCoreBuilt() core build that a warm local tree skips (build-theme.data-tokens.test.mjs:117 — "ensureCoreBuilt() only checks that dist exists"). The local run had strictly less work and still took 3.5x longer.

I've rewritten the comment to state what CI measures, and to record why the local figure isn't comparable.

What this changes

Four runs-on: values, two in each workflow. deploy.yml carried the same allocation.

Nothing else moves. In particular the repo-wide guardrail steps stay on test-ui, so the "they ride the LIGHTER lane" claim is now simply corrected in place rather than acted on — they cost ~18s of an 892s job, and moving them is a separate change that isn't worth coupling to this one.

Verification

.github/scripts/ci-test-routing.test.mjs passes unchanged — 17/17. Both labels are already in KNOWN_RUNNERS (:47-52) and no assertion in the file binds a lane to a runner, which is why this needed no test edit.

The real verification is this PR's own CI run: it reports the swapped allocation directly, against the numbers above.

What this does not do

This does not close #4339's original criterion. On run 34000689684 test-ui finished 385s after build-sandbox, and the workflow ended 17s later — the test lane is still the critical path, just no longer a re-push casualty. Subdividing ui is the next lever and is deliberately not in this PR; a strategy: matrix inside the existing test-ui job passes all 17 contract cases, so it can land separately on measured numbers.

Filed because the headroom is thin: test-ui is at 14m52s against a wall its own comment puts at ~20 minutes, and the ui project went 439s → 834s in roughly three weeks.

@Han5991


 perf(ci): give the heavier test lane the larger runner

a4d11e6

`test-ui` and `test-node` hold their runners the wrong way round relative
to the work they carry. The ui lane runs ~5x the node lane on CI, but ui
sits on `2-core-ubuntu-arm` while node holds `4-core-ubuntu`.
Run 34000689684 (main, 2026年09月06日, success):
 test-ui 14m52s 2-core-ubuntu-arm (suite step 834s)
 test-node 3m18s 4-core-ubuntu (suite step 165s)
Reproduced on 33994177354 (926s/189s) and 33980770638 (974s/194s), and on
deploy runs 33980455596 and 33952374356.
The lane comment cites a local measurement in the other direction — node
570s against ui 244s. That comparison is not reproducible here. facebook#6051's
validation notes record the node suite as run "with bounded workers"
while the ui suite carries no such note, and node is the project whose
cost is per-assertion `node bin/astryx.mjs` spawns under the forks pool
(vitest.config.ts) — the work a bounded pool serializes most. Unbounded on
CI the node lane finishes in 165s while also paying the cold
`ensureCoreBuilt()` core build that a warm local tree skips, so the local
run had strictly less work and still took 3.5x longer.
deploy.yml carried the same allocation and is swapped with it.
`.github/scripts/ci-test-routing.test.mjs` passes unchanged (17/17): both
labels are already in KNOWN_RUNNERS and no assertion binds a lane to a
runner.

@Han5991

Han5991 requested review from cixzhang, imdreamrunner and josephfarina as code owners

September 6, 2026 01:49

@vercel

vercel Bot commented Sep 6, 2026 •

edited

Loading

Copy link

Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
astryx	Ready Ready	Preview	Sep 6, 2026 1:53am UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed label

Sep 6, 2026

@github-actions github-actions Bot added community needs:code-review labels

Sep 6, 2026

@vercel

vercel Bot deployed to Preview

September 6, 2026 01:53

View deployment

@Han5991

Han5991 commented Sep 6, 2026

Copy link

ばつ2 + ×ばつ4 = **2,576** core-seconds - this PR: ×ばつ4 + ×ばつ2 = **2,790** core-seconds **+8.3% core-seconds for −41.6% gate wall clock.** If runner-seconds are the binding constraint rather than latency, that trade is arguable and I'd rather it be argued explicitly than hidden. ## Follow-up this makes smaller Sharding `ui` was the obvious next lever at 834s. At 459s it is no longer urgent, and I'd rather not spend the complexity until the number asks for it again. Recording the option here so it doesn't have to be rediscovered: a `strategy: matrix` inside the existing `test-ui` job passes all 17 cases in `ci-test-routing.test.mjs` unchanged. ## Caveat n=1 on each side. The baseline is consistent with the wider sample I looked at (`test-ui` 834–974s across 33994177354, 33980770638 and others), so 459s sits well outside that band — but it is still one run against one run. " data-view-component="true"> Copy Markdown

Contributor Author

Measured

This PR's own CI run against a pull_request run on the same base. Both runs are pull_request, same suite, no test-file changes in this branch.

baseline 34000689684	this PR 34004880026	Δ
`test-ui` job	892s · `2-core-ubuntu-arm`	521s · `4-core-ubuntu`	−371s / −41.6%
`test-ui` suite step	834s	459s	−375s / −45.0%
`test-node` job	198s · `4-core-ubuntu`	353s · `2-core-ubuntu-arm`	+155s / +78.3%
`test-node` suite step	165s	305s	+140s / +84.8%
`test` gate (= slower lane)	892s	521s	−371s / −41.6%
whole CI run	951s	615s	−336s / −35.3%

Both lanes green.

What it settles

ui scales with cores; node less so. Doubling ui's cores took 45% off its suite step. Halving node's cost it 85% — real, but from a much smaller base. That ordering is the opposite of the one the lane comment encoded, and it is now measured on this CI rather than inferred.

test is no longer the critical path by any meaningful margin. On the baseline, test-ui finished 385s after build-sandbox. Here it finishes 22s after it (01:59:25Z vs 01:59:03Z). The gate and the longest build job now land together.

Headroom. test-ui goes from 14m52s to 8m41s against the ~20 minute wall the lane comment describes. That was the thing I was actually worried about — ui went 439s → 834s in about three weeks, and at 892s there were roughly five minutes left.

The cost, stated plainly

This is not free. Weighting each lane by its cores:

baseline: ×ばつ2 + ×ばつ4 = 2,576 core-seconds
this PR: ×ばつ4 + ×ばつ2 = 2,790 core-seconds

+8.3% core-seconds for −41.6% gate wall clock. If runner-seconds are the binding constraint rather than latency, that trade is arguable and I'd rather it be argued explicitly than hidden.

Follow-up this makes smaller

Sharding ui was the obvious next lever at 834s. At 459s it is no longer urgent, and I'd rather not spend the complexity until the number asks for it again. Recording the option here so it doesn't have to be rediscovered: a strategy: matrix inside the existing test-ui job passes all 17 cases in ci-test-routing.test.mjs unchanged.

Caveat

n=1 on each side. The baseline is consistent with the wider sample I looked at (test-ui 834–974s across 33994177354, 33980770638 and others), so 459s sits well outside that band — but it is still one run against one run.

github-actions Bot added a commit that referenced this pull request

Sep 6, 2026

@github-actions


 Deploy PR #6080 preview

9f2dc85

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link

Copy Markdown

Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

No component packages changed.

Accessibility Audit

Status: No accessibility violations detected.

_{Generated by PR Enrichment workflow | Storybook | Sandbox | View full report}

Labels

CLA Signed community needs:code-review

1 participant

@Han5991

Navigation Menu

Uh oh!

perf(ci): give the heavier test lane the larger runner - #6080

perf(ci): give the heavier test lane the larger runner #6080
Han5991 wants to merge 1 commit into
facebook:main from
Han5991:ci/swap-test-lane-runners

Conversation

@Han5991 Han5991 commented Sep 6, 2026

Why

The number that put it there

What this changes

Verification

What this does not do

Uh oh!

vercel Bot commented Sep 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Han5991 commented Sep 6, 2026

Measured

What it settles

The cost, stated plainly

Follow-up this makes smaller

Caveat

Uh oh!

github-actions Bot commented Sep 6, 2026

PR Analysis Report

📚 Storybook Preview

🧪 Sandbox Preview

Bundle Size Summary

Accessibility Audit

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

@Han5991 Han5991 commented Sep 6, 2026

Why

The number that put it there

What this changes

Verification

What this does not do

Uh oh!

vercel Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Han5991 commented Sep 6, 2026

Measured

What it settles

The cost, stated plainly

Follow-up this makes smaller

Caveat

Uh oh!

github-actions Bot commented Sep 6, 2026

PR Analysis Report

📚 Storybook Preview

🧪 Sandbox Preview

Bundle Size Summary

Accessibility Audit

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Sep 6, 2026 •

edited

Loading