build: pipeline package SSA into LLVM backends - #2242
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: build — pipeline package SSA into LLVM backends
Reviewed the incremental range the PR requests, 455fd3e4..6e0a230a (12 files, +1050/-19), across four dimensions (quality, performance, security/concurrency, docs). The change is well-structured and the trickiest invariants (single shared -p budget, patched-package exclusivity, immutable merged type view, frozen per-backend caller-tracking) are documented and hold up. Edge cases I checked are correct: empty input, -p=1 legacy SSA-then-backend ordering, mid-flight SSA error draining, and patched-backend exclusivity.
Concurrency/security: no data-race findings. Shared maps (ctx.patches, ctx.sfilesCache, caller-tracking summaries) are provably populated/frozen before any goroutine launches, panics are recovered on both SSA and backend paths, and no goroutine-leak or blocked-send paths were found (events is buffered to parallelism, active never exceeds it).
Findings below (all non-blocking). Inline comments carry the specifics.
Summary of findings
internal/build/package_build.go/ scheduler (worth confirming): a non-patched coordinator backend (plan9asm package) runsexecuteCoordinatorPackageWithCallerTrackingviatask := *ctx, i.e. on the sharedctx.prog, whileserialActiveonly serializes coordinators against each other — isolated backends can run concurrently. Only patched coordinators get fullexclusiveActiveexclusivity. Please confirm building a coordinator on sharedctx.prog/ctx.dedupconcurrently with isolated sessions is race-free; this is a behavioral change from the legacy strictly-sequential group ordering, and plan9asm packages may not be exercised by the-racetargets listed in the PR.- Performance (medium):
O(n²)scheduler scans —allBackendsDone()and thenextBackend()isolated fallback linearly scantaskson every scheduling event. AdoneCountcounter and a dependency-indexed ready-queue would remove it for large dependency graphs. - Minor cleanups: dead
packagePipelineSSA.orderfield; duplicatectx.patches[...]lookup + redundantpatchedlocal; SSA-centric stall-error message; a couple of doc-comment wording nits.
No praise-only or low-confidence items are included.
|
|
||
| func (ctx *context) executeCoordinatorPackageWithCallerTracking(spec packageBuildSpec, tracking *cl.CallerTracking, verbose bool) error { | ||
| task := *ctx | ||
| task.callerTracking = tracking |
There was a problem hiding this comment.
Please confirm this is race-free — potential behavioral change from the legacy ordering.
executeCoordinatorPackageWithCallerTracking does task := *ctx, so it builds on the shared ctx.prog (and shares ctx.dedup, ctx.pkgs, ...). In buildPackagePipeline, serialActive only serializes coordinators against each other; isolated backends (each with their own session.prog) can still be launched while a coordinator runs. Only patched coordinators get full exclusivity via exclusiveActive/active==0.
A non-patched coordinator arises for plan9asm packages (packageRequiresCoordinator). In the legacy buildPreflightedPackageGroup, the patched→coordinator→isolated groups ran strictly sequentially, so coordinators finished before any isolated backend started. Here they overlap. If building a plan9asm coordinator on the shared ctx.prog concurrently with isolated sessions is safe, a one-line comment stating why would prevent a future regression; plan9asm packages may not be covered by the -race targets in the PR description.
| } | ||
| return nil | ||
| } | ||
| allBackendsDone := func() bool { |
There was a problem hiding this comment.
Performance (medium): O(n^2) scheduler scans. allBackendsDone() here, and the isolated fallback loop in nextBackend (lines 199-203), each do a full linear scan of tasks. allBackendsDone() runs on every outer-loop iteration (line 232) and again at line 285; nextBackend() runs on every inner-loop iteration. With ~n SSA events + ~n backend events, aggregate cost is O(n^2) for a graph of n packages (hundreds–thousands transitively).
Suggested mitigation: keep a running doneCount (bumped where tasks are marked done) and compare against len(tasks) instead of scanning; index dependents by SSA node (you already know which dependents to reconsider when a node finishes at line 305) to feed a ready-queue rather than re-scanning all tasks. advanceSerial is already correctly amortized via the serialPos cursor, so only these two spots are affected.
| node.entry.fixOrder = node.entry.fixOrder || entry.fixOrder | ||
| continue | ||
| } | ||
| node := &packagePipelineSSA{entry: entry, order: len(nodes)} |
There was a problem hiding this comment.
Minor: packagePipelineSSA.order is effectively dead in production — it is assigned here (order: len(nodes)) but only read by package_pipeline_test.go. The real ordering is the topological sort in orderPackagePipelineSSA, which never reads order. Consider dropping the field (adjusting the test) or adding a comment that it exists solely as a stable first-seen index for tests.
| } | ||
|
|
||
| patched := false | ||
| if _, ok := ctx.patches[spec.pkg.PkgPath]; ok { |
There was a problem hiding this comment.
Minor cleanup: ctx.patches[spec.pkg.PkgPath] is looked up twice (here and at line 441 as patch, ok), and the patched local (line 449) duplicates the task.patched field set just below. You can reuse the line-441 ok/patch and read task.patched directly, removing the second lookup and the shadow variable.
| if allBackendsDone() && ssaDone == len(ssaNodes) { | ||
| break | ||
| } | ||
| return nil, fmt.Errorf("package pipeline stalled with %d/%d SSA packages complete", ssaDone, len(ssaNodes)) |
There was a problem hiding this comment.
Minor: this stall error is reachable only on an internal invariant break, but with ssaDone == len(ssaNodes) the remaining stall cause is backend-side (an unschedulable ready()/ssaDeps case), while the message reports only SSA completion counts. Including remaining undone backend-task counts would make a future stall easier to diagnose.
|
|
||
| // NewPackageCallerTrackingForPackages is NewPackageCallerTracking for a | ||
| // backend that compiles more than one SSA package, such as an alternate-package | ||
| // patch. |
There was a problem hiding this comment.
Doc nit: NewPackageCallerTrackingForPackages is described as being NewPackageCallerTracking "for a backend that compiles more than one SSA package," but it is actually the general/primary form — NewPackageCallerTracking (line 1050) is the single-package wrapper that delegates here. Consider "generalizes NewPackageCallerTracking to a backend that may compile more than one SSA package" to avoid implying the singular form is primary.
6e0a230 to
475dc12
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
No main baseline exists yet; all metrics are marked Warning
|
efc18f2 to
c81529c
Compare
3dd516d to
fbe1ed3
Compare
fbe1ed3 to
a456e8d
Compare
a456e8d to
e389cd8
Compare
e389cd8 to
65b5dde
Compare
1 participant
Summary
-pconcurrency budgetgo/ssapackage type pointersDependency and review boundary
Depends on #2182. This remains an optional follow-up; in-memory package archiving is implemented earlier in #2175 and does not depend on this pipeline.
The intended diff is:
da5685a0(current build: run LLVM backends in parallel by package #2182 head)a456e8d7Please review only
da5685a0..a456e8d7.Validation
-a -p=8cgo + Plan9 build and runtime checkar, no LLVM package temporary.ogit diff --checkThe earlier pipeline-only cold-cache measurements remain historical; absolute timings should be refreshed after the new #2175 archive path lands.