build: run LLVM package backends in parallel - #2182
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: build: run isolated LLVM backends by package
Reviewed the changes in this PR's own commit (build: run isolated LLVM backends by package) across internal/build/{build,collect,package_build,plan9asm}.go and the added tests. The isolation design (per-worker backendSession, immutable template, snapshot-before-dispose, serialized Plan9 asm path) is coherent and the level scheduling / cache-hit finalization reuse are handled carefully. The findings below are ordered by severity.
Correctness (concurrency)
- Cache-hit packages still compile on the shared
ctx.progwhile a level runs concurrently. InbuildPreflightedPackageLevel, whencanUseIsolatedBackend() && len(level) > 1, every spec in the level runsbuildPreflightedPackage(ctx, ...)on the shared coordinatorctxfrom multiple goroutines. InsideexecutePackageBuild, the isolated-backend branch is gated on!aPkg.CacheHit, so a cache-hit package falls through tobuildPkg(ctx, ...)→preparePackageModule→cl.NewPackageExWithEmbed(ctx.prog, ...)(the early return for cache hits happens after that call). Two or more cache-hit packages in the same level therefore register into the samellssa.Programconcurrently — a data race. This is the common incremental-build case (warm cache). See inline note atinternal/build/build.go. Worth confirming withgo test -race ./internal/buildon a build whose ready level contains ≥2 cache-hit packages.
Robustness
- Panics in worker goroutines are not recovered.
preparePackageModule(and other frontend paths) usecheck(err)whichpanics. On the serial path this unwound to a returned error; on the new pooled path (buildPreflightedPackageLevel,preflightPackageBuilds) a compile error panics inside a worker goroutine with norecover, crashing the process and hangingwg.Wait()instead of being aggregated intofirstErr. Consider adefer recover()in each worker that converts the panic into aresult{err: ...}.
Performance
- Each worker Program replays the entire program's syntax metadata.
newProgram()iterates the full immutablesyntaxInputsset (ParsePkgSyntax+PreCollectLinknamesover every file of every package) pluspreCollectRuntimeLinknamesfor every per-package session. Since a session is created per compiled package, this is O(packages²) frontend replay and each concurrently-live Program holds its own copy of the program-wide maps (peak memory ≈parallelism × program-metadata). Consider building this metadata once and sharing it read-only, or pooling one session per worker goroutine rather than one per package. Inline note atinternal/build/build.go. plan9asmGlobalContextMuis held across module transforms andmod.String(). Only thellvm.GlobalContext-basedTranslateSourceModuleForPkgcall needs the global lock; holding it acrossLowerLargeAggregates,TransformModule, and the (potentially expensive)mod.String()serializes that work across all workers for asm-heavy packages (runtime,syscall,internal/*). The siblingplan9asmSigsForPkgalready scopes its lock narrowly to just the translate call. Inline note atinternal/build/plan9asm.go.
Maintainability
- Unsynchronized lazy nil-init of shared
ctx.sfiles/ctx.plan9asm.plan9asmEnabled,getCachedSFiles, andcacheSFilesdoif ctx.X == nil { ctx.X = &...{} }as an unguarded write on a sharedctx, reachable from the concurrent worker path. It is currently benign becauseDo()/initializePackageBuildStatealways pre-populate these fields, but the dead nil branches imply a safety that isn't there. Recommend removing them and relying on eager init (and consolidating the threesfilesStateconstruction sites, two of which leavecachenil). - Stale comments.
preparePackageModulestill says it "remains serial because it updates Program-wide registration state" (internal/build/build.go:1825-1826) — it now runs concurrently against a per-worker Program on the isolated path.compilePackageModule's comment "so later PRs can give it a worker-local backend context" (internal/build/build.go:1873-1875) is now stale since this PR already provides that worker-local context. Please update both.
| // each package, so packageBuildPlan can schedule ready transactions under -p. | ||
| func executePackageBuild(ctx *context, spec packageBuildSpec, verbose bool) error { | ||
| aPkg := spec.pkg | ||
| if !aPkg.CacheHit && ctx.canUseIsolatedBackend() { |
There was a problem hiding this comment.
Data race on the shared ctx.prog for cache-hit packages. This gate uses the isolated backend only when !aPkg.CacheHit. A cache-hit package falls through to buildPkg(ctx, ...) (the shared ctx passed by buildPreflightedPackageLevel) → preparePackageModule → cl.NewPackageExWithEmbed(ctx.prog, ...), which runs before the cache-hit early return. When canUseIsolatedBackend() && len(level) > 1, workers run concurrently on the shared ctx, so ≥2 cache-hit packages in one level register into the same llssa.Program simultaneously — a race. Route cache-hit frontend registration through an isolated/serialized path too.
| for index := range jobs { | ||
| preflight := preflights[level[index].pkg.ID] | ||
| value, err := buildPreflightedPackage(ctx, preflight, verbose) | ||
| completed <- result{index: index, value: value, err: err} |
There was a problem hiding this comment.
No panic recovery in the worker. buildPreflightedPackage reaches check(err) (which panics) via preparePackageModule/clFile/appendExternalLinkArgs. On the old serial path this surfaced as a returned error; here a panic in a worker goroutine has no recover, so it crashes the process and wg.Wait() never returns instead of being aggregated into firstErr. Add a defer func(){ if r:=recover(); r!=nil { completed <- result{index:index, err: fmt.Errorf("%v", r)} } }() (or return errors instead of panicking on this path).
| if t.pythonPackage != nil { | ||
| prog.SetPython(t.pythonPackage) | ||
| } | ||
| for _, input := range t.syntaxInputs { |
There was a problem hiding this comment.
O(packages²) frontend replay + duplicated memory. Because a session is created per compiled package, this loop re-walks the full immutable syntaxInputs set (ParsePkgSyntax + PreCollectLinknames over every file of every package), and preCollectRuntimeLinknames runs again below, for every worker Program. Each concurrently-live Program also keeps its own copy of the program-wide linkname/type-background maps, so peak memory scales with parallelism × program-metadata. Consider building this metadata once and sharing it read-only, or pooling one session per worker goroutine instead of one per package.
| if shouldSkipDarwinDynimportTrampolineAsm(skipDarwinDynimportTrampolines, sfile, src) { | ||
| continue | ||
| } | ||
| plan9asmGlobalContextMu.Lock() |
There was a problem hiding this comment.
Lock scope is wider than the GlobalContext dependency requires. Only TranslateSourceModuleForPkg touches llvm.GlobalContext; holding plan9asmGlobalContextMu across LowerLargeAggregates, TransformModule, and mod.String() (below) serializes those across all workers for asm-heavy packages. plan9asmSigsForPkg already scopes its lock to just the translate call — matching that here would recover most of the parallelism for runtime/stdlib builds.
|
|
||
| func (ctx *context) plan9asmEnabled(pkgPath string) bool { | ||
| ctx.plan9asmOnce.Do(func() { | ||
| if ctx.plan9asm == nil { |
There was a problem hiding this comment.
Unsynchronized write to a shared ctx field. if ctx.plan9asm == nil { ctx.plan9asm = &plan9asmState{} } (and the equivalent for ctx.sfiles in getCachedSFiles/cacheSFiles) mutates the shared ctx without a lock, and this path is reachable from concurrent workers. It's currently benign only because Do()/initializePackageBuildState always pre-populate these fields — so the nil branch is dead code that implies a safety it doesn't provide. Recommend removing the lazy init and relying on eager initialization.
70052aa to
8aeb8a1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
e1dbc60 to
c175b7f
Compare
zhouguangyuan0718
changed the title
0a057c6 to
4454cde
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
d49a547 to
356136d
Compare
4501ff4 to
7a66581
Compare
|
The recursive type-conversion fix has been split into prerequisite PR #2280. This branch was reordered to use the exact #2280 commit as its first commit, followed by the package-parallel commits. The resulting source tree is byte-for-byte unchanged from the previously validated head. After #2280 merges, rebasing this PR onto main will naturally remove that prerequisite commit. |
3a58b7e to
daf714b
Compare
1730ef1 to
342e9e2
Compare
zhouguangyuan0718
changed the title
342e9e2 to
21650d7
Compare
|
@xgopilot please review the current PR head (21650d7) from scratch. Focus on concurrency safety, shared mutable state, deterministic output, LLVM context/module lifetime, cache-hit behavior, error and panic cleanup, and bounded worker scheduling. Please ignore findings that only apply to earlier revisions. |
|
An internal error occurred. Please try again later. |
21650d7 to
ea7bd9a
Compare
Review complete — parallel LLVM package backends (
|
There was a problem hiding this comment.
Review — parallel LLVM package backends (ea7bd9a)
I reviewed the requested areas against the fixed range 6670dae...ea7bd9a. Overall this is a careful, well-reasoned design: worker-local LLVM state is genuinely isolated, cross-Context boundaries are handled correctly, and the retained-Program lifetime is disposed on normal/error/panic paths. I found no blocking correctness bug. The notes below are one fragility risk worth guarding and a few smaller observations.
Note: ssa, cl, internal/build, and internal/dcepass link against LLVM, which is not installed in this review sandbox, so I could not compile or run their tests here. xtool/env builds and its tests pass. The findings below are from static review.
What holds up well
- Bounded scheduling (
runBoundedPackageJobs): fixed worker pool bounded bymin(max(1,-p), len),-pdefaulting toGOMAXPROCS. Per-job panics are recovered into errors; errors are collected by index and returned lowest-index-first, so the surfaced error is deterministic regardless of completion order. Workers write only to distinctresults[index]slots — no aliasing. - Worker-local LLVM state:
newBackendSession/NewBackendProgramgive each package a freshContext,TargetMachine,gocvt(ownstyps/cvtneed), ABI state, and C ABI transformer. SharedpackageSyntax/localities/callerTrackingare read-only after preload, and the mutating setters onpackageSyntaxare all bypassed on thePreloadedSyntaxlowering path (initFiles/importPkgearly-return) and additionally guarded by itsRWMutex. - Cross-context DCE (
dcepass.cloneType): re-interning every type into the destinationContext, opaque-pointer handling, identified-vs-literal struct re-interning, and theConstNamedStruct-only path are correct and the rationale in the comments matches the LLVM verifier's constraints.panicon unsupported constant/expr/type kinds is a reasonable fail-fast. - Deterministic output: link order derives from
allPkgs/linkedOrder, not completion order.AbiTypes()sorts by name;retainedBackendAbiTypes()concatenation order only affects which duplicate "wins" inRegisterAbiTypes, and duplicates share the same Go type identity (RuntimeName(t)), so the result is order-independent. Immediate per-package publication doesn't affect final artifact contents. - Program/Module lifetime: retained Programs are kept alive through links + DCE + strong-ABI override emission, then
LPkgcleared before anyDispose(), with disposal wired on the success path (build.go:750) and viadeferfor error/panic.executeIsolatedPackage'sownedflag correctly transfers ownership only afterretainBackendProgram.
Fragility risk worth guarding (not a live bug)
The correctness of the parallel path rests on an unstated invariant: every *ssa.Package a worker can query — and every callee package it reaches — must already be memoized before workers start. CallerTracking.base/extended are plain maps with no mutex; runtimeCallerFuncSet writes on a miss. Today Precompute(progSSA.AllPackages()) runs after all SSA packages are created (build.go:645-646), so every worker lookup is a pure read and there is no race. But nothing enforces this at the point of use — a future change that lazily creates an SSA package during lowering, or queries a package outside AllPackages(), would silently reintroduce an unsynchronized concurrent map write. Consider one of: a race-detector-friendly guard/assertion on miss during the parallel phase, or a short doc-contract at the runtimeCallerFuncSet read sites. Inline notes below.
Smaller observations
pkgSFilesreturns a "not prepared before backend execution" error when a frozen cache misses. SincepreparePackageSFilescoverspkg.Packageandpkg.AltPkgfor every task inprePackageBuilds, this should never fire in normal flow — good defensive belt. Worth confirming no backend path queriesSFilesfor a package that wasn't a build task (e.g. a dependency-only package).xtool/env.lookPathInEnvironmentdoes executable-bit resolution but no WindowsPATHEXT/.exehandling. Fine for the current host-only helper-command use; flagging only in case cross-platform host builds are in scope.
Verification I could not run here
go test ./ssa ./cl ./internal/build ./internal/dcepassand the-racescheduling/retained-Program/cross-context-DCE tests (LLVM absent). The PR description lists these as passing; I recommend CI confirm the-racesuite specifically exercises thep=8path with packages whose callee tracking spans multiple packages.
| // Precompute before workers start and then share the plain maps read-only. | ||
| type CallerTracking struct { | ||
| base map[*ssa.Package]map[*ssa.Function]bool | ||
| extended map[*ssa.Package]map[*ssa.Function]bool |
There was a problem hiding this comment.
base/extended are unsynchronized plain maps, and runtimeCallerFuncSet/runtimeCallerBaseSet write into them on a miss (lines 972, 1037). The parallel path is safe only because Precompute(progSSA.AllPackages()) fully populates both maps before any worker runs, making every worker lookup a pure read.
This invariant is load-bearing but unenforced. If a future change lazily creates an *ssa.Package during lowering, or a worker queries a package/callee not in AllPackages() at precompute time, this becomes an unsynchronized concurrent map write (data race + possible corruption) that the -race suite may not catch unless it hits that exact path.
Suggestion: add a guard so a miss during the parallel phase is loud rather than silent — e.g. a flag set after Precompute that makes runtimeCallerFuncSet panic on a miss instead of writing, or at minimum a doc-contract at the read sites (compile.go:2281-2282, instr.go:894/904) stating that all lookups must be precomputed.
There was a problem hiding this comment.
Addressed in fa52bf7: CallerTracking is sealed after Precompute, and any later base/extended miss now panics instead of mutating the shared maps. Added focused tests for both miss paths and ran them under -race.
| commands: ctx.commands, | ||
| frontendOptions: ctx.frontendOptions, | ||
| cTransformer: session.transformer, | ||
| sfilesCache: ctx.sfilesCache, |
There was a problem hiding this comment.
Backend contexts share the coordinator's sfilesCache map by reference with sfilesFrozen: true. This is safe because preparePackageSFiles pre-populates every task's pkg.Package/pkg.AltPkg keys during prePackageBuilds, so worker reads hit the early if v, ok := ctx.sfilesCache[pkg.ID]; ok return before any write path. Worth a one-line comment here making the "pre-populated + read-only under freeze" contract explicit, since a concurrent write into this shared map (e.g. a backend querying an unprepared package) would be an unguarded data race rather than the intended sfilesFrozen error.
There was a problem hiding this comment.
Documented the pre-populated/read-only contract in fa52bf7. A frozen miss returns before the write path, so worker contexts cannot mutate the shared map.
47ddcce to
276f715
Compare
276f715 to
1273b8a
Compare
2 participants
Stack
Summary
-pcontrols the bound and the default isGOMAXPROCSThere is no PackageSummary dependency in this implementation. A future summary can reduce retained peak memory, but it is not required to enable package-level LLVM parallelism safely.
Validation