build: run LLVM backends in parallel by package - #2182
build: run LLVM backends in parallel by package#2182zhouguangyuan0718 wants to merge 22 commits into
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❌ Patch coverage is 📢 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
No main baseline exists yet; all metrics are marked Warning
|
d49a547 to
356136d
Compare
4501ff4 to
7a66581
Compare
2e5bfea to
5d1e8e7
Compare
5d1e8e7 to
da5685a
Compare
da5685a to
1636aca
Compare
1636aca to
0d38bb4
Compare
1 participant
Summary
-pbound without dependency-level LLVM barrierscommandEnvpackage Cexport mappings in every worker Program.ofilesThe scheduler, ABI type transfer, locality replay, and C export replay are intentionally one atomic layer: #2182 is the first layer that uses independent sessions for production package builds, and enabling them without these state bridges creates known reflect, locality, and cgo regressions.
Dependency and review boundary
Depends on #2180.
The intended diff for this PR is:
8d7ec246(current build: isolate package backend sessions #2180 head)da5685a0Please review only
8d7ec246..da5685a0.Tests
go test ./internal/build -count=1-racetests for backend sessions, bounded workers, immediate backend-to-publish task order, ordered errors, worker panic recovery, runtime ABI transfer, package-C export replay, and in-memory archive success/error cleanup-p=8cgo + Plan9 build: 78 LLVM archives, no externalar, no LLVM package temporary.o, runtime output verifiedTestBuildAndCheckSymbolsFromTestdrop, and concurrent deadcode invocations under-race-p=1and-p=8git diff --check