Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: WASM bounded-worker syscall/js cross-worker sharing
This is a well-structured change. The forwarding/executor pattern is sound: the single-channel-per-G design serializes caller and executor (no intra-call concurrency), nested calls on worker 0 correctly bypass forwarding (isRemoteJSWorker() is false there, avoiding deadlock and recursive round-trips), the spawnWorker save/restore around the synchronous go fn() is correct, and the channel is lazily created per caller-G and closed in finishWasmG. Cross-worker handoff of the call/done struct is correctly synchronized by the unbuffered channel. The Goexit and panic paths are handled, and test/wasm/js-workers/main.go exercises shared values, nested callbacks, panics, Goexit, and executor-leak detection.
Findings below are mostly minor/latent plus one build-tag correctness issue. No high-severity memory-safety or security issues were found.
Additional notes not placed inline:
- Stale "realm affinity" comment (
runtime/internal/runtime/proc_wasm_workers.go:46-48, outside this diff's hunks): the comment "Override both inherited realm affinity and callback-event pinning" is stale — this PR removed thejsRealmaffinity model, so only callback-event pinning is overridden now. - Per-op round-trip amplification: each remote
syscall/jscall is a full synchronous round-trip to worker 0 (channel send → cross-worker switch →<-done). Method chains off worker 0 (e.g. iterating a JS array viaIndex/Length) become O(N) cross-worker handoffs where the single-worker build had O(N) direct FFI calls. There is no batching path. Worth documenting as a constraint for JS-heavy loops on non-zero workers. - Executor-G lifetime: one executor G is created per calling G and lives (holding a native + Asyncify stack on worker 0) until the caller G exits;
jsCallsis never closed early. Many short-lived caller Gs each touching JS once will accumulate executor Gs/stacks concentrated on worker 0. Consider documenting this, or a shared/bounded executor pool.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 |
8cbc757 to
b76a7b8
Compare
b76a7b8 to
8f26b66
Compare
|
@fennoai please review the updated single commit. It adds cooperative finalizer locking, callback scheduling/registry fixes and regression coverage. Complete local worker acceptance and the Node/Chrome 2/4-worker Memory32/64 JS matrix pass; no CI jobs were retried. |
Re-review completeReviewed the updated commit ( All three findings from the prior review are resolved: build-tag coverage gap, local-path closure allocation (now New mechanisms verified sound: the callback scheduling state machine ( Remaining notes (all minor):
|
There was a problem hiding this comment.
Re-review of updated commit (8f26b66)
Reviewed the full PR diff at the new head. This revision is high quality and addresses every finding from my prior review:
- Build-tag gap fixed —
worker_call_single.go,func_mutex_other.go, andmfinal_lock_wasm_other.gonow use the(!llgo || !llgo.wasm.workers)form, so the matrix is fully covered with no gap or overlap. - Local-path closure allocation fixed — forwarding closures are isolated in
//go:noinline remoteXhelpers (worker_calls.go), so SSA no longer hoists the heap alloc before theisRemoteJSWorker()check; the worker-0 fast path is allocation-free and the helpers are dead-code-eliminated in single-worker builds. - Stale
jsCallsafter Goexit fixed —CallMainJSWorkernow detaches (jsCalls = nil) and closes the channel beforeGoexit(), andfinishWasmGguards on non-nil, so there is no double-close.
I verified the new mechanisms the commit introduces:
- Callback scheduling (
popWasmWorkerRunq/popWasmCallbackChild+jsCallbackID+callbackResume): children are drained before reopening a host callback stack; id 0 is reserved as the "no callback" sentinel so non-callback children never spuriously match;runqueue.Removepreserves head/tail/size/queued invariants on every branch and is covered by the newrunqueue_test.go. - Finalizer locking (
mfinal_wasm.go+mfinal_lock_wasm_{workers,other}.go): the lazy-init-returns-locked contract is honored at all four call sites with a matching unlock on every path; the collector callback runscallWasmFinalizeroutside the lock. No deadlock or missed unlock. emscripten_set_immediate(workers.c): correctly removes thesetTimeout(0)~4ms nested-timeout clamp from every cross-worker handoff; available under the pthread/worker targets used here.
No memory-safety, use-after-free, deadlock, or data-race defects were found. The notes below are minor (performance + stale comments + one robustness hardening).
Notes not placed inline (outside diff hunks or whole-mechanism):
- Stale "realm affinity" comment —
runtime/internal/runtime/proc_wasm_workers.go:47:// Override both inherited realm affinity and callback-event pinningstill describes the removedjsRealmmodel. In the new code,independentSpawngates only the callback-event-pinning branch; worker affinity is now carried byspawnWorker(read first at line 316) and is unaffected byindependentSpawn. Suggest rewording to reference only callback-event pinning (andjsCallbackIDinheritance), dropping "both … realm affinity." - Possible dropped-finalizer window (please confirm) —
mfinal_wasm.go:97-114:tinygogc.AddFinalizerregisters the collector record (line 97) before the entry is published intowasmFinalizers.m[key](line 113); the callback early-returns ifm[key] != entry. This looks mitigated byKeepAlive(obj)at line 115 (the object stays reachable past the map store, so the collector cannot queue the callback in that window), but it is worth a quick confirmation thatobjPtrcannot be collected independently of whatKeepAlive(obj)retains (e.g. interior pointers). If safe, a one-line comment noting theKeepAliveordering guarantee would make the intent explicit.
8f26b66 to
3234270
Compare
|
@fennoai please verify the two updated findings at |
Both findings verified fixed at
|
3234270 to
4bd1194
Compare
|
@fennoai please review The focused changes avoid remote requests for reserved primitive Type checks and ValueOf(Value/Func/nil/bool), isolate host-only captures in a noinline helper, and use the nonpreempting funcMutex for callback poll registration too. The remote primitive benchmark (5000 Type/ValueOf pairs) changed from a median 149 ms/about 5 MB to 0.25 ms/0 bytes; this does not describe general JS host calls. The executor regression fails against the old forwarding implementation and passes the final implementation. Consolidated the 128-task GC stress in dedicated Node/Chrome worker acceptance, with an explicit stress argument; optimized filesystem smoke remains covered. Removed the generic browser timeout configuration and optimization timeout changes from this PR. The dedicated Memory64 browser stress has a bounded budget; other worker checks retain theirs. The filesystem success marker now appears only after successful cleanup, and an intentional panic is rejected by both Node and Chrome without that marker. No CI jobs were retried. Complete local worker acceptance, O3/Os/Oz Node/Chrome boundaries, single-worker JS and runqueue tests pass. Final filesystem checks pass all eight host/width/worker combinations. Same-checkout-path size comparison against main: cprintf/println unchanged, fmtprintf +3955 bytes (0.13%; code +312 bytes). Full source-specific qualification: https://github.com/cpunion/llgo-compat-ci/actions/runs/37407152952 . Two clarifications from the previous review: RemoveAfter is O(1) unlinking, but child matching still scans the queue; repeated scans behind unrelated nodes can still be quadratic, so there is no general O(1) scheduling claim. The finalizer publication order is unchanged from main: obj remains an SSA use through KeepAlive, which makes the compiler publish its interface pointer in the GC root frame across AddFinalizer and registry publication; roots remain linked through the function return. cl/gcroot.go and the root-frame implementation provide that lifetime, rather than an assumed LLVM KeepAlive intrinsic. Requests still allocate state and executors retain fibers until their callers exit; these costs are now documented. |
Review complete —
|
There was a problem hiding this comment.
Review of 4bd1194
Reviewed the incremental changes against the previously-reviewed head. This revision is focused and correct; I verified the new fast paths adversarially and found no correctness issues.
New local fast paths verified realm-invariant (the correctness-sensitive part):
Value.Type()now resolves the reserved refs (undefined/null/true/false, handles 2/4/6/8) before the remote-dispatch check. These are compile-time constants (not TLS) and match Emscripten's globally-reserved emval handles — dynamic handles start at ≥10, so there is no collision, and answering locally needs no realm state. Correct on any worker.remoteValueOfhandlesValue/Func/nil/boollocally (returns a copy or a reserved handle) and forwards the rest via the//go:noinline forwardValueOf, keeping request allocations in the host-only helper. Each local case is realm-invariant.func.go: movingemval_install_invoke/ensureCallbackPolloutsidefuncsMu, switchingcallbackPollto the nonpreemptingfuncMutex, and gatingstopCallbackPollLockedbehind!keepWasmCallbackPollintroduce no race, no lock-ordering inversion (funcsMu → callbackPollis the only nesting), and cannot wrongly disable the poll (const-true in the workers build).
The stale "realm affinity" comment flagged earlier is now accurate, and no MarkCurrentJSRealm/jsRealm references remain. The new regression tests (testPortablePrimitives asserting portable conversions create no executor, testCallbackYield covering nested-event resume scope, testExecutorLifetime, testFinalizerContention) directly exercise these paths.
On your two clarifications — both acknowledged and correct:
- Scheduling is not O(1). Agreed, and my earlier framing overstated it.
RemoveAftermakes the unlink O(1), butpopWasmCallbackChildstill linearly scans for the matching child, so repeated scans behind unrelated nodes remain potentially quadratic. There is no general O(1) scheduling claim — thanks for the correction. - Finalizer publication order. Understood:
objstays an SSA use throughKeepAlive, so the compiler keeps its interface pointer live in the GC root frame acrossAddFinalizerand registry publication (viacl/gcroot.go+ the root-frame implementation, not an LLVMKeepAliveintrinsic), and roots stay linked through the function return. That resolves my prior "please confirm" note; the ordering is unchanged from main and sound.
The documented costs (per-request state allocation, executors retaining fibers until their callers exit, non-O(1) child matching) are the right things to document. One minor readability note inline; otherwise no further findings.
4bd1194 to
2d5697d
Compare
cpunion
changed the title
2d5697d to
411e3ca
Compare
|
The CI callback timeout is fixed at Full local worker acceptance passes: Node/Chrome, Memory32/64, 2/4 workers, private roots, finalizers, sync.Pool and filesystem GC stress. It also exposed a nondeterministic portable-primitive assertion: process-global NumGoroutine can grow when a background G starts. The assertion now checks this caller's actual executor through a small internal testing accessor, which is unused and removed from ordinary modules. No CI jobs were retried. @fennoai please review these focused test changes and the executor accessor. |
Review complete —
|
There was a problem hiding this comment.
Review of 411e3ca
Scoped the diff to the three files that actually differ from the previously-reviewed head (4bd1194). All changes are test-only plus one test-only accessor, and all are correct. No findings.
js.goType() — adds the one-line comment I suggested last round (// Reserved refs are valid in every realm; resolve them before remote dispatch.), resolving my only outstanding note. No behavior change.HasMainJSWorkerExecutorForTesting()(js_worker_call_wasm_workers.go) — a read-only, nil-guarded accessor (gp != nil && gp.context.platform.jsCalls != nil) under thellgo && js && wasm && llgo.wasm.workersbuild tag. Referenced only bytest/wasm/js-workers/main.govia//go:linkname; no ordinary-module caller. It follows the package's established*ForTestingconvention (GMPForTesting,CurrentGForTesting,ParkForTesting,ReadyForTesting,SchedulerStateForTesting).testPortablePrimitives— replaces the flakyruntime.NumGoroutine()before/after delta (which a background timer/finalizer G could perturb) withhasMainJSWorkerExecutor()checks that inspect this caller's executor directly. Strictly more precise and deterministic, and it also asserts a fresh caller starts without an executor.testCallbacks— the CI-timeout fix: swaps the single aggregateresultchannel (8 waits) for aprogresschannel drained8*3times, bounding each child JS completion, synchronous return, and external event on its own axis instead of charging all concurrent GC/callback work to one 30s timer. The counts line up: each outer iteration emits 3progresssends (nested child + post-invoke + post-external-callback) × 8 = 24 =8*3. The original 30s no-progress bound per wait and the outer process bound are unchanged, as described.
The behavioral invariants from prior revisions (all eight callbacks, 128 nested/yielding calls, GC, synchronous identity assertions) remain in place. I relied on your reported local worker acceptance results (Node/Chrome, Memory32/64, 2/4 workers, finalizers, sync.Pool, filesystem GC stress); I don't have CI-log access and did not re-run the matrix.
411e3ca to
87995c1
Compare
|
The nested callback GC stress is corrected at Full local Node/Chrome worker acceptance passes (Memory32/64, 2/4 workers, private roots, finalizers, sync.Pool and filesystem GC). The callback/shared-value probe also passes under an emulated Linux 0.18-CPU quota. No CI jobs were retried; the code push triggered fresh CI. Standard-library qualification additionally exposed worker-local libffi MakeFunc table entries. A separate follow-up contribution will address that compatibility failure. @fennoai please review the callback GC barrier change. |
Review complete —
|
There was a problem hiding this comment.
Review of 87995c1
The only change vs the prior reviewed head (411e3ca) is the callback GC barrier in testCallbacks (test/wasm/js-workers/main.go, +12/-1). It is correct and well-commented — no findings.
The barrier works as intended:
- Each callback's child G now signals
gcReady <- 1and parks on<-gcReleaseinstead of callingruntime.GC()itself. - The main G drains all 8
gcReadysignals (so every callback child is parked with its JS roots live), runs oneruntime.GC(), thenclose(gcRelease)to release all children together.
Correctness points I checked:
- No deadlock / no lost signal —
gcReadyis buffered to exactly 8 (one per callback child), so no child blocks on the send before parking ongcRelease; the main G is free to reach the drain loop while the callback Gs park inawait(result)and the children run on pool workers.close(gcRelease)is the correct one-shot broadcast release for all 8 blocked receivers. - Single collection covers all roots — GC now runs once while all eight callbacks' roots are simultaneously live, replacing eight serialized collections that each charged their cumulative wall time to an independent 30-second result deadline. This matches the stated CI-timeout fix.
progressaccounting unchanged — the child still emits itsprogress <- 1after the barrier, so the final8*3drain still balances;gcReady/gcReleaseare separate channels and don't interfere. All 30-second waits, the eight synchronous/external callbacks, and the 128 yielding/nested invocations remain.
Concurrent (unsynchronized) GC stress is still exercised by the separate shared-values and finalizer tests, as you note, so moving this one test to a coordinated single collection doesn't reduce coverage.
Noted for the record: the follow-up on worker-local libffi MakeFunc table entries is out of scope here. I relied on your reported local acceptance results (Node/Chrome, Memory32/64, 2/4 workers, the 0.18-CPU-quota probe); I have no CI-log access and did not re-run the matrix.
|
@fennoai please review cc47f7a, especially the three runqueue waits now using the existing in-place GC callback. A controlled contended-lock/budget-exhaustion probe reproduces invalid goroutine status transition with the old callback and passes with the fix. The permanent fixture exercises actual Gosched under contention both with and without GC, and full Memory32/64 four-worker JS tests pass. onJSWorker now always invokes its callback rather than discarding a false execution result. The callback-child scan is confined to active synchronous Go callbacks; ordinary remote property operations remain FIFO. Full worker acceptance and fresh CI are running. |
Review complete —
|
There was a problem hiding this comment.
Review of cc47f7a
Scoped to the six files that differ from the prior reviewed head (87995c1): the three runqueue-wait yield-callback changes, the onJSWorker always-invoke fix, a comment relocation, and a new test-only fixture. All correct — no findings.
Three runqueue waits → wasmGCAllocatorYield (the core fix) — correct.
enqueueWasmG, popWasmWorkerRunq, and wasmWorkerRunqLen now acquire worker.lock with wasmGCAllocatorYield instead of CooperativeSafepoint. I verified the two callbacks:
CooperativeSafepointcan, in its slow path, swap to the system fiber for a GC request and callgoschedBackend()when the runq is non-empty — i.e. it can reschedule / transition the current G mid-wait. Running that while the caller is manipulating the runq (a G that is "runnable but not queued") is exactly what re-enters the status transition and produces the invalid-transition fault.wasmGCAllocatorYieldonly acknowledges an STW request in place viawasmWorkerStopForGCand explicitly does not schedule another G (it may sit below a foreign C frame). The same G resumes after the world is released.
So the swap removes the illegal reschedule from these brief runq critical sections while still honoring STW. Mutex.Lock(yield) calls yield while spinning and tolerates nil, so the contract holds. The enqueueWasmG comment accurately describes the "runnable but not queued" hazard.
onJSWorker now always invokes fn — real correctness fix.
Previously onJSWorker returned CallMainJSWorker(fn)'s bool and every remoteX helper discarded it. CallMainJSWorker returns false (without running fn) when the caller is already on worker 0 / no worker. If the G is on worker 0 at the CallMainJSWorker re-check (e.g. a race against the earlier isRemoteJSWorker() test), the old path skipped fn entirely and the helper returned a zero-valued result. The new onJSWorker runs fn() locally in that case, so the result is always produced. The single-worker variant (fn()) matches.
FIFO preserved. popWasmCallbackChild remains gated behind an active jsEvent, so ordinary remote property operations are unaffected; only active synchronous Go callbacks trigger the child scan. Unchanged by this diff.
Comment relocation (executor-reuse note moved from runMainJSCall to CallMainJSWorker) correctly attaches it to the function that lazily creates the per-G executor. Cosmetic.
New fixture ExerciseWasmRunqueueTransitionForTesting — build-tag confined (llgo && js && wasm && llgo.wasm.workers), referenced only by the test via //go:linkname. It faithfully reproduces the failing scenario: holds the worker lock from another worker, forces budget exhaustion (pollbudget.New(1)) so an exhausted budget must not re-enter Gosched, then drives goschedBackend() under contention with GC racing — run both with a no-op and with runtime.GC. A permanent fixture that reproduced the fault on the old callback and passes on the new one is the right regression guard.
I relied on your reported acceptance results (Memory32/64 four-worker JS, worker acceptance); I have no CI-log access and did not re-run the matrix. The libffi MakeFunc follow-up remains out of scope.
|
@fennoai Please review the focused GC follow-up in 875508f and 584723c: contiguous-heap/zero-word scan fast paths and bounded handoff to contending allocators after collection. In the combined reflection-worker source, the unchanged full test/go package passes Memory32/64 under its 60-second limit; the non-yielding collector exceeds the 90-second diagnostic process bound. GC waits remain in place and do not swap Go fibers below foreign frames. |
Review complete —
|
There was a problem hiding this comment.
Review of 584723c (GC follow-up)
Scoped to the six files that differ from the prior reviewed head (cc47f7a): two GC scan fast paths, the bounded post-collection allocator handoff, a Contended() accessor, and a CI matrix split. All correct — no findings.
Zero-word scan fast path (tinygogc/gc_tinygo.go startMark) — skipping word == 0 before segmentForAddress is a safe hot-loop short-circuit: a null word is never a heap pointer, and the lookup would return nil for it anyway, so behavior is unchanged.
Contiguous-heap fast path (tinygogc/segments.go segmentForAddress) — correct and risk-free. It is gated on the compile-time const segmentedHeap (false only for the default contiguous heap, true for wasi-threads), so the branch is dead-code-eliminated where it doesn't apply. The fast path checks heapSegments[0] with start <= addr < metadata, which is exactly what the single-segment binary-search path returns (it only yields the segment when addr < metadata, else nil). Semantically identical, just without the arena index + binary search per scanned word.
Bounded post-collection handoff (tinygogc/mutex_wasm_workers.go unlockForGC) — sound. I confirmed unlockForGC is called only from GC() right after the collection cycle completes and the world resumes; the ordinary Free/alloc path uses plain unlock, so hot-path allocation is unaffected. The bounded wait is further gated on Contended(), so uncontended collections pay nothing. gcHandoffWord stays 0 with an expected value of 0, so wasmworkers.Wait(&gcHandoffWord, 0, 1_000_000) is a bounded ≤1 ms futex-timeout yield that is never woken and cannot stall — it just gives a woken remote mutator a scheduling window before the collector (possibly the only runnable G on its worker) re-wins the allocator CAS. This is a timed wait, not a fiber swap or Gosched, so it upholds the "GC waits don't swap Go fibers below foreign frames" invariant.
Mutex.Contended() (wasmsync/mutex.go) — read-only atomic accessor over waiters; correct.
CI matrix (llgo.yml, test_ci_workflows.py) — adds a dedicated workers suite with consistent step gating: Chrome now set up for matrix.suite != 'test-command' (i.e. both runtime and workers); Wasmtime / Wasmer / single-worker scheduler steps are gated off for workers; the bounded browser-workers step now runs only on workers. The expected job count bump (21 → 22) matches the added matrix leg.
I relied on your reported acceptance results (reflection-worker source, Memory32/64 under the 60s limit; the non-yielding collector exceeding the 90s diagnostic bound); I have no CI-log access and did not re-run the matrix.
58f7baa to
d2611e8
Compare
|
@fennoai The final scan follow-up is d2611e8: skip only aligned, bounded, completely zero 16-byte spans on Memory32; nonzero words still use the original four-byte Go/C pointer scan. Full test/go passes on latest main for Memory32/64, and the WAMR threaded-GC acceptance passes. All allocator handoff/scan changes are now rebased on main f167e3c; the experimental ticket lock, epoch-loop change and reflection cache are not included. |
Review complete —
|
There was a problem hiding this comment.
Review of d2611e8 (final scan follow-up, rebased on f167e3c)
I diffed the PR against its new declared base (f167e3c...d2611e8) so the rebase's upstream SIMD/CPU-feature churn is excluded — that machinery is part of the new base, not this PR. The PR's own change set is the familiar wasm-workers + GC content I've reviewed across prior revisions, and the only new logic this revision is the zero-16-byte-span scan fast path in tinygogc. It is correct — no findings.
Zero-span scan fast path (gc_tinygo.go startMark, with scanword_wasm32.go/scanword_default.go):
if gcScanZeroSpans && addr&7 == 0 && end-addr >= 16 &&
*(*uint64)(addr) == 0 && *(*uint64)(addr+8) == 0 { addr += 16 - gcScanWordSize; continue }
- Platform-gated:
gcScanZeroSpansis aconst—trueonly inscanword_wasm32.go(!memory64,gcScanWordSize = 4),falseinscanword_default.go(memory64 / baremetal). So the 16-byte skip compiles in only on Memory32 and is dead-code-eliminated elsewhere, matching "on Memory32." - Alignment:
addr&7 == 0makes both*(*uint64)loads 8-aligned. Blocks arebytesPerBlock = wordsPerBlock(4) × 4 = 16bytes on wasm32, sostart/endare 16-aligned andaddrstays 16-aligned across fast-path hops (+16) and 4-aligned on the fallthrough (+4). - Bounded:
end-addr >= 16guarantees both loads and the+16advance stay within[start,end)— no read past the object and, sinceendis 16-aligned, theaddr != endloop still terminates exactly. - No missed pointer: the span is skipped only when all 16 bytes are zero (= four zero 4-byte words), which hold no pointers. Any nonzero sub-word fails the condition and falls through to the original 4-byte
loadGCScanWordscan, preserving Go-low-word / C-either-word discovery. The conservative scan cannot drop a live root. - Advance math:
addr += 16 - gcScanWordSize(+12) plus the loop's+gcScanWordSize(+4) = exactly one 16-byte span, no word skipped or overshot.
The adjacent word == 0 single-word skip was already reviewed at 584723c and is unchanged.
I confirmed the experimental ticket lock / epoch-loop / reflection cache you mentioned are not present in the diff. I relied on your reported acceptance results (test/go on latest main for Memory32/64; WAMR threaded-GC acceptance); I have no CI-log access and did not re-run the matrix.
|
@fennoai Please review the GC metadata follow-up in 1bf3662. The full Memory32 worker qualification hit its 60-second package deadline during repeated collections. Contiguous block lookup now uses the single arena directly. Object-end lookup, overflow rescanning and sweeping batch only complete metadata bytes; mixed/partial bytes retain individual state handling, and freed tail groups are cleared over the same heap range as before. The new wide-graph regression exercises mark-stack overflow, mixed allocation lengths, live contents and zeroed new allocations on Memory32/64. Full test/go passes locally in about 24/7 seconds under the original 60-second limit; threaded GC passes on both WAMR and pinned patched Wasmer. #2743 is rebased onto this change; current contribution CI is running. |
Review complete —
|
There was a problem hiding this comment.
Review of 1bf3662 (GC metadata follow-up)
Both heads share base f167e3c, so this is a clean three-file delta: whole-metadata-byte batching in tinygogc plus a wide-graph regression. I verified each batch is behaviorally identical to the per-block path it accelerates (states: Free=00, Head=01, Tail=10, Mark=11; blocksPerStateByte = 4; blockStateByteAllTails = 0xAA). No findings.
segmentForBlockcontiguous fast path — gated onconst !segmentedHeap; checksheapSegments[0]withfirst <= block <= last(the inclusive upper bound matches the binary-search path's in-range boundary) and panics identically otherwise. Mirrors thesegmentForAddressfast path from the prior revision.gcFindNextInall-tails skip — when byte-aligned with ≥4 blocks left and the byte is0xAA(four tails), skip 4; otherwise fall through to the per-blockblockAddr++. This is the forward mirror of the long-standinggcFindHeadInbyte-skip, and an all-tails byte provably contains no object boundary.finishMarkno-mark-byte skip — the teststateByte & (stateByte>>1) & (blockStateByteAllTails>>1) == 0is exactly "no11(Mark) nibble":& (>>1)sets a state's low bit iff both its bits are 1 (Mark), and0xAA>>1 = 0x55masks the low bit of each 2-bit state. SincefinishMarkonly acts on Mark blocks, skipping a byte with no marked head is a no-op — correct.sweepall-free / all-tails batches —stateByte == 0adds4 × bytesPerBlockfree bytes (identical to fourblockStateFreecases, and leavesfreeCurrentObjectuntouched).stateByte == 0xAAwithfreeCurrentObjectzeroes the metadata byte andMemsets4 × bytesPerBlockof heap,freed += 4— I confirmed this exactly reproduces fourgcMarkFreeIncalls, which each clear the nibble and zero the block'swordsPerBlockwords, so the heap range cleared is the same as before. WithfreeCurrentObject == falsethe tails are live and skipped untouched, matching the per-block path.
All batches are gated on %blocksPerStateByte == 0 && remaining >= blocksPerStateByte, so partial/trailing bytes always fall through to the exact per-state handling, and block += blocksPerStateByte - 1 + the loop block++ advances exactly one byte with no overshoot. "Mixed/partial bytes retain individual handling" and "freed tails cleared over the same range" both hold.
The new gc_bitmap_test.go (wide graph → mark-stack overflow, varied object lengths, live contents, zeroed new allocations) directly exercises all four touched paths on Memory32/64.
I relied on your reported results (test/go ~24/7s under the 60s limit; threaded GC on WAMR and pinned patched Wasmer); I have no CI-log access and did not re-run the matrix.
1bf3662 to
ab4626f
Compare
|
@fennoai Please review the two follow-ups in ab4626f (rebased on main be669ac). Browser Memory64 filesystem GC stress exposed raw malloc buffers held only by JS numbers; payload/request/response now use explicit rooted allocations and release every root in finally. The regression forces collection between allocations, reproduces zeroed payloads with the old proxy, and checks Memory32/64 data and root cleanup. Actual Chrome stress passes in both widths. The GC scan now batches four aligned metadata bytes (16 blocks), bounded by each arena's usable blocks, with byte fallback at mixed/partial boundaries. Existing live graph/overflow tests pass in Memory32/64; host tests exercise metadata alignment, end boundaries, retained/freed tails, cleared storage and counters. WAMR and pinned Wasmer threaded GC pass. The host extraction test now includes the real batch constants and segmented-heap selector, fixing its native CI build failure. No CI retries or deadline increases. |
Review complete —
|
There was a problem hiding this comment.
Review of ab4626f (two follow-ups, rebased on be669ac)
Reviewed against the new base (three-dot) so the rebase's upstream churn (setup-gdb, ssa/di.go, test/goroot/*) is excluded. Both follow-ups are correct — no findings.
1. Browser FS rooted-allocation fix — a genuine GC-safety fix.
The proxy's payload/request/response buffers were allocated with raw _malloc, whose only reference was a JS Number — invisible to the Go collector, so a collection between allocations could reclaim and zero them (the Memory64 stress failure). They now use llgo_browser_fs_malloc/_free, which route through AllocRoot/FreeRoot. I confirmed tinygogc.AllocRoot links the allocation into the rootAllocations list (scanned, uncollectable until freed — equivalent to GC_malloc_uncollectable), so the buffers survive any intervening collection, and the finally block releases all three on every exit path via FreeRoot (which unlinks and frees). The //export llgo_browser_fs_malloc/_free names match the JS _llgo_browser_fs_* calls and the updated __deps; float64 keeps the boundary numeric in both widths (a Memory64 address fits a double's 53-bit mantissa for realistic heaps). Build tag llgo && js && wasm is right. The .mjs test now zeroes non-rooted allocations on each _malloc (modeling collection) and asserts roots.size == 0, directly reproducing the old zeroed-payload bug and verifying root cleanup in both widths.
2. 16-block (4-byte word) metadata batching — sound extension of the byte batching reviewed previously.
blockStateWordAllTails = 0xAA × 0x01010101 = 0xAAAAAAAA; the finishMark no-mark test states & (states>>1) & (blockStateWordAllTails>>1 = 0x55555555) == 0 is the exact 16-block analogue of the byte test (11/Mark detection); the sweep word cases (states == 0 → 16×free; states == 0xAAAAAAAA + freeCurrentObject → zero the metadata word + Memset 16×bytesPerBlock + freed += 16) reproduce 16 per-block operations, consistent with the gcMarkFreeIn metadata-clear + block-zero semantics I verified last revision. Every word path is gated on metadata&3 == 0 (safe aligned 32-bit read) and segment.last-block >= blocksPerStateWord, falling back to the single-byte batch and then per-block for 4-byte-unaligned metadata, partial/trailing bytes, or mixed states — so "bounded by each arena's usable blocks, byte fallback at mixed/partial boundaries" holds. gcFindNextIn's loop restructure preserves break-on-first-non-tail. The advance (+= blocksPerStateWord - 1 then loop block++) is exactly 16 blocks.
The host extraction test (wasm_gc_segments_test.go) now pulls in gcFindNextIn, the blockStateByteAllTails/blocksPerStateWord consts, and a segmentedHeap selector, which is what fixes the native CI build; the new TestSweepMetadataBatches (8 offsets × keep/free) and TestContiguousHeapLookup exercise the batch boundaries, retained/freed tails, cleared storage, counters, and the contiguous lookup at partial-byte offsets.
I relied on your reported results (Chrome Memory32/64 FS stress; live-graph/overflow + host metadata tests; WAMR and pinned Wasmer threaded GC); I have no CI-log access and did not re-run the matrix.
1 participant
Allow goroutines on different Emscripten workers to share public
syscall/js.Valueandjs.Func. Host operations and callbacks execute in Go main's worker-zero JavaScript realm; remote callers park for synchronous results or exceptions and reuse an executor that retires with the caller. Worker-zero calls and reserved primitives avoid forwarding allocations; raw C/Emscripten handles remain local to their worker.Support nested/yielding callbacks, Goexit defers and GC-safe executor cleanup. Contended runqueue/allocator paths acknowledge GC in place without replaying foreign frames. A bounded host handoff prevents a continuous collector from starving allocators. Contiguous heaps avoid arena searches; object/metadata scans batch bounded, aligned zero/free/tail spans while retaining individual handling for mixed states and partial boundaries.
Root filesystem proxy payload/request/response allocations until their JS callers finish: numeric JS addresses alone do not keep Go allocations alive. A deterministic regression collects between allocations and checks data plus cleanup in both memory widths; the original proxy loses payload bytes under this test. Chrome Memory32/64 filesystem GC stress now passes. Add host metadata tests for segment boundaries, alignment, retained/freed tails and zeroed storage, alongside the Wasm wide-graph/overflow regression.
Validation includes Node/Chrome Memory32/64 with 2/4 workers, eight live callbacks/children at a GC barrier, private roots, exceptions, finalizer contention and executor retirement. WAMR and pinned Wasmer threaded GC pass. With #2743, full Memory32
test/gopasses locally in about 20–21 seconds under the original 60-second limit; hosted CI and full compatibility qualification remain pending for this head. Rebased on mainbe669acd8. The dedicated worker CI retains every acceptance case and its existing bounds.Fixes #2737.