perf(codegen): gate the guarded element store's write barrier on the stored value (#7715) - #7959
Conversation
|
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 (1)
📝 WalkthroughWalkthroughGuarded array-element stores now emit write barriers only when the stored value may contain a heap pointer and the parent requires generation or incremental-marking handling. LLVM IR tests validate predicate ordering and barrier emission. ChangesGuarded array-store barrier gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GuardedArrayStore
participant WriteBarrierEmitter
participant RuntimeBarrier
GuardedArrayStore->>WriteBarrierEmitter: Pass stored value and slot metadata
WriteBarrierEmitter->>WriteBarrierEmitter: Check heap-pointer value tags
WriteBarrierEmitter->>WriteBarrierEmitter: Check parent generation and marking state
WriteBarrierEmitter->>RuntimeBarrier: Emit js_write_barrier_slot when checks pass
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/index_set_barrier_tests.rs (1)
79-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the local-receiver in-bounds path.
This fixture intentionally bypasses
lower_index_set_fast, butcrates/perry-codegen/src/expr/index.rsalso changed its deferred barrier integration. Add a sibling IR fixture with a stack-local array receiver. Assert itsidxset.inbounds.barrier.maybepath reaches the value gate and barrier call.🤖 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/index_set_barrier_tests.rs` around lines 79 - 90, Add a sibling IR fixture in index_set_barrier_tests.rs using a stack-local array receiver, while preserving the integer-literal-seeded for-loop index and Any value setup. Assert that the idxset.inbounds.barrier.maybe path reaches the value gate and invokes the deferred barrier call, covering the local-receiver integration in index.rs.
🤖 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.
Inline comments:
In `@crates/perry-codegen/src/expr/index_set_barrier_tests.rs`:
- Around line 241-256: Extend the assertions in the index-set barrier test to
verify the final value-gate OR includes the raw-address AND predicate from
emit_may_carry_heap_pointer_check. Also assert that this raw-address branch
compares the extracted top-16 tag against zero and the value bits against the
4096 floor, preserving coverage for untagged heap addresses alongside the three
tagged forms.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/index_set_barrier_tests.rs`:
- Around line 79-90: Add a sibling IR fixture in index_set_barrier_tests.rs
using a stack-local array receiver, while preserving the integer-literal-seeded
for-loop index and Any value setup. Assert that the
idxset.inbounds.barrier.maybe path reaches the value gate and invokes the
deferred barrier call, covering the local-receiver integration in index.rs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a27a88e0-f02e-41f0-98cb-9250502a7bdf
📒 Files selected for processing (7)
changelog.d/7715-element-store-barrier-value-gate.mdcrates/perry-codegen/src/expr/class_field_barrier_tests.rscrates/perry-codegen/src/expr/index.rscrates/perry-codegen/src/expr/index_set_barrier_tests.rscrates/perry-codegen/src/expr/index_set_guarded.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/write_barrier.rs
| // Every heap tag the runtime's `decode_heap_addr` accepts must be compared | ||
| // against. Dropping one is the direction that STRANDS a child, so the whole | ||
| // comparand set is pinned, not just its shape. | ||
| for tag in ["32765", "32767", "32762"] { | ||
| assert!( | ||
| body.contains(&format!(", {tag}\n")) || body.contains(&format!(", {tag}")), | ||
| "the value gate never compares against tag {tag}; a value carrying \ | ||
| it would skip the barrier while the runtime would have decoded a \ | ||
| heap address from it:\n{body}" | ||
| ); | ||
| } | ||
| assert!( | ||
| body.contains("lshr i64") && body.contains(", 48"), | ||
| "the value gate does not derive the top 16 bits of the stored value, \ | ||
| so it is testing something other than the NaN-box tag:\n{body}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the raw heap-address predicate.
emit_may_carry_heap_pointer_check also accepts an untagged raw heap address when top16 == 0 and value_bits >= 4096. This test checks only the three tagged forms. A regression that removes is_raw_addr will pass this test but skip a required barrier for that value form.
Assert that the final value-gate or includes the raw-address and, then assert its zero-tag and floor comparisons.
🤖 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/index_set_barrier_tests.rs` around lines 241 -
256, Extend the assertions in the index-set barrier test to verify the final
value-gate OR includes the raw-address AND predicate from
emit_may_carry_heap_pointer_check. Also assert that this raw-address branch
compares the extracted top-16 tag against zero and the value bits against the
4096 floor, preserving coverage for untagged heap addresses alongside the three
tagged forms.
…stored value (#7715) Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
3cce02e to
6a58d34
Compare
|
★ Follow-up push: Updated, not weakened: the assertion now follows the edge — the in-bounds That test is also the standing proof that Full |
1 participant
Part of #7715 (write-barrier tower, B3 "slot elision").
The measurement that chose this change
#7715scopes B3 as "skip the write-barrier slot computation where the storeprovably cannot create an old→young edge", and B4 as "carry generation in the
header". I re-profiled first, as the issue instructs, and the data pointed
somewhere neither bullet did.
Static census of
js_write_barrier{,_slot}call sites across all 19 GC-corpusprograms (
--trace llvm, bucketed by the block they sit in):*.barrier, #7511/#7871)idxset.recv_prop.fastwb.maybeDynamic counters (
PERRY_GC_TRACE=1 PERRY_GC_DIAG=1, per-cyclewrite_barrierblock summed):
pipeline'sRegistry.setrunsthis.vals[i] = v1.44 M times. Every one ofthose calls decodes the stored value, finds it is a number, and returns having
done nothing — and
parent_not_old_skipsis 0, so the parent-generationgate #7511/#7871 shipped would have skipped nothing there. The lever at this
site is the VALUE test, not the generation test.
The change
js_write_barrier_slotat the two array-element store sites(
emit_guarded_inbounds_array_store, andlower_index_set_fast's JSValuein-bounds arm) now sits behind
emit_may_carry_heap_pointer_check, and thenbehind
emit_parent_may_need_remembering_check— the same two predicates theclass-field store has carried since #7511/#7871, nested value-first, not
fused into one
and.Nesting is load-bearing: the value test is pure register arithmetic on the
stored bits, while the parent test does a
monotonicload of@PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNTthat LLVM may not hoist out ofthe loop. A fused
andwould charge that non-hoistable load to exactly thenumeric stores this exists to make free.
Soundness
Skipping on the value test alone is sound because
write_barrier_slot_inner'sfirst action is
barrier_child_prologue(child), which returns — before the SATBshading, the armed check, the parent decode and the remembered set — when
decode_heap_addr(child) == 0. The emitted predicate is a documentedsuperset of that decode;
gc::tests::inline_pointer_bearing_contractenumerates the whole 16-bit tag space against it. A value carrying no heap
pointer has nothing to shade, which is why the incremental clause belongs to the
parent half and not this one; the parent half's obligations are unchanged and
stay pinned by
gc::tests::inline_generation_gate_contract.Reading the parent's header byte at
arr_handle - 7is safe on both arms: eachreached its block through a guard that already read
obj_typeat-8andgc_flagsat-7, and has already performed a raw store intoarr_handle + 8 + i*8.What deliberately stays outside the value test: the layout note. The
class-field emitter puts its note behind the same test, but that argument does
not transfer —
layout_note_slotfunnelscrate::array::note_element_store(#7480), and a non-pointer stored over apointer is exactly the store that must clear
GC_ARRAY_ELEMENT_SHAPE.js_array_note_numeric_writeis likewise not skippable on a pointer test:undefined/booleans are non-pointers that still have to downgrade the array'sraw-f64 layout.
Tests
expr/index_set_barrier_tests.rswalks the emitted def chain rather thanlooking for instructions nearby. It fails on: a hard-wired
br i1 true/falsethat leaves the dead predicate in the block; dropping any of the three heap-tag
comparands from the value predicate; dropping either disjunct of the parent
predicate; putting the barrier on the false edge; and eliding the call instead
of guarding it.
B4 — reported as measured-negative, not attempted
Recorded in
gc-handoff/BARRIER-NOTES.md§2.3/§2.5 so it is not re-derived:gc_flagsis full —gc/types.rs:975says so in terms("This is the last bit in the u8 gc_flags. Adding more flags requires
extending GcHeader ... extending breaks ABI everywhere") — and
_reservedisfully allocated and already
obj_type-overloaded.churn'sparent_not_old_skipsare theOld ⊊ TENUREDgap: objects the non-movingminor promoted logically (
gc/oldgen.rs) carryTENUREDwhile still livingin the nursery, so they pass the codegen gate and fail the runtime's
classify_heap_generation == Old.gc/trace.rs:799-810is explicit that thisstate must still be traced, which is why the approximation is sound and why
closing it needs a genuinely new bit. Outside
churnthe whole corpus totals494 such calls.
!TENUREDshort-circuit is worth nothing either, for thesame reason: every call that reaches
barrier_parent_needs_rememberinggotthere by passing the gate, so its parent is TENURED.
Measured
Both arms built from this worktree at
ac52a5c38(0.5.1490; the branch wasrebased onto
b392f7b5dafterwards, and the diff is unchanged). The change iscodegen-only
(
git diff --name-onlytouches nothing underperry-runtime/perry-stdlib),so both link the same
libperry_{runtime,stdlib}.aand the corpus wascompiled with the same output basenames in different directories.
cmpover the 19-program GC corpus: 16 identical, 3 differ — and the 3 areexactly
pipeline,interp,iso_miss, i.e. precisely the programs the censussays contain the site. The other 16 are provably unchanged and need no timing.
Instructions retired (
ab_instr.py, best of 7, dev box, spread ≤ 0.86%):(Direction only — final wall-clock belongs on the quiet mini.
interp's cyclescolumn moves with the box; its instruction count is flat to three digits and its
binary differs by one gated site.)
Correctness
m0810/expected/, exit 0 — includingthe
iso_misscanary (checksum 437840 misses 0).PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, underPERRY_GC_VERIFY_EVACUATION=1,and (the 3 that differ) under
PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_SCHEDULE_ALLOC_KB=64.cargo test -p perry-codegen --lib: 921 passed, 0 failed.hard-wiring the value gate's branch to
true(leaving the dead predicate inthe block) fails only
the_element_store_barrier_sits_behind_a_live_value_test;dropping the incremental disjunct fails only the two parent-gate tests
(including perf(codegen): interp 1.095 -> 0.844 s — gate the class-field barrier on the parent's generation, and let a hot recursive function inline its bump allocator #7871's existing one); eliding the call inside the barrier block
fails only
the_gated_element_store_still_reaches_the_barrier_call.Summary by CodeRabbit
Bug Fixes
Tests