build: prepare bounded parallel compilation#2174
Conversation
There was a problem hiding this comment.
Review: build: prepare bounded parallel compilation
This is a clean, well-documented preparatory refactor. The buildOnePackage extraction preserves the semantics of the old buildOne closure across all PkgKind cases and needRuntime/needPyInit propagation; the bounded SSA worker pool is free of deadlocks/double-builds; effectiveDependencies correctly fixes the stale-cache case for alt-package imports; and the doc comments accurately match the code (including the "ssa.Package.Build is thread-safe" claim, which holds for the pinned x/tools).
One notable finding is flagged inline. A few minor observations follow.
Minor observations
-
internal/build/build.go(buildSSAPkgs, ~L2036): the dedup keeps the firstssaBuildEntryper*ssa.Packagepointer. Entries are concatenated asaltEntries, pkgEntries, depEntries; alt entries carryfixOrder:falsewhile ordinary ones carryfixOrder:true. Alt and ordinary packages currently have disjoint pointers, so this is not reachable today, butfixSSAOrdercoverage relies on that invariant rather than on the dedup itself. Consider OR-ingfixOrderacross duplicate entries (or documenting the invariant) to keep it robust against future changes. -
internal/build/build.go(parallelism, ~L21 vsDo, ~L29):parallelism()guardsc != nil, butDodereferencesconfunconditionally, so the nil check is effectively dead. Minor inconsistency — either drop the dead guard or document thatDorequires a non-nilconf. -
internal/build/build.go(Config.Paralleldoc) vsinternal/goflags/gobuild.go(parseBuildParallel): the field doc says "Zero uses GOMAXPROCS," but-p=0is rejected as "must be a positive integer." That's fine (omitting-pleaves the zero default → GOMAXPROCS), but a one-line note that-pmust be>= 1while the unset default means GOMAXPROCS would avoid confusion. -
internal/build/build.go(buildSSAPkgs, ~L2046): thejobschannel is unbuffered, so each dispatch blocks on a worker wake-up. For the common many-tiny-packages case a small buffer (e.g.make(chan ssaBuildEntry, workers)) would cut hand-off stalls. Optional.
Scope note: the parallelism here covers only ssa.Package.Build(); the dominant frontend/LLVM compile pipeline (buildOnePackage → compilePackageModule) is still serial. This matches the stated intent ("prepare"), so measurable speedup is deferred to a follow-up — flagging only so the "parallel compilation" framing isn't read as already delivering wall-clock gains. Worth a -race run over the concurrent SSA build path to lock in the thread-safety claim.
| "gcflags", | ||
| "gccgoflags", | ||
| "ldflags", | ||
| "p", |
There was a problem hiding this comment.
Adding "p" to argumentListFlagNames has an asymmetric side effect in flag files.
This list is used both by normalizeBuildFlags (so -p 3 → -p=3, which parseBuildParallel needs) and by wholeLineValueFlag in this file, which treats the entire remainder of a flags-file line as a single value. That greedy behavior is correct for genuine argument-list flags like -ldflags, but -p takes a single integer.
Consequence: a flags-file line combining -p with other flags, e.g.
-p=4 -trimpath
is collapsed into the single token -p=4 -trimpath. strconv.Atoi("4 -trimpath") then fails with -p must be a positive integer, got "4 -trimpath", and -trimpath is silently swallowed. The equivalent command-line form works because normalizeBuildFlags processes each arg independently.
Suggestion: allow -p to be normalized without adding it to the whole-line argumentListFlagNames set — e.g. a separate list for wholeLineValueFlag, or special-case -p so it only claims the single following integer. A test covering -p combined with other flags on one flags-file line would guard this.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
-pto typed LLGo parallelism and use it for the SSA build worker poolValidation
GOCACHE=/private/tmp/llgo-pr0-gocache go test ./internal/build ./internal/goflags ./cmd/internal/flags -count=1GOCACHE=/private/tmp/llgo-pr0-gocache go test ./internal/build -run TestModuleHookReceivesMainPackageModule -count=1This is PR0 of the staged parallel-build refactor. Later PRs will stack on this branch.