runtime: stabilize goroutine hooks and GOROOT classifications - #2421
runtime: stabilize goroutine hooks and GOROOT classifications#2421cpunion wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Solid, carefully-commented PR. The procPin/procUnpin move to procpin.go preserves the prior logic (renamed identifiers; both sync and sync/atomic hooks now route through one shared procPin() helper), the build tags are consistent, and the new tests (runtime_g_state_test.go, TestBaselineEnvironmentsForceLocalToolchain, TestWriteStdlibImportCfgIgnoresRepositoryModule, TestStackIsGloballyNotApplicable) lock in the intended behavior. The test-harness changes (GOTOOLCHAIN=local, go list std in t.TempDir()) are good hardening and were checked for shell-injection/TOCTOU — none found.
Findings below are all minor / non-blocking — mostly maintainability and documentation notes. No correctness defects in current behavior.
Additional note (not inline): LLGO_GOROOT_GOMAXPROCS is added to .github/workflows/goroot.yml (line 23) alongside the existing LLGO_GOROOT_HEARTBEAT_SECONDS / LLGO_GOROOT_VERBOSE, but like those two it is not read anywhere in the repo at these SHAs — presumably consumed by an out-of-repo harness. Consistent with existing convention; worth confirming the consumer exists.
Config consistency (test/goroot/notapplicable.yaml): since notApplicableConfig.Match now ignores version:/platform: selectors, the several version/platform-scoped entries in this file (e.g. the rangegen.go variants around lines 51-64) are functionally global. Consider adding a one-line note to the file header stating those selectors are inert for not-applicable entries, or validating them at load time, so a future maintainer isn't misled into thinking a version:-scoped entry actually scopes classification.
| procPinOnce.Do(initProcPinMu) | ||
| procPinMu.Lock() | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Panic-safety of the global serialization (minor / latent). procPin/procUnpin are not defer-paired, so if a caller ever panics between procPin() and procUnpin(), procPinMu stays locked and every subsequent procPin on any thread deadlocks. Today's darwin/linux callers (sync/atomic.Value.Store; the repo's patched sync.Pool uses a TLS slot and doesn't call runtime_procPin) don't panic while pinned, so this is latent — but the invariants it relies on are load-bearing and unstated:
procUnpinmust run on the same OS thread that calledprocPin(the mutex isPTHREAD_MUTEX_NORMALviaInit(nil); cross-thread unlock is UB), and- procPin regions must never nest (non-recursive mutex would self-deadlock).
Both hold under the current 1:1 goroutine↔OS-thread backend, but a future M:N scheduler change could silently break them. Consider a short comment recording these two invariants (and a "must not panic while pinned" note).
| // LLGo has no Go P to pin a goroutine to. Serialize procPin regions instead, | ||
| // preserving the exclusion that sync.Pool and sync/atomic.Value require. | ||
| func procPin() int { | ||
| procPinOnce.Do(initProcPinMu) |
There was a problem hiding this comment.
Per-call pthread_once on a hot path (minor perf). psync.Once.Do links to libc pthread_once, so every sync.Pool / sync/atomic.Value operation that hits runtime_procPin now pays a pthread_once call in addition to the mutex lock. pthread_once is cheap after first completion, but it's avoidable: pthread_mutex_init(nil) is equivalent to PTHREAD_MUTEX_INITIALIZER, so procPinMu could be eagerly/statically initialized (or initialized once in an init/first-getg) and the Once guard dropped entirely.
Separately worth flagging in a comment or tracking issue: the single process-wide procPinMu turns what is a contention-free per-P op in upstream Go into a globally serialized critical section, which becomes a scalability ceiling for sync.Pool-heavy concurrent workloads. This is the deliberate correctness tradeoff noted in the comment above — just calling out the ceiling explicitly.
| // Applicability is an LLGo design property, not an observation tied to | ||
| // one hosted platform or Go release. Keep accepting legacy selectors in | ||
| // the YAML, but intentionally do not use them when classifying a case. | ||
| if !matchEntry("", "", entry.Directive, entry.Case, goVersion, platform, tc) { |
There was a problem hiding this comment.
Ignored version/platform selectors are a maintainability trap (minor). Passing "", "" here intentionally makes not-applicable classification global, and the comment explains why. But because xfailEntry still parses version:/platform:, a maintainer can add a platform:-scoped not-applicable entry expecting it to be honored, and it will instead apply everywhere. Consider failing config load if a not-applicable entry sets version/platform, or documenting in notapplicable.yaml that those selectors are inert for not-applicable entries. (TestNotApplicableMatch and TestStackIsGloballyNotApplicable do assert the global behavior.)
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
Full-matrix result collected from the former combined tree: https://github.com/cpunion/llgo/actions/runs/32947330715
Both failures are addressed on the current #2421 head:
All other 4,168 observed cases were classified successfully. Standard CI is rerunning on the corrected head. |
|
Post-split full GOROOT validation for the current implementation head |
1 participant
Summary
This is the implementation half split from #2419. It is stacked on #2419 so that #2419 remains a one-file, deletion-only PR. The commits after #2419:
runtime.NumGoroutinereport LLGo's live runtime contexts;procPinhooks into the core runtime so bothsync.Poolandsync/atomic.Valueuse the same serialized implementation;goprint.goandfixedbugs/issue52612.go;GOTOOLCHAIN=local;notapplicable.yamlas a global design classification and accept either successful or ordinary failed execution while keeping resource-guard failures fatal;stack.gofrom four platform/version flake selectors to one global not-applicable entry;GOMAXPROCS=1, in addition to the existing build/run timeouts and RSS guard;Direct expectation removals
goprint.go: 2 flake selectors removed afterruntime.NumGoroutinelearned to track live workers;fixedbugs/issue52612.go: 2 flake selectors removed aftersync/atomic.Valuegained the shared core-runtimeprocPinhooks;stack.go: 4 flake selectors removed and replaced by one globalnotapplicable.yamldesign classification.The large set of already-passing hosted expectations is not deleted by these commits; that cleanup is isolated in #2419.
Full-matrix findings
fixedbugs/issue16016.go,chan/goroutines.go, andchanlinear.gointentionally create or repeatedly double enough goroutines to exhaust LLGo's current one-thread-per-goroutine host limit, so their host skips are global; the oldchanlinear.gotimeout and flake selectors are removed to keep classifications disjoint;index0.goreproducibly crashes LLVM 19 object emission below the 3 GiB RSS guard even withGOMAXPROCS=1;fixedbugs/issue34123.go, Go 1.26 Linuxconvert5.go, andheapsampling.gohave deterministic semantic mismatches and remain executed as ordinary xfails;typeparam/chans.gois genuinely flaky across platforms: on Go 1.26 macOS, bounded single-case repetitions produced one 61-second channel/select panic followed by two passes, so its former Linux-only flake selector is global.Relationship to other PRs
fixedbugs/bug449.goremoval remain owned by ssa: lower large array comparisons to runtime equality algorithms #2416;fixedbugs/issue5089.goandcldiagnostics remain owned by cl: preserve diagnostics for invalid method receivers #2417.Validation
GOMAXPROCS=2 go test -p=2 -parallel=2 -count=1 ./test/goroot;TestRuntimeNumGoroutineTracksWorkerswith Go 1.25 and Go 1.26;typeparam/chans.gorepetitions withGOMAXPROCS=1, explicit build/run timeouts, and a 3 GiB RSS guard;git diff --check;The complete 16-shard Darwin/Linux × Go 1.25.0/1.26.5 integration run received every report and observed all 4,170 selected cases. Its only failures were the now-reproduced and globally classified
typeparam/chans.goflake and the now-global host-unsafechanlinear.go; handling details and the run link are recorded in the PR comments.