cl: precompute caller tracking for backend workers#2181
cl: precompute caller tracking for backend workers#2181zhouguangyuan0718 wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Review: bounded parallel compilation
Solid, well-staged refactor. The stage split (register → preflight → execute → finalize), the immutable packageBuildSpec/PackageSummary types, the dependency-level DAG, and the CallerTracking freeze are clean and well-tested. Shared *context maps are consistently guarded (builtMu, cacheManagerMu, llvmVersionMu, sfilesMu), the PackageSummary cache round-trip is faithful, and dropping the data.Metadata = nil shortcut in saveToCache is safe because (*PackageSummary).metadata() has an explicit nil guard and finalizePackageBuild always sets a non-nil Summary first.
A few items worth addressing or confirming before this lands. One inline finding (the panic in a worker goroutine) is the one I'd fix now; the rest are verification/design notes since the backend is still serial in this PR.
Concurrency correctness to verify (not proven bugs):
- Parallel fingerprint recursion.
preflightPackageBuildsruns a whole dependency level concurrently; each worker'scollectFingerprint→dependencyFingerprintmay read/modify a sharedaDepfromctx.pkgByID(internal/build/collect.go:228-234). This is safe iff every in-plan dependency is always at a strictly lower level (so it is already fingerprinted and hits the early return atcollect.go:40), which holds only ifpkgByIDmembership and plan (byID) membership coincide — both come fromregisterSSAPkgs, so they should, but the invariant is neither enforced nor documented. Please run a real multi-package build undergo test -race(the added tests only cover single packages / serial planning). CallerTracking.Precomputetiming.Precomputeruns right afterbuildSSAPkgsand freezes the maps, whereas the previous lazy path computed caller sets duringbuildPkg— afternewPackageExreassignspkg.Pkg = patch.Typesfor patched packages (cl/compile.go). Sets are keyed by the stable*ssa.Packagepointer so lookups still hit, but they now reflect pre-patch analysis. Please confirm patching cannot change caller-frame membership (criteria 1/2), otherwise frozen results silently differ from the old behavior and can change frame-pinning/inlining decisions.
Design / follow-up:
- Per-level preflight barrier. Each dependency level spins up and tears down a fresh worker pool with a
wg.Wait()barrier (internal/build/package_build.go:48-81). For deep, narrow graphs this caps effective parallelism at level width and adds depth-proportional pool churn. A single in-degree-driven pool over the whole DAG would saturate better. Reasonable to defer, but worth a comment noting the tradeoff. - Frozen
CallerTrackingread is unsynchronized.c.frozenis read without a lock inruntimeCallerBaseSet/runtimeCallerFuncSet(cl/instr.go:928,1059). Safe today only becausePrecomputecompletes-before any worker and the backend is still serial. Once the backend is parallelized (the stated goal) these unguarded reads and post-freeze map reads need an explicit memory-safety guarantee; consider a debug assertion that a frozen miss never occurs. llssaInitOnce(build.go:59-61) is safe because every call site passes the constantllssa.InitAlland the LLVMInitializeAll*calls are idempotent, but async.Oncewould silently ignore a different flag set from a future caller — a one-line comment would prevent that footgun.
| if spec.isLinkOnly() && !spec.hasSource() { | ||
| pkg.ExportFile = "" | ||
| if spec.kind == cl.PkgLinkExtern { | ||
| appendExternalLinkArgs(ctx, aPkg, spec.kindParam) |
There was a problem hiding this comment.
appendExternalLinkArgs panics (build.go:1069, :1086) when an external library cannot be resolved. It is now reachable from a preflight worker goroutine (preflightPackageBuild runs inside the pool in preflightPackageBuilds), so an unresolvable/failed link spec crashes the whole process with an unrecovered goroutine panic instead of surfacing as the error the level loop is designed to collect and return. Consider returning an error from appendExternalLinkArgs (or recovering in the worker) so external-library resolution failures propagate cleanly through the parallel path.
| preflights := make(map[string]packagePreflight, len(plan.specs)) | ||
| for _, level := range plan.levels { | ||
| workers := min(ctx.buildConf.parallelism(), len(level)) | ||
| jobs := make(chan int) |
There was a problem hiding this comment.
The jobs channel is unbuffered here while results is buffered to len(level). Since preflight items are short (mostly syscalls: file reads, os.Stat, manifest reads), the unbuffered channel forces a synchronous producer→worker rendezvous per item. The sibling worker pool in buildSSAPkgs was explicitly changed to a buffered jobs channel (make(chan ssaBuildEntry, len(unique))) in commit 02 for exactly this reason; buffering jobs to len(level) here would restore that consistency and let the producer enqueue without blocking.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Stacked on #2180 (and therefore includes #2174-#2180).\n\nPrecomputes CallerTracking for all SSA packages after SSA Build and freezes its memoization before package backend work begins. This preserves cross-package runtime.Caller analysis while making later worker reads race-free. Includes a concurrent read test under -race.