build: parallelize cache preflight by dependency level - #2178
build: parallelize cache preflight by dependency level#2178zhouguangyuan0718 wants to merge 12 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: build: parallelize cache preflight by dependency level
The refactor is well-structured: the packageBuildSpec/packagePreflight/packageBuildResult split, the topological readyLevels scheduler (with cycle + duplicate-ID guards), the deferred-fixSSAOrder SSA worker pool, and the new mutexes (builtMu, cacheManagerMu, llvmVersionMu, sfilesMu) are sound. The -p flag parsing validates input and updates config atomically on error. Findings below, most important first.
Concurrency (high): The per-level barrier guarantees a dependency's preflight ran before dependents preflight — but not that its fingerprint was computed. preflightPackageBuild returns skip=true for decl-only and link-only-without-source packages before calling collectFingerprint, so those shared aPackages keep Fingerprint == "". When two packages in the same level (built concurrently) both import such a skipped package, each worker independently takes the aDep.Fingerprint == "" branch in dependencyFingerprint and writes aDep.Manifest/aDep.Fingerprint concurrently, with no lock on those fields. This is a real write/write race and can also poison the cache key with a partially-written fingerprint. See inline comment. Consider fingerprinting skipped packages in their own level too, or guarding the fingerprint fields with a mutex / sync.Once per package. Worth running the package build tests under -race.
Additional findings are left as inline comments on collect.go, plan9asm.go, and package_build.go.
Minor (not inlined): packageBuildResult.cacheHit and archiveFile (internal/build/package_build.go) are populated but never read in this PR. The type documents itself as scaffolding for later scheduler PRs, which is reasonable, but as merged these fields are dead — confirm the team is comfortable landing them ahead of their consumer.
| 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.
Data race on shared aPackage fingerprint fields. This lazy-fill branch writes aDep.Manifest/aDep.Fingerprint (lines 67-68) for a dependency whose fields are still empty. During parallel preflight, two packages in the same dependency level run concurrently; if both import a dependency that skipped collectFingerprint in preflight (decl-only, or link-only-without-source — preflightPackageBuild returns skip=true before fingerprinting those), both workers reach this branch for the same aDep and write its fields concurrently. There is no lock on aPackage.Fingerprint/Manifest, so this is a write/write + read/write race that can also produce a blank/partial fingerprint used as a cache key.
The stated invariant ("every dependency's fingerprint is complete before a worker reads it") holds only for deps that were fingerprinted in an earlier level — it is violated precisely by the skip paths. Suggest either fingerprinting skipped packages too, or guarding these fields (e.g. per-package sync.Once / a mutex). Please verify with go test -race.
| if c.sfilesCache == nil { | ||
| c.sfilesCache = make(map[string][]string) | ||
| } | ||
| c.llvmVersion = detectLLVMVersion(c) |
There was a problem hiding this comment.
detectLLVMVersion now runs unconditionally at startup (it execs an external tool, e.g. clang --version), even when cacheEnabled() is false. Previously the LLVM version was detected lazily and is only needed for fingerprinting/caching, so this adds an unconditional subprocess to every Do call on the cache-disabled path. Also, because this eagerly sets llvmVersionReady = true before any goroutine starts, the lazy branch in getLLVMVersion (line ~270) is now effectively dead code, leaving two overlapping init strategies. Consider gating detection on cacheEnabled() (mirroring the cacheManager handling just below), or routing through the mutex-guarded getLLVMVersion() so there is a single source of truth.
| } | ||
| } | ||
|
|
||
| ctx.sfilesMu.Lock() |
There was a problem hiding this comment.
sfilesMu is held across the entire function body, which includes the go list subprocess exec below. Since collectPackageInputs calls pkgSFiles for every package during the now-parallel preflight, any level containing multiple packages with .s/.S files will run their go list invocations strictly serially — negating the parallelism this PR adds for asm-heavy levels (e.g. runtime/std subtrees). Consider holding the lock only around the cache map read/write and running go list outside the critical section (with per-key single-flight to avoid duplicate execs).
| } | ||
| } | ||
|
|
||
| // packageBuildPlan is the immutable dependency graph consumed by a future |
There was a problem hiding this comment.
Stale comment: this PR consumes the plan now. plan.levels is actively used by preflightPackageBuilds to drive parallel, level-by-level preflight, so "consumed by a future scheduler" and "levels records the safe ready sets" understate current behavior. Suggest updating to note that levels already drives parallel preflight, while only the serial backend (specs order) awaits a future change.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
d081bba to
e78600a
Compare
e78600a to
1666b2d
Compare
1666b2d to
3fa3f53
Compare
3fa3f53 to
4782448
Compare
|
Closing as obsolete after the architecture review: recursive cache preflight remains serial, while the expensive LLVM package backends run directly in parallel in #2182. The dependency-level preflight implementation has been removed. |
1 participant
Summary
-pValidation
GOCACHE=/private/tmp/llgo-pr4-gocache go test ./internal/build ./internal/goflags ./cmd/internal/flags -count=1GOCACHE=/private/tmp/llgo-pr4-race-gocache go test -race ./internal/build -run TestModuleHookReceivesMainPackageModule -count=1Dependency
This PR includes and depends on #2174, #2175, #2176, and #2177. Merge/rebase it after those PRs; it will then contain only the parallel-preflight stage.