Skip to content

test: share package builds before parallel test runs - #2234

Open
cpunion wants to merge 6 commits into
xgo-dev:mainfrom
cpunion:codex/test-shared-build-parallel-run
Open

test: share package builds before parallel test runs#2234
cpunion wants to merge 6 commits into
xgo-dev:mainfrom
cpunion:codex/test-shared-build-parallel-run

Conversation

@cpunion

@cpunion cpunion commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • remove the per-package child llgo test fan-out introduced by test: parallelize llgo test packages #2193
  • restore one multi-root build.Do call so package loading, SSA construction, compilation, and linking share the union dependency graph
  • rely on the bounded package-backend scheduler already merged in build: run LLVM package backends in parallel #2182; this PR does not add a second build scheduler
  • keep test-main linking on the coordinator, collect the completed native test binaries, then run only those binaries concurrently according to -p
  • preserve buffered per-package output, package result reporting, -failfast, JSON output, and sequential execution for shared profile/fuzz output modes
  • canonicalize byte/uint8 and rune/int32 generic ABI symbols, removing an order-dependent mismatch exposed by multi-root loading

Phase ownership

multi-root load/SSA -> parallel package backends (#2182, bounded by -p) -> sequential test-main links -> parallel native test execution (this PR, bounded by -p)

The current main still fans multi-package tests out to child llgo test processes and forces each child to -p=1. Moving those roots back into one build.Do call lets the existing #2182 scheduler build their union dependency graph in parallel while compiling shared packages such as testing only once.

This PR owns test orchestration and execution scheduling. Test-main linking remains sequential because it consumes coordinator-owned whole-program state. The generic basic-alias ABI fix remains a separate commit in this PR because the single multi-root load exposes that latent symbol mismatch.

The multi-package runtime regression verifies that four test roots share one build of the common testing package and report all four test results. Focused tests cover native runner success, exit-error, start-error, concurrency bounds, -failfast, JSON output, and forced sequential execution.

Behavioral trade-off

The shared multi-root build has invocation-level failure semantics. If package loading, backend compilation, or test-main linking fails for any root, build.Do returns before runNativeTestPrograms, so tests for other successfully built roots are not run. This intentionally trades the per-package fault isolation of #2193 for one shared union build. Preserving go test ./...-style partial progress would require root-aware build errors and scheduling and is outside this PR.

Validation after rebasing onto #2182

  • go test ./internal/build -run '^(TestExtest|TestRunNativeTest|TestRunNativeTestProgramsSequential|TestRunTestPrograms.*)$' -count=1
  • go test ./cmd/internal/test ./ssa/abi -count=1
  • go test ./cl -run '^TestRunAndTestFromTestgo/tpnamed$' -count=1

The pre-#2182 timing comparison is intentionally removed because it measured a branch where package backend compilation was still serial and is no longer representative of this PR on current main.

@fennoai fennoai Bot 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.

Review: share builds before parallel runs

Reworks llgo test to build all native test binaries once inside build.Do and then run them concurrently (runNativeTestPrograms/runTestPrograms), replacing the old fork-a-child-llgo-test-per-package model. The rewrite is clean: the worker pool, buffered result channel (sized to parallelism), fail-fast/skip accounting, and edge-case guards (empty slice, parallelism==0 → GOMAXPROCS, <1 → 1, clamp to len) all check out, and the removed LLGO_TEST_PARALLEL_WORKER / go list / child-arg machinery leaves no stragglers (verified by grep). TestRunSequential is wired only for the process-wide-output flags plus -fuzz, matching the prior guard, and the new unit tests cover concurrency limiting, fail-fast, and JSON output.

A couple of inline nits on comment wording. One non-blocking design note:

  • Serial build, then parallel run. Compilation and linking in Do remain sequential; only the run phase is now parallelized. The win is deduping shared-dep builds (e.g. testing built once), but for compile-dominated suites the now-serial build phase becomes the bottleneck versus the old fully-concurrent per-package pipeline. Worth a benchmark on a multi-package suite; overlapping run-as-linked could recover throughput if the build phase dominates. Not a blocker.

No correctness, security, or regression issues found in the concurrency mechanics.

Comment thread cmd/internal/test/test.go Outdated
Comment thread internal/build/build.go Outdated
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/build/run.go 95.77% 3 Missing ⚠️
internal/build/build.go 85.71% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@cpunion

cpunion commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

The first CI run exposed an existing order-dependent generic ABI symbol issue after this PR began loading all roots together. go/types may retain either the alias spelling (byte/rune) or the underlying spelling (uint8/int32) for a cached generic instance, while LLGo previously included that spelling directly in the symbol name. This produced a weak.Pointer[byte].Value reference with a weak.Pointer[uint8].Value definition.

I pushed c2b9cda0a to canonicalize those basic aliases in generic ABI symbols, plus focused ABI/golden tests. I also added direct success/exit/start-error coverage for the native test runner in c0654d7de.

Local validation includes the exact 96-package primary macOS shard with a cold LLGo build cache (-p=4): all packages loaded/built together, all 96 links completed before test execution began, then tests ran concurrently; every package passed in 1077s. The previously failing crypto/ed25519 + unique + weak combination also passes both linking and execution. Race tests, focused ABI/golden tests, and vet pass. I will wait for both CI platforms and aggregated Codecov before evaluating the final result.

@cpunion

cpunion commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

All required checks are green on ca23d8043, including the final dual-platform Codecov aggregation (94.38% patch coverage). The exact 96-package cold-cache local run also passed, with every link completed before concurrent execution.

Latest Actions run llgo test timing versus #2193:

  • Ubuntu primary: 41:40 -> 46:17 total; critical path 22:10 -> 26:19
  • Ubuntu compatibility: 20:33 -> 20:07 total; critical path 11:24 -> 13:05
  • macOS primary shard 0: 25:05 -> 34:07

The timings confirm the intended shared-build behavior but not a wall-time improvement for compile-heavy shards: build/link remain sequential and no longer overlap test execution. Parallel build work remains outside this PR.

@fennoai please review the final changes, including the generic basic-alias ABI canonicalization and added runner coverage.

@fennoai

fennoai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review: shared build before parallel test runs

I checked out fennoai/pr-2234 (head ca23d8043) and reviewed the five PR commits (2ec8470e..ca23d804), which touch exactly the nine listed files. The test/goroot/* and setup-embed-deps entries visible in a raw main...HEAD diff come from already-merged PRs #2218/#2215 and are not part of this change.

Verdict: looks good. The refactor is well-structured, the ABI canonicalization is correct and minimal, and the new tests give strong coverage of the scheduling edges. No blocking issues found. A few observations below, all non-blocking.

Architecture — restoring the single multi-root build.Do

Collapsing the per-package llgo test fan-out (#2193) back into one build.Do call is the right call: package loading, SSA construction, and linking now share one union dependency graph, so common packages like testing are built once. build.go:601-704 cleanly separates the sequential build/link phase (appending to testPrograms) from a single post-build runNativeTestPrograms call. The CompileOnly, Target != "", profile/fuzz, and -failfast paths are all preserved. build_test.go's TestExtest now asserts testing is built exactly once via a ModuleHook counter — a good regression guard for the core claim.

Scheduler (run.go:94-148) — correct and race-clean

  • Parallelism normalization (0 → GOMAXPROCS, clamp <1 → 1, clamp to len(programs)) is right.
  • The prime-then-refill loop preserves the concurrency bound; TestRunTestProgramsLimitAndFailure verifies max concurrency never exceeds the cap under -race.
  • -failfast stops launching new binaries once a failure is seen but lets in-flight binaries finish (they can't be cancelled), and skipped = len(programs) - next counts only the never-started ones. TestRunTestProgramsFailFast confirms skipped == 2 and only one run after the first failure.
  • Per-package output is buffered into a bytes.Buffer and flushed whole in reportTestProgramResult, so JSON lines never interleave mid-record; TestRunTestProgramsJSONOutput confirms no plain-text ok line leaks into a -json run.

Behavior change worth noting (non-blocking)

In runNativeTestPrograms the run closure passes the same buffer as both stdout and stderr (return runNativeTest(program, conf, output, output)). Two consequences vs. the old streamed single-package path:

  • Test-binary stderr is now folded into stdout (the buffered package output), rather than going to the real os.Stderr. The FAIL\t<pkg> / ok \t<pkg> summary markers still route to the real streams via reportTestProgramResult, so this matches how go test merges per-package output — just calling it out since it's a visible difference from test: parallelize llgo test packages #2193's streaming.
  • With -x (PrintCommands), the command-trace line is written into that same per-package buffer instead of live stderr. This actually keeps the trace grouped with its package, which reads as an improvement.

Output is no longer streamed live during a package's run (it appears when the binary completes). That's the documented and intended trade-off of buffering for clean concurrent output.

ABI canonicalization (abi.go:245-257) — correct fix at the right layer

Canonicalizing byteuint8 and runeint32 inside typeArgString's *types.Basic case is the right chokepoint: it's the single mangling path for generic type arguments, so both spellings now produce identical ABI symbols regardless of which go/types cached the instance first. This directly removes the order-dependent mismatch that multi-root loading exposes. TestTypeArgs_CanonicalizesBasicAliases asserts the two spellings mangle identically, and the updated tpnamed/in.go golden ([0]byte[0]uint8) plus FormatsCommonKinds (byteuint8) confirm the observable output. Verified go test ./ssa/abi -run TestTypeArgs -count=1 passes locally and go vet ./ssa/abi is clean.

Minor: byte/rune are the only predeclared basic aliases in Go, so the two-case switch is complete; no other basic-kind spellings need the same treatment.

Verification

  • Ran locally: go test ./ssa/abi -run TestTypeArgs -count=1 (pass), go vet ./ssa/abi (clean).
  • Could not build internal/build or cmd/internal/test in this sandbox — they cgo-link against LLVM (llvm-c/Core.h headers unavailable here). The runner and scheduler tests, the -race runs, and the full-shard cold-cache validation reported in the PR description and your latest comment therefore rest on CI, which you note is green on ca23d8043 (94.38% patch coverage, all links completing before parallel execution).

On the timings

The reported per-shard regressions (e.g. macOS primary shard 0: 25:05 → 34:07) are consistent with the design: build/link are sequential and no longer overlap test execution, so compile-heavy shards lose the previous build/run overlap. The PR is explicit that it only parallelizes execution and defers parallel builds — nothing here contradicts that, and correctness (shared build, all results reported) is what this change is responsible for. Whether the wall-time cost on compile-heavy shards is acceptable ahead of the parallel-build follow-up is a maintainer call, not a code issue.

@cpunion
cpunion force-pushed the codex/test-shared-build-parallel-run branch from ca23d80 to 14c1bb0 Compare August 16, 2026 02:35
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

4979b567cb00 | workflow run | long-term charts

Program measurements

Platform Workload File size vs base Build vs base Run vs base
Linux cprintf 18656 B +0.0% 298.461 ms +1.6% (worse) 1.397 ms +0.9% (worse)
Linux fmtprintf 1874288 B +0.0% (worse) 2.563 s -0.8% (better) 3.588 ms -2.3% (better)
Linux println 67992 B +0.0% 305.947 ms +2.2% (worse) 1.742 ms -0.1% (better)
macOS cprintf 84624 B +0.0% 396.476 ms -7.8% (better) 4.205 ms +38.3% (worse)
macOS fmtprintf 1889984 B +0.0% (worse) 4.300 s +86.7% (worse) 22.205 ms +76.9% (worse)
macOS println 121168 B +0.0% 367.869 ms -0.9% (better) 4.355 ms +3.0% (worse)
Core language and compiler benchmarks
Platform Benchmark ns/op vs base
Linux BenchmarkLookupPCRandom 12.300 ns/op +0.1% (worse)
Linux BenchmarkMergeCompilerFlags 144.600 ns/op -0.3% (better)
Linux BenchmarkMergeLinkerFlags 93.820 ns/op -0.4% (better)
Linux BenchmarkChannelBuffered 36.300 ns/op -0.8% (better)
Linux BenchmarkChannelHandoff 23845 ns/op +1.7% (worse)
Linux BenchmarkDefer 46.110 ns/op +2.5% (worse)
Linux BenchmarkDirectCall 1.759 ns/op +0.2% (worse)
Linux BenchmarkGlobalRead 1.757 ns/op +0.1% (worse)
Linux BenchmarkGlobalWrite 2.806 ns/op +0.0%
Linux BenchmarkGoroutine 29740 ns/op -17.3% (better)
Linux BenchmarkInterfaceCall 8.679 ns/op +2.4% (worse)
Linux BenchmarkRuntimeGetG 2.111 ns/op -14.3% (better)
macOS BenchmarkLookupPCRandom 20.820 ns/op +67.4% (worse)
macOS BenchmarkMergeCompilerFlags 266.600 ns/op +105.2% (worse)
macOS BenchmarkMergeLinkerFlags 142.700 ns/op +64.6% (worse)
macOS BenchmarkChannelBuffered 24.960 ns/op -16.3% (better)
macOS BenchmarkChannelHandoff 6886 ns/op -26.5% (better)
macOS BenchmarkDefer 28.690 ns/op -36.8% (better)
macOS BenchmarkDirectCall 1.079 ns/op -9.8% (better)
macOS BenchmarkGlobalRead 1.203 ns/op +9.4% (worse)
macOS BenchmarkGlobalWrite 1.398 ns/op +27.3% (worse)
macOS BenchmarkGoroutine 30698 ns/op +6.5% (worse)
macOS BenchmarkInterfaceCall 4.785 ns/op -16.6% (better)
macOS BenchmarkRuntimeGetG 2.312 ns/op -7.6% (better)

Compared with 9a344b47dddb measured in the same runner job.

@cpunion

cpunion commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased scope update: current main now includes #2182, so package backend compilation is already bounded and parallel inside the single multi-root build.Do. This PR now explicitly owns only test orchestration: reuse that shared build, keep test-main links sequential, and run completed native test binaries concurrently under the same -p bound.

The earlier review/timing note that package compilation remained serial is therefore obsolete; the PR description and phase-boundary comment have been updated. Focused internal/build, cmd/internal/test, ssa/abi, and tpnamed tests pass on 4979b567c.

@fennoai please review the integration with the existing #2182 package scheduler at the current head.

@fennoai

fennoai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review: integration with the #2182 package scheduler

Reviewed the fixed range 9a344b47...4979b567 at head 4979b567c. The integration is sound: the PR removes the #2193 child-process fan-out and lets one multi-root build.Do feed the existing #2182 bounded package-backend scheduler, then runs only the already-linked native test binaries concurrently under the same -p bound. The two phases do not overlap (runNativeTestPrograms runs after the link loop and disposeBackendPrograms()), so sharing one -p bound sequentially across compile and run phases is correct.

What checks out

  • Shared build reuse. Moving roots into a single build.Do means the union dependency graph is built once; TestExtest now asserts testing is compiled exactly once (testingBuilds == 1) across four roots via ModuleHook. Good regression guard.
  • -p bounding. runTestPrograms clamps parallelism (0 → GOMAXPROCS, <1 → 1, > len → len) and TestRunTestProgramsLimitAndFailure proves at most 2 run concurrently. Matches the phase-ownership statement.
  • testFail handling. For native tests (Target == ""), all binaries now flow through testPrograms; nothing else sets ctx.testFail on that path, so ctx.testFail = result.failed (build.go:817) is a safe assignment, not a lost-OR. run.go:199 only fires for the emulator/device path.
  • Output ordering. Per-program bytes.Buffer flushed in the single coordinator receive loop preserves non-interleaved output; RunArgs is read-only across goroutines, so concurrent exec.Command reads are safe.
  • Sequential/failfast/JSON. testRunsMustBeSequential() preserves the prior profile/trace/fuzz forced-sequential set; failfast skip-count semantics (skipped = len - next) and JSON suppression of the plain ok line match the removed code. Covered by focused tests.
  • ABI fix. Canonicalizing byte→uint8 and rune→int32 in typeArgString removes the order-dependent symbol mismatch that single multi-root loading exposes; kept as its own commit (c4332864) with a targeted TestTypeArgs_CanonicalizesBasicAliases. Correct and minimal.
  • Dead-symbol cleanup. DefaultBuildTags, parallelWorkerEnv, runTestPackages, listTestPackages, canRunPackagesInParallel, buildParallelChildArgs are all removed with no remaining references (TestDefaultBuildTags uses the unexported defaultBuildTags, so it still compiles).

One behavior change worth a conscious decision (non-blocking)

  • Build-failure fault isolation across roots is reduced. With one build.Do, a compile or link failure in any root makes buildAllPkgs/linkMainPkg return early (build.go:695,728,731), so no test binaries run for that invocation. The removed per-package child fan-out isolated this: a build failure in one package still let the other packages' tests run (closer to go test ./..., which reports the failed package but runs the rest). This is an inherent consequence of sharing the union graph and is likely acceptable given the PR's design goals — but it is a real semantic change from current main, so it's worth confirming it's intended and, ideally, noting it in the PR description. No test currently covers the "one root fails to build, others still report" scenario.

Verification

  • go test ./ssa/abi -run TestTypeArgs -count=1pass in this sandbox.
  • internal/build and cmd/internal/test could not be compiled or tested here: the environment lacks the LLVM C headers (llvm-c/*.h) that github.com/xgo-dev/llvm needs via cgo. I relied on static review for those packages and did not independently reproduce the internal/build, cmd/internal/test, or tpnamed results the PR reports as passing on 4979b567c.

Overall: the orchestration change integrates cleanly with the #2182 scheduler, the test coverage for the new runner is thorough, and the ABI canonicalization is correct. The only item I'd surface is the reduced build-failure fault isolation across multi-root test runs — confirm that trade-off is intended.

@cpunion

cpunion commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed: the reduced per-root build-failure isolation is an intentional trade-off for this PR. The current Build pipeline and error model are invocation-wide, so retaining partial progress would require root-aware error propagation and scheduling rather than a small runner change.

I added a Behavioral trade-off section to the PR description documenting that load/compile/link failure in any root prevents all native test binaries from running, and that go test ./...-style fault isolation is outside this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant