build: publish package archives concurrently - #2245
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: concurrent package archive publishing
Solid, carefully-reasoned change. The scheduler in package_pipeline.go centralizes all cross-goroutine mutation in the single coordinator loop (workers only communicate via the events channel), which is the right design and avoids the races a naive worker pool would introduce. Isolated backends get their own LLVM session and a fresh plan9asmSigs map; error/warning reporting is deterministic. Nice.
Findings below are one high-severity concurrency risk plus a few performance/maintainability items. Details are inline; a couple of cross-cutting notes here:
Concurrency (high). See the inline comment on internal/build/plan9asm.go — the reworked pkgSFiles writes into the shared, unsynchronized ctx.sfilesCache on its fast-path branches before the sfilesFrozen guard. During the concurrent backend phase the map is shared by reference across all backend tasks (package_build.go:354), so a cache miss on that path is an unsynchronized concurrent map write → potential fatal error: concurrent map writes. Safety currently rests entirely on preparePackageSFiles pre-populating every package during serial preflight; that invariant is fragile because preparePackageSFiles only covers pkg.Package/pkg.AltPkg.Package, while pkgSFiles is also reached via plan9asmSigsForPkg/compilePkgSFiles for other packages during the concurrent phase. Recommend moving the sfilesFrozen check to the top of pkgSFiles so it is strictly read-only when frozen, or guarding the map with a mutex. A -race build of a repo containing a package with no .s files that isn't pre-populated would exercise this.
Performance (scheduler is O(N²) in package count). Two inline items share one root cause: completed tasks are never pruned from the tasks slice, so both the loop-condition allBackendsDone() and nextBackend()'s isolated scan re-walk every task on every iteration. For the large repos the trace code explicitly anticipates, scheduler overhead grows quadratically. Both are cheap to fix with the counter/frontier pattern the code already uses for ssaDone/normalRemaining/advanceSerial.
Minor / non-blocking (not inlined):
internal/build/package_pipeline.go:189-209nextBackend: when the serial head is apatchedcoordinator and SSA is done, returningnilwhileactive != 0also stalls unrelated isolated work until the in-flight set drains to zero. Intentional exclusivity barrier, but wider than necessary and undocumented — a clarifying comment (or narrowing the barrier) would help.internal/build/package_pipeline.go:372packagePublishFuncreturns two bareerrorvalues(result, error, error); a doc comment noting the(result, warning, err)order would prevent caller confusion.internal/build/xtool/env/env.golookPathInEnvironment: themode & 0o111executable test is Unix-only and won't find.exe/.baton a custom Windows PATH, silently falling back to process-global PATH. If Windows is in scope, mirrorexec.LookPath; otherwise a comment on the assumption would help.
No blocking issues beyond the concurrent-map risk, which is worth confirming under -race before merge.
| } | ||
| if v, ok := ctx.sfilesCache[pkg.ID]; ok { | ||
| return v, nil | ||
| if ctx.sfilesFrozen { |
There was a problem hiding this comment.
Potential fatal error: concurrent map writes (high). The fast-path branches above (lines 415-424) write ctx.sfilesCache[pkg.ID] = nil before this sfilesFrozen guard. During the concurrent backend phase sfilesCache is shared by reference across all backend tasks (package_build.go:354) with no mutex, and pkgSFiles is reachable from multiple goroutines via compilePkgSFiles/plan9asmSigsForPkg. A cache miss that lands on the "no .s files" fast path is then an unsynchronized concurrent map write, which Go turns into an unrecoverable process crash.
The only thing preventing this today is that preparePackageSFiles pre-populates the cache during serial preflight — but it only covers pkg.Package and pkg.AltPkg.Package, so any package queried via plan9asmSigsForPkg during the concurrent phase that isn't pre-populated will hit the unguarded write.
Suggest moving the sfilesFrozen check to the top of pkgSFiles (return the not-prepared error before any map write, making the frozen path strictly read-only), or guarding sfilesCache with a mutex. Please verify under a -race build.
| return true | ||
| } | ||
|
|
||
| for ssaDone != len(ssaNodes) || !allBackendsDone() || active != 0 { |
There was a problem hiding this comment.
allBackendsDone() linearly rescans the entire tasks slice, and it is evaluated in this loop condition on every iteration (plus again at line 308). The loop runs ~once per completed event (O(N) for N packages), so this is O(N²) task.done checks over a full build, on top of the nextBackend() scan below. Completed tasks are never removed from tasks, so they're re-walked every time.
Consider maintaining a running counter of remaining (!done) backend tasks, decremented in the event.task branch — the same pattern already used for ssaDone and normalRemaining. This matters for the large repos the trace code (line ~274) explicitly anticipates.
| return task | ||
| } | ||
| } | ||
| for _, task := range tasks { |
There was a problem hiding this comment.
Same O(N²) root cause: nextBackend() scans the full tasks slice for a ready isolated task on every call, ready() additionally iterates each task's ssaDeps, and this is called up to parallelism times per outer iteration. Already-done/running tasks are rescanned every time.
A ready/pending worklist (or a frontier index over isolated tasks, like advanceSerial does for serialOrder) would avoid re-walking finished work.
| if serial { | ||
| return executePackageBuild(ctx, preflight.spec, verbose) | ||
| } | ||
| return ctx.executeIsolatedPackage(preflight.spec, verbose) |
There was a problem hiding this comment.
executePreflightedPackage is only invoked for the patched partition produced by partitionPackageExecutions, yet it re-invokes ctx.packageRequiresCoordinator(preflight.spec) and re-branches on serial — re-deriving a classification the partitioning already established (and potentially a second packageUsesPlan9Asm filesystem probe). Consider threading the known class through instead of recomputing it here.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
No main baseline exists yet; all metrics are marked Warning
|
023baa3 to
35b4170
Compare
|
Superseded by the restacked package worker design. Archive publication is now part of each backend worker in #2182 ( |
1 participant
Stacked on #2243. Review range:
25c203a2..35b41702.Summary
MemoryBuffermembers directly; no LLVM package object is written as a temporary.o.aonlymin(-p, 2)workers after all LLVM backends succeeddsymutilllgo.publishworker spans in build tracesRelationship to the archive change
The old publication benchmark was dominated by external
arrereading temporary LLVM objects. That path no longer exists, so those absolute timings and the previous backend/publish-overlap conclusion are not used as evidence for this rebased PR. Pipeline overlap can be reevaluated later; it is not required for the in-memory archive optimization.Validation
go test ./internal/build -count=1go test -race ./internal/build -run 'Test(NormalizeToArchive|PublishPackagePipelineTasks|CleanupTemporaryArchives)' -count=1\n- direct-cache test uses a real LLVM bitcode MemoryBuffer\n- clean-a -p=8cgo + Plan9 build: 78 LLVM archives, 0 externalar, 0 temporary package.a, 0 temporary LLVM package.o; runtime output verified\n-git diff --check