build: prepare read-only package backend state - #2282
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: parallel-build backend overlay scaffolding
The "freeze-then-share" design is coherent and applied consistently: each accessor checks its Program-local map first, then the read-only baseline (packageSyntaxBase / typbgBase / frozen locality state), which is the right shape for lock-free concurrent reads. FreezePackageSyntaxState deep-copies into a fresh snapshot so the coordinator's continued local writes don't mutate shared state, and assertMutable() gives a fail-fast guard. Tests cover the sharing/overlay/concurrency invariants well.
Findings are inline. Summary of the notable ones:
- Latent nil-deref in the worker session path (
newSession/newProgram): the "no Python" case is handled gracefully by the coordinator's lazy closure but not by the eager template path — a worker could nil-derefpyget. Not yet live becausenewSessionis unwired, but worth fixing before it lands. - Unlanded session machinery (
newSession,backendSession,replayProgramState,ctx.backend) is exercised only by tests — no production caller.newProgramis wired in (coordinator). If the session path is intentional staged scaffolding, a tracking reference would help; otherwise it risks silent drift. - Redundant work in
CallerTracking.Precompute: the innerpkg.Prog.AllPackages()fan-out re-allocates the full package set N times whenpkgsalready equalsAllPackages(). Low severity (runs once per compilation) but easy to eliminate. - Doc inaccuracy on
FreezeLocalityState: the "parse markers remain local to each Program" sentence is both misplaced (markers moved offlocalityInfos) and contradicted by the shared-baseline behavior the tests assert. - Minor:
copyshadows the builtin in two spots.
Additional context (no change requested):
- Concurrency safety of the lock-free shared reads currently relies on an undocumented happens-before:
Precompute/freeze fully completes before any worker spawns, andfrozenis a plain (non-atomic) bool with no test spawning writers across the freeze transition. Correct today; worth documenting the invariant (or makingfrozenatomic) so a future change fails loudly under-racerather than silently. - The frozen-coordinator locality path is safe only because
ParsePkgSyntaxshort-circuits onPackageSyntaxParsed; a patched package (different*types.Packagepointer) declaring a package var absent from the original could reachassertMutable()and panic. Fragile invariant — an explicit assertion/comment would harden it.
| prog.SetRuntime(t.runtimePackage) | ||
| } | ||
| if t.pythonPackage != nil { | ||
| prog.SetPython(t.pythonPackage) |
There was a problem hiding this comment.
Latent nil-deref in the worker session path. The coordinator installs Python via a nil-safe lazy closure (SetPython(func() *types.Package { ... return nil })), so python() degrades gracefully when Python isn't in the build. Here a worker only calls SetPython when t.pythonPackage != nil, leaving both p.py == nil and p.pyget == nil. If any worker code path then reaches Program.python() it calls p.pyget() on a nil func value and panics (ssa/python.go).
This isn't live yet because newSession has no production caller, but it should be fixed before the parallel path lands: install the same nil-returning closure fallback (or set unconditionally). The SetRuntime/runtime() pair has the same asymmetry but is lower risk since runtimePackage is effectively always set (altPkgs[0].Types).
| return prog | ||
| } | ||
|
|
||
| func (t backendProgramTemplate) newSession() (backendSession, error) { |
There was a problem hiding this comment.
newSession, backendSession, replayProgramState, and the ctx.backend field (written at ctx construction, never read in production) are exercised only by backend_program_test.go — no production code spawns a session. newProgram is wired in as the coordinator program. This is a sizeable chunk of code whose behavior is guaranteed only by unit tests, with no production caller to keep it honest as the surrounding build logic evolves.
If this is intentional staged scaffolding for a follow-up parallel-build PR, a tracking issue/comment reference would make that clear. Otherwise consider gating it behind a clearly-labeled experimental path or deferring it.
Relatedly, replayProgramState calls ParsePkgSyntax (which runs DeclareLocality on the session's own fresh locality map) before UseLocalityState swaps in the frozen shared state, so those writes are discarded. Harmless today only because the shared frozen state already contains those declarations; consider installing the shared locality state first, or documenting why the pre-replay writes are intentionally thrown away.
| func newBackendProgramTemplate(target *llssa.Target, conf *Config, funcInfoMetadata, funcInfoSites bool) backendProgramTemplate { | ||
| var targetCopy *llssa.Target | ||
| if target != nil { | ||
| copy := *target |
There was a problem hiding this comment.
copy shadows the copy builtin here (and again at the newProgram target-copy below). Legal but a maintenance hazard — a later edit needing copy() in this scope would misbehave. Suggest renaming to targetCopy (as already done for the outer variable in this function).
| } | ||
| all[pkg] = true | ||
| if pkg.Prog != nil { | ||
| for _, programPkg := range pkg.Prog.AllPackages() { |
There was a problem hiding this comment.
Redundant O(N·M) work. Precompute is called as ctx.callerTracking.Precompute(ctx.progSSA.AllPackages()), so pkgs already is the full program package set. All packages share the same *ssa.Program, so pkg.Prog.AllPackages() returns the identical full set on every iteration — and AllPackages() allocates a fresh slice and re-iterates the package map each call, giving N slice allocations and N·M insertions.
Low severity (runs once per compilation, dwarfed by the analysis it feeds), but pure redundant work: iterate pkgs directly, or hoist a single AllPackages() call as a fallback only if pkgs may be a subset.
| } | ||
|
|
||
| // FreezeLocalityState freezes p's locality metadata and returns it without | ||
| // copying. Package syntax parse markers remain local to each Program. |
There was a problem hiding this comment.
This comment is inaccurate and misplaced. parsedPackages no longer lives on localityInfos — it moved to Program (ssa/package.go), so a note about parse markers doesn't belong on FreezeLocalityState. More importantly, the claim that parse markers "remain local to each Program" is contradicted by the code: the coordinator's markers are copied into the shared PackageSyntaxState snapshot in FreezePackageSyntaxState and read across all Programs via packageSyntaxBase in PackageSyntaxParsed (and package_syntax_test.go explicitly asserts a coordinator-marked package IS visible through the shared baseline). Only newly worker-discovered markers stay Program-local.
Suggest dropping this sentence from FreezeLocalityState, and if useful adding a corrected version to FreezePackageSyntaxState.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
8f51e28 to
a70be63
Compare
zhouguangyuan0718
changed the title
a70be63 to
4db979f
Compare
40ba68d to
9cc28fa
Compare
zhouguangyuan0718
changed the title
9cc28fa to
5b7167c
Compare
1 participant
Summary
Design boundary
This PR is stacked on #2280. It deliberately replaces the earlier snapshot/overlay approach: there is no freeze API, syntax base/delta, worker merge, syntax validation pass, or metadata copying. One-shot compiler users retain Program-local mutation; the build driver switches to read-only syntax mode only after all package preparation is complete.
Parallel scheduling remains in the following PR; this change only establishes the minimal safe state boundary it needs.
Validation