build: snapshot package linker state - #2179
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review — parallel build pipeline
Solid, well-structured refactor. The register/build split, the packageBuildPlan/dependency-level algorithm, and the PackageSummary decoupling (linking no longer reads live LLVM modules) are coherent, and the concurrency-sensitive paths are guarded (builtMu, sfilesMu, cacheManagerMu, llvmVersionMu). Channel/waitgroup usage in both worker pools is correct (no deadlock or goroutine leak). The -p flag parsing, atomic-on-error config application, and the flagfile whole-line-value split are all covered by tests. No blocking issues.
A few concrete points, mostly inline. Two non-inline items worth flagging:
1. -p speedup is bounded by a serial backend (performance). Only two stages run in parallel: buildSSAPkgs (ssa.Package.Build) and the cache/fingerprint preflight (preflightPackageBuilds). The dominant cost — buildPreflightedPackage → executePackageBuild → buildPkg (frontend + LLVM RunPasses/object emission) — still runs in the strictly serial for _, spec := range normalPkgs loop in buildAllPkgs. The comments acknowledge this is intentional ("backend remains deliberately serial until it owns an isolated LLVM context"), so this is a heads-up rather than a defect: on cold builds -p mainly speeds up type-check/SSA and cache-stat work, not the backend. Consider noting this expectation wherever -p is user-documented.
2. Cache manifest is trusted without re-verifying it against the fingerprint (pre-existing, not a regression). tryLoadFromCache uses the fingerprint only to derive the on-disk filename, then trusts meta.LinkArgs and the new meta.Summary (which now includes LinkArgs flowing onto the linker command line) without recomputing/comparing the fingerprint of the stored manifest. This PR widens the trusted payload but does not change the model (the cache dir is local). Not blocking; recomputing the fingerprint on load would make entries self-authenticating and close this for both old and new fields.
Automated review; treat as suggestions. The concurrency note below (shared fingerprint state) is the item I'd most want confirmed before merge.
| if aDep, ok := c.pkgByID[dep.ID]; ok { | ||
| if aDep.Fingerprint == "" { | ||
| if err := c.collectFingerprint(aDep); err != nil { | ||
| if err := c.collectFingerprintWithStack(aDep, fingerprinting); err != nil { |
There was a problem hiding this comment.
Parallel preflight correctness here rests on a DAG-ordering invariant that is worth confirming and testing. dependencyFingerprint reaches into the shared ctx.pkgByID[dep.ID] aPackage and, when its Fingerprint == "", writes aDep.Fingerprint/aDep.Manifest on that shared object. Within a dependency level, workers run concurrently, so this is only race-free if every in-plan dependency was already fully fingerprinted in an earlier level.
That holds today because newPackageBuildPlan and collectDependencyInputs both derive edges from effectiveDependencies, and pkgByID/plan.byID come from the same allPkgs set — so an in-plan dep is always an earlier level, and out-of-plan deps use fresh local temp objects. But it's fragile: any future case where a fingerprinted dependency is reachable via pkgByID yet not represented as a plan edge (e.g. a dep filtered from the plan but still traversed here, or a plan/fingerprint classification mismatch) would let two same-level workers write the same shared aDep concurrently — a data race producing corrupt manifests.
Coverage is thin: only module_hook_test.go sets Parallel = 2, and there's no -race run over a real multi-package dependency graph. Recommend adding a go test -race case with Parallel >= 2 across a non-trivial import graph, and/or guarding the shared aDep fingerprint write rather than relying solely on level ordering.
| PCLNModeSet bool | ||
| AllowNoBody bool // allow declarations without bodies, as go tool compile does | ||
| // Parallel is the maximum number of LLGo's internally parallel build tasks. | ||
| // Zero uses GOMAXPROCS; command-line -p, like go build -p, must be >= 1. |
There was a problem hiding this comment.
The field's own contract differs from this comment. The comment describes the CLI rule ("-p ... must be >= 1"), but the Parallel field itself accepts 0 programmatically (parallelism() maps 0 → GOMAXPROCS, and Do rejects only negative values). For a direct API caller, 0 is valid and means "use GOMAXPROCS". Consider rewording so the field contract (0 = default/GOMAXPROCS, negative = error) is stated for the field, with the >= 1 note scoped to the -p flag.
| } | ||
| // Snapshot every linker-facing LLVM fact before cache publication. The | ||
| // summary is the hand-off point to a future worker-local Program. | ||
| aPkg.Summary = summarizePackage(aPkg) |
There was a problem hiding this comment.
summarizePackage currently always returns non-nil on this path, so this is fine today. As defensive robustness: if saveToCache were ever reached with pkg.Summary == nil, metadata() returns nil and the entry is written with summary: absent — which tryLoadFromCache then treats as a permanent cache miss (meta.Summary == nil guard). That is a silently non-cacheable entry rather than an error. A short assertion/comment here that Summary must be set before saveToCache would make the invariant explicit.
| if err != nil || parallel <= 0 { | ||
| return 0, false, fmt.Errorf("-p must be a positive integer, got %q", value) | ||
| } | ||
| present = true |
There was a problem hiding this comment.
Minor: when -p= appears multiple times in the normalized flags, this silently uses the last occurrence. That matches go build's last-wins semantics, so it's defensible, but it's undocumented — a one-line comment noting the intentional last-wins behavior would help future readers.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
68b3b55 to
b818839
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared only with the latest matching platform in the main series. |
c72f759 to
942bc28
Compare
942bc28 to
ebd6988
Compare
1 participant
Summary
PackageSummaryas the linker-facing package resultDependency and review boundary
Depends on #2175.
The intended diff for this PR is:
dcf4cca4(current build: split package compilation stages #2175 head)3a677a12Please review only
dcf4cca4..3a677a12.Tests
git diff --check