WIP: investigate runtime GC starvation during MakeFunc startup - #2573
WIP: investigate runtime GC starvation during MakeFunc startup#2573cpunion wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Review: fix MakeFunc GC starvation in test
A clean, well-targeted test-only change. The GC-stressing goroutine now runs runtime.GC() at least once before checking the stop channel, and throttles collections with a 1ms sleep instead of a back-to-back busy loop that starved MakeFunc/startup allocations in BDWGC on LLGo's pthread backend.
Verified:
- Rationale is accurate —
goschedBackend()is an empty no-op on the pthread backend (runtime/internal/runtime/proc_pthread.go), sotime.Sleepoverruntime.Gosched()is justified. - Teardown remains race-free via the
stopGC/gcDonehandshake; no leaked goroutines across subtests. - No security or performance regressions; the change removes a CPU-starving busy-GC loop.
One minor readability nit inline. Nothing blocking.
| return | ||
| default: | ||
| runtime.GC() | ||
| // Back-to-back collections can starve MakeFunc/startup allocations |
There was a problem hiding this comment.
[P3] Comment references Gosched, which is not present in the code
The comment says "Gosched is a no-op on LLGo's pthread backend," but runtime.Gosched() doesn't appear in this function. The note reads like PR-discussion rationale (why time.Sleep was chosen over Gosched) rather than a description of the code in front of the reader. Consider tying the "why" to the present code, e.g.: "Sleep rather than Gosched: Gosched is a no-op on LLGo's pthread backend and would still allow back-to-back collections that starve MakeFunc/startup allocations in BDWGC."
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The earlier |
|
Correction to the CI hypothesis: reconstructing the exact |
cpunion
left a comment
There was a problem hiding this comment.
diagnose-qiniu-shard0 — Run the temporary, single Linux diagnostic added in ef24e8d. This replays the actual failed merge 9467709 on [qiniu, ubuntu-24.04-large], with the original arguments, dependency setup, and the old main package-cache key used by attempt 1. The compiler and runtime are unmodified. The harness saves process snapshots, takes explicitly marked late native/Go stack samples, and enforces an external 18-minute deadline so artifacts can upload before runner teardown. This is evidence collection, not a production fix or a claim that the MakeFunc stress failure explains shard 0. The diagnostic commit skips the unrelated normal CI matrix.
cpunion
left a comment
There was a problem hiding this comment.
diagnose-qiniu-shard0 — Clean-checkout control using 63127b6. The preceding diagnostic run 34698144515 remains useful as a dirty-checkout control, but its nested diagnostic checkout can change Go build metadata and LLGo cache identity. This revision moves the harness and all output outside the measured checkout, verifies git status is clean before building, and records go version -m for the actual compiler. Both trials still use the same failed merge, Qiniu runner labels, source, and original test arguments; neither changes runtime behavior or introduces a retry fix. Only this dedicated Linux job is requested.
cpunion
left a comment
There was a problem hiding this comment.
diagnose-qiniu-shard0 — Compare the exact previously passing CI merge 40c7bf8 (PR head cd6b1e5) with the repeatedly timed-out merge 9467709, as requested. Each receives one Qiniu Ubuntu 24.04 large job with the same original dependency setup, old-main cache key, test arguments, and external diagnostics. Harness/output stay outside the source checkout. Run 34698417780 failed only the added clean-status preflight before compiler build, so it is not a test reproduction; this revision records status before/after the original setup instead of rejecting its generated files. No source cleanup, GC workaround, or retry is applied. Only these two bounded Linux comparison jobs are requested.
cpunion
left a comment
There was a problem hiding this comment.
diagnose-qiniu-shard0: Validate only the complete Qiniu Linux LLVM 22 / Go 1.27 shard-0 job for #2575 (merge abe30d3) and #2567 rebased onto it (merge 7c9b607). Include the same public weak-pointer stress test, and require the runtime-only lane to reproduce the old runtime self-deadlock with matching native stack evidence. Both target PRs remain [skip ci]; start their ordinary matrices only after both targeted jobs pass.
cpunion
left a comment
There was a problem hiding this comment.
diagnose-qiniu-shard0: Revalidate only #2575 merge abe30d3. Run 34702086188 already passed the complete #2567 job. The old-runtime public stress test reproduced the expected guarded self-deadlock there, but its stack allocated in mapdelete_fast64 instead of the validator’s previous mapaccess1_fast64-only pattern; this revision accepts either allocating weak-registry map operation while retaining the same-thread nested cleanup, nested GC finalizer, and weakState mutex requirements.
Draft — the allocator-starvation reproducer also fails on native Linux amd64 and still has no production fix. The separate Qiniu shard-0 investigation has now captured a different defect, a weak-cleanup self-deadlock; its production fix is isolated in #2575, not this PR. The original
Sleep(1ms)mitigation has been removed andtest/go/reflect_makefunc_goroutine_test.gois unchanged from main. Reducing test pressure is not an acceptable fix. The temporary Qiniu diagnostic workflow added below is investigation-only and must not ship as the production fix.Reproducer
Add
test/_stress/runtime/gc, with allocation-only and MakeFunc goroutine-startup workloads competing against continuousruntime.GC. Four workers each perform 256 operations by default; the MakeFunc workload exercises both pointer-argument and zero-argument callbacks. The collector neither sleeps nor yields, and no failed run is retried by the test. A five-second no-progress check runs on the collector thread itself so an allocating observer cannot be starved; an independent helper-process deadline also bounds failures of the tested runtime's timeout machinery. The output reports allocation, callback, and collection counts.LLGO_STRESS_PROFILEscales the workload, following the existing stress-suite convention. These tests are opt-in and are not added to normal PR or daily CI.Controlled result
With the same test executable built from unmodified main
b07cd12ad, identical parameters,GC_MARKERS=1, and the default stress profile on macOS arm64:runtime.GCcalls, zero completed allocation operations, zero callbacks-race -count=3, both stress testsA further three full-suite repetitions with the experimental library passed: allocation-only took 0.47–0.52 s, and MakeFunc took 6.88–7.27 s. The test executable was not rebuilt between the original-library and experimental-library runs. Existing unmodified
TestReflectMakeFuncGoroutineStartupalso changes from three independent 20 s external timeouts to repeated 2.86–3.12 s passes when only this mutex policy changes.With
GC_MARKERSunset, the same MakeFunc stress test also fails on the original library after 5.18 s (1,984 GC calls, zero operations/callbacks) and passes with the experimental library in 5.05 s (all 1,024 operations/callbacks, 1,814 GC calls). Thus the reproduction is not limited to the forced single-marker configuration.Cause and remaining work
Process sampling shows allocation and goroutine-startup threads blocked on BDWGC's allocator mutex while another thread repeatedly executes full collections. The fair-lock intervention strongly implicates unfair reacquisition of that lock on macOS. The experimental library patch is diagnostic only, is not included here, and is not a production fix: initialization ordering, other platforms, allocator performance, and supported dependency integration still need validation. A pure C pthread/BDWGC probe also demonstrates excessive collections during a small number of allocations without LLGo.
The Linux arm64 original startup test completes in 3.64–5.54 s. The subsequent native Ubuntu 24.04 amd64 / LLVM 22 comparison reproduced the stress failure on both unmodified main and the panic-fix branch: all six recorded MakeFunc stress trials failed, with either zero progress under
GC_MARKERS=1or an external helper timeout with default marker settings. Native stacks show allocator/thread-registration calls waiting for BDWGC's mutex while the collector runs full collections. Official Go controls pass. The diagnostic workflow itself succeeds because it preserves each trial's failure as data; that green workflow does not mean the stress tests passed. A production fix passing the same unthrottled tests is still required before this PR is ready.Separate Qiniu shard-0 timeout investigation
The MakeFunc test belongs to shard 1 and is not the unfinished test in the captured shard-0 failure. In diagnostic run 34698144515, 96 of 97 tested packages completed;
crypto/ecdsaremained blocked. Native samples at minutes 14 and 16 show the same thread holdingweakState.mu, allocating insidemapaccess1_fast64, recursively invoking another weak cleanup, and waiting for its own non-recursive mutex. The compiler waits for that test child, rather than looping in LLVM compilation. The old three one-hour timeout attempts lack surviving stacks, so they cannot each be assigned this cause with certainty.The unperturbed Qiniu comparison subsequently passed on both the previously green
cd6b1e5fsource tree and the later failed merge94677096; first test invocations took 120 and 138 seconds respectively. Both versions contain the same unsafe weak callback. Passing controls do not fix the reentrant lock or establish that its trigger frequency is unchanged.The actual weak-runtime fix and public-API pressure regression are in #2575. On local macOS arm64, the unchanged test fails on old runtime in 3/3 executions with matching native self-deadlock stacks, and passes with the fix 53 times with default GC markers plus 20 times with one marker. This result must not be represented as fixing the separate continuous-GC allocator-starvation tests above.
This draft temporarily hosts a review-triggered Qiniu workflow to validate the exact CI merge trees of #2575 and #2567 rebased onto it. Both target PRs are initially pushed with
[skip ci]. Each diagnostic job includes the complete original Linux shard-0 job, including symbol/build-mode checks and all later integration checks, followed by the weak pressure regression. The runtime-only lane also verifies the old-runtime failure with native stack evidence. The harness lives outside the measured source checkout; captured stacks and logs are uploaded, and a ptrace-sampled run cannot count as an unperturbed passing trial. Only after both targeted jobs pass will the target PRs' ordinary CI matrices be started. This draft's diagnostic workflow and unrelated starvation test are not included in either production contribution.