Skip to content

cl: precompute caller tracking for backend workers - #2181

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

cl: precompute caller tracking for backend workers#2181
zhouguangyuan0718 wants to merge 25 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

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cl/instr.go 92.59% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@zhouguangyuan0718
zhouguangyuan0718 force-pushed the agent/parallel-build-pr7 branch 6 times, most recently from 795a979 to 74d9ea0 Compare July 26, 2026 13:29
@zhouguangyuan0718
zhouguangyuan0718 force-pushed the agent/parallel-build-pr7 branch from 74d9ea0 to 9ee69f5 Compare July 27, 2026 02:47
@zhouguangyuan0718
zhouguangyuan0718 force-pushed the agent/parallel-build-pr7 branch from 9ee69f5 to d166fbe Compare July 27, 2026 04:35
@zhouguangyuan0718

Copy link
Copy Markdown
Contributor Author

This PR has been absorbed into #2180. Backend isolation, caller-tracking precomputation, and the concurrent runtime FuncForPC initialization fix are now reviewed together there.

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