Repository navigation
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
1 participant
Builds on #2738.
reflect.MakeFuncgoroutines trapped with “table index is out of bounds” because libffi installed a dynamic entry only in the creating worker's table. Reuse compiler-generated Wasm reflection entries for JavaScript providers so every instance has the same callable entry. Native reflection and C libffi retain their backends; remove obsolete Go-reflection JS trampolines/Asyncify exclusions. Preserve reachability gating, signature deduplication and cache fingerprints.Validation covers old-backend failure/new-backend pass, reflect/defer/recover, real profile/group/cache tests, and Node/Chrome Memory32/64 with 2/4 workers. Worker CI runs the complete
test/gopackage with its original 60-second limit. The standalone reflection fixture is restricted to Wasm workers. The parent fixes allocator progress, bounded GC scanning and rooted filesystem buffers; full Memory32 passes locally around 20–21 seconds. Rebased onto the parent follow-ups and latest mainbe669acd8; current hosted CI and new-source compatibility qualification remain pending.Size tradeoff on the minimal workload: Wasm grows about 62 KB and JS shrinks about 13 KB. Large
test/gogrows from about 15.75 MB to 18.20 MB; bridge/metadata size optimization remains tracked in #2679.