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
5b7167c to
c1728af
Compare
|
@xgopilot please review the current PR head (c1728af) from scratch. Focus on whether package syntax, locality, caller-tracking, runtime/Python package data, and frontend options are fully prepared before backend workers start; whether shared state is truly read-only; whether worker-local writes can leak or race; patched/alternate-package behavior; order independence and deterministic output; and regression-test completeness. Please ignore findings that only apply to earlier revisions. |
|
An internal error occurred. Please try again later. |
Review posted ✅Reviewed head Verdict: solid, well-tested preparatory change. No blocking issues.
Non-blocking points raised: (1) Verification limitation: could not run |
There was a problem hiding this comment.
Review — read-only package backend state
Reviewed head c1728af against base 6670dae over the locked range. Focus areas: freeze-before-share correctness, worker read-only state, patched/alternate packages, races, determinism, and missing tests.
Overall: solid, well-tested preparatory change. The state boundary is coherent: packageSyntaxData gathers all Go-owned source metadata (linknames, exports, closure-env, nointerface, type backgrounds, parsed-package set) behind one sync.RWMutex, and NewBackendProgram shares that plus localities read-only while giving each backend a fresh LLVM context, TargetMachine, and lowering caches. Field migration is complete — no stale readers of the old linkname/noInterface/closureEnvDirectives/gocvt.typbg/parsedPackages fields remain. gofmt is clean on all changed sources.
Correctness — verified
type_cvt.gorecursive-conversion rewrite is the riskiest change and looks correct. The newnamedNeedsTypeConversiondecides the whole recursive graph up front, then publishes the converted placeholder before descending, so mutually-recursive named types no longer depend on which cycle member is visited first. I checked theneedsTypeConversionpredicate case-by-case againstcvtType/cvtInterface/cvtStruct/cvtTuple/cvtUnion/cvtClosureand they stay in lock-step (Signature/Union always-convert, closure-struct never-convert, interface method+embedded coverage, etc.). The negative-result caching only storesconversionNotNeededwhen the top-level query is fully negative, so cycle back-edges can't poison the cache.TestNamedTypeConversionIsIndependentOfTraversalOrder,TestRecursiveGenericNamedTypeConversion,TestTypeConversionRequirementShapes, andTestRecursiveNamedTypesWithoutConversionKeepTheirIdentitycover exactly these hazards.- Generic instance underlying ordering is safe. The instance placeholder is published to
p.typsbeforeorigin.SetUnderlying(tund), butcvtNamed/cvtTypeback-edges only return the cached pointer without dereferencing.Underlying(), so no caller observes the pre-SetUnderlying(Int) underlying. //exportand C-packageX-prefix handling preserves prior semantics. Collection now records exports intoprogduringParsePkgSyntaxWithOptions, and preloadedinitFilesreplays them viaPackageExport→pkg.SetExport. The C-package conditionpkg.Name()=="C"matches the oldcPkg(pkgName=="C"atcompile.go:2293), andPreloadedSyntaxgates the split cleanly.- Determinism.
preloadPatchedPackageSyntaxsorts patch paths and clones syntax slices; caller-tracking sets are membership maps (order-independent);namedNeedsTypeConversion's map iteration only writes an order-independent value. No new nondeterminism.
Points worth confirming (non-blocking)
Precomputecompleteness is a precondition, not yet enforced. Criterion-2 inruntimeCallerFuncSetreaches intocallee.Pkgand lazily writesc.base[callee.Pkg]. If a static callee's package is ever absent from the slice passed toPrecompute, a worker would mutate the shared map during the concurrent phase (the future PR). Today everything still runs sequentially so there's no race, andAllPackages()is a reasonable superset — but the safety of the shared read-only maps hinges on this invariant. Worth an assertion or a comment tying it toAllPackages()when scheduling lands.newBackendSession/NewBackendProgramare not wired into the production build path yet (only tests reference them). So the read-only sharing is exercised by tests, not by real builds in this PR — expected for a boundary-only change, just flagging that end-to-end coverage arrives with the scheduler.SetPythonbehavior change: it now captures a possibly-nil*types.Packageeagerly instead of the previous lazydedup.Check(PkgPython).Types(which would have panicked on a nil result). This is strictly more defensive; just confirm no downstream code relied on the old non-nil guarantee.
Tests
Coverage is strong for the areas in scope: backend-program state sharing (ssa/backend_program_test.go, internal/build/backend_program_test.go), preloaded-syntax replay (cl/preloaded_syntax_test.go), concurrent caller-tracking reads (cl/caller_tracking_precompute_test.go), and the type-conversion rewrite. One gap: there's no direct test asserting two NewBackendProgram instances observe the same shared syntax mutation done on the coordinator after creation is complete (the invariant that they read, never write, shared metadata) — the existing tests check field identity and value propagation, which is close but not a concurrency assertion.
Limitation: I could not execute go test ./ssa ./cl ./internal/build in this sandbox — the LLVM C headers (llvm-c/Core.h) required by github.com/xgo-dev/llvm are not installed, so the LLVM-dependent packages don't compile here. The findings above are from static review; the PR's own validation notes report the suites, race tests, and ~92.3% patch coverage passing.
No blocking issues found.
| origin.SetTypeParams(list) | ||
| } | ||
| named := origin | ||
| if typ, ok := Instantiate(origin, t); ok { |
There was a problem hiding this comment.
Nice fix. Instantiating origin here and publishing the resulting instance as the placeholder before the cvtType(t.Underlying()) descent is what makes recursive generic back-edges (*My[T]) resolve to the converted instance. Verified that no back-edge dereferences .Underlying() during the descent — cvtNamed/cvtType return the cached pointer only — so filling origin.SetUnderlying afterward is safe. Covered by TestRecursiveGenericNamedTypeConversion.
| if linkCollected || item.Args == "" { | ||
| continue | ||
| } | ||
| if item.Args != inPkgName && !options.ExportRename { |
There was a problem hiding this comment.
Equivalence check vs the old initLink export path holds for the common single-token //export name form (Args is the export name; matches SetLinkname+SetExport). One edge case to confirm: the legacy initLink treated //export args by splitting on the first space, whereas directive.Parse puts everything after //export into item.Args verbatim. A two-token //export foo bar directive would now flow through as item.Args == "foo bar" rather than name/link split. Unusual for //export, but worth a line of test or comment if that form is meant to be supported.
| return nil, err | ||
| } | ||
| buildSSAPkgs(ctx, append(append(altEntries, pkgEntries...), depEntries...)) | ||
| ctx.callerTracking.Precompute(ctx.progSSA.AllPackages()) |
There was a problem hiding this comment.
The read-only safety of the shared caller-tracking maps depends on every package that can appear as a static callee during lowering being present here. runtimeCallerFuncSet (criterion 2) lazily writes c.base[callee.Pkg], so a callee package missing from this slice would cause a shared-map write once workers run concurrently (next PR). Sequential today, so no race — but consider asserting or documenting the AllPackages()-is-complete invariant so the scheduler PR can rely on it.
2 participants
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