perf(codegen): put the numeric array push's GC bookkeeping behind one live test (push_num 0.149 -> 0.069) - #7839
Conversation
📝 WalkthroughWalkthroughNumeric array pushes now store values unconditionally and guard GC bookkeeping with live-bit heap-pointer checks. Array admission masks cover additional numeric layouts. New LLVM IR tests verify bookkeeping placement, runtime routing, pointer-push behavior, and non-constant guard conditions. ChangesNumeric push GC bookkeeping
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ArrayPushCodegen
participant HeapPointerCheck
participant NumericSlotStore
participant GCBookkeepingOps
ArrayPushCodegen->>NumericSlotStore: store numeric value unconditionally
ArrayPushCodegen->>HeapPointerCheck: test live bits for heap pointers
HeapPointerCheck-->>ArrayPushCodegen: computed pointer-carry result
ArrayPushCodegen->>GCBookkeepingOps: emit addref, layout note, and write barrier when needed
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
… live test
The inline array-append tier emitted `js_string_addref_if_heap_string`,
`js_gc_note_slot_layout` and a seq_cst load of
`PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT` on EVERY element. On
`bench/push_num.ts` — 20,000,000 pushes of a double into a `number[]` — all
three are dead on all 20M of them.
The static proof that retires them cannot be made for the shape that matters:
`keep.push(base + j)` is an `Expr::Binary { Add }`, and
`expr_produces_non_pointer_bits_by_construction` answers `false` there
unconditionally, because `+` is string concatenation for non-numeric operands.
This is #7511's answer to the identical problem on class-field stores, applied
to the array append: ask the question ONCE inline, on the live bits, and branch
over all three calls. The array's half of the proof rides the header test the
`nofwd` block already performs — the integrity mask widens from 0x0407 to
0x3C07, so reaching the inline store additionally proves ELEMENT_SHAPE,
TYPED_LAYOUT_INTACT and ALL_POINTERS clear, the three states in which
`js_gc_note_slot_layout` does real work for a non-pointer value.
A guard, not an elision: Perry does not validate declared types, so a
`number`-annotated value that is a heap string at runtime takes the guarded arm
and records the slot exactly as it always did.
98b9956 to
8a828ec
Compare
…push guard Two sabotage-verified IR gates, prompted by review of the #7831/#7837 family against #7839's guard. `a_declared_type_lie_is_routed_to_the_runtime_tier_not_the_guard` — a `number[]` really can hold heap strings at runtime, and `is_numeric_expr` admits an element read off one (#7810). What keeps that value off the inline guard is `expr_produces_canonical_raw_f64` excluding every READ, which routes it to the pre-existing runtime numeric tier instead. Widening that predicate to admit a read fails this test. `the_guard_branches_on_the_live_bits_not_on_a_constant` — pins the guard's condition to a computed register and its predicate to the full heap-tag set. Hard-wiring the branch to `false` fails this test; it is invisible to every output-equality probe, because the elided bookkeeping is a GC-liveness fact rather than an arithmetic one.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/array_push_guard_tests.rs (1)
243-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the write barrier remains in the guarded control-flow path.
These tests verify only
js_gc_note_slot_layoutandjs_string_addref_if_heap_string. They do not verifyjs_write_barrier_slot.If
emit_numeric_push_store_pointer_testedreturns barrier bits,crates/perry-codegen/src/expr/array_push.rsLines 1025-1036 emit the parent-generation gate afterapush.gc_bookkeeping.done. The current tests still pass, but every numeric push pays the barrier gate again.Assert that the barrier call remains emitted and that its parent-generation gate is reachable only from
apush.gc_bookkeeping.🤖 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 `@crates/perry-codegen/src/expr/array_push_guard_tests.rs` around lines 243 - 289, Extend the numeric push guard tests around emit_numeric_push_store_pointer_tested to verify js_write_barrier_slot is emitted and remains in the guarded control-flow path. Assert the barrier call is absent from the inline inbounds block and that its parent-generation gate is reachable only after apush.gc_bookkeeping.done, preserving the existing assertions for the note and addref calls.
🤖 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 `@crates/perry-codegen/src/expr/array_push_guard_tests.rs`:
- Around line 243-289: Extend the numeric push guard tests around
emit_numeric_push_store_pointer_tested to verify js_write_barrier_slot is
emitted and remains in the guarded control-flow path. Assert the barrier call is
absent from the inline inbounds block and that its parent-generation gate is
reachable only after apush.gc_bookkeeping.done, preserving the existing
assertions for the note and addref calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d611dda-7e33-4f74-b934-23611e7f41c0
📒 Files selected for processing (4)
changelog.d/7839-numeric-push-pointer-tested.mdcrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/array_push_guard_tests.rscrates/perry-codegen/src/expr/mod.rs
What
The inline array-append tier emitted three GC-bookkeeping obligations on every
element:
js_string_addref_if_heap_string,js_gc_note_slot_layout, and aseq_cst load of
PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNTto gatejs_write_barrier_slot. Ongc-handoff/bench/push_num.ts— 20,000,000 pushes ofa double into a
number[]— all three are dead on all 20M of them.This is #7511's answer to the identical problem on class-field stores, applied to
the array append: ask the question once, inline, on the live bits, and branch
over all three calls.
Why the static proof cannot do it
array_store_needs_layout_noterests onexpr_produces_non_pointer_bits_by_construction, and the shape that matters iskeep.push(base + j)— anExpr::Binary { Add }, where that predicate answersfalseunconditionally because+is string concatenation for non-numericoperands. It fires only for a bare canonical-i32 local, which is why
keep.push(j)andkeep.push(base + j)compile to materially different loopstoday.
The two halves of the guard
emit_may_carry_heap_pointer_check— already the codegen mirrorof
layout_pointer_bearing_bits/decode_heap_addr, already contract-testedover the whole 16-bit tag space. The store itself stays unconditional and
outside the branch; only the bookkeeping moves.
nofwdblock already performs: theintegrity mask widens
0x0407→0x3C07, so reaching the inline storeadditionally proves
GC_ARRAY_ELEMENT_SHAPE,GC_OBJ_TYPED_LAYOUT_INTACTandGC_LAYOUT_ALL_POINTERSclear — the three states in whichjs_gc_note_slot_layoutdoes real work for a non-pointer value. An array inany of them takes
js_array_push_f64, which notes the slot exactly as before.Same
and, sameicmp, wider constant.GC_LAYOUT_SIDE_MASKis deliberately not in the mask: skipping the note thereleaves a stale set bit over a non-pointer, and
mark_field_into_worklistre-validates every slot word, so the cost is one rejected visit and never a
stranded child — the identical argument
class_field_store_needs_layout_notealready ships.
A guard, not an elision.
the_guarded_arm_still_reaches_every_call_it_movedasserts the calls are still emitted, so a future "simplification" to an outright
elision fails there rather than as heap corruption.
Gated on
is_numeric_expr, so a pointer-pushing loop emits byte-identical IR.Measured — absolute seconds, quiet M1 mini
best-of-25, interleaved with rotated start position, exit codes checked, output
byte-identical to
node --experimental-strip-types. Load 1.17 before / 1.99after, zero foreign benchmark or compiler processes at both ends.
main@1ee158d27push_num0.1494 → 0.0692 s, −53.7%. It was the only benchmark in the corpusPerry lost to both node and scriptc; it is now 0.60× node and 0.74×
scriptc. The distributions are disjoint — this PR's worst of 25 (0.0896) is
below scriptc's best (0.0931).
No regression anywhere else — by construction, not by timing
24 of the 25 corpus programs compile BYTE-IDENTICALLY between
mainand thisbranch (
cmpovergc-handoff/m0810's full list plus the_realarms). Onlypush_numdiffers. Nothing else can have moved.The two byte-identical pairs were timed anyway as an in-run noise floor: the same
binary under two names read +0.19% (
push_cls) and −0.05% (churn)apart, so the host's floor on this run is ~0.2% and the −53.7% is ~250× it.
_realarmschurn_real,churn_alloc_real,push_cls_real,cycles_real,tree_real,retain_realare all in the byte-identical set, so their bootstrap penalty isunchanged by construction.
The declared-type lie (#7831/#7837) — reviewed, and it cannot reach this guard
A peer raised the case my own probes could not reach: a
number[]fed a heapstring built by
+. I checked it rather than assumed, and the answer is worthmore than the test would have been.
gc-handoff/m0810/numarr_lie.tsdoes not reach the guard. IR census: 0apush.gc_bookkeepingblocks. Its pushed value is(s1 + s2 + String(i))withs1/s2typedany, sois_numeric_expris false,guarded_numeric_bookkeepingis false, and the push keeps the historical unguarded tier untouched by this PR.
Empirically it prints
2000 string hello wor0 hello wor1999 6000on pre-#7839main, on this branch, and on a deliberately sabotaged compiler with the guard
hard-wired to
false— so it cannot fail on a broken guard. It is a fineregression test for the pre-existing path; it is not a detector for #7839.
Four further lie shapes — a
numberparameter, anumberobject field, amodule-level
numberglobal, and an element read off anumber[]— all emitzero guard blocks. The reason is structural, and is the clearest statement of
why this guard is safe:
is_numeric_exprdoes admit a read off anumber[](fix(codegen): stop typing a symbol-keyed element read as a number (#7796) #7810), so an erasedannotation alone would put a heap string on a numeric push path.
expr_produces_canonical_raw_f64excludes every read ("cold fallbacksreturn boxed bits"), so
keep_guarded_numeric_pushstays true and those pushesroute to the pre-existing runtime numeric tier
(
js_array_numeric_push_f64_unboxedbehind its feedback guard), whichvalidates the value at runtime.
canonical raw f64 by construction — a machine FP op. Such a value cannot be a
pointer except by ARM NaN-payload propagation from a NaN-boxed operand, which
is exactly what the live-bits test catches.
Two sabotage-verified detectors
Both are IR gates that run in
cargo test, and both were confirmed to fail ona deliberately broken compiler — the bar the runtime probes could not clear:
the_guard_branches_on_the_live_bits_not_on_a_constantbr i1 false(bookkeeping arm unreachable)a_declared_type_lie_is_routed_to_the_runtime_tier_not_the_guardexpr_produces_canonical_raw_f64widened to admit a readThe first pins the guard's condition to a computed register and its predicate to
the full heap-tag comparand set. The second pins the routing above, so a future
widening of the numeric proof surfaces here rather than as a guard resting on an
annotation.
Correctness
cargo test --release -p perry-codegen --lib— 861 passed, 0 failed,including five IR-census tests for this change.
the recorded node expectations.
gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0, plainand under
PERRY_GC_SCHEDULE_RATE=1.numarr_lie.tsmatches node underPERRY_GC_SCHEDULE_RATE=1,…_ALLOC_KB=0and
PERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_FORCE_EVACUATE=1.cargo fmt --all -- --checkclean.Honest limitations
The runtime probes are not detectors, which is why the two IR gates exist. A
sabotaged compiler with the guard hard-wired off passed every runtime probe I
wrote, and passed
numarr_lie.tstoo. Soundness rests on the routing argumentabove and on the two sabotage-verified gates, not on a runtime detector.
I am not citing the from-space protector as evidence. Under
PERRY_GC_DIAG=1it prints zero[gc-fromspace-protect] retired_set=lineson these programs, so the instrument never ran and that arm proves nothing.
Side observation for anyone extending this: for these shapes the layout note
appears less load-bearing than the surrounding comments imply, most likely
because the arrays end up
GC_LAYOUT_UNKNOWNand are tag-scanned anyway.Not run locally
The gap suite — it needs a quiet host, and the shared mini had several sessions
contending for it all day.
Pre-existing red gates, not from this branch
scripts/check_file_size.shfails onmain@1ee158d27 forcrates/perry-hir/src/lower/pre_scan.rs(2011 lines) andcrates/perry-runtime/src/gc/layout.rs(2023) — both were under the cap at0a2bf15bd. Neither is touched here.