Fix: preserve force_interpreter setting after invokeInterpreterOnly (#31)#32
Conversation
jtakakura
force-pushed
the
fix/force-interpreter-persistence
branch
7 times, most recently
from
e42843f to
a1ed281
Compare
jtakakura
force-pushed
the
fix/force-interpreter-persistence
branch
from
a1ed281 to
c08d5e6
Compare
…ly (clojurewasm#31) The v1 fix for clojurewasm#31 added persistent module.{fuel,timeout_ms,force_interpreter} fields and had invoke() apply them unconditionally on every call. That broke the exact pattern clojurewasm#31's reproduction cites -- `module.vm.force_interpreter = true; module.invoke(...)` -- because invoke() would overwrite vm.force_interpreter with the default-null persistent field. The same silent override affected vm.fuel (wiped to null) and the vm deadline (setDeadlineTimeoutMs(null) clears). Guard every `self.* -> vm.*` write with `if (self.*) |v|`. A null persistent field means "leave vm untouched", so callers who mutate self.vm.* directly (the legacy pattern in zwasm 1.7.x and earlier) continue to work. Vm.reset() does not touch force_interpreter/fuel/deadline_ns, so the caller's value survives across invoke() boundaries as it used to. invokeInterpreterOnly() saves the live vm.force_interpreter and restores it via defer, instead of restoring the persistent self.force_interpreter which may not match what the caller configured. Changes: - force_interpreter: `bool` -> `?bool` on WasmModule and loadCore - guard self.* writes in loadCore start-function block, invoke(), and invokeInterpreterOnly() - revert `self.wasi_ctx.?` -> `if (self.wasi_ctx) |*wc|` in loadWasiWith* (the unwrap lost a defensive check without resolving a real build error) - drop redundant module.vm.setDeadlineTimeoutMs(ms) in cli.zig cmdRun (invoke now applies it via module.timeout_ms, guarded) - expand the regression test to cover legacy-vm, new-module-field, and mixed patterns for fuel / timeout / force_interpreter Mac merge gate (local): zig build test PASS zig build test -Djit=false -Dcomponent=false -Dwat=false PASS python3 test/spec/run_spec.py --build --summary 62263/62263 bash test/c_api/run_ffi_test.sh --build 68/68 bash test/e2e/run_e2e.sh --convert --summary 796/796 bash bench/run_bench.sh --quick no regression Co-authored-by: Junji Takakura <j.takakura@gmail.com>
|
Thanks for the fix, @jtakakura 🙏 — the defer-reset in I pushed Regression the v1 fix introduced (caught during review — not blocking anything in the wild yet, since main hasn't shipped this): The v1 module.vm.force_interpreter = true; // legacy direct-vm pattern (zwasm 1.7.x)
module.invoke(...); // <- v1 overwrote vm.force_interpreter
// with self.force_interpreter (= false)Same for Fix approach (minimal delta on top of yours): flip Also: reverted Merge gate (Mac aarch64 + Ubuntu x86_64 via OrbStack, both):
Will merge once CI is green. Keeping you as co-author on the follow-up commit. |
Establishes a reference point on main (201d13c, after PRs #32/#33/#35 merged) for comparison against upcoming changes that touch the VM hot path — notably PR #28's cancellation checks in consumeInstructionBudget. Delta vs previous baseline w45-simd-loop-persist (2026-03-26, 1e406db): major speedups in realworld C benchmarks (rw_c_matrix -82%, rw_c_math -69%, rw_cpp_sort -28%) from unrelated interim commits; sub-ms regressions on st_ackermann and st_nestedloop are at-or-below the noise floor and not attributable to PR #32 (its invoke() change adds three null-branch guards with no observable cost when persistent fields are unset, which is the bench configuration).
|
@chaploud Thank you for the detailed review and the follow-up commit. |
2 participants
This PR fixes a bug where the force_interpreter option was lost after calling invokeInterpreterOnly (see #31). Now, modules loaded with force_interpreter=true always run in interpreter mode, even after debug calls. Includes a test to verify the fix.
Closes #31