perf(gc): gate the array-push write barrier on a live parent-generation test (#7511) - #7602
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughArray-push code generation now stores the element unconditionally and emits a write barrier only when the parent is tenured or incremental marking is active. Runtime and IR tests validate the generation contract, remembered-set behavior, barrier placement, and incremental-marking behavior. The workspace version is updated to 0.5.1341. ChangesGeneration-aware array barriers
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArrayPush
participant WriteBarrierEmitter
participant RuntimeGC
ArrayPush->>WriteBarrierEmitter: provide parent bits, slot address, and child bits
WriteBarrierEmitter->>RuntimeGC: check GC_FLAG_TENURED and marking count
RuntimeGC-->>WriteBarrierEmitter: return barrier-needed condition
WriteBarrierEmitter->>RuntimeGC: emit js_write_barrier_slot when condition is true
Possibly related issues
Possibly related PRs
Suggested reviewers: ✨ 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 |
…on test (#7511) The array push is the only surviving barrier call site on push_cls / churn_alloc / churn, and PERRY_GC_TRACE counts 19,945,222 calls of which 19,743,573 (99.0%) end in parent_not_old_skips, with old_to_young_slow_hits and new_inserts both ZERO. #7536's value-side test cannot reach any of it (non_pointer_child_skips == 0 — the pushed value really is a heap pointer); the waste is entirely parent-side. Skip the call when the parent's live header carries no GC_FLAG_TENURED AND no incremental cycle is active. The second clause is what keeps the SATB / insertion shading in barrier_child_prologue alive, and reuses the same PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT gate shadow_inline.rs already emits for the root shading barrier. Also assert Old => TENURED where the classification is made, so the invariant the codegen gate rests on can actually fail.
Runtime: gc::tests::inline_generation_gate_contract pins the codegen comparand against GC_FLAG_TENURED, asserts Old => TENURED over the three birth paths where the bit is a caller's obligation rather than a consequence of surviving, and carries a stranding witness whose sabotaged always-false gate leaves the old->young edge unrecorded. Codegen: array_push::parent_gate_tests pins the emitted structure — the barrier alone inside apush.barrier.<n>, the slot store outside it, and both clauses of the gate present. Lives in the crate so --lib sees it rather than the tag-only integration suite. Moves the invariant predicate to gc/types.rs beside the flag it reads, keeping barrier.rs under the 2000-line cap.
Replacing the gate's `or` with a constant-true left both `and i8 …, 32` and the incremental global in the IR — the clauses are still computed, just no longer consulted — so the structural test stayed green while the gate had stopped gating. Follow the cond_br's condition back to its definition and require an `or i1` of an i8 header test and an i32 count test.
A debug_assert! in barrier_parent_needs_remembering was the better enforcement point, but dozens of existing tests build old-gen fixtures straight from arena_alloc_gc_old without the bit — some deliberately — so it fired on fixtures, not defects. The invariant stays pinned over the PRODUCTION birth paths by inline_generation_gate_contract, and the reason the assert is absent is recorded where someone would try to add it.
6124650 to
a3a610f
Compare
Audit before merge — verified, merged as v0.5.1341Perf reproduced: interleaved best-of-5 both arms, my own builds of current The soundness evidence that matters most reproduced bit-for-bit: Sabotage re-verified, both directions:
Root-dominance re-run because codegen changed: corpus 128/128, One honest limitation of my auditI could not reproduce the ZEAL+PROTECT_FROMSPACE arm: with Kept from the report, for the recordThe |
1 participant
Closes part of #7511.
Re-measured first — the headline had NOT collapsed
The ticket's 16.1% / ~25% figures predate #7536, #7594 and #7596, so this starts
with a fresh symbolicated profile at v0.5.1339 (
PERRY_DEBUG_SYMBOLS=1+sample,push_cls.ts, 2286 leaf samples undermain):js_write_barrier_slotwrite_barrier_decoded_parentbarrier_child_prologueincremental_mark_barrier_valueIt had grown, not collapsed. All four symbols are the GC write barrier
(
gc/barrier.rs), not thePtr<Shape>promotion barrier — checked, per #7187.All of it is one call site. The call graph names it:
perry_fn_push_cls_ts__chunk + 568,i.e.
keep.push(new Node(...)).push_cls_ts__Node_constructorappears as a leafwith no barrier beneath it — the constructor's two
numberfield stores alreadypay nothing, because a declared-
numberfield selects the raw-f64 representationand its store is guarded by an inline plain-finite check with a downgrading cold
fallback. That is why #7536 correctly measured
push_clsunchanged.What #7536 already covered, and why it cannot reach this
#7536 put the class-field store's three bookkeeping calls behind one inline test of
the stored value's bits.
PERRY_GC_TRACEshows why that test cannot fire here:non_pointer_child_skipsparent_not_old_skipsold_to_young_slow_hitsnew_insertspush_clschurn_allocchurntreecyclesretainnon_pointer_child_skips == 0: the pushed value genuinely is a heap pointer, soexpr_produces_non_pointer_bits_by_constructionis not merely unhelpful, it iscorrect to say "pointer". And the remembered set is never inserted into — not
once — in 20 million calls. The waste is entirely parent-side.
#7536's own fragment hands off the
put.pic.hitpath as the follow-up; that turnedout to be the wrong lead for these benches (it is
PutValueSet, andpush_cls'sfield stores never reach a barrier at all), so this takes the lever the counters
actually point at.
The change
A parent-side question admits no by-construction proof —
keepcrosses hundreds ofcollections between its allocation and its last push, so "the parent is young" is
exactly the claim #7501 showed gets revoked at runtime. So it is decided by a live
test at the store:
Both clauses are load-bearing, for unrelated reasons.
TENURED. The remembered set only ever needs an entry when
barrier_parent_needs_rememberingclassifies the parentOld. Soundness is thesuperset property #7536 used:
Old ⟹ TENURED, so!TENURED ⟹ !Old, and this gatecan only skip a subset of what the runtime already skips. Every path that places an
object into an old-gen block sets the bit in the same breath —
gc/copying.rs:612-637selects
arena_alloc_gc_oldandGC_FLAG_TENUREDfrom onepromoteexpression;gc/oldgen.rs:1740and:1838;buffer/header.rs:486;typedarray/mod.rs:722;json_tape.rsviaarena_alloc_gc_old_born_tenured— and nothing clears it on alive object. The stronger reading: a parent that is neither physically old nor
logically tenured is fully traced by every minor GC (
gc/trace.rs:747), so itsedges are rediscovered.
Incremental. Skipping the call also skips
barrier_child_prologue'sincremental_mark_barrier_value— the insertion/SATB shading, which is not agenerational question and must never be dropped while a cycle is live. A zero count
proves this thread's
INCREMENTAL_MARK_BARRIER_VALID_PTRSis null, becauseincremental_mark_barrier_enableinstalls the thread-local before incrementingthe count, an ordering
gc/barrier.rsalready documents as load-bearing. This reusesthe exact gate
expr/shadow_inline.rsandexpr/shadow_slot.rsalready emit for theroot shading barrier — no new runtime symbol.
The gate reads the array's header byte at
arr_handle - 7, which thenofwdblockalready loads for its forwarding test in the dominating block, so the whole thing
is one
and, oneicmp, one global load and anor. It is emitted only insideapush.inbounds, reached only after that header test, so the dereference rests on avalidation the existing code already performs — the "NaN-boxed parent bits would be
dereferenced" hazard does not apply. The slot store stays unconditional and outside
the branch. Under
PERRY_WRITE_BARRIERS=0nothing is emitted at all.Emitted IR (real
push_cls.ts,--trace llvm):Effect
Barrier calls, same counters:
push_clschurn_allocchurnpush_numtree/cycles/deeplistretainunarmed_skips)retain's real work is bit-identical:old_to_young_slow_hits3,204,922 andnew_inserts6,268 in both arms. Every genuine remembered-set insert is preserved.GC cycle counts identical in both arms on every bench (
push_cls/churn_alloc/churn105,
tree42,cycles16,retain7,deeplist4,push_num18).Timing — pinned quiet M1 mini, load ~1.5, best-of-5
user+sys, both arms linkedagainst the same runtime archive, baseline measured twice:
push_clschurn_allocpush_numchurncyclestreeretainchurn_readdeeplistdeeplistis the only row that could read as a regression; base run1 measured 1.290,identical to treat, so it is noise.
tree/cycles/deeplistgain little becausetheir barrier traffic is class-field stores, not array pushes — that is the honest
scope limit of this PR, and the remaining #7511 lever.
Validation
Local only — CI has a deep backlog, so none of this is CI-confirmed.
rc=0, includingcls_mistyped.ts(the mistyped-number-field GC probe: 20000 strings survive).PERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_VERIFY_MARK=1clean on both arms.PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_DIAG=1clean on both arms,instrument proven live: 110
[gc-fromspace-protect] mode=ProtectPages retired_set=#Nlines on
push_cls, 12 onretain— copying minors really ran and from-space wasquarantined and
mprotected.gc_root_dominance_corpus.sh→ 128/128 sources, 148.ll. Both gated modes green:--moving-only --seeded-violations 40reports 40 planted, 40 caught, 0 missed;--unrooted-allocasreports 0 over 7772 gc-capable allocas. Allowlist untouched.cargo test -p perry-runtime --lib --no-fail-fast: 1848 passed / 0 failed on 3 of 4runs. One run had a single failure I could not reproduce in three further runs —
consistent with the known macOS parallel-test flakiness, but I am flagging it rather
than calling it clean.
cargo test -p perry-codegen --lib: 674 passed / 0 failed.raw_handle_debt.py998 (baseline 998);addr_class_inventory.pypassed;class_id_collisions.pypassed;check_file_size.shOK;cargo fmt --all -- --checkclean.Sabotage evidence (every new test shown able to fail)
0x40codegen_tenured_comparand_matches_the_runtime_flag+ witness REDthe_incremental_clause_forces_the_call_for_a_young_parentREDbuffer_allocforgetsGC_FLAG_TENUREDevery_old_gen_birth_path_sets_tenuredREDor→ constant-truearray_push_barrier_is_gated_on_the_parent_headerREDarray_push_slot_store_stays_outside_the_gateREDS4 initially did NOT fail, and that is worth reading. Replacing the
orwith aconstant-true leaves both
and i8 …, 32and the incremental global in the IR — theclauses are still computed, just no longer consulted — so a substring-matching test
stayed green while the gate had stopped gating: CLAUDE.md hazard 4 in my own test.
The test now follows the
cond_br's condition back to its definition and requires anor i1of an i8 header test and an i32 count test.One thing I tried and reverted, deliberately
A
debug_assert!inbarrier_parent_needs_rememberingcheckingOld ⟹ TENUREDatthe point the classification is made. That is the better enforcement — every old-parent
store in every debug run would recheck it — and
arena_alloc_gc_oldgenuinely doesnot establish the invariant (it writes
GC_FLAG_ARENA | gc_birth_extra_flags()andleaves the bit to its eight callers). But dozens of existing tests build old-gen
fixtures straight from that allocator without the bit —
alloc_old_test_object,alloc_old_test_array,alloc_old_test_promise, most ofgc/tests/oldgen.rs— somedeliberately, so it fired on fixtures rather than defects and turned
cargo-testred.The invariant is instead pinned over the production birth paths by
every_old_gen_birth_path_sets_tenured(sabotage S3), and the reason the assert isabsent is recorded where someone would next try to add it. Making it shippable means
fixing those fixtures first, which is its own change.
Summary by CodeRabbit
Performance
Reliability
Release