Skip to content

cl: precompute caller tracking for backend workers#2181

Open
zhouguangyuan0718 wants to merge 12 commits into
xgo-dev:mainfrom
zhouguangyuan0718:agent/parallel-build-pr7
Open

cl: precompute caller tracking for backend workers#2181
zhouguangyuan0718 wants to merge 12 commits into
xgo-dev:mainfrom
zhouguangyuan0718:agent/parallel-build-pr7

Conversation

@zhouguangyuan0718

Copy link
Copy Markdown
Contributor

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.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. preflightPackageBuilds runs a whole dependency level concurrently; each worker's collectFingerprintdependencyFingerprint may read/modify a shared aDep from ctx.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 at collect.go:40), which holds only if pkgByID membership and plan (byID) membership coincide — both come from registerSSAPkgs, so they should, but the invariant is neither enforced nor documented. Please run a real multi-package build under go test -race (the added tests only cover single packages / serial planning).
  • CallerTracking.Precompute timing. Precompute runs right after buildSSAPkgs and freezes the maps, whereas the previous lazy path computed caller sets during buildPkgafter newPackageEx reassigns pkg.Pkg = patch.Types for patched packages (cl/compile.go). Sets are keyed by the stable *ssa.Package pointer 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 CallerTracking read is unsynchronized. c.frozen is read without a lock in runtimeCallerBaseSet/runtimeCallerFuncSet (cl/instr.go:928,1059). Safe today only because Precompute completes-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 constant llssa.InitAll and the LLVM InitializeAll* calls are idempotent, but a sync.Once would silently ignore a different flag set from a future caller — a one-line comment would prevent that footgun.

View job run

Comment thread internal/build/build.go
if spec.isLinkOnly() && !spec.hasSource() {
pkg.ExportFile = ""
if spec.kind == cl.PkgLinkExtern {
appendExternalLinkArgs(ctx, aPkg, spec.kindParam)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant