perf(codegen): keep the element-shape clone's accumulator and counter in native domains - #10255
proggeramlug wants to merge 7 commits into
Conversation
… in native domains The fast clone now redirects the accumulator into a promotable f64 alloca and defers the counter's double storage to an i32 slot the Update lowering advances alone, publishing both at a side-exit trampoline and on the fall-through exit.
…ounter IR census for the f64 accumulator redirect, the i32 counter, the side-exit trampoline and the fall-through write-back; the carried-commit and side-exit helpers count the trampoline spelling of an exit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe fast element-shape loop clone now keeps eligible accumulators in f64 storage and counters in i32 storage. It writes both values back on exits, preserves slow-clone fallback behavior, and adds IR and runtime tests for numeric edge cases. ChangesNative element-shape loop domains
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant ElementShapeVersionedLoop
participant NativeLoopDomains
participant FastClone
participant SlowClone
ElementShapeVersionedLoop->>NativeLoopDomains: enter with counter start and accumulator value
NativeLoopDomains->>FastClone: provide f64 accumulator and i32 counter storage
FastClone->>NativeLoopDomains: route residual side exit
NativeLoopDomains->>SlowClone: write back values and branch to slow preheader
FastClone->>NativeLoopDomains: finish at merge
Merge Risk: ⚪ Minimal · up to The loop optimization retains its cleanup and fallback behavior, with no remaining merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
Independent check before taking this out of draft. Review. Reads of the deferred counter inside the clone go to the registered i32 slot ( Re-measured on the bench mini with this PR's worker and a main worker (
These agree with the description's table. |
1 participant
Summary
The element-shape loop clone (#7480 → #10171 → #10185) now keeps its accumulator as an
f64SSA value and its loop counter as ani32for the clone's duration. The four JSON access cells that missed parity at the official settings (repeat×3, 16kfields) now beat Node 26.5.1 and Bun 1.3.14, and all 12 cells win with the official settings.Root cause
The fast clone was already what executed. The
repeatloop (sum += rows[7].id) cost 16 instructions at 13.0 cycles per iteration because two loop-carried chains crossed the integer/float register boundary on every iteration:any-typedsumlives in a precise GC root slot. Every reload of a root slot goes through the RS4GC launder (function/precise_roots.rs,ROOT_RELOAD_LAUNDER, anasmidentity LLVM cannot see through). So mem2reg promoted the slot as NaN-boxi64bits, givingx25 → fmov → fadd → fmov → x25.repeatform and the carriedrandomform) had no i32 slot. It stayed a double (fadd d8, #1.0) and was compared withfcmpagainstcount.countis also a root slot, so it was reloaded through the launder every iteration, even though the preheader had already turned it into a checked i32.Change (
stmt/element_shape_native.rs, wired instmt/element_shape_loop.rs)alloca double, using thenumeric_accumulator_f64_slotsredirect the packed clones already use. It is seeded with the value the deref block just tag-tested as a Number.Updatelowering advances only that slot (deferred_integer_update_accumulators), so the precomputed i32 trip count turns the condition intoicmp slt i32. A canonical-i32 counter (the indexingsequential/fieldsforms) has no double storage and is left alone.element_shape.loop.side_exittrampoline. The trampoline writes both values back to their real slots and then enters the slow clone. The fall-through exit writes them back inelement_shape.loop.fast.write_back.Soundness (the rules the clone enforced are unchanged)
Updateruns after the body. That is the state the slow clone re-runs the iteration from. The trampoline never writes the carried binding: its real slot keeps the previous iteration's commit, which a unit test pins.sitofp. They are created inside the block range the post-emissioncontains_gc_unsafe_callscan covers. These clones emit no poll, so no collection can see the stale root slot, and that slot holds a Number, which a scan treats as data.+. Nothing new is treated as a raw double. The accumulator is the fact'snumeric_accumulator, whichis_numeric_expralready consumed as a raw double inside this clone. Thefadds are the same IEEE operations on the same operands in the same order, so-0, NaN and overflow to Infinity are bit-identical. A non-Number entry value (string concatenation) still fails the preheader test and takes the slow clone.0..=i32::MAX,arr.length, ormaterialize_loop_i32's integral0..=i32::MAXvalue, and the start is an integer literal in that range. Soi < bound <= i32::MAXholds before everyadd i32 1. Fractional, NaN, negative and out-of-range bounds still route to the slow clone.New
repeatloop (arm64,perry_fn_access_worker_ts__run$spec_b_b, whole body)Before (
main): 16 instructions, withfmov d2, x12; fadd; fmov x25, d1forsum,fadd d8, d8, d0fori, andmov x12, x26; fmov d1, x12; fcmp d8, d1forcount.Instructions and cycles per iteration
Measured with
/usr/bin/time -lon the bench mini: 50M iterations minus a 0-iteration run.The PR's instruction counts are identical on 1m and 20m. Its cycles on 1m/20m are: sequential 5.2/10.9, random 16.3/17.3, fields 9.0/11.1, repeat 3.0/3.0.
Access screen: official settings (1M iterations, warmup 0)
Bench mini, best of 9 interleaved rounds, CPU ns/iteration.
main=eb13fa188d, this PR's compiler =17363b000c. The later commits change only tests, the changelog andmoddeclaration order.All 12 cells are at or better than the better of Node and Bun. main missed 4.
Access screen: warmed (50M iterations, 1M warmup)
Best of 5 interleaved rounds.
main lost all 12 warmed cells; this PR wins 3. The 9 remaining misses have these structural causes, none of which is the register-domain problem this PR fixes:
repeat(0.94 vs 0.31–0.52). One IEEE add per iteration is 3 cycles of latency on this core, and the loop is now exactly that. The JITs' 1.0–1.6 cycles mean an int32-speculated accumulator, which the brief rules out (JS double semantics forsum).fields(2.81 vs Node 2.77). Three serial IEEE adds make a 9-cycle chain (measured 9.02 cycles/iteration). That is the floor for the source-order fold in the double domain.random(4.86/5.09/5.39 vs 4.63/4.47/4.64).sdiv/msub: 15.6 cycles, unchanged by this PR.srem, which it needs for exactness overcarried ∈ [0, i32::MAX], is ~5 % slower than an int32 division. A same-host C microbenchmark measured 4.69 vs 4.46 ns, which accounts for the whole 16k gap.sequential1m/20m (1.65/3.38 vs 1.32/2.02). The same 22 instructions take 3.4 → 5.2 → 10.9 cycles from 16k to 20m, so these cells are memory-bound, with the same locality difference.JSON matrix (50 rows, 5 interleaved rounds, best-of, PR vs main)
scan, is 0.5–1.1 % faster on all four sizes. No row's peak RSS went up.long_string_1m:stringify: −4.4 %.escaped_1m:stringify: +10.3 % (160.8 → 177.3 ms, reproduced on symbolized binaries).escaped_1m:stringifyregression is code layout, not this change:perry_runtime::json::stringify_flat::emit_piece. The runtime archives differ only in same-size members (the build-commit stamp).0x1004c8dd0on main and0x1004c8d50on this PR, because the worker module got smaller.Tests and gates
cargo test --release -p perry-codegen --lib:test result: ok. 1538 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out.stmt/element_shape_native_tests.rs, 5 IR-census tests.repeatcondition isicmp slt i32on the seeded counter, and the update touches only the i32 slot.fieldsredirects only the accumulator.70 passed; 5 failed).side_exit_countnow also counts the trampoline spelling of an exit. The carried "published exactly once" check now counts the store of the converted carried value, because the accumulator store in that block is now also astore double.test-files/test_gap_json_record_loop_clone.tsgains section 10:-0/+0 accumulation, overflow to Infinity,Infinity + -Infinity, and a NaN start.repeatform.randomside exit on the fifth iteration.node --experimental-strip-types(v26.5.1) with both this PR's compiler and main's (86 lines).--trace llvmconfirms the clone fires and emits the trampoline in every new function.scripts/check_file_size.sh:OK: no Rust source files exceed 2000 lines.scripts/run_lint_gates.sh:run_lint_gates: 3 of 83 FAILED; 2 CI-only skipped. All three are pre-existing:[Public benchmark evidence freshness] python3 benchmarks/ci_public_baseline_check.py[warnings: rustc warnings (host-compatible, all targets)]: dead code incrates/perry-runtime/src/object/global_this_webassembly.rs:201.[check: Check for API docs drift]: the two docs files were restored afterwards.-D warningscheck stops at perry-runtime,RUSTFLAGS="-D warnings" cargo check -p perry-codegen --all-targetswas run separately: clean. Clippy reports nothing in the new files.Summary by CodeRabbit
Performance
Bug Fixes
NaN, signed zero, and unsupported counter values.Tests