Repository navigation
wasm: make reflection entries callable across workers - #2743
Conversation
|
@fennoai please review the reflection follow-up range 87995c1..958f455, especially worker-local Wasm table correctness, typed bridge reachability/cache consistency and native backend isolation. Full std qualification reproduced the old backend trap on both Memory32/64. Existing reflection/defer/recover tests and compiler/cache tests pass with the fix; the new Node/Chrome worker fixture is running locally. The PR records the measured module size cost rather than claiming this backend change reduces size. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This PR reworks WASM Emscripten worker mode so syscall/js values are shared across workers (all remote JS ops forward to worker zero via a per-caller executor G), and switches Emscripten reflection from libffi to compiler-generated typed bridges so MakeFunc values survive crossing workers. The engineering is careful and well-commented: build-tag partitioning is mutually exclusive and exhaustive, the executor reuse / //go:noinline pattern is deliberate, the runqueue Remove/RemoveAfter/Front additions and the finalizer-mutex refactor are correct, and the bilingual proposal/doc updates match the code with no stale jsRealm/MarkCurrentJSRealm/libffi-reflection references remaining.
Findings below. The scheduler items (reentrancy + O(n) child scan) are the ones worth a close look; the rest are minor.
Body-level finding (location not inline-commentable):
Possible reentrant goschedBackend while a G is mid status-transition. goschedBackend (runtime/internal/runtime/proc_wasm_workers.go:399-408) transitions gp to _Grunnable via casgstatus(gp, _Grunning, _Grunnable) (line 402) before acquiring the worker lock in enqueueWasmG (line 403 → worker.lock.Lock(CooperativeSafepoint) at 449). On a contended lock, wasmsync.Mutex.Lock invokes the yield (CooperativeSafepoint). If the safepoint budget is exhausted and no GC stop is pending, cooperativeSafepointSlow (runtime/internal/runtime/safepoint_wasm_workers.go:63-65, not in this diff) calls goschedBackend() again when the runq is non-empty, re-running casgstatus(gp, _Grunning, _Grunnable) on a G that is already _Grunnable — a failed transition that would fatal. goready (line 431-433) has the same shape. This is narrow (needs same-worker lock contention + budget expiry at that exact poll + non-empty runq) and may be unreachable for reasons I can't fully confirm without running the scheduler, but since this PR makes cross-worker enqueues (and thus lock contention) much more frequent, it's worth confirming the queue-lock yield cannot reenter goschedBackend while a caller holds gp in a transient status.
Reflection follow-up review —
|
958f455 to
f54fe33
Compare
|
The body-level runqueue finding was confirmed with a controlled contended-lock/budget-exhaustion probe: the preemptive callback fails with invalid goroutine status transition; the in-place GC callback passes. Parent #2738 now uses the existing in-place GC wait at all three queue-lock sites (cc47f7a), with a regression exercising actual Gosched under contention both without and with concurrent GC. This PR is rebased onto that fix. The reflection changes also remove the obsolete Asyncify trampoline exclusions; C libffi imports remain supported. |
|
@fennoai please review only the reflection follow-up range cc47f7a..f54fe33; cc47f7a is the parent #2738 and contains the scheduler fixes discussed above. The follow-up replaces worker-local dynamic reflection entries with shared static ones, keeps reachability/cache gating, and removes obsolete reflection-only Asyncify exclusions. Full local Node/Chrome 2/4-worker Memory32/64 acceptance passes; fresh std qualification is pinned to the reflection fix at 958f455. Native/C libffi backends remain supported. The outstanding scan-complexity discussion belongs to the parent callback scheduler and is recorded as performance qualification rather than changed by this reflection patch. |
Reflection follow-up review —
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b14688f to
18a911e
Compare
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
6740337 to
2861a8d
Compare
2861a8d to
2189079
Compare
16207fb to
ad9784f
Compare
visualfc
left a comment
visualfc
left a comment
There was a problem hiding this comment.
Non-blocking size follow-up: keep typed bridges as the default for JS reflection, but consider an explicit flag to restore libffi when cross-worker sharing is not needed.
8ce2711 to
6fd8e06
Compare
Follows merged #2738.
reflect.MakeFuncgoroutines trapped with “table index is out of bounds” because libffi installed a dynamic entry only in the creating worker’s table. JavaScript providers now use compiler-generated Wasm reflection entries by default, shared by every instance. Preserve reachability gating, signature deduplication and cache fingerprints.Single-worker JS builds may opt into the previous libffi reflection backend with
-tags=llgo.wasm.reflect.libffi. Restore its originalFuncTypedescriptor and JS replay trampolines; enable their three Asyncify exclusions only for this tag. Reject the tag for multiple workers, explicit worker tags, WASI and native targets. Preserve the caller/GC exclusions,ffi_call_jsasync import and table-growth setting. Native reflection and C libffi retain their backends.Validation: profile/group/cache and build-entry rejection tests; Node/Chrome Memory32/64 with 2/4 workers; complete
test/gopackages for worker Memory32/64 and libffi Emscripten Memory32/64 plus GoJS. Compiler selector/validation functions have 100% focused coverage. Full-package tests use a 180s alarm within the existing 300s process limit, allowing CI headroom for repeated collections. The allocator progress, bounded GC metadata scans and rooted filesystem buffers from #2738 are already in main. Remove the duplicate segmented-heap constant in the host GC fixture so both heap modes compile and remain covered.The reflection sample’s single-worker Wasm decreases from 1,510,276 to 1,446,947 bytes with libffi; JS increases by 9,974 bytes. A default → libffi → default build/run sequence preserves results and the default Wasm size with the shared cache. Earlier default-bridge measurements grew large
test/gofrom about 15.75 MB to 18.20 MB; further bridge/metadata optimization is tracked in #2679.Rebased on main
45bdf2db2, which includes #2738 and #2742.