Skip to content

perf: queue the heaviest workspace first - #4920

Merged
Astro-Han merged 2 commits into
apache:mainfrom
orangeCatDeveloper:perf/workspace-longest-first
Sep 7, 2026
Merged

perf: queue the heaviest workspace first#4920
Astro-Han merged 2 commits into
apache:mainfrom
orangeCatDeveloper:perf/workspace-longest-first

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

run-workspace-tests-parallel.mjs drained its three slots in the order
package.json happens to declare, which has nothing to do with how long each
suite 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) of
the 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 suites
again) and node_modules (not ours to weigh). Weighing all eleven workspaces
takes 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:

} else if (testA.duration != null && testB.duration != null) {
  return testA.duration < testB.duration ? 1 : -1;
} else {
  return fileSize(testA) < fileSize(testB) ? 1 : -1;

if that information is not available they are sorted based on file size since
big test files usually take longer to complete.

packages/jest-test-sequencer/src/index.ts, v29.7.0

Verification

The resulting order, weights in bytes:

 4747888 packages/runtime
 3867780 packages/runtime-host
 3383671 apps/desktop
 1836248 packages/storage
 1482070 packages/cli
  778777 packages/core
  464808 packages/ui
  245210 packages/mcp
  152898 packages/eval
  145103 packages/computer-use
    9842 website

runtime-host has its own CI step and never enters this queue, which leaves
runtime, 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.

$ node --test scripts/run-workspace-tests-parallel.test.mjs
ℹ tests 6
ℹ pass 6
ℹ fail 0

$ node --test --test-concurrency=1 scripts/ci-workflow-policy.test.mjs scripts/ci-test-plan.test.mjs
ℹ tests 76
ℹ pass 76
ℹ fail 0

The new file is added to the planner step so it actually runs in CI.

Weighing test source rather than the dist/**/*.test.js the suites actually
execute is a real approximation, and one file skews it: ai-sdk-backend.test.ts
is 541,107 bytes across 215 tests, mostly generated wire-format fixtures. So the
two weights were compared directly, in bytes:

workspace                      src      dist
packages/runtime           4747888   4981207
packages/runtime-host      3867780   3946765
apps/desktop               3239329   2955212
packages/storage           1836248   1897846
packages/cli               1482070   1456889
packages/core               778777    828156
packages/ui                 464808    483860
packages/mcp                245210    253990
packages/eval               152898    148116
packages/computer-use       145103    145849
website                          0         0

Both produce the same order, so the skew does not reach the schedule. Weighing
dist instead was rejected for that reason: it buys no different order and it
reads nothing when the tree has not been built. apps/desktop restricts its
suite to dist/main/**, and every .test.* file under its src already lives
in 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-cache file written by a previous run, and a
CI 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/storage lane does not walk a tree to sort one entry.

--concurrency=1 no longer follows package.json order. The doc comment is
updated. 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:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, and the affected suites pass locally, including
    check:release's contract tests — an earlier revision claimed this
    without having run them, and CI caught a fixture path of the shape
    packages/<name>/dist/*.js that release-cli-file-policy reserves for
    real workspace modules. The fixture tree no longer uses that shape.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the perf/workspace-longest-first branch from 77c96bc to daaa598 Compare September 6, 2026 11:33
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
@orangeCatDeveloper
orangeCatDeveloper force-pushed the perf/workspace-longest-first branch from daaa598 to 4ffc653 Compare September 6, 2026 11:36

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread scripts/run-workspace-tests-parallel.test.mjs Outdated
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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 授权批准与合并。

@Astro-Han
Astro-Han merged commit 111b571 into apache:main Sep 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants