perf(codegen): unboxed reduce accumulators in packed fast clones - #9091
Conversation
26bb564 to
fc541a3
Compare
|
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 (1)
📝 WalkthroughWalkthroughPacked loop codegen now admits numeric reduce accumulators into unboxed F64 slots, restores them on exits, and supports additional call-free read loops and float arithmetic in dense masked stores. All ChangesPacked loop optimizations
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change keeps eligible numeric accumulators in temporary floating-point storage during packed-loop execution and restores their canonical values on exits. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant LoopMatcher
participant FastClone
participant AccumulatorSlots
participant SlowClone
LoopMatcher->>FastClone: emit guarded packed loop
FastClone->>AccumulatorSlots: admit numeric accumulators
FastClone->>AccumulatorSlots: perform unboxed F64 updates
FastClone->>SlowClone: write back values on side exit
FastClone->>SlowClone: write back values before fallback
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, concrete implementation changes, performance results, correctness notes, and test status. It does not use all template headings and omits the related-issue section, explicit test commands, and checklist, but the required information is mostly present.
✨ 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 |
After the packed-clone accumulator proofs, `s += a[i]` in a fast clone
is a bare guarded fadd — but `s` still lived in its GC-root nanbox
slot, so every iteration paid a store-to-load-forward + fadd dependency
chain (~15 cycles/element; node keeps `s` in a register; profiling
showed the whole loop in four PCs with the slot chain as the floor).
Move each admitted accumulator into a plain addrspace-0 F64 alloca for
the clone's duration:
- The fast preheader's existing admission (one emit_js_value_is_number
tag test per accumulator — which IS the strict genuine-double window:
0x7FF9..0x7FFF covers every boxed tag, so an INT32-boxed number
correctly fails to the slow clone) also stores the tested value into
the alloca and registers it in ctx.numeric_accumulator_f64_slots.
- In-clone LocalGet/LocalSet of the accumulator redirect to the alloca:
no shadow bookkeeping (the real slot holds a stale NUMBER for the
clone's duration — consistent with any prior shadow state, and
scanning a number nanbox is harmless), no barrier (numbers carry no
heap edge). mem2reg promotes the alloca to a register.
- EVERY clone exit writes the value back: the fall-through exit, and a
per-clone side-exit trampoline the scope's packed facts carry as
their store_side_exit_label — a mid-iteration hole-check or
masked-store value-check side exit restores correct slot state
before the slow clone re-executes the iteration.
- v1 unboxes LocalSet-only accumulators (collect_local_writes check);
Update-written ones (c++) keep the slot — the Update lowering does
not consult the redirect, and integer counters are served by the
i32-slot machinery anyway.
Admission stays collect_numeric_accumulators — the single source shared
with the stable clone, whose author verified the slot-canonicalization
invariant this inherits (every admitted producer emits canonical raw
doubles).
Isolated (dev box; node 26.5 in parens):
literal-bound reduce 4.16 -> 0.98 ns/el (1.01) — ahead of node
len-bound reduce 4.14 -> 1.29 (0.99; residual = length IC,
removed by the PerryTS#9070 hoist)
module-global reduce 4.20 -> 2.70 (2.85) — ahead of node
IR census of the fast clone: one receiver root re-derive, one fadd, the
loop poll — nothing else. Nine-probe differential byte-identical; the
any-seeded accumulator repro from the PerryTS#9087 investigation is unchanged
(that divergence is the pre-existing runtime bug, untouched here).
perry-codegen suites 1829/0.
cargo fmt --all -- --check is a lint gate; three hunks in entry.rs and loops.rs were mis-indented.
fc541a3 to
a5d2b1a
Compare
|
Head is
Both reduce rows are now ahead of node — #9070's hoist removed the length IC exactly as scoped, and this PR's register-promoted accumulator was the last serial chain. The count loop's residual is the Update-written ( |
a5d2b1a to
3747c56
Compare
|
Merged, after rebasing off the merged #9084 and adding a rustfmt commit (three hunks in This moves a value out of its GC-root slot into a plain addrspace-0 alloca, which is the shape CLAUDE.md singles out as a known-weak area, so I went at the "every clone exit writes the value back" claim rather than around it. First, a correction to my own method, because it nearly produced a wrong review. I benchmarked three fixtures and measured 1.00x / 1.01x / 1.00x. Before reporting "no improvement" I diffed the emitted IR between arms — identical. The optimization never fired on my fixture, so the timing measured nothing. Worse, my original 31-case correctness probe had zero Re-done against a fixture where the subject is live — 6
Worth recording that Performance, on a shape that fires (4096 elements × 4000 reps, interleaved best-of-5, main confirmed at 0
So the slot store-to-load-forward chain was real, and this clears it — comfortably ahead of node on both. One gap, not blocking. The rebase dropped |
1 participant
Stacked on #9084 (merge that first) — the follow-on mechanism its PR text scoped: the reduce rows' remaining gap was not calls but the accumulator's GC-root slot.
What
After #9084, a packed fast clone's
s += a[i]is a bare guardedfadd— butsstill lived in its GC-root nanbox slot, so every iteration paid a store-to-load-forward + fadd dependency chain (~15 cycles/element; node keepssin a register). Profiling showed the whole loop in 4 PCs with the slot chain as the floor.This moves each admitted accumulator into a plain addrspace-0 F64 alloca for the clone's duration:
emit_js_value_is_numbertag test per accumulator — which IS the strict genuine-double window: the 0x7FF9..0x7FFF band covers every boxed tag, so INT32-boxed numbers correctly fail to the slow clone) now also stores the tested value into the alloca and registers it inctx.numeric_accumulator_f64_slots.LocalGet/LocalSetof the accumulator redirect to the alloca — no shadow bookkeeping (the real slot holds a stale number for the clone's duration, consistent with any prior shadow state; scanning a number nanbox is harmless), no barrier (numbers carry no heap edge). mem2reg promotes the alloca to a register.store_side_exit_label— so a mid-iteration hole-check or masked-store value-check side exit restores correct slot state before the slow clone re-executes the iteration.collect_local_writescheck); Update-written ones (c++) keep the slot — the Update lowering doesn't consult the redirect, and integer counters are served by the i32-slot machinery anyway.Admission stays
collect_numeric_accumulators(#9060) — single source shared with the stable clone, per coordination with its author, who also verified the slot-canonicalization invariant the mechanism inherits (every admitted producer emits canonical raw doubles).Numbers (isolated, dev box; node 26.5 in parens)
i<8192, s+=a[i]i < a.lengthreduce (local array)The
a.lengthvariant's residual is the per-iteration length IC; #9070's hoist removes it at merge. IR census of the fast clone after this PR: one receiver root re-derive, onefadd, the loop poll — nothing else.Correctness
any-seeded accumulator repro from the correctness: class reference + number treats the INT32-boxed class ref as a number (should be string concat) #9087 investigation behaves identically before/after (the class-ref+divergence is that pre-existing runtime bug, unchanged by this machinery; the boolean-seeded arm is correct both sides).Summary by CodeRabbit