Repository navigation
Conversation
Collaborator
Author
|
@fennoai Please review the W2-A range |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: WASM W2-A host integration and reflection
This is a large, well-structured PR (163 files, ~7.5k additions) that lands the JavaScript-host boundary and reflection layers. Overall quality is high: build-tag partitioning across call_bridge_*/makefunc_* is careful, ABI/scheduler rationale is documented at the point of decision, and the Asyncify-replay boundaries in host.c/emval.cpp correctly re-fetch HEAPU8.buffer after Go-reentrant calls. Reviewed for correctness, memory safety, performance, and doc accuracy.
Findings below are non-blocking. The performance and KeepAlive items are the two most worth acting on; the rest are minor/robustness suggestions.
Verified safe (no action): reflection argument detachment deep-copies indirect args out of the transient FFI frame; result stores respect per-type alignment; conservative 4-byte wasm32 GC scanning keeps pointer bit-patterns in []uint32 slot arrays reachable; docs are unusually well-synced with code.
Contributor
|
FennoAI is processing your request, please wait a moment... |
Collaborator
Author
|
The first upstream run exposed an environment gap rather than a runtime failure: the xgo-dev qiniu runner does not preinstall Chrome, while the fork runner used by the component PR did. Commit |
Contributor
There was a problem hiding this comment.
Review: WASM W2-A host integration and reflection
Reviewed the W2-A range fcafb8546e97..ee39c0764529 across code quality, performance, security, and documentation accuracy.
Overall this is high-quality, well-tested work. The core design concerns you called out hold up under review:
- Provider isolation is correct. Typed
Call_/Make_bridges are gated onWasmProvider == "wasi"(wasm_reflect.go:40-41) and theusesWasmReflectBridges()double-check atssa/target.go, so JS/GoJS and Emscripten profiles emit no typed bridges and stay on the libffi backend. The build-tag split (functype_wasm_bridge.govsfunctype_default.go) reinforces this. - Cache invalidation is sound. Both
WasmReflectBridgesandWasmFuncInfoEntriesflow intocommonSectionand the fingerprintempty()check, so per-package cache entries invalidate when the reachability decision flips (covered by the fingerprint tests). - Host-boundary memory safety is careful.
host.c/emval.cppre-derive memory views after any growth-capable call, embedded-NUL strings pass explicit byte lengths, Asyncify replay saves/clears call-stack state to avoid re-running JS side effects, and emval handle lifetime is anchored to a finalizer-backedgcPtrcopied alongside everyValue. No memory-corruption or root-lifetime findings.
The findings below are optimizations and one benchmark/doc mismatch — none are blocking.
Low-confidence items (not posted inline)
runtime/_patch/syscall/js/host_llgo.go:175-186:hostEventDispatchingis reset tofalseinside the launched goroutine beforedispatchHostEvent()runs, so the flag isfalseduring actual dispatch. Likely benign under the single-threaded cooperative model, but the name doesn't match its lifetime (it guards only the event fetch). Consider renaming or a clarifying comment.runtime/internal/lib/reflect/makefunc_callback_js.go:70-74:storeJSMakeFuncResultNrounds every field offset by the return aggregate's single alignment rather than each field's own alignment. Mirrors the existing convention invalue.go, so any issue would predate this PR — worth confirming against the C-side layout inmakefunc_wasm_js.c.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Addressed the two low-confidence review-body notes as well on current head
Focused |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Re-review: WASM W2-A — final head 400bf710e
Re-reviewed the incremental changes since my prior review (ee39c07…400bf71, 19 files) against every point from the last round plus the new items you called out. All five findings are resolved correctly, and the new work is sound. No blocking issues.
Prior findings — resolved
- Shared RTA cache (P2).
configureWasmReflectBridgesandconfigureWasmFuncInfoEntriesnow go throughwasmProgramUseFor(ctx), which computeswasmReflectRoots+rta.Analyzeonce behind async.Onceon thecontextand sharesreachable/allbetween both checks. The duplicate whole-program analysis is gone. (internal/build/wasm_reflect.go:71-96,build.go:1450-1452) - Signature set (P2).
programMayCallWasmReflectBridgeIndirectlyreplaces the[]*types.Signature+ per-calltypes.Identicalscan with atypeutil.Mapand an O(1)functionSignatures.At(common.Signature())lookup. The O(calls × signatures) hot path is eliminated. (wasm_reflect.go:173-205) - Benchmark/doc mismatch (P3). README and the
reflectcallcomment now state that both W32 entries emit typed bridges and that only the namedw32-wasientry is timed (with both module sizes still recorded) to keep CI cost bounded. Accurate now. (benchmark/wasm/README.md:29,benchmark/wasm/main.go:62-64) - Host event flag rename.
hostEventDispatching→hostEventFetching, anddispatchHostEventnow has an explicit comment on why the handler's bool return is discarded. Scheduling semantics unchanged. (runtime/_patch/syscall/js/host_llgo.go:180-201) - Mixed-result libffi layout.
Value.call,bind1/bindn, and the JS store paths now walk libffi result fields viaffiResultField+ffi.TypeElement, using each field's own size/alignment instead of a single aggregate alignment, and clamp copies tomin(ffiType.Size, typ.Size_).storeMakeFuncResult1_jscorrectly threadscifthrough the C shim. This is the right fix for the(int64, int8, int16)regression. (makefunc.go:92-112,value.go:2604-2636,makefunc_ffi.go,makefunc_callback_js.go,makefunc_wasm_js.c)
New items reviewed — all good
- Host
KeepAlivecoverage.runtime.KeepAliveis now applied to every Go-owned buffer passed across the host frame boundary — string data, property keys, arg slices, and byte slices incopyBytesTo{Go,JS}/valueLoadString. This correctly prevents the GC from reclaiming backing storage while the host reads the raw pointers stored in the frame. Solid defensive coverage. - One-time sentinel init.
wasmReflectOnlyPointer/wasmMakeFuncInvokePointerare computed once ininit()rather than percallWasmBridge/makeProviderFunccall — removes repeatedValueOf(...).UnsafePointer()work from the reflection call path. (call_bridge_wasm.go:14-23) - GC-root shape encoding. Splitting
vkString/vkSlicefrom thebyte('a'+kind)arithmetic into explicits0r/l0rtokens removes the risk of an accidental collision with other shape letters (r,i) in the dedup/fingerprint key. Good robustness improvement. (ssa/reflect_bridge.go:133-142) ffi.TypeElementstays within bounds: callers index0..n-1over the result-field count, andffiResultFieldpanics rather than reading past a nil element.
Verified the helper wiring (align, memmove, ffi.Type.Elements) and the added regression tests (reflect_test.go, wasm_reflect_test.go, pointer_array_test.go). I could not compile locally — the sandbox lacks the LLVM headers that internal/build/ssa require — so I relied on source review; you've reported focused tests, actionlint, and git diff --check passing.
LGTM. 🚢
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 |
Collaborator
Author
|
@fennoai Please re-review current head |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Re-review: WASM W2-A — head 7d63156a1fa5
Diffed 400bf710e…7d63156 (the range since my previous LGTM). The only change is the single test-only fix you described, and it is correct.
internal/build/build_test.go (TestExecuteInitialPackageLinkRawWasmRunUsesHostRunner): PATH is now built as binDir + os.PathListSeparator + os.Getenv("PATH") instead of replacing PATH with binDir alone, applied consistently to both t.Setenv and the commandEnv.
This is the right fix:
- The temp
binDirstays first, so the linkednode/node.exestill shadows any real Node on the runner — the dispatch behavior under test is unchanged. - Retaining the inherited
PATHkeeps MinGW runtime DLLs discoverable on Windows, where the test binary (os.Args[0]) is the shadow executable. os.PathListSeparatoris the portable separator (;on Windows,:elsewhere), so it works cross-platform.
No production code changed since the previous approval. LGTM stands. 🚢
cpunion
force-pushed
the
codex/wasm-w2-host-reflect-20260913
branch
from
September 13, 2026 17:44
7d63156 to
8a7f17a
Compare
Collaborator
Author
|
@fennoai Please re-review W2-A after rebasing its 33-commit range from W1 fcafb85 onto the gated W1 head db71cc5 (new W2-A head 8a7f17a). Git range-diff reports all 33 commits exactly equivalent, and focused host-adapter, reflection, funcinfo, target, runner, script, and ffi tests pass locally. Please confirm content equivalence, prior findings remain resolved, and no interaction regression is introduced by current main through the rebased W1 layer. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Re-review: WASM W2-A — rebased head 8a7f17a8a
Confirmed content equivalence and no interaction regression through the rebased W1 layer. The rebase is clean; my previous LGTM carries over.
Commit-level equivalence
git range-diff fcafb8546..7d63156a1 db71cc582..8a7f17a8a reports all 33 commits = (byte-identical patches) — every commit maps 1:1 with no content delta, matching your report.
Net-content equivalence (stronger check)
I compared the full net W2-A diff against both bases:
git diff fcafb8546..7d63156a1(old range)git diff db71cc582..8a7f17a8a(rebased range)
Both are 6036 lines and byte-for-byte identical apart from git blob-hash index lines. Because the context lines are also identical, the rebased W1 head presents the same surrounding code at every site W2-A touches — so current main, carried in through the rebased W1 layer, introduces no interaction regression at the W2-A boundary. 29 of the 83 W2-A files also moved in the base (fcafb…4564a01), and even those show identical context, which is the direct evidence for the no-regression question you raised.
Baseline sanity
- W1 head
db71cc582is a proper ancestor of the new head, and exactly 33 commits sit on top of it. - PR base
4564a01aa(main) is the merge-base with the head — the range is cleanly stacked. git diff --checkclean; no conflict markers; working tree clean at head.
Prior findings
Since the W2-A content is identical to the previously-approved head 7d63156a1, all earlier findings (shared RTA cache, typeutil.Map signature set, host KeepAlive coverage, one-time sentinel init, libffi mixed-result layout, and the PATH portability fix) remain resolved by construction.
LGTM. 🚢
cpunion
force-pushed
the
codex/wasm-w2-host-reflect-20260913
branch
from
September 15, 2026 04:00
8a7f17a to
81562ba
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
1 participant
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.
Tracks #2152. Stacked on #2578; review this PR's 33-commit W2-A range after W1.
This combines the JavaScript-host and reflection layers because they share the host-call boundary and backend selection. It deliberately excludes scheduler/GC/standard-library completeness (W2-B) and broad acceptance (W3).
Scope
syscall/jssemantics for J32/GoJS (wasm32 with the Go-compatible JavaScript host), while retaining the Emscripten adapters for J32/Emscripten (wasm32 with the Emscripten JavaScript host) and J64/Emscripten Memory64 (wasm64 with the Emscripten JavaScript host).reflect.Value.Call,CallSlice, method expressions and values,reflect.MakeFunc, aggregate/multiple results, closures, variadics, and runtime-createdFuncOfdescriptors.Call_/Make_bridges on demand when whole-program reachability finds dynamic reflection. Deduplicate them by lowered LLVM signature plus GC-root shape, use compact package-local identifiers, and include selection in cache fingerprints.Validation
Current head is
81562bae43de, rebased onto main3ca152e610319. The review base is4488ff8ca9d2, the W1 foundation replayed onto that main; #2578 itself remains at6b7350457dc8. All 33 W2-A commits are patch-equivalent in range-diff (83 files, +4,278/-369). The sole conflict in the prerequisite W1 range preserves both main's entry-block local allocation and WASM physical storage/alignment. Local focused SSA/WASM and loop-allocation regressions pass. Current-head CI, coverage, review, and benchmark results are pending; the results below describe the pre-rebase head8a7f17a8a227.ssaWebAssembly lowering and constant extraction, C-layout offsets, target/raw-profile selection, runner/output/cache behavior, and function metadata.llgo testcommand gate passes the supported J32/Emscripten, J64/Emscripten Memory64, W32/WASI, raw J32/GoJS, and raw W32/WASI matrices.git diff --checkpasses, and the range adds no diagnostic probes, newt.Skip, TODO/FIXME markers, or temporary dependency changes.Size and build-cost audit
The consolidated benchmark passed and measured base and head sequentially on the same runner. Relative to W1, non-reflection
cprintfand empty/printlnmodules are effectively unchanged across all profiles (0.00%-0.01%, at most 14 bytes);fmtprintfis -0.22% to +1.14%. JavaScript/Emscripten therefore pays no typed-bridge module cost, and the W32/WASI bridge cost remains demand-driven.The apparent cross-run 6%-19% build-time increase was runner variance: the identical
mainbase was also 11%-18% slower in the consolidated run than in the W1 run. Paired base/head ratios remain consistent with W1. The repeated W32 reflection build completes in 56.45 seconds on the constrained two-core benchmark runner; this intentionally measures the only profile that emits typed bridges. The reflection fixture sizes are 1,828,137 bytes for J32/Emscripten, 1,829,529 bytes for raw J32/GoJS, 1,651,643 bytes for J64/Emscripten Memory64, 1,849,657 bytes for W32/WASI, and 1,921,038 bytes for raw W32/WASI.Review boundary
The intended review range is
4488ff8ca9d2..81562bae43de. It covers host/build adapters, JS runtime shims, callback/exception/memory-growth behavior, reflection/function metadata, provider-specific reflection backends, and their tests. It does not change the W1 data model or include the later scheduler/GC and broad acceptance layers.