test: parallelize llgo test packages - #2193
Conversation
There was a problem hiding this comment.
Review: parallelize llgo test packages
The change is well-scoped and the parallel scheduler in runTestPackages is correct and leak-free: the results channel is buffered to parallelism, each worker sends exactly once, a result is drained before a replacement worker starts, and fail-fast drains all in-flight workers before returning. -p parsing, atomic-on-error config updates, and listTestPackages dedup are also sound, and test coverage for the new paths is good. Security is a non-issue: both subprocess spawns use the argv-slice form of os/exec, so there is no shell/command injection surface.
A few concrete items below are worth addressing before this lands. None are blocking correctness bugs; the top two affect output readability and resource usage under load.
Key points
- Interleaved child output (
cmd/internal/test/test.go:93): parallel children write straight to the sharedos.Stdout/os.Stderr, so output from different packages interleaves (possibly mid-line, since cross-process writes aren't line-atomic).cmd/gobuffers each package's output and flushes it atomically. Consider per-package buffering, or streaming only whenparallelism == 1. -pre-forwarded to each child (cmd/internal/test/test.go:88):flagArgsretains the user's-p=Nand passes it to every child, where it flows intopackages.Config.BuildFlags(internal/build/build.go:361-365) as the child's own build/load concurrency. With up toNchildren each runninggo/packagesloading at-p=N, this can oversubscribe the host. Consider stripping-p(or passing-p=1) to children so the parent fan-out is the only package-level parallelism.
Diagnosability / fail-fast
- When fail-fast triggers, already-running children are not cancelled and skipped packages are not reported, and on a normal test failure (
*exec.ExitError) the parent prints no per-package summary — identification relies entirely on the child's ownFAIL <pkg>line surviving the interleave (compounds the first point). A brief end-of-run per-package result summary would help.
Minor
listTestPackagesappends user patterns after subcommand flags without a--terminator (cmd/internal/test/test.go:162); a pattern beginning with-is interpreted bygo listas a flag. Inserting a literal"--"before the patterns removes the ambiguity.- The
Config.BuildParallelismdoc comment (internal/build/build.go:173) reads as if the build pipeline honors-p, but the field is only consumed byllgo test. Worth clarifying scope. effectiveParallelismis computed twice (incanRunPackagesInParalleland again inrunCmd); minor duplication.- CI shard-halving in
.github/workflows/llgo.ymlassumes near-linear-pspeedup; worth validating peak RSS on the runners given the-pre-forwarding above.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Parallelism and resource bounds
The parent owns package fan-out. Each child receives -p=1, preventing N parent workers from each starting N go/packages build/load actions.
An initial macOS process-isolation prototype measured 114.77s sequential versus 63.44s at parent -p=2 on the same four packages with the LLGo cache disabled. That prototype still forwarded -p=2 to children. After review, children are deliberately limited to -p=1, so the PR CI run is the authoritative validation for final wall time and runner memory rather than reusing that 44.7% figure.
Against the exact merge-base run, the changed test matrix drops from 10 jobs to 5. Job runner time falls from 161m20s to 99m39s (-38.2%), and the run-llgo-test steps fall from 133m24s to 87m18s (-34.6%).
On macOS, two baseline shards used 30m08s of test time and 36m42s of job time. The final single shard uses 25m05s of test time and 27m55s of job time, reducing those totals by 16.8% and 23.9%. The tradeoff is critical-path latency: the longest baseline macOS test step was 18m35s, while the single shard takes 25m05s (+35.0%). Three workers are required to retain enough margin under the unchanged 30-minute job timeout; two workers timed out on a slower hosted runner.
Ubuntu keeps two shards because the Go 1.26 primary lane also runs serial c-shared/c-archive std build-mode checks. Those checks consumed about 25.5 runner-minutes across four baseline shards and are intentionally outside this change.
Validation
Coverage for
internal/goflagsis 98.9% andnormalize.gois 100%; the package scheduler is covered at 92.0%, output reporting and child argument rewriting at 100%.