cl: prepare caller tracking inputs once - #2286
Conversation
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This is a large, well-tested PR that (1) precomputes caller-tracking data serially before launching parallel package backend builds on isolated LLVM Programs, (2) makes recursive named-type conversion order-independent, and (3) adds cross-context LLVM type/constant cloning in dcepass. The concurrency model is largely sound: backend Programs share only read-only packageSyntaxData/localities under an RWMutex, per-Program typs/cvtneed/abiSymbol are not shared, and retained Programs correctly outlive the deadcode/ABI consumers before disposal.
I found one latent correctness regression (inline) plus a few fragility/accuracy notes. Details are inline; additional items below.
Additional findings
-
xtool/env/env.go:110(lookPathInEnvironment) — cwd/relative executable resolution. The custom PATH resolver rewrites an empty PATH entry to.and joins relative PATH entries withdir, then executes any matching executable regular file. This bypasses the stdlibLookPathErrDotprotection (Go 1.19+) that refuses cwd-relative execution. The subcommand strings feedingExpandEnvToArgsWithcome from#cgo/cflags directives in the compiled packages (appendExternalLinkArgs,clFiles); when building untrusted third-party code, apkg-config/llvm-configbinary placed in the build dir (or a relative PATH entry) could be executed. Consider skipping empty/relative PATH entries or refusing candidates resolved inside the working directory, matchingErrDotsemantics. There is also aos.Stat→exec.CommandTOCTOU window (env.go:129/:81), elevated by the above. -
Memory scaling (informational).
executeIsolatedPackagetransfers every successful package's Program toctx.retained, disposed only after all links complete (build.go/package_build.go:305). Peak memory therefore scales with total package count rather than in-flight workers. The code comment acknowledges this is intentional pending a futurePackageSummary; noting as the main scalability trade-off. -
runBoundedPackageJobs(package_build.go:206) — no sibling cancellation on first error (informational). Workers drain the full job queue even after a job fails; the first submitted error is returned afterwg.Wait(). Deterministic error ordering is preserved, but a failing build does more work than necessary.
| if item.Args != inPkgName && !options.ExportRename { | ||
| return false, fmt.Errorf("export comment has wrong name %q", item.Args) | ||
| } | ||
| prog.SetLinkname(fullName, item.Args) |
There was a problem hiding this comment.
The new "export" case does not handle the two-token //export Name Rename form that the legacy initLink path supports (cl/import.go initLink splits text on the first space into inPkgName and link).
For //export Name Rename, directive.ParseGroup yields item.Args = "Name Rename" as a single string. This code then:
- compares the whole
"Name Rename"againstinPkgName, so the wrong-name check fires incorrectly, and - calls
prog.SetLinkname(fullName, item.Args)with the literal"Name Rename"instead of"Rename".
Currently latent (no two-token //export exists in the repo, and single-token //export foo works), but under ExportRename (target builds) this would silently produce a malformed linkname. Suggest splitting item.Args on whitespace to recover inPkgName/link, or explicitly rejecting the two-token form here.
| // `go list` (PkgPath not in any module, and Dir/Standard/Goroot unset). | ||
| // In that case, treat the package as having no selected .s files. | ||
| if pkg.Dir == "" { | ||
| ctx.sfilesCache[pkg.ID] = nil |
There was a problem hiding this comment.
When sfilesFrozen is true, these fast-path branches still write ctx.sfilesCache[pkg.ID] = nil (also at lines 422 and 483) before the frozen guard is reached. In backend workers sfilesCache is the shared coordinator map (newBackendTask passes it directly) and workers run concurrently with no lock on this map.
It is not hit today because preparePackageSFiles pre-populates every task's entry (so the cache-hit returns first) and plan9-asm packages are routed to the serial coordinator. But the safety relies entirely on those invariants holding for every package a worker can reach. Consider checking the frozen guard before any fast-path write so a frozen context never mutates the shared map, or at least asserting the invariant at these write sites.
| c.retained.programs = append(c.retained.programs, retainedBackendProgram{ | ||
| pkg: pkg, | ||
| prog: prog, | ||
| abiTypes: prog.AbiTypes(), |
There was a problem hiding this comment.
prog.AbiTypes() allocates and sorts the package's ABI-symbol name list, and it runs inside the retained.mu critical section. This serializes every worker's publish step on that sort. Compute abiTypes := prog.AbiTypes() before taking the lock so the critical section is just the slice append. Minor, but it is on the join path of the parallel phase.
| task.pkg.setNeedRuntimeOrPyInit(task.pkg.LPkg.NeedRuntime, task.pkg.LPkg.NeedPyInit) | ||
| } | ||
| // Linking still consumes live package state: method tables, globals, | ||
| // funcinfo/PCLN, C exports, and DCE source modules. Cache hits intentionally |
There was a problem hiding this comment.
The comment "Cache hits intentionally follow the serial path" is inaccurate. partitionPackageExecutions / packageRequiresCoordinator route isolated-vs-coordinator purely from canUseIsolatedBackend() and Plan9 asm usage — cache-hit status is never consulted. A cache-hit package in a normal exe build runs on this isolated path, not the serial/coordinator path. The described behavior (rebuild frontend, skip backend, keep module alive via retention) is correct; only the "serial path" attribution is wrong.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
5d05b7e to
e15a47d
Compare
2 participants
Based directly on the current
main, which already contains #2182. The PR now has two reviewable commits:a4acd4c98— prepare caller-tracking inputs oncee15a47dc6— remove process-wide frontend optionsCaller-precompute optimization
ssa.Program.RuntimeTypes()once per Program and partition it by declaring packageMethodValuecalls so shared SSA mutation has a stable orderCallerTrackingcontract used by isolated package backendsFrontend-state cleanup
EnableDebug,EnableDbgSyms,EnableTrace, andEnableExportRenamecompatibility entry pointslegacyOptionsand thecontext.optionsSetfallback; lowering reads only the package context's explicitOptionsOptionscltest, debug/export/locality, and shadow-stack tests to pass options explicitlydisableInlineflag and its dead branchesThe normal build path already constructs one
cl.Optionsvalue per invocation and copies it into each isolated package task. This cleanup removes the remaining test/one-shot paths that could change frontend semantics through process-global state.DAG experiment
I also tested a bounded two-level DAG: independent per-package base nodes followed by extended nodes. It reduced the already-optimized caller phase only from about 0.13s to 0.06s, but changed generated code between serial and parallel analysis because caller analysis can still trigger lazy shared SSA method resolution. One observed difference swapped the two calls in
internal/sync.init. The DAG scheduler is therefore intentionally not included.Measurements
Original forced-cold etcd server measurement (
LLGO_BUILD_CACHE=off,-a,-p=8):After rebasing onto current
main, the final two-commit version built etcd server in 37.98s wall (user 208.45s,sys 10.09s) with the LLGo build cache disabled and-a -p=8.Validation
go test ./cl -count=1go test ./internal/build ./ssa ./internal/dcepass -count=1go test -race ./cl -run '^TestCallerTrackingPrecompute' -count=1go test -race ./internal/build -run '^TestConcurrentInvocationsIsolateFrontendOptions$' -count=1LLGO_BUILD_CACHE=off -a -p=8