runtime/wasip1: add opt-in threaded GC for WAMR - #2669
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: WASI threaded GC support
Thorough, carefully-engineered change. The concurrency-sensitive code (STW epoch parity, wake-race snapshotting, GC-root rebuild guards) is well-reasoned and unusually well-commented, downloads are checksum-pinned, and invariant violations trap rather than corrupting memory. Ran four review passes (quality, performance, security, docs).
Highlights of what was checked and found sound: futex-backed waits (not spin loops) in wasmWorkerStopForGC/wasmGCStopTheWorld; overflow guards in AllocRoot/gc_wasm.c stack math; path-traversal protection in the test server.mjs; no-shell subprocess invocation in the python/emulator paths; the STW target/ready registration race (handled via initWasmWorkerGCSystem's increment-then-stop ordering).
The one item I'd treat as blocking-worthy is the llgo run exit-code regression (inline). The rest are performance/robustness suggestions.
Additional notes (no reliable inline anchor)
-
O(n²) goroutine churn in the GC-root registry (
runtime/internal/gcroot/gcroot.go): every goroutine spawn callsRegister→registeredLocked(O(n) duplicate-check scan) and every exit callsUnregister(O(n) linked-list walk), both under the single global registry lock that also blocksVisit. For workloads spawning many short-lived goroutines this is O(n²) under one lock. The duplicate-check scan could be debug-only, andUnregistercould use an intrusive doubly-linked list (asrootAllocationinroot_wasm_workers.goalready does) for O(1) removal. -
Binaryen source switched to a project fork (
.github/actions/setup-binaryen/action.yml): the toolchain now pullswasm-optfromgithub.com/xgo-dev/binaryen(tagllgo-v132.3) instead of upstreamWebAssembly/binaryen. Mitigation is solid — per-platform SHA-256 pinned inchecksums.txt, malformed-checksum guard, no trust in a server-provided.sha256. Flagging so a maintainer can confirm the fork + pinned hashes are intentional and verified.
Additional findings
internal/build/run.go:335: [P1] llgo run of a native binary loses the guest exit code:ModeRunnow returnscmd.Run()'s error directly instead of the previousmockable.Exit(s.ExitCode()). The error flows up torunCmdEx(cmd/internal/run/run.go:104-107), which unconditionally callsmockable.Exit(1). So a native program that exits with code 2, 3, ... now makesllgo runexit with 1, whereas before it propagated the child's exact code. The inline comment only justifies the zero-exit case and doesn't acknowledge the non-zero regression.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/wasi-threaded-gc
branch
2 times, most recently
from
September 27, 2026 21:19
f82231b to
d9546bd
Compare
Collaborator
Author
|
Thanks for the review. This branch is now restacked on current Correction on the callback-poll flag: I initially added a The GC-root registry's linear registration/removal under churn is a valid performance concern. I am keeping the current duplicate-registration check and list representation in this functional GC PR; an intrusive O(1) registry should be benchmarked and tested separately because it changes shared root-enumeration invariants. The restacked WAMR threaded-GC acceptance passes locally. The full J32 Worker test passed on rerun after one finalizer timeout, and the focused finalizer tests each passed five repetitions. CI will rerun on the callback fix. |
cpunion
force-pushed
the
codex/wasi-threaded-gc
branch
from
September 28, 2026 14:47
54d0796 to
aef734f
Compare
This was referenced Sep 29, 2026
cpunion
force-pushed
the
codex/wasi-threaded-gc
branch
from
September 29, 2026 09:28
aef734f to
884219e
Compare
Collaborator
Author
|
Addressed the source review of 884219e in ac4a1fd.
The global-mutex safepoint fast path, 20 ms polling and repeated linear segment lookup observations apply to this PR's reviewed head; their optimizations are already in dependent #2695. The dependent PRs are only being rebased onto this fix. No parallel collector or native scheduler redesign is included. Validation: |
Collaborator
Author
|
Fixed the two failure causes observed across the current PR stack in 63eb425 and ba903fd.
Validation: configuration regressions for amd64/arm64, absent mirror lists, preserved source/signature configuration and unrelated repositories; real #2695, #2696 and #2697 are only rebased onto these common fixes. #2697's duplicate timer-test commit is removed by rebase. Its previous head had no failed checks at the last audit. Fresh CI is running/queued for the updated stack; full CI success is not yet confirmed. |
visualfc
reviewed
Sep 30, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
Non-blocking follow-up: parkInitialWasiThread never acknowledges STW. Fine to merge as-is; the 500 ms skip path already covers uninstrumented C, and the current Goexit probes do not collect after main returns.
cpunion
force-pushed
the
codex/wasi-threaded-gc
branch
from
September 30, 2026 02:30
ba903fd to
e702f81
Compare
visualfc
approved these changes
Sep 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WAMR's WASI pthread backend currently requires
-tags nogc. WithLLGO_WASI_THREADS=1, this PR enables the linear-memory collector by default while preserving-tags nogcas an explicit override. It supports multiple application pthreads: one collector stops the others and performs serial, non-moving, conservative mark-and-sweep. The current scheduler still uses one goroutine per pthread.Threads publish compiler root chains and pthread stack bounds before acknowledging a stop. The runtime also roots host-TLS slots used by
sync.Pool, uses the correct pthread startup and main-thread TLS, and defaults worker stacks to 1 MiB. Go allocations occupy independent libc arenas so they cannot overwrite pthread stacks or TLS.Arena capacity includes block-state metadata and alignment padding. The allocator now accounts for both before choosing an arena size, fixing requests at and just below 32 MiB that previously added multiple arenas without finding a sufficiently large contiguous allocation. Overflow is rejected before calling the allocator. Adding a disjoint WASI arena under the allocator lock no longer requests another STW, avoiding a second 500 ms wait after an unsuccessful collection.
An uninstrumented C call can prevent a safe collection. After 500 ms, GC resumes stopped threads without marking or sweeping;
runtime.GC()returns without incrementingMemStats.NumGC. Repeated allocations may still attempt further collections and grow until the memory limit is reached. Foreign C threads that explicitly enter Go remain registered until pthread destruction, including while idle after a callback. Empty arenas are not returned to libc. These progress and memory-ownership limits are documented in the threaded GC contract.The branch also clears retired browser Fiber/Asyncify buffers to avoid retaining finalizable objects, parks the initial WASI execution environment on main
Goexit, and excludes the persistent timer service from the user-G deadlock count. The callback-poll hook keeps an explicit normal-return reset because a Godeferis unsafe on the system Fiber without a G.Validation:
go test ./internal/build -run '^TestWasmGC' -count=1passes.llgo run/test, startup with 100 goroutines, GC/nogc, filesystem access, weak references, select stress and focused W32 standard-library packages. The Node/Chrome worker acceptance and retired-fiber finalizer regressions were validated before this focused WASI follow-up. Fresh CI runs on the updated head.This contribution is rebased on main
07a0059ce. WASI pthread mode is still opt-in here. Follow-up #2695 addresses broader WAMR exception/thread stability, safepoint fast paths, GC-aware pthread waits and segment lookup cost; #2697 makes the thread profile the default. Those follow-ups are not claimed as changes in this PR.