runtime: single-pass numeric-window add kernel — bench_numeric_array_downgrade beats node (16→3ms) - #9307
Merged
proggeramlug merged 1 commit intoAug 31, 2026
Conversation
bench_numeric_array_downgrade: 16ms -> 3ms against node's 4ms on an idle Mac
mini -- perry now BEATS node on the benchmark (was 3.60x). Checksums
identical; spreads flat.
The diagnosis first established that the benchmark's 3.6x was almost
entirely the DECLARED-TYPE effect (any[] vs number[] on an identical packed
layout: 9ms vs 3ms) and not the planted heterogeneity (~0ms extra) -- and
then that the any[] loop was already claimed by the purpose-built
mixed-layout tier (`match_numeric_range_add_loop`). The entire cost sat in
its runtime kernel: TWO full passes over a 1MB window (validate every slot,
then mutate every slot), a branchy NaN-box decode per slot per pass, ~2.9ns
per element against node's 0.64.
The all-or-nothing contract those two passes bought is stronger than the
source semantics require. Each element receives exactly one `+ delta`
whether the kernel or the ordinary loop applies it, so mutating up to the
first non-number and letting the ordinary loop RESUME there is observably
identical -- and halves the memory traffic, which was the whole cost.
New contract:
ret >= 0 window done; ret is the counter on exit.
ret == -1 receiver-level decline (type/frozen/descriptors/window);
nothing mutated -- this half stays fully transactional.
ret <= -2 slots [start, k) updated, k = -ret - 2; the caller resumes
the ordinary loop at k.
The lowering seeds the counter with the resume index before entering the
fallback loop. Safe because `lower_for` lowers the init before any matcher
runs, so the fallback cannot re-run it and double-apply the prefix. The
double lane is decoded first (after the first call every slot holds a boxed
double); the int lane keeps the class-ref exclusion in the shared decoder.
The old `numeric_range_add_failure_is_transactional` test failed exactly as
designed -- a documentation test for the contract this change deliberately
replaces -- and is rewritten to pin the new one: prefix mutated, marker and
suffix untouched, resume index encoded (-3 for index 1, -2 for index 0),
and receiver-level -1 still writing nothing.
Also names the packed-f64 versioned matcher's one silent gate
(`no_length_hoist`): a loop whose bound is not `arr.length` exited before
any named reject could fire, making every literal- or parameter-bounded
loop invisible to PERRY_PACKED_LOOP_TRACE -- the gap that forced this
diagnosis through binary instrumentation instead of one trace run.
The resume path is pinned by integration tests the benchmark never
exercises (its windows are entirely numeric): a non-number mid-window gets
node's exact semantics -- one increment per element, concatenation where
`+` concatenates ("[object Object]1", "mid1"), NaN slots staying NaN --
under normal and forced-evacuation runs, with expected values taken from
node.
perry-runtime lib 2894/0 (single-threaded); perry-codegen lib 1378/0;
packed-loop integration 6 files green; GC store-site inventory unchanged;
rustfmt clean.
Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe numeric range-add kernel now uses one fused pass with an encoded resume contract. Codegen resumes the generic loop after partial progress. Runtime and integration tests cover receiver declines, mid-window non-numbers, NaN values, tier selection, and moving GC. ChangesNumeric range-add execution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The numeric range-add runtime now updates a numeric prefix before returning a resume status for mixed-value windows, while repository-local callers preserve exactly-once behavior. Merge is reasonable with owner awareness that external FFI consumers relying on the former all-or-nothing failure behavior must be confirmed or updated to avoid duplicate prefix updates. Sequence Diagram(s)sequenceDiagram
participant NumericRangeAddLowering
participant array_numeric_range_add_impl
participant OrdinaryFallbackLoop
NumericRangeAddLowering->>array_numeric_range_add_impl: call numeric range-add helper
array_numeric_range_add_impl-->>NumericRangeAddLowering: return completion or resume index
NumericRangeAddLowering->>OrdinaryFallbackLoop: continue from resume index
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, diagnosis, implementation changes, semantic rationale, test coverage, and reported validation results. It does not use the template headings or include an explicit related-issue value, command list, or checklist, but the core information is complete. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
proggeramlug
added a commit
that referenced
this pull request
Aug 31, 2026
* codegen: a float accumulator over masked reads earns the dense range clone
17_loop_data_dependent: 475 ms -> 219 ms against node's 220 ms on an idle
Mac mini -- parity, from 2.16x. Sums bit-identical across 100M data-dependent
float recurrence steps.
sum = sum * x[i & 63] + x[(i * 7) & 63] // rejected
sum = sum * x[i & 63] // admitted
The discriminator was the accumulator's static numeric proof. `+` can be
concatenation, so the dense tier's per-statement proof demands both operands
numeric; a reassigned accumulator has no such proof, because its own writes
read the guarded array, whose element proof only exists once the guard has
run. A chicken-and-egg that `*` never faces -- multiplication needs only the
weaker inert fact. Confirmed by instrumenting the two conjuncts of the dense
LocalSet arm: the failing one is the proof, on exactly the fixtures whose
accumulator writes contain a plain-array read.
The matcher now peels the accumulator: when the proof fails on the LocalSet
target of a self-accumulating write, it retries with the target treated as
numeric BY CONTRACT, records it pending, and then verifies every pending
local with the same collector the lowering runs
(`collect_numeric_accumulators`), rejecting the whole dense match with its
own named trace reasons (`accumulator_needs_single_array`,
`accumulator_not_provable`) if the two disagree -- so the clone can never
contain a dynamic `+` under facts that forbid one.
The contract is enforced at run time twice over: the clone's entry emits a
genuine-double tag check on the accumulator, and the dense entry guard
validates the whole masked window hole-free. A string-seeded accumulator and
a string element both route to the slow copy and produce node's
concatenation, verified under PERRY_GC_FORCE_EVACUATE.
Supporting changes:
* `accumulator_rhs_is_numeric` accepts masked static-window reads of the
tracked array (`masked_reads_validated`), sound because the dense guard
validated the window union hole-free. Fixing that exposed a match-arm
reachability bug: `_ if offset_reads_inlined` was a guarded catch-all, so
ANY arm placed after it was unreachable whenever the flag was set -- the
first version of this change sat exactly there and verified as a no-op.
The two tests are now one combined catch-all.
* `emit_range_loop_accumulator_admission` admits a masked-only single array
(counter-bearing arrays keep priority; multiple arrays still decline).
* `MaskedWindowArrayFact` carries `numeric_accumulators` so `is_numeric_expr`
sees admitted accumulators while the clone lowers -- without this the add
inside the clone would stay dynamic, which is a collecting call under facts
that assume none (the #9259 cascade shape). Mirrors the string-window
fact's field (#9160).
perry-codegen lib 1378/0; packed-loop integration suite 59/0 across 11 files;
3 new regression tests (admission + node-identical result, string-seeded
accumulator, string element), each under forced evacuation.
Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
* refactor(runtime): split ic_miss.rs and array/tests.rs under the file cap
#9302 and #9307 each tipped a file that was already within ~35 lines of the
2000-line gate. Extracts the C3C PIC test module and the Array.prototype
method-discriminator tests into sibling files; no behaviour change.
* fix(codegen): drop the now-dead catch-all after the combined accumulator arm
#9303's combined `_ =>` arm made the trailing `_ => false` unreachable, which
is a `-D warnings` failure. Removing it is #9308's fix, which the combined arm
needs to be complete.
* style: rustfmt
* chore: changelog fragment for the train13 follow-up
---------
Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
bench_numeric_array_downgrade, the worst remaining benchmark loser at 3.60×, now beats node: 16 ms → 3 ms against node's 4 ms (idle Mac mini, min of 7, flat spreads, identical checksums).Diagnosis
Two findings on the way to the fix, each narrowing it:
any[]vsnumber[]on an identical packed runtime layout: 9 ms vs 3 ms; planting the actual object+string cost ~0 more.any[]loop was already claimed by a purpose-built tier —match_numeric_range_add_loop, the mixed-layout numeric-window matcher. The entire cost sat in its runtime kernel: two full passes over a 1 MB window (validate every slot, then mutate every slot) with a branchy NaN-box decode per slot per pass — ~2.9 ns/element against node's 0.64.The diagnosis was trace-blind for a while because the versioned matcher's bound gate is silent; this PR also names it (
no_length_hoist), so the next person gets it fromPERRY_PACKED_LOOP_TRACE=1in one run instead of binary instrumentation.The fix
The all-or-nothing contract the two passes bought is stronger than the source semantics require: each element receives exactly one
+ deltawhether the kernel or the ordinary loop applies it. So the kernel now mutates in a single fused pass and, at the first non-number, returns the resume index:>= 0-1<= -2[start, k)updated,k = -ret - 2; the ordinary loop resumes atkThe lowering seeds the counter with the resume index before entering the fallback — safe because
lower_forlowers the init before any matcher runs, so the fallback cannot re-run it and double-apply the prefix. The double lane decodes first (after call one, every slot is a boxed double); the int lane keeps the class-ref exclusion in the shared decoder.The old test failing was the system working
numeric_range_add_failure_is_transactionalfailed exactly as designed — a documentation test for the contract this change deliberately replaces. It is rewritten, not deleted, to pin the new contract: prefix mutated, marker and suffix untouched, resume encoding (-3for index 1,-2for the index-0 boundary), and receiver-level-1still writing nothing.Semantics pinned where the benchmark can't see them
The benchmark's windows are entirely numeric, so its checksum can't distinguish resume from rollback. The new integration tests pin the part it never exercises: a non-number mid-window gets node's exact answer — one increment per element, concatenation where
+concatenates ("[object Object]1","mid1"), NaN slots staying NaN, never corrupting into tag space — under normal andPERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1runs, with expected values taken from node.Gates
perry-runtimelib 2894/0 (single-threaded),perry-codegenlib 1378/0, packed-loop integration suite 6 files green, GC store-site inventory unchanged (the auditedPOINTER_FREEstore keeps its marker), rustfmt clean.https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
Summary by CodeRabbit
Performance
Bug Fixes
Tests