Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This is a large, carefully-reasoned compiler change: a persistent per-SCC cache of escape-analysis facts plus the supporting escape/placement infrastructure. Most of the diff is regenerated test expectations; I focused on the production code (internal/goescape/*, internal/build/escape_cache.go, cl/escape.go, cl/compile.go, cl/instr*.go, ssa/*, internal/goflags/*, runtime helpers) and the new docs.
The cache correctness/trust model is sound. The component cache key transitively incorporates each dependency's own component key, and loadCache applies strong defense-in-depth (envelope digest, schema/version/key equality, per-function ID re-check, summary/object/capture count validation, MaxStackBytes bound, and the ReadOnly-only-on-non-heap-*ssa.Convert re-validation at cache.go:327-331). Every validation failure falls back to full analysis rather than emitting partial facts. Path construction from cache keys is hex-only and fixed-length, so no traversal is possible, and reads are size-bounded. I found no correctness defect that would cause miscompilation.
Findings below are performance and maintainability, not correctness. A few inline; one cross-file note:
Stack-buffer sizing policy is duplicated across three locations. The small-slice/string stack-buffer eligibility and capacity computation is expressed independently in cl/escape.go (escapeStackLayout and forceEscapeHeap) and in ssa/datastruct.go (makeSlice, using SmallStackBufferSize/elemSize capacity and the MaxImplicitStackVarSize bound). These must agree exactly: if the analyzer promotes a slice/string that lowering then cannot place within its buffer (or vice-versa), placement desyncs. They are consistent today, but the triplication means a future change to one constant/branch can silently diverge. Consider a single shared helper consulted by both the analyzer callbacks and the SSA lowering.
I also confirmed the new docs (doc/escape-analysis.md, doc/escape-analysis-comparison.md, benchmark/escape/README.md) are unusually accurate against the implementation (Version/const values, cache fallback invariant, capture-by-value semantics, LLGO_BUILD_CACHE, context limits, -m placement strings all match).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
314276b to
329df38
Compare
Use main's bootstrap safepoint handling and repeat fresh worker processes to cover GC during initialization without changing worker startup order.
7625c64 to
f31a33b
Compare
Warm builds currently repeat whole-program escape solving, even when dependency archives are reused:
Persist generic effects together with all object placements, stack budgets and capture layouts. Validate source/SSA, queried types, target/layout policies and actual dependencies; corrupt, incomplete or ambiguous records fall back to analysis. Atomic package shards batch I/O. Ordinary source bodies reuse archive-input identities; generic instantiations retain exact SSA fingerprints.
Persistence is opt-in: validation can cost more than solving small programs, so default builds keep the original analysis path.
Dependency SSA is still needed for RTA, caller tracking and LLVM body import. This skips repeated escape solving; it does not yet implement export-only dependency loading or claim a universal build-time speedup.
Depends on #3022 (which includes #3021); #2997 is merged. Incremental diff: cpunion/llgo@8b11bfa...feat/escape-summary-cache-3012
Validation: race tests; fresh-process cold/hot cache and caller-rebuild checks with 1/8 compiler workers; stale source/type/policy/dependency invalidation; corrupt and incomplete records; oversized-shard pruning and preservation of the last valid file; same-name wrappers, including different callees in one recursive SCC. Full compiler, SSA, escape, C ABI and compile-command suites pass; fresh-process cache and environment checks pass with explicit opt-in. Go runtime tests pass on native macOS/arm64 and Linux/amd64 and actual J32/J64/W32, including two-worker J32 and the complete J32/J64 browser-debug matrix.
Part 3 of #3012 (section 10).