perf(interp): expand JIT coverage for container ops and equality#158
Merged
Conversation
The linear-scan rewriter released a virtual register only when its last reference appeared as a read; a value whose final reference was a write kept its physical register bound for the rest of the stream. Long no-spill emissions (trace loops with many blocks) leaked one register per dead store and rejected compilation with register pressure.
Scalar struct-field stores now mirror the primitive ARRAY_SET pattern: capture keeps recording past the store (cloneTarget reports a scalar struct store as continuable), and lowering emits a state barrier whose shape/bounds/value/kind guards share one resumable snapshot before the guarded store. Ref-field stores and stores after inlined calls remain terminal. Hot loops that mutate scalar struct fields now stay native instead of deopting every iteration.
Threaded CONST_GET interns a string at runtime while the JIT embeds the constant-pool ref, so the two runtimes disagreed on a string's canonical heap address (and an embedded interned ref could dangle after a free). Registering each string constant's pool cell in the intern table at load makes runtime interning return the pinned pool address, aligning both paths for the interpreter's whole lifetime.
…4 JIT Ref and interned-string equality are boxed-word compares. The new refEq lowering keeps the compare native when at most one operand owns its retain (releasing the one owned operand in place, like REF_IS_NULL) and falls back terminally when both operands are owned, where releasing both natively could double-release across a deopt.
ARRAY_FILL, ARRAY_COPY, ARRAY_APPEND, and MAP_SET previously aborted the whole trace, so a hot loop containing one of them never went native. They now record as ERROR_NEW-style terminal boundaries: the trace stops without stepping the clone, the JIT lowers the op to an unconditional deopt, and the threaded handler performs the real mutation. Loop anchors additionally accept a returned straight-line root, so such a loop compiles its prefix natively and re-enters at the header each iteration.
Any function containing STRUCT_GET was rejected by the static frontend because the opcode's generic KindAny result aborted the dataflow pass. The planner now tracks declared struct types and known i32 constants, so a constant-index field read of a struct-typed local, upvalue, or known heap struct resolves its result kind and compiles without trace warmup. The synthesized step.seen only conveys the kind; the lowering's runtime itab, type-pointer, and per-field kind guards stay in place.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands ARM64 JIT support for comparisons, scalar struct stores, terminal mutations, and static struct access. It also updates tracing, runtime string interning, register release, documentation, benchmarks, and broadens or reorganizes test coverage. ChangesARM64 JIT planning and execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Tracer
participant StaticPlanner
participant ARM64JIT
participant Interpreter
Tracer->>StaticPlanner: capture mutation and struct access metadata
StaticPlanner->>ARM64JIT: emit native plan
ARM64JIT->>ARM64JIT: compare refs or store scalar struct field
ARM64JIT->>Interpreter: deopt at owned or bulk mutation boundary
Interpreter-->>ARM64JIT: resume execution at fallback opcode
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 29.63% 29.68% +0.04%
==========================================
Files 86 86
Lines 60292 60350 +58
==========================================
+ Hits 17870 17917 +47
- Misses 41146 41156 +10
- Partials 1276 1277 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
interp/jit_arm64_test.go (1)
2201-2325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every newly enabled opcode in the parity suites.
REF_NE,ARRAY_COPY, andARRAY_APPENDare lowered by this PR but never emitted here. Add parity subtests for their result, refcounts, and native entry/deopt behavior.As per coding guidelines, test changes must follow the ownership and opcode-coverage guidance in
docs/testing.md.Also applies to: 2332-2410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/jit_arm64_test.go` around lines 2201 - 2325, Add parity subtests alongside the existing cases in TestARM64_RefEqLoop for REF_NE, ARRAY_COPY, and ARRAY_APPEND, emitting each newly lowered opcode through the program.Builder helpers. Validate threaded versus JIT results and reference counts via runParity, and ensure each case exercises native entry and deoptimization behavior while following the ownership rules in docs/testing.md.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interp/interp.go`:
- Around line 305-313: Update constant-pool loading around the default branch
and i.interned so duplicate types.String values reuse the previously interned
boxed reference instead of allocating another cell. Ensure both compile-time
constants and runtime interning resolve equal strings to the same address, while
preserving unique-string allocation and interpreter lifetime behavior. Add a
regression test with two separate constant-pool entries containing identical
string values.
---
Nitpick comments:
In `@interp/jit_arm64_test.go`:
- Around line 2201-2325: Add parity subtests alongside the existing cases in
TestARM64_RefEqLoop for REF_NE, ARRAY_COPY, and ARRAY_APPEND, emitting each
newly lowered opcode through the program.Builder helpers. Validate threaded
versus JIT results and reference counts via runParity, and ensure each case
exercises native entry and deoptimization behavior while following the ownership
rules in docs/testing.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da3e65df-ebc4-4792-ac81-ef48121611b9
📒 Files selected for processing (10)
asm/rewriter.godocs/instruction-set.mddocs/jit-internals.mdinterp/interp.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan.gointerp/jit_plan_test.gointerp/trace.gointerp/trace_test.go
siyul-park
commented
Jul 17, 2026
siyul-park
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed the full change set at 3059f19a69511a7b0315cf101ccf3d1e6fda9de0.
One blocking correctness issue remains in interp/interp.go: duplicate equal types.String entries in the constant pool are each allocated before i.interned is updated, so the constant slots can retain different heap references. Both threaded and native STRING_EQ compare boxed references, which makes two equal string constants compare unequal when the pool was constructed directly (for example with program.WithConstants). The existing unresolved inline thread on lines 305–313 correctly identifies this.
The current equality tests use program.Builder, whose Const method deduplicates comparable equal values, so they cannot expose the bug. Please canonicalize string constants through i.intern before assigning the constant slot and add a regression with two distinct constant-pool entries containing the same string, covering at least threaded execution and ARM64 JIT parity.
I did not find another blocking correctness defect in the register-release, scalar STRUCT_SET, terminal-mutation, or static STRUCT_GET changes from source inspection. The Remote Desktop Commander host was offline, so I could not independently rerun the repository checks locally.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interp/interp_test.go`:
- Around line 3999-4003: Strengthen the “interns duplicate string constants”
test by asserting observable string-constant behavior rather than equality of
private i.constants entries. Verify both constants are valid KindRef values,
then compare their Ref() results or dereferenced strings, using the
interpreter’s public evaluation/access path instead of directly relying on
i.constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7612a6ff-2156-486e-a6f2-d19a3bf14a62
📒 Files selected for processing (2)
interp/interp.gointerp/interp_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- interp/interp.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interp/jit_plan_test.go (1)
132-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep helper cases under a single top-level test.
TestMergeSlot,TestStore, andTestNoSpilltest unexported helpers and were split from grouped subtests into separate top-level tests. Keep these cases under the owning top-level test usingt.Runinstead.As per coding guidelines,
**/*_test.go: define one top-level test per public symbol and put sub-cases undert.Runrather than splitting them into parallel top-level tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/jit_plan_test.go` around lines 132 - 166, Group the helper coverage for mergeSlot, store, and noSpill under a single owning top-level test, replacing TestMergeSlot, TestStore, and TestNoSpill with t.Run subtests. Preserve each subtest’s existing assertions and inputs while ensuring the file has one top-level test for these unexported helper cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@interp/jit_plan_test.go`:
- Around line 132-166: Group the helper coverage for mergeSlot, store, and
noSpill under a single owning top-level test, replacing TestMergeSlot,
TestStore, and TestNoSpill with t.Run subtests. Preserve each subtest’s existing
assertions and inputs while ensuring the file has one top-level test for these
unexported helper cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61f443cc-8238-4dfc-acf8-87aedb78a420
📒 Files selected for processing (4)
interp/example_test.gointerp/fuzz_test.gointerp/jit_arm64_test.gointerp/jit_plan_test.go
💤 Files with no reviewable changes (2)
- interp/fuzz_test.go
- interp/example_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- interp/jit_arm64_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
interp/jit_arm64_test.go (1)
1957-1976: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegister cleanup before running assertions.
A
requirefailure exits the helper before Lines 1975-1976, leaking both interpreters and native resources into later subtests.Proposed fix
jit := New(prog, WithTick(1), WithThreshold(0), WithProfiler(profile)) + t.Cleanup(func() { require.NoError(t, jit.Close()) }) threaded := New(prog, WithTick(1), WithThreshold(-1)) + t.Cleanup(func() { require.NoError(t, threaded.Close()) }) @@ - require.NoError(t, jit.Close()) - require.NoError(t, threaded.Close())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/jit_arm64_test.go` around lines 1957 - 1976, Update the runParity helper to register cleanup for both jit and threaded immediately after they are created, before any assertions or Run calls. Ensure the cleanup closes both interpreters and releases their native resources even when a require assertion fails, while preserving the existing comparison loop.
🧹 Nitpick comments (1)
interp/pool_test.go (1)
560-581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting private
stubstate.Use profiler/cache outcomes plus execution behavior to establish the warmed shared-JIT condition, rather than coupling the benchmark to
Interpreter.stubinternals. As per coding guidelines, tests must assert observable behavior rather than private implementation shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/pool_test.go` around lines 560 - 581, Update the benchmark setup and assertions around pool.Get to stop calling first.stub(0) and second.stub(0). Establish the warmed shared-JIT condition using profiler or cache outcomes, then verify it through observable execution behavior while preserving the existing allocation measurement and pool.Get timing checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interp/jit_arm64_test.go`:
- Around line 2060-2069: Update the checkExits assertion in the profile.Metrics
loop to require that vm_jit_native_exits_total contains a reason label with
value "loop-exit". Track whether this matching metric/label is observed and
assert it after scanning all metrics, while preserving the existing label
validation.
In `@interp/pool_test.go`:
- Around line 501-526: Assert the errors from pool.Get and pool.Close
immediately within each benchmark iteration so failures cannot be overwritten by
later iterations. In interp/pool_test.go lines 501-526, check both results
before the next loop iteration; in lines 551-589, assert pool.Close in each
iteration instead of retaining only the final error.
---
Outside diff comments:
In `@interp/jit_arm64_test.go`:
- Around line 1957-1976: Update the runParity helper to register cleanup for
both jit and threaded immediately after they are created, before any assertions
or Run calls. Ensure the cleanup closes both interpreters and releases their
native resources even when a require assertion fails, while preserving the
existing comparison loop.
---
Nitpick comments:
In `@interp/pool_test.go`:
- Around line 560-581: Update the benchmark setup and assertions around pool.Get
to stop calling first.stub(0) and second.stub(0). Establish the warmed
shared-JIT condition using profiler or cache outcomes, then verify it through
observable execution behavior while preserving the existing allocation
measurement and pool.Get timing checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 680d14b8-c463-434e-9ec0-03a345c99cc4
📒 Files selected for processing (4)
interp/interp_test.gointerp/jit_arm64_test.gointerp/pool_test.gointerp/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- interp/trace_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/benchmark_test.go`:
- Around line 93-110: Update the benchmark loop around vm.Run and vm.PopBoxed to
detect the first non-nil runErr or popErr and stop iterating immediately,
preserving that failure for the existing assertions after the loop. Ensure the
VM is reset appropriately before exiting and avoid allowing later iterations to
overwrite the captured error.
In `@interp/interp_test.go`:
- Around line 2526-2660: Replace the parity assertions in the parityCases test
with observable behavior checks rather than private interpreter state such as
fr, stack layout, interned size, or aggregate rc counts. For each case, compare
returned values, errors, globals, resumability, and subsequent reference
usability across WithTick(1) and WithThreshold(-1), including equivalent trap
behavior. Apply the same approach to the additional parity test range noted in
the review, while preserving coverage of ownership and execution semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 432a42ba-978c-4baa-9ef0-03a9b73bd6d0
📒 Files selected for processing (13)
Makefileanalysis/blocks_test.goasm/arm64/encoder_test.gobenchmarks/benchmark_test.gobenchmarks/call_test.gobenchmarks/control_test.gobenchmarks/memory_test.gobenchmarks/numeric_test.gocli/display_test.gointerp/interp_test.gotypes/error_test.gotypes/primitive_test.gotypes/string_test.go
💤 Files with no reviewable changes (6)
- cli/display_test.go
- Makefile
- benchmarks/control_test.go
- benchmarks/memory_test.go
- benchmarks/numeric_test.go
- benchmarks/call_test.go
Comment on lines
+2526
to
+2660
| type parityState struct { | ||
| code types.ErrorCode | ||
| ip int | ||
| fp int | ||
| sp int | ||
| stack []types.Boxed | ||
| globals []types.Boxed | ||
| live int | ||
| interned int | ||
| } | ||
|
|
||
| t.Run("fused ref drops preserve exact ownership", func(t *testing.T) { | ||
| type snapshot struct { | ||
| sp int | ||
| stack []types.Boxed | ||
| live int | ||
| interned int | ||
| } | ||
| run := func(t *testing.T, prog *program.Program, opts ...func(*option)) snapshot { | ||
| t.Helper() | ||
| i := New(prog, opts...) | ||
| defer i.Close() | ||
| require.NoError(t, i.Run(context.Background())) | ||
| live := 0 | ||
| for _, rc := range i.rc[1:] { | ||
| if rc > 0 { | ||
| live += rc | ||
| huge := int64(1) << 50 | ||
| fn := types.NewFunctionBuilder(nil).Emit(instr.New(instr.RETURN)).MustBuild() | ||
| parityCases := []struct { | ||
| name string | ||
| prog *program.Program | ||
| err error | ||
| }{ | ||
| { | ||
| name: "promoted i64 eqz branch preserves state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.I64_EQZ), | ||
| instr.New(instr.BR_IF, 0), | ||
| }), | ||
| }, | ||
| { | ||
| name: "promoted i64 comparison branch preserves state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.I64_EQ), | ||
| instr.New(instr.BR_IF, 0), | ||
| }), | ||
| }, | ||
| { | ||
| name: "promoted i64 local binary preserves state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(1)), | ||
| instr.New(instr.LOCAL_SET, 0), | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.LOCAL_GET, 0), | ||
| instr.New(instr.I64_ADD), | ||
| instr.New(instr.DROP), | ||
| }, program.WithLocals(types.TypeI64)), | ||
| }, | ||
| { | ||
| name: "local ref drop preserves ownership", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I32_CONST, 7), | ||
| instr.New(instr.REF_NEW), | ||
| instr.New(instr.LOCAL_SET, 0), | ||
| instr.New(instr.LOCAL_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithLocals(types.TypeRef)), | ||
| }, | ||
| { | ||
| name: "function constant drop preserves ownership", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.CONST_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithConstants(fn)), | ||
| }, | ||
| { | ||
| name: "string constant drop preserves ownership", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.CONST_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithConstants(types.String("value"))), | ||
| }, | ||
| { | ||
| name: "i32 divide by zero preserves trap state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I32_CONST, 90), | ||
| instr.New(instr.I32_CONST, 0), | ||
| instr.New(instr.I32_DIV_S), | ||
| }), | ||
| err: ErrDivideByZero, | ||
| }, | ||
| { | ||
| name: "promoted i64 divide by zero preserves trap state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.I64_CONST, 0), | ||
| instr.New(instr.I64_DIV_S), | ||
| }), | ||
| err: ErrDivideByZero, | ||
| }, | ||
| { | ||
| name: "promoted i64 local divide by zero preserves trap state", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.LOCAL_SET, 0), | ||
| instr.New(instr.LOCAL_GET, 0), | ||
| instr.New(instr.I64_CONST, 0), | ||
| instr.New(instr.I64_DIV_S), | ||
| }, program.WithLocals(types.TypeI64)), | ||
| err: ErrDivideByZero, | ||
| }, | ||
| } | ||
| for _, tt := range parityCases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| states := make([]parityState, 0, 2) | ||
| for _, opts := range [][]func(*option){ | ||
| {WithTick(1)}, | ||
| {WithThreshold(-1)}, | ||
| } { | ||
| i := New(tt.prog, opts...) | ||
| err := i.Run(context.Background()) | ||
| if tt.err == nil { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.ErrorIs(t, err, tt.err) | ||
| } | ||
| } | ||
| return snapshot{ | ||
| sp: i.sp, | ||
| stack: append([]types.Boxed(nil), i.stack[:i.sp]...), | ||
| live: live, | ||
| interned: len(i.interned), | ||
| } | ||
| } | ||
|
|
||
| fn := types.NewFunctionBuilder(nil).Emit(instr.New(instr.RETURN)).MustBuild() | ||
| cases := []struct { | ||
| name string | ||
| prog *program.Program | ||
| }{ | ||
| { | ||
| name: "local ref", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I32_CONST, 7), | ||
| instr.New(instr.REF_NEW), | ||
| instr.New(instr.LOCAL_SET, 0), | ||
| instr.New(instr.LOCAL_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithLocals(types.TypeRef)), | ||
| }, | ||
| { | ||
| name: "function constant", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.CONST_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithConstants(fn)), | ||
| }, | ||
| { | ||
| name: "string constant", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.CONST_GET, 0), | ||
| instr.New(instr.DROP), | ||
| }, program.WithConstants(types.String("value"))), | ||
| }, | ||
| } | ||
| for _, tt := range cases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| exact := run(t, tt.prog, WithTick(1)) | ||
| fused := run(t, tt.prog, WithThreshold(-1)) | ||
| require.Equal(t, exact, fused) | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("fused numeric traps preserve exact state", func(t *testing.T) { | ||
| type snapshot struct { | ||
| ip int | ||
| fp int | ||
| sp int | ||
| stack []types.Boxed | ||
| live int | ||
| } | ||
| run := func(t *testing.T, prog *program.Program, opts ...func(*option)) (snapshot, error) { | ||
| t.Helper() | ||
| i := New(prog, opts...) | ||
| defer i.Close() | ||
| err := i.Run(context.Background()) | ||
| live := 0 | ||
| for _, rc := range i.rc[1:] { | ||
| if rc > 0 { | ||
| live += rc | ||
| state := parityState{ | ||
| code: ErrorCode(err), | ||
| ip: i.fr.ip, | ||
| fp: i.fp, | ||
| sp: i.sp, | ||
| stack: append([]types.Boxed(nil), i.stack[:i.sp]...), | ||
| globals: append([]types.Boxed(nil), i.globals...), | ||
| interned: len(i.interned), | ||
| } | ||
| for _, rc := range i.rc[1:] { | ||
| if rc > 0 { | ||
| state.live += rc | ||
| } | ||
| } | ||
| states = append(states, state) | ||
| require.NoError(t, i.Close()) | ||
| } | ||
| return snapshot{ | ||
| ip: i.fr.ip, | ||
| fp: i.fp, | ||
| sp: i.sp, | ||
| stack: append([]types.Boxed(nil), i.stack[:i.sp]...), | ||
| live: live, | ||
| }, err | ||
| } | ||
|
|
||
| huge := int64(1) << 50 | ||
| cases := []struct { | ||
| name string | ||
| prog *program.Program | ||
| }{ | ||
| { | ||
| name: "i32 constants", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I32_CONST, 90), | ||
| instr.New(instr.I32_CONST, 0), | ||
| instr.New(instr.I32_DIV_S), | ||
| }), | ||
| }, | ||
| { | ||
| name: "promoted i64 constants", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.I64_CONST, 0), | ||
| instr.New(instr.I64_DIV_S), | ||
| }), | ||
| }, | ||
| { | ||
| name: "promoted i64 local", | ||
| prog: program.New([]instr.Instruction{ | ||
| instr.New(instr.I64_CONST, i64operand(huge)), | ||
| instr.New(instr.LOCAL_SET, 0), | ||
| instr.New(instr.LOCAL_GET, 0), | ||
| instr.New(instr.I64_CONST, 0), | ||
| instr.New(instr.I64_DIV_S), | ||
| }, program.WithLocals(types.TypeI64)), | ||
| }, | ||
| } | ||
| for _, tt := range cases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| exact, exactErr := run(t, tt.prog, WithTick(1)) | ||
| fused, fusedErr := run(t, tt.prog, WithThreshold(-1)) | ||
| require.ErrorIs(t, exactErr, ErrDivideByZero) | ||
| require.ErrorIs(t, fusedErr, ErrDivideByZero) | ||
| require.Equal(t, exact, fused) | ||
| }) | ||
| } | ||
| }) | ||
| require.Equal(t, states[0], states[1]) | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test runtime parity through observable behavior, not private state.
Comparing fr, stack layout, intern-table size, and aggregate rc totals is brittle; the summed live value can also hide offsetting ownership errors. Validate returned values, errors, globals, resumability, and subsequent reference usability instead.
As per coding guidelines: “Tests must assert observable behavior rather than private implementation shape.”
Also applies to: 2676-2745
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@interp/interp_test.go` around lines 2526 - 2660, Replace the parity
assertions in the parityCases test with observable behavior checks rather than
private interpreter state such as fr, stack layout, interned size, or aggregate
rc counts. For each case, compare returned values, errors, globals,
resumability, and subsequent reference usability across WithTick(1) and
WithThreshold(-1), including equivalent trap behavior. Apply the same approach
to the additional parity test range noted in the review, while preserving
coverage of ownership and execution semantics.
Source: Coding guidelines
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.
Summary
Expands ARM64 JIT coverage in four steps, plus two enabling fixes uncovered along the way.
1. Scalar
STRUCT_SETbecomes continuablecloneTargetreports it continuable; ref-field and post-call stores stay terminal).structSetgains a continuable branch mirroring the primitiveARRAY_SETstate-barrier pattern: one resumable snapshot shared by the shape/bounds/value/kind guards, aggressive guard-register recycling, owned-container rc release after all guards, deferred containers elide the release.2.
REF_EQ/REF_NE/STRING_EQ/STRING_NElower nativelyrefEqlowering keeps the compare native with at most one owned operand (single in-place release, likeREF_IS_NULL) and falls back terminally with two owned operands (double-release hazard across a deopt).3. Bulk mutations stop killing traces
ARRAY_FILL/ARRAY_COPY/ARRAY_APPEND/MAP_SETmove from trace abort to ERROR_NEW-style terminal boundaries: the compiled prefix runs native, the threaded handler performs the mutation, and the loop re-enters at the header.returnedstraight-line root, so such loops compile their prefix as a per-entry fragment.4.
STRUCT_GETworks in static plansEnabling fixes
Test plan
go test -race ./...(all packages)TestARM64_StructSetLoop,TestARM64_RefEqLoop,TestARM64_TerminalMutationLoop,TestARM64_StructGetStaticPlan(each diffs results and exact refcounts against a threaded twin and asserts native entries)STRUCT_SETcontinuable, ref-field still terminal, bulk mutation records a terminal boundary, allocation still abortsSTRUCT_GETkind resolution, unknown-index rejectionmake fuzz(bounded parity soak),make lint,make benchmark-pr(no regression; JITWarm ≈ Fused on the quick suite)make coverage-checkfails from the known goenv go1.25/1.26 toolchain split, unrelated to this diff🤖 Generated with Claude Code
Summary by CodeRabbit
REF_EQ/REF_NEandSTRING_EQ/STRING_NE, with correct terminal fallback and refcount/ownership handling.ARRAY_FILL,ARRAY_COPY,ARRAY_APPEND,MAP_SET) as terminal boundaries to keep native prefixes safer.STRUCT_SETcontinuability in traces and improved staticSTRUCT_GETplanning.