perf: queue the heaviest workspace first - #4920
Conversation
77c96bc to
daaa598
Compare
Slots drained in declared order, so a long suite could be picked up last and run alone while the others idled. Test source bytes stand in for duration; no history service is consulted and ties keep their declared position. Generated-by: Claude Code
daaa598 to
4ffc653
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
One P3 test-fixture lifecycle issue is noted inline at 4ffc653; no reproduced P0–P2 production correctness defect. The scheduler weighs test-source bytes, sorts descending with stable ties, then uses the existing bounded worker pool.
Validation: 124 scheduler/planner/workflow/Windows-harness tests pass. Controlled completion tests verify the initial heavy-workspace set, three-slot concurrency limit, one execution per workspace, continued scheduling after a failure and final error aggregation. A separate fixture using real npm/Node child processes passes and cleans its temp namespaces. Removing the sort makes two of the six new tests fail. The inline fixture defect is independently reproduced by checking cwd existence at spawn.
Test-source bytes are a heuristic, not measured execution duration: actual tasks include costs such as website builds and Python tests. I did not run same-runner full-suite A/B timing and cannot confirm end-to-end speedup. A source-only release-policy test also requires absent dist artifacts; that is not established as a PR regression. Please correct the fixture and obtain representative timing before treating the performance benefit as demonstrated. This is not an approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
The helper returned the async body's promise, so cleanup removed the tree before runWorkspace resumed and spawned into a directory that no longer existed. The fake spawn now asserts its cwd exists so this cannot pass again. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed the complete three-file diff at 26b3e47. The prior P3 fixture-lifetime finding is fixed; no new substantiated P0–P3 finding.
withWorkspaceTree now awaits the asynchronous body before removing its directory, and the fake spawn asserts that cwd exists. Reverting just that await makes the new guard fail. The production scheduler still prioritizes test-source byte weights while retaining the existing worker bound, failure aggregation and per-workspace temporary-directory cleanup.
124 focused planner/workflow/runner tests passed on this head. Fresh production probes verified ordering, bounded concurrency, refill and failure behavior, and cleanup; removing sorting fails two tests. Real npm/Node fixture processes also executed heavier-first and their temporary directories were removed. Hosted test is successful on this exact head.
Test-source size remains only a duration heuristic. I did not run a full-suite same-runner A/B benchmark, so this review does not assert a measured speedup. No production source was changed and no merge approval is given.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the change. Approving exact head 26b3e471072789a3bc9bfe4a2731c338457d4157 following the completed review and Astro-Han’s explicit acceptance of this merge batch. Current checks pass and no review threads remain unresolved.
Late scheduling of heavy workspace suites left test-runner slots idle near completion. Order the existing bounded worker queue by test-source byte weight, excluding generated output and dependencies. No test selection or concurrency-limit changes.
AI assistance: Codex performed the review and final-state verification; Astro-Han authorized approval and merge.
中文
感谢改动。基于已完成的审查和 Astro-Han 对本批次的明确认可,批准当前精确 head;检查通过,讨论已结清。此前说明的验证边界与后续事项保持不变。本次由 Codex 执行审查和状态核对,Astro-Han 授权批准与合并。
Summary
run-workspace-tests-parallel.mjsdrained its three slots in the orderpackage.jsonhappens to declare, which has nothing to do with how long eachsuite runs. A long suite declared late is picked up late, so it can end up
running alone while the other two slots sit idle. Queueing the heaviest
workspace first removes that tail.
This is Graham's LPT rule, whose worst case is bounded at
(4m - 1) / (3m)ofthe optimal makespan — 11/9 for the three slots used here. Ordering needs a
duration per workspace, and this script has none: it receives a list of
directories. Rather than pin a hand-maintained list that goes stale as packages
grow, or stand up a service that records timings, the weight is computed from
what is already on disk — bytes of test source, skipping
dist(the same suitesagain) and
node_modules(not ours to weigh). Weighing all eleven workspacestakes 18ms and stats files without reading them.
Jest's sequencer makes the same substitution for the same reason, ordering by
cached duration when it has one and by file size when it does not:
—
packages/jest-test-sequencer/src/index.ts, v29.7.0Verification
The resulting order, weights in bytes:
runtime-hosthas its own CI step and never enters this queue, which leavesruntime, desktop and storage at the head — the same three that recent CI logs
show finishing last.
The script had no test file. This adds one, covering the ordering, the stability
of ties, a single workspace never being weighed, the excluded trees, and an
unreadable workspace weighing nothing rather than failing the run. Replacing the
sort with the declared order fails two of them.
The new file is added to the planner step so it actually runs in CI.
Weighing test source rather than the
dist/**/*.test.jsthe suites actuallyexecute is a real approximation, and one file skews it:
ai-sdk-backend.test.tsis 541,107 bytes across 215 tests, mostly generated wire-format fixtures. So the
two weights were compared directly, in bytes:
Both produce the same order, so the skew does not reach the schedule. Weighing
distinstead was rejected for that reason: it buys no different order and itreads nothing when the tree has not been built.
apps/desktoprestricts itssuite to
dist/main/**, and every.test.*file under itssrcalready livesin
src/main, so nothing counted there goes unrun.Ordering real durations would be better still, which is why Jest prefers them.
But its durations come from a
perf-cachefile written by a previous run, and aCI runner starts without one — the size fallback is the path a fresh runner
takes either way, absent a step that persists the cache between runs.
What this PR does not establish
The wall-clock gain has not been measured. A fixed-duration model over recent
logs puts it at 34-51s for the wider selections, but that model assumes a
workspace takes the same time wherever it is scheduled, and this lane is three
concurrent processes on one four-vCPU runner. Reordering changes which suites
contend with which, so the model is an upper bound rather than a prediction —
starting the three heaviest together could plausibly slow each of them down.
Settling it means running the same selection under both orders on a runner of
this shape and comparing totals. Reviewers who want that number before merging
are right to ask; it is not in this PR.
A single workspace is returned unweighed, so the
--concurrency=1 --workspaces=packages/storagelane does not walk a tree to sort one entry.--concurrency=1no longer followspackage.jsonorder. The doc comment isupdated. Ordering is unconditional because a single slot takes the same total
either way, and gating on the concurrency made the behavior untestable through
the public entry point.
AI use
Select exactly one:
Tool(s) and scope: Claude Code wrote the ordering, the weight function, the new
test file and this description, and located the prior art cited above. Reviewed
by the author.
Checklist
check:release's contract tests — an earlier revision claimed thiswithout having run them, and CI caught a fixture path of the shape
packages/<name>/dist/*.jsthatrelease-cli-file-policyreserves forreal workspace modules. The fixture tree no longer uses that shape.
Does this PR entail a change in behavior?