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
No main baseline exists yet; all metrics are marked Warning
|
31f9df9 to
2381d97
Compare
2381d97 to
a8402ae
Compare
a8402ae to
ebc13d3
Compare
ebc13d3 to
39937cc
Compare
1 participant
Depends on #2242. The trace-specific review range is
a456e8d7..a8402aea.Summary
llgo build -debug-trace=<file>with Chrome Trace Event JSON output (this is a build scheduler trace, notruntime/trace)-pvalueValidation
internal/buildandcmd/llgo-p=8mixed cgo/Plan9 archive buildgit diff --checkOpen generated JSON directly in
chrome://tracingor https://ui.perfetto.dev.