Repository navigation
feat(wasm): integrate browser filesystem with bounded workers - #2696
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: share browser filesystem across scheduler workers
Solid, well-tested change. The build-tag matrix (_default/_wasi_threads/_stub pairs) is disciplined, the segmented-heap refactor threads segment.first-relative arithmetic consistently, and the STW protocol in wasi_gc_world.c is carefully guarded (excludes the requesting thread from the wait count, timeout fallback). No correctness-blocking defects found. A few items worth considering, inline and below.
Performance — segmented heap scan on the targeted path. segmentForBlock/segmentForAddress (segments.go:49, :60) are O(segments) linear scans, and they're called from the innermost GC primitives (gcStateOf, gcAddressOf) for every block in sweep/finishMark and inside Alloc's scan loop. Several call sites re-resolve the segment 2–3× for the same block (gcSetState, gcMarkFree). This PR's whole premise is that workers blocked in host FS calls force growHeap to add disjoint arenas (up to maxHeapSegments = 128), so under the exact workload it targets, per-block ops degrade toward O(blocks × segments). Consider hoisting the segment pointer out of the per-block loops (it's loop-invariant within sweep/finishMark) and passing it into the helpers rather than re-resolving.
Performance — FS proxy serializes byte payloads through JSON. Every proxied read/write embeds bytes as a decimal JSON array via Array.from (browser_fs.js:9, :97) plus multiple copies, on a synchronous cross-thread round-trip. Bulk I/O will be dominated by JSON encoding rather than the FS work. Consider transferring read/write payloads through shared linear memory (offset/length) and reserving JSON for the small metadata envelope.
Clarity — gc_wasm.c arena reservation. llgo_wasi_gc_init_arena() mallocs a fixed 32 MiB region backing llgo_gc_heap_base()/llgo_gc_memory_size(), but llgo_gc_new_arena(size) just malloc(size) and ignores those globals — so the _start/_end globals only ever describe segment 0 and llgo_gc_grow_memory can never actually grow on the wasi-threads path. The naming/comments imply a reserved region that later arenas carve from; either simplify to a per-segment malloc model or document that growth is segment-driven.
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 |
cpunion
force-pushed
the
codex/browser-fs-workers
branch
from
September 29, 2026 04:52
0d9e322 to
14c1d0c
Compare
Collaborator
Author
|
Addressed all four inline findings and the summary notes; rebased onto current main after #2539 merged. The remaining dependency is #2669 and this PR is ready for review.
Proxy contract tests pass for both pointer widths, including malformed requests, partial reads, repeated installation and callback exceptions. All four Memory32/Memory64 × 1/2-worker configurations pass in Node and Chrome after rebase. Fresh CI is queued. |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/browser-fs-workers
branch
3 times, most recently
from
September 29, 2026 11:34
246d737 to
c627831
Compare
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: the runtime-thread HEAPU8 view can go stale after a worker-side payload malloc grows WebAssembly.Memory. Fine to merge as-is; current Node/Chrome fixtures use small buffers and would not hit this.
cpunion
force-pushed
the
codex/browser-fs-workers
branch
from
September 30, 2026 05:48
c627831 to
249a8b3
Compare
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.
Browser filesystem calls need to work on every scheduler worker and refer to the same files, descriptors, and cwd as C code. Loading a host shim only in the embedding page leaves worker globals uninitialized or creates independent filesystem state.
Include the browser host in generated Emscripten modules and proxy worker operations to the runtime thread's Module.FS. Allocate response storage on the requesting worker to avoid forbidden Atomics.wait on the browser main thread. Keep Node's native fs, proxy its worker cwd/chdir, and make callback-bridge installation thread-local. Fix fstat to inspect the open node, including after unlink.
Rebased onto main at abe36f6, including merged #2669, #2695, #2539, and #2700; none are outstanding dependencies. Duplicate HTML host and callback bridge changes are supplied by main. Native runtime behavior is unchanged.
Validation on macOS arm64:
The four filesystem configurations now run in the existing worker CI job. The proxy transfers byte payloads through bounded shared memory and serializes only metadata. Filesystem persistence and throughput tuning remain separate work.
Review follow-up: allow-list host methods, install worker wrappers once, validate buffer ranges and returned byte counts, preserve partial-read offsets, and transfer read/write bytes through shared memory instead of JSON arrays. Memory32/64 proxy regressions cover malformed requests, callbacks, cleanup and repeated installation. All four Node/Chrome filesystem configurations pass again after rebase. The collector segment lookup/sweep improvement now comes from main via #2695; its duplicate contribution commit has been removed.
Memory-growth follow-up: refresh the runtime thread's memory views before decoding a worker request and checking its shared payload. The proxy regression now uses shared WebAssembly.Memory and forces worker-side allocation to grow memory for both large writes and reads, with offsets, byte contents, surrounding bytes, cleanup, and metadata-only JSON checked in both pointer widths. The new regression reproduces EINVAL before the fix and passes afterward. All four filesystem configurations pass Wasm validation, Node, and Chrome on this rebased head.