perf(interp): fuse arithmetic-to-local stores and array loads - #167
Merged
Conversation
Add a feature-free fast path in Interpreter.dispatch that skips the tick/hook/gas/profiler bookkeeping loop entirely when none of those features are configured, running threaded closures back to back. Extend the geninterp fusion catalog with two new families: - non-trapping scalar binary results stored directly into typed locals, skipping stack materialization - typed-array constants indexed by scalar producers feeding array.get, validated against the current heap value's concrete type and bounds on every execution Add reset-time reference-array header reuse: Reset keeps released *types.Array headers (Typ/Elems cleared) in an interpreter-local pool capped by the run's dynamic heap size, and newArray draws from that pool before allocating. Arrays detached through Pop or reclaimed before reset are not pooled. Update docs/fusion.md, docs/instruction-set.md, docs/memory-model.md, and docs/benchmarks.md to describe the new fusion patterns, the array header reuse invariant, and re-measured benchmark numbers (issue #164).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe interpreter now generates direct local arithmetic stores and typed-array access paths, expands threaded numeric execution handlers, reuses array headers across resets, and adds corresponding tests and documentation updates. ChangesRuntime execution changes
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PatternGenerator
participant CodeGenerator
participant Interpreter
PatternGenerator->>CodeGenerator: produce local-store and typed-array patterns
CodeGenerator->>Interpreter: emit fused handlers
Interpreter->>Interpreter: validate operands and update locals or array results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
- Coverage 32.25% 31.20% -1.06%
==========================================
Files 85 85
Lines 56013 59612 +3599
==========================================
+ Hits 18069 18601 +532
- Misses 36807 39846 +3039
- Partials 1137 1165 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 `@docs/benchmarks.md`:
- Around line 24-27: Correct the measurement-date statement in the benchmark
documentation: replace the future-dated July 31, 2026 minivm re-measurement
claim with the actual completed date, or remove/defer that claim until the
measurements have been performed. Preserve the surrounding sample and commit
details.
In `@docs/memory-model.md`:
- Around line 129-137: Update the Reset description to say it invalidates every
live dynamic object still owned by the interpreter, rather than every live
dynamic object. Keep the existing Pop exception and pooling behavior unchanged.
🪄 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: 5279de4a-3cc9-447a-b915-70409f727653
📒 Files selected for processing (10)
docs/benchmarks.mddocs/fusion.mddocs/instruction-set.mddocs/memory-model.mdinternal/cmd/geninterp/generate_test.gointernal/cmd/geninterp/lower.gointernal/cmd/geninterp/pattern.gointerp/interp.gointerp/interp_test.gointerp/threaded.go
👮 Files not reviewed due to content moderation or server errors (1)
- interp/threaded.go
Comment on lines
+24
to
+27
| The external-runtime rows were measured on July 30, 2026 with three sequential | ||
| samples. Every minivm row was re-measured on July 31, 2026 from five | ||
| interleaved baseline/current pairs against commit `10d6adf`. The public API | ||
| cost tables below were measured on July 16, 2026: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future-dated measurement claim.
As of July 30, 2026, the document cannot truthfully state that minivm was re-measured on July 31, 2026. Replace it with the actual completed measurement date, or defer this documentation update until those measurements exist.
🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: Ensure spelling is correct
Context: ...26 with three sequential samples. Every minivm row was re-measured on July 31, 2026 fr...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/benchmarks.md` around lines 24 - 27, Correct the measurement-date
statement in the benchmark documentation: replace the future-dated July 31, 2026
minivm re-measurement claim with the actual completed date, or remove/defer that
claim until the measurements have been performed. Preserve the surrounding
sample and commit details.
Comment on lines
+129
to
+137
| `Reset` invalidates every live dynamic object. Before clearing those slots, the | ||
| interpreter keeps released `*types.Array` headers in an interpreter-local pool | ||
| capped by that run's dynamic heap size. VM array construction, slicing, and map | ||
| key extraction may reuse those headers on the next run. | ||
|
|
||
| Only the Go object header is reused. `Typ` and `Elems` are cleared at reset, and | ||
| every new array receives a fresh element backing store, so zeroing and retained | ||
| memory behavior stay unchanged. Arrays detached through `Pop` or reclaimed | ||
| before reset are not pooled; their returned Go values remain intact. `Close` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify Reset’s ownership scope.
“Every live dynamic object” contradicts the following Pop exception. Limit this to dynamic objects still owned by the interpreter.
Proposed fix
-`Reset` invalidates every live dynamic object. Before clearing those slots, the
+`Reset` invalidates every dynamic object still owned by the interpreter. Before clearing those slots, the📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `Reset` invalidates every live dynamic object. Before clearing those slots, the | |
| interpreter keeps released `*types.Array` headers in an interpreter-local pool | |
| capped by that run's dynamic heap size. VM array construction, slicing, and map | |
| key extraction may reuse those headers on the next run. | |
| Only the Go object header is reused. `Typ` and `Elems` are cleared at reset, and | |
| every new array receives a fresh element backing store, so zeroing and retained | |
| memory behavior stay unchanged. Arrays detached through `Pop` or reclaimed | |
| before reset are not pooled; their returned Go values remain intact. `Close` | |
| `Reset` invalidates every dynamic object still owned by the interpreter. Before clearing those slots, the | |
| interpreter keeps released `*types.Array` headers in an interpreter-local pool | |
| capped by that run's dynamic heap size. VM array construction, slicing, and map | |
| key extraction may reuse those headers on the next run. | |
| Only the Go object header is reused. `Typ` and `Elems` are cleared at reset, and | |
| every new array receives a fresh element backing store, so zeroing and retained | |
| memory behavior stay unchanged. Arrays detached through `Pop` or reclaimed | |
| before reset are not pooled; their returned Go values remain intact. `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 `@docs/memory-model.md` around lines 129 - 137, Update the Reset description to
say it invalidates every live dynamic object still owned by the interpreter,
rather than every live dynamic object. Keep the existing Pop exception and
pooling behavior unchanged.
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
Interpreter.dispatchthat skips tick/hook/gas/profiler bookkeeping and runs threaded closures back to back when none of those features are configured.array.get(still guarded by a runtime type/bounds check).Resetpools released*types.Arrayheaders (withTyp/Elemscleared) capped by the run's dynamic heap size;newArraydraws from that pool before allocating fresh. Arrays detached viaPopor reclaimed pre-reset are excluded.docs/fusion.md,docs/instruction-set.md,docs/memory-model.md, anddocs/benchmarks.mdwith the new fusion patterns, the array-header-reuse invariant, and re-measured benchmarks (issue perf(interp): beat every scripting VM in threaded mode on the three lagging kernels #164).Testing
make check-generated— generatedinterp/threaded.gomatches the generator output.make lint/make vet— clean.go test -race -count=1 ./interp/... ./internal/cmd/geninterp/...— pass, including new tests for local-arithmetic fusion, typed-array-load fusion, and array-header pooling/detachment semantics.make check-arm64(GOOS=linux GOARCH=arm64 build + test) — pass.go test -race ./...— pass across the full module.Summary by CodeRabbit
Performance
Documentation
Bug Fixes
Tests