build: add Chrome scheduler trace - #2243
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI review — build scheduler trace
Reviewed the trace-specific range 475dc124..d917b0ce across code quality, performance/concurrency, security, and documentation. The design is solid: the nil-tracer convention keeps disabled builds allocation-free, span/close double-invocation is guarded by sync.Once, and flow edges are deliberately limited to direct SSA dependencies to avoid quadratic trace size.
Concurrency, verified safe (no change needed): the lanes semaphore never deadlocks — every startWorker caller runs under the same parallelism() bound that sizes the lane channel, and the serial coordinator phases never overlap the parallel worker phases. The node.traceSpan / from.end cross-goroutine reads in flow() have a valid happens-before via the pipeline events channel and ready() gating.
Findings are inline. The most impactful is the build-fails-on-trace-write-error behavior in internal/build/build.go.
Minor note not placed inline: the load-bearing invariant behind the lane semaphore ("never open more than parallelism worker spans concurrently, never nest worker spans on one goroutine") is currently undocumented — worth a doc comment on startWorker/buildTracer to protect future callers, since violating it would deadlink real build work rather than just skew the visualization.
| buildSpan.done() | ||
| if closeErr := buildTrace.close(); closeErr != nil && resultErr == nil { | ||
| result = nil | ||
| resultErr = fmt.Errorf("write build trace: %w", closeErr) |
There was a problem hiding this comment.
A diagnostic trace-write failure turns a fully successful build into a reported failure: on closeErr != nil the deferred func discards the built packages (result = nil) and returns resultErr. -debug-trace is a debugging aid; failing the whole build because the trace file couldn't be flushed is a heavy, asymmetric policy (a write error mid-build is already silently swallowed via the sticky writeErr, and only surfaces here at close). Consider logging the trace-write error to stderr and preserving the build result instead. If failing hard is intended, a short comment justifying it would help, since the behavior is non-obvious.
| path = filepath.Join(dir, path) | ||
| } | ||
| if filepath.Ext(path) == ".go" { | ||
| return nil, fmt.Errorf("refusing to overwrite Go source file %s", path) |
There was a problem hiding this comment.
This guard refuses only a .go extension, then os.Create (line 86) truncates any other existing target. So -debug-trace=go.mod, -debug-trace=Makefile, an existing archive, etc. are silently overwritten, and relative paths resolve against the source dir. The message "refusing to overwrite Go source file" implies a general overwrite protection that doesn't exist. Either drop the check (and be honest it's a plain os.Create), or if protection is intended use os.OpenFile with O_CREATE|O_EXCL to refuse overwriting any existing file. Low severity since the path is a locally-supplied flag.
| } | ||
|
|
||
| // buildTracer is owned by one Build invocation. The lane semaphore mirrors | ||
| // the build's -p limit, so overlapping worker-lane events visualize the same |
There was a problem hiding this comment.
The comment says the lane semaphore "mirrors the build's -p limit," but the lane count comes from conf.parallelism(), which returns BuildParallelism only when > 0 and otherwise falls back to GOMAXPROCS. When -p isn't supplied the lanes mirror GOMAXPROCS, not a user -p value. Suggest wording like "the build's effective package parallelism (-p, or GOMAXPROCS by default)."
| // output file; test and run may coordinate multiple child invocations. | ||
| func AddBuildTraceFlag(fs *flag.FlagSet) { | ||
| BuildTrace = "" | ||
| fs.StringVar(&BuildTrace, "debug-trace", "", "Write a Chrome/Perfetto build trace to file") |
There was a problem hiding this comment.
The PR stresses this is a build scheduler trace, not a runtime/execution trace, and this same file already defines -trace ("Write an execution trace to the specified file"). Neither the flag name -debug-trace nor this usage string conveys "scheduler," so the two are easy to conflate. The doc comment on AddBuildTraceFlag and the internal test both say "scheduler" — consider aligning the user-facing usage string, e.g. "Write a Chrome/Perfetto build-scheduler trace to file".
| } else { | ||
| traceSpan.setArg("class", "isolated") | ||
| } | ||
| if ctx.buildTrace != nil { |
There was a problem hiding this comment.
This explicit if ctx.buildTrace != nil is redundant with flow()'s own nil-receiver guard, and inconsistent with every other call site here (startWorker, setArg, done are all called unconditionally on a possibly-nil tracer). If the intent is to skip building the callerNodes loop when tracing is off, a one-line comment would clarify it; otherwise drop it for consistency.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
e1fee5d to
d6a38e5
Compare
ec33fd7 to
a938582
Compare
a938582 to
ae83ba5
Compare
2 participants
Architecture proposal: #2284.
This PR is based on the package-worker pipeline and caller-tracking preparation
that are now present on
mainthrough #2286.Summary
llgo build -debug-trace=<file>scheduler trace in ChromeTrace Event JSON format; this traces the build pipeline, not the compiled
program and not
runtime/traceeffective Go
-pvalue (GOMAXPROCSwhen-pis not specified)serial SSA repair and caller tracking, package preparation, backend/archive
publication, and final linking
existing single
backend+publishtaskpackages.Package.ID, avoiding ambiguous flow arrows between test variantsthat share a
PkgPathnot share output files, lanes, flow IDs, or mutable state
encoding/json;there is no custom JSON framing or process-global tracing dependency
This does not restore the superseded preflight partition/result,
PackageSummary, syntax-delta validation, state snapshot, or worker-overlaydesigns.
Activation and disabled behavior
Tracing is disabled by default:
-debug-tracedefaults to an empty pathConfig.BuildTraceworker-lane semaphore and do not take timestamps
The flag is intentionally owned by
llgo build, which has one build invocationand one trace output. It is not added to
llgo runorllgo test, whose commandlayer can coordinate multiple builds.
Trace model
backend+publishworkX): stage duration and package metadatas/f): SSA completion to the matching package backendM): process and lane names for Chrome/PerfettoRelative output paths are resolved from the build invocation directory. Trace
creation uses
O_EXCL, so an existing file is never overwritten. A final JSONencoding or close error is reported as a warning and does not turn an otherwise
successful build into a failure.
Open the generated file in
chrome://tracingorhttps://ui.perfetto.dev/.
Actual etcd trace
Command, with LLGo package build outputs disabled and every package forced:
Observed on the local etcd server checkout:
user 215.43s,sys 11.13s)backend+publishtasks; peak 8, average 7.20--versionThe generated trace contained 2,917 events and was 677 KiB. Its measured wall
time was within run-to-run noise of the 37.98s untraced build.
Validation
go test ./internal/build ./cmd/internal/build ./cmd/internal/flagsgo test -race ./internal/build -run 'BuildTrace' -count=1preparation, backend, caller-tracking, and SSA-to-backend flow events
parallelism
lifecycle tests
-a -p=8build with the LLGo build cache disabledgit diff --check