runtime: migrate local state to locality directives - #2210
Conversation
There was a problem hiding this comment.
Review: runtime locality directive migration
Reviewed by four passes (quality, performance, security, docs) against the full PR diff. The locality-directive migration for getg/setg, caller.go, and the FIPS bypass flag is sound — the currentG uintptr slot is safe because the runtimeContext is AllocRoot/uncollectable, and the FIPS //llgo:gls flag preserves per-entry isolation (zeroed on LocalContext creation). No memory-safety defect was confirmed in those areas.
The findings concentrate in the sync.Pool rewrite and one SSA locality owner-filtering gap. Inline comments below.
Summary of concerns
sync.Poolunbounded retention (high):poolLocalsis a TLS/root-reachable map keyed by*Poolthat is never pruned. Every*Poolever pinned on a thread — plus its cached objects — is retained for the thread's lifetime, so a discardedPoolcan never be collected. Long-running processes that create many short-livedPools grow without bound.sync.Poolreuse regression (high):getSlownow unconditionally returnsnil; the victim cache and cross-PpopTailstealing are gone. ObjectsPuton one thread can never be retrieved by aGeton another, and objects are dropped rather than surviving a GC cycle — the core allocation-amortization benefit ofsync.Poolis reduced. If intended, please justify in the PR description anddoc/defer-tls-gc.md.sync.Poolhot-path cost (medium):pin()now does a Go map lookup (hash*Pool, probe buckets) on everyGet/Putin place of a singleatomic.LoadPointer. This is a per-op regression on the type's hot path.- SSA owner filtering not applied at lowering (medium): the new
PackageLocalitiesForowner tracking is used inPrepareLocalVariables, butplanLocalPackage(the codegen lowering path vialocalPackageFor) andvalidateLocalInitializersstill use the unfilteredPackageLocalities(path)— see inline.
Non-blocking review (event: COMMENT).
Findings without inline locations
runtime/internal/lib/sync/pool.go:124: Stale doc comment:pin()no longer disables preemption, and there is noruntime_procUnpin()anywhere in the package — callers (Get/Put) never unpin. Please update this comment to describe the new TLS-map lookup and drop the unpin obligation.runtime/internal/lib/sync/pool.go:63: After this rewrite,local,localSize,victim,victimSize, andonceare dead —grepfinds zero reads/writes ofp.local/p.victim/p.once/localSize/victimSizein the package. Consider removing them (keepingnoCopy/New) or adding a note explaining why they are retained; the field comments ("local from previous cycle", etc.) now describe unused state.
3bd7bf7 to
0c3a746
Compare
a4475dc to
05a84d1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5c1c5f5 to
8e672ef
Compare
8e672ef to
13452a3
Compare
|
补充处理: |
|
@fennoai 最新 head |
Re-review complete — LGTM ✅Re-reviewed the full 24-file diff (+903/-229) against base I submitted the detailed findings as a PR review. Highlights
Non-blocking note
Verification noteI reviewed correctness by reading. I could not run the Go test suite locally — the sandbox has no LLVM C headers, so the |
There was a problem hiding this comment.
Re-review at 150c757ab — LGTM
Reviewed the full 24-file diff against base 2f75ed28 on the latest head. The locality-directive migration is coherent, behavior-preserving, and well-tested. No blocking issues found. I could not run the Go test suite locally (the sandbox has no LLVM C headers, so the ssa/cl/internal/build cgo packages don't compile here); I relied on the green CI you reported at 150c757ab for build/test verification and reviewed correctness by reading.
What I verified
getg/setg lifecycle (runtime/internal/runtime/g_tls.go) — faithful to the removed g_pthread.go:
- Runtime-owned M threads (
mstart→setg(gp)) never touch the pthread key:currentGHasLifecyclestaysfalseon a fresh thread, sosetgonly writes the//llgo:tlscurrentGslot.mexitcallssetg(nil)(lifecycle-false path →currentG = 0) and frees the root exactly once — no double free, since the destructor-transfer branch is unreachable on runtime-owned threads. - Lazy main/foreign-thread contexts still install the pthread destructor via
setAutoG, matching the oldgetgpath plus the new TLS fast read. - The
setgownership-transfer branch (old != gp→destroyG(old)) is defensive for a future thread-reuse case; harmless today and correctly avoids freeing a transferred same-G root.
Build-tag partitioning — the new split files are mutually exclusive and exhaustive:
- caller:
(!llgo || baremetal)vs(llgo && !baremetal). - fipsbypass: both gated on
go1.26, same partition;fipsBypassDepthhas exactly one definition per configuration and one use site (fipsbypass_llgo.go, taggo1.26), so no missing/duplicate symbol. TestBaremetalRuntimeAvoidsLocalityDirectivesguards the stated bare-metal limitation directly.
Owner-aware locality metadata (ssa/locality.go, cl/locality.go) — the *For(pkg,...) API cleanly isolates standard vs. alternate (llgo/_pkg) packages that share canonical symbol names. NeedsLocalContext correctly gates declaration entries on activePackages and skips legacy entries that are shadowed by ownerless/declaration maps, so inactive alternate packages no longer force a LocalContext. PrepareInactiveLocalVariables + the build driver's active-root walk (internal/build/build.go) implement this end-to-end. Ownership edge cases are covered by the new cl/ssa/build tests.
Cleanup — removed SSA pthread-key helpers (pthreadKeyCreate/Getspecific/Setspecific + their signature caches) have no remaining references; caller.go no longer imports clite/tls.
Non-blocking observations
runtime/internal/lib/runtime/fipsbypass_llgo.gocarries only//go:build go1.26(nollgo), yet the!llgoglobal-state variant makesfipsBypassDeptha process-wide global rather than per-thread. This is fine for the overlay's host-compilation path and for single-context bare-metal, but the file's_llgoname slightly undersells that it also compiles for non-llgo hosts — worth a one-line note if it ever gains real host runtime semantics.- The retention rationale for
sync.Pool(dynamicclite/tls) and the_demo/c/thread/thd.gopthread.Keydemo are well-argued in the PR body anddoc/defer-tls-gc.md; agreed these are the correct boundaries under the current 1:1 P/M/thread backend. The doc now reads as an accurate map of the TLS/GLS/dynamic-TLS split.
Confirming your notes: the migration boundaries and sync.Pool dynamic-TLS semantics are unchanged, and the thd.go pthread-key demo is intentionally preserved. Nice work on the directive-lifetime documentation — the "record intended future lifetime even though TLS/GLS share one owner today" framing is clear.
Depends on #2209. Follow-up to #2079.
Summary
Lifecycle choices
TLS and GLS currently share one physical owner in the 1:1 backend, but the directives record the intended future lifetime.
Remaining dynamic TLS and pthread-key boundaries
getg no longer calls pthread_getspecific. Its pthread key remains only as a destructor sidecar for lazy main/foreign-thread runtime contexts; runtime-owned M threads use direct TLS and mexit.
sync.Pool remains the sole clite/tls consumer. Each dynamically created Pool needs its own per-execution-resource slot plus thread-exit victim handoff. Replacing it with one static TLS map adds a never-pruned *Pool key set, a map lookup on every Get/Put, and drops the existing victim behavior. Fully replacing this helper requires a dynamic locality/P-local API and GC-cycle pool cleanup.
_demo/c/thread/thd.go intentionally retains pthread.Key: it is a pthread API demonstration that verifies key isolation between the main and child threads, not runtime-owned locality state.
Bare-metal limitation
The current GLS package-block accessor uses a native TLS address cache. LLVM backends without native TLS relocation support therefore cannot lower even logically GLS state. Bare-metal runtime builds currently have one logical context, so caller/FIPS use ordinary-global fallback files there. This is an implementation limitation of locality lowering, separate from the stdlib-overlay ownership and activation fixes in #2209.
Tests