perf(class-fields): typed slot stores for pointer-typed class fields (#5094) - #7686
Conversation
📝 WalkthroughWalkthroughThe PR extends sloppy class-field lowering to boxed fields, permits pointer-bearing typed layouts at allocation, broadens safe constructor-prologue analysis, updates GC classification, and adds regression coverage and benchmark documentation. ChangesClass-field lowering and typed layouts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Constructor
participant field_init
participant typed_shape
participant PutValueSet
participant property_set
participant js_put_value_set
Constructor->>field_init: analyze constructor field assignments
field_init->>typed_shape: determine allocation-time layout eligibility
typed_shape-->>Constructor: declare typed layout with pointer mask
PutValueSet->>property_set: lower sloppy class-field store
property_set-->>PutValueSet: perform guarded boxed slot store
property_set->>js_put_value_set: use strict = 0 fallback on guard miss
Possibly related issues
Possibly related PRs
🚥 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: 2
🤖 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/property_set.rs`:
- Around line 341-366: Wrap the body of try_lower_sloppy_class_field_boxed_store
in rooting::with_operands_rooted(ctx, &[object, value], ...). Ensure recv_box,
value, and all subsequent store-address and write-barrier operands are derived
inside that rooted closure so they remain valid after an evacuating RHS
collection.
In `@crates/perry-codegen/tests/typed_shape_declared_at_allocation.rs`:
- Around line 410-413: Strengthen the `Type::Any` assertion in the test by
extracting the typed-shape declaration line, verifying it contains
`@perry_typed_shape_mask_`, and asserting `INIT_CALL` is absent. Keep the
existing `DECLARE_CALL` check and mirror the pointer-field test’s ABI
assertions.
🪄 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: 0c4d87f4-d572-4c4d-874a-620f3849caf3
📒 Files selected for processing (8)
changelog.d/7686-pointer-class-field-slot-stores.mdcrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/lower_call/field_init.rscrates/perry-codegen/src/typed_shape.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/typed_shape_declared_at_allocation.rsscripts/gc_root_dominance_check.py
| // Operand order mirrors the raw-f64 arm and the strict class-field arm | ||
| // verbatim: the assignment reference is evaluated before the RHS, and the | ||
| // receiver's relocation across an allocating RHS is handled by the same | ||
| // statepoint re-read those arms rely on. | ||
| let recv_box = lower_expr(ctx, object)?; | ||
| let val_double = lower_expr(ctx, value)?; | ||
|
|
||
| // Computed before the block builder is borrowed below. | ||
| let barrier_needed = !expr_produces_non_pointer_bits_by_construction(ctx, value); | ||
| let layout_note_needed = class_field_store_needs_layout_note(ctx, value); | ||
| let string_addref_needed = class_field_store_needs_string_addref(ctx, value); | ||
|
|
||
| let key_idx = ctx.strings.intern(property); | ||
| let key_handle_global = format!("@{}", ctx.strings.entry(key_idx).handle_global); | ||
| let field_idx_str = field_index.to_string(); | ||
| let expected_class_id_str = expected_class_id.to_string(); | ||
|
|
||
| let (obj_bits, obj_handle, key_box, val_bits, expected_keys) = { | ||
| let blk = ctx.block(); | ||
| let obj_bits = blk.bitcast_double_to_i64(&recv_box); | ||
| let obj_handle = blk.and(I64, &obj_bits, POINTER_MASK_I64); | ||
| let key_box = blk.load(DOUBLE, &key_handle_global); | ||
| let val_bits = blk.bitcast_double_to_i64(&val_double); | ||
| let expected_keys = blk.load(I64, &format!("@{}", keys_global_name)); | ||
| (obj_bits, obj_handle, key_box, val_bits, expected_keys) | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for the statepoint re-read mechanism that would relocate `recv_box` across the RHS.
set -euo pipefail
rg -n -C 6 'statepoint' crates/perry-codegen/src | head -100
rg -n -C 4 'with_operands_rooted|guard_store_operand|StoreOperandGuard|with_rooted_group' crates/perry-codegen/src/expr/property_set.rs
ast-grep outline crates/perry-codegen/src/expr/rooting.rs --items allRepository: PerryTS/perry
Length of output: 10436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching property_set.rs/rooting:"
fd -a 'property_set|rooting' crates/perry-codegen/src | sed 's#^\./##'
echo
echo "Outline property_set:"
ast-grep outline crates/perry-codegen/src/expr/property_set.rs --match try_lower_sloppy_class_field_boxed_store --view expanded || true
ast-grep outline crates/perry-codegen/src/expr/property_set.rs --match try_lower_strict_class_field_boxed_store --view expanded || true
echo
echo "Relevant slices in property_set.rs:"
sed -n '332,450p' crates/perry-codegen/src/expr/property_set.rs
echo "----"
sed -n '800,880p' crates/perry-codegen/src/expr/property_set.rs
echo "----"
sed -n '1430,1495p' crates/perry-codegen/src/expr/property_set.rs
echo
echo "Search rooting.rs references:"
rg -n 'pub\(in crate::rooting\) mod|mod rooting|with_operands_rooted|with_operands_rooted_across|GuardStoreOperand|GuardStoreOperand' crates/perry-codegen/src -g '*.rs' | head -200Repository: PerryTS/perry
Length of output: 38036
🏁 Script executed:
#!/bin/bash
set - euo pipefail
echo "Rooting module outline:"
ast-grep outline crates/perry-codegen/src/rooting/mod.rs --view expanded | sed -n '1,240p' || true
echo
echo "Relevant rooting implementation slices:"
sed -n '500,680p' crates/perry-codegen/src/rooting/mod.rs
echo "----"
sed -n '900,945p' crates/perry-codegen/src/rooting/mod.rs
echo "----"
sed -n '1360,1405p' crates/perry-codegen/src/rooting/mod.rs
echo
echo "Adjacent strict/raw-f64 arms around try_lower_sloppy_class_field_boxed_store:"
sed -n '260,350p' crates/perry-codegen/src/expr/property_set.rs
sed -n '350,410p' crates/perry-codegen/src/expr/property_set.rsRepository: PerryTS/perry
Length of output: 26874
Root the receiver across the boxed-class-field store.
try_lower_sloppy_class_field_boxed_store lowers object for recv_box, then lowers value. The subsequent store address and write-barrier operands are derived from recv_box/obj_handle, so they are stale if the RHS triggers an evacuating collection. Wrap this helper body in rooting::with_operands_rooted(ctx, &[object, value], ...) so the receiver, value, and post-collection store/barrier operands come from the same rooted group.
🤖 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/property_set.rs` around lines 341 - 366, Wrap
the body of try_lower_sloppy_class_field_boxed_store in
rooting::with_operands_rooted(ctx, &[object, value], ...). Ensure recv_box,
value, and all subsequent store-address and write-barrier operands are derived
inside that rooted closure so they remain valid after an evacuating RHS
collection.
Source: Coding guidelines
| assert!( | ||
| !ir.contains(DECLARE_CALL), | ||
| "`Any` is pointer-bearing:\n{ir}" | ||
| ir.contains(DECLARE_CALL), | ||
| "`Any` is pointer-bearing, which is now a reason TO declare:\n{ir}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the Any pointer-mask ABI.
Type::Any is pointer-bearing, but this assertion only checks for a declaration call. A regression can pass this test while passing a null pointer mask or retaining js_gc_init_typed_shape_layout.
Extract the declaration line and assert @perry_typed_shape_mask_ is present. Also assert that INIT_CALL is absent, as in the pointer-field test.
Proposed test strengthening
- assert!(
- ir.contains(DECLARE_CALL),
- "`Any` is pointer-bearing, which is now a reason TO declare:\n{ir}"
- );
+ let line = ir
+ .lines()
+ .find(|l| l.contains(DECLARE_CALL))
+ .unwrap_or_else(|| panic!("`Any` must declare:\n{ir}"));
+ assert!(
+ line.contains("`@perry_typed_shape_mask_`"),
+ "`Any` must pass a non-null pointer mask: {line}"
+ );
+ assert!(
+ !ir.contains(INIT_CALL),
+ "the declaration must replace post-constructor initialization:\n{ir}"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert!( | |
| !ir.contains(DECLARE_CALL), | |
| "`Any` is pointer-bearing:\n{ir}" | |
| ir.contains(DECLARE_CALL), | |
| "`Any` is pointer-bearing, which is now a reason TO declare:\n{ir}" | |
| ); | |
| let line = ir | |
| .lines() | |
| .find(|l| l.contains(DECLARE_CALL)) | |
| .unwrap_or_else(|| panic!("`Any` must declare:\n{ir}")); | |
| assert!( | |
| line.contains("`@perry_typed_shape_mask_`"), | |
| "`Any` must pass a non-null pointer mask: {line}" | |
| ); | |
| assert!( | |
| !ir.contains(INIT_CALL), | |
| "the declaration must replace post-constructor initialization:\n{ir}" | |
| ); |
🤖 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/tests/typed_shape_declared_at_allocation.rs` around
lines 410 - 413, Strengthen the `Type::Any` assertion in the test by extracting
the typed-shape declaration line, verifying it contains
`@perry_typed_shape_mask_`, and asserting `INIT_CALL` is absent. Keep the
existing `DECLARE_CALL` check and mirror the pointer-field test’s ABI
assertions.
Blocking: a pointer store on the new path does not reach the remembered setThe perf work looks right and the writeup is careful — but the obligation this PR names as "the dangerous half" is not met. A young pointer stored into an old object never enters the remembered set on this branch. EvidenceBoth arms built
Why the PR's own GC section didn't catch itThose probes are pointer-cycle / linked-list / wide-tree shapes, which build their edges young-to-young. The barrier obligation only bites when the parent has already been promoted and then takes a pointer to a fresh object. My probe forces that ordering explicitly: class LNode { v: number; next: LNode | null; constructor(v: number) { this.v = v; this.next = null; } }
class Cell { n: number; peer: Cell | null; constructor(n: number) { this.n = n; this.peer = null; } }
function build(n: number): LNode {
let head = new LNode(0);
for (let i = 1; i < n; i++) { const nd = new LNode(i); nd.next = head; head = nd; }
return head;
}
// 1. allocate the parents, 2. churn hard so they get promoted,
// 3. THEN store a fresh young pointer into them.
const olds: Cell[] = [];
for (let i = 0; i < 400; i++) olds.push(new Cell(i));
let churn: any[] = [];
for (let i = 0; i < 200000; i++) { churn.push({ a: i, b: "s" + (i & 255) }); if (churn.length > 1000) churn = []; }
for (let i = 0; i < 400; i++) olds[i]!.peer = new Cell(i + 10000); // OLD <- YOUNG
let psum = 0; for (let i = 0; i < 400; i++) psum += olds[i]!.peer!.n + olds[i]!.n;
const head = build(30000);
let walk = 0, nodes = 0; let cur: LNode | null = head;
while (cur) { walk += cur.v; nodes++; cur = cur.next; }
console.log(psum, walk, nodes);Both arms print It is also invisible without zeal — Where I'd lookChange 1 — the sloppy-mode class-field route reaching boxed slots. #7288 opened that route for raw-f64 slots, which need no barrier; the boxed arm does. If the guard-free constant-slot store inherited the raw arm's barrier-free emission, this is exactly what you would see: correct value, correct dispatch, no dirty bit. Worth checking against #7602's rule while you're there — the array-push barrier gates on the parent's Everything else I checked, which is clean
The gate-drift finding is worth keeping regardless
Happy to re-audit as soon as the barrier is in; the reproducer above is self-contained. |
…d store contracts
…ape_layout cannot collect
Withdrawing my block — and the cause was not in this PRMy earlier review reported a deterministic Re-tested on current
Not vacuous — the copying minor still runs on both (10 and 14 What changed is And the barrier is present. I read the emitted IR rather than inferring from the runtime result. I should have read the IR before writing the review. The runtime abort was strong evidence something was wrong, and it was — but attributing it to a missing barrier rather than to an upstream moving collection cost you a round trip. The audit, now that it standsVerified on current main: 24/24 lint, Pre-existing failure confirmed as claimed, by checkout rather than inspection: The gate-drift finding stands on its own merits. The three changes landing together is right, and the reason is worth keeping: #7510 required an empty pointer mask, which excluded exactly these classes, so their descriptor arrived after every constructor store and none could pass its intact-bit guard. Discharging obligation 2 by pre-filling every slot with
|
3c394b7 to
13ba119
Compare
1 participant
P1 (#5094): one pointer field no longer demotes an object's whole store set
A single pointer-typed class field (
peer: Cell | null,next: LNode | null,left: Tree | null) put every field store on that object — including itsnumberfields — onjs_object_set_field_by_name: by-name dispatch, aRuntimeHandleScope,layout_note_slot, and a per-object side-table entry, forstores whose slot index is a compile-time constant.
Measured — quiet M1 mini, best-of-3 wall, both arms run back-to-back
cyclesdeeplisttree_widetreecycles,treeandtree_widenow beat scriptc;tree_wideis 4.0×.Unchanged, as required:
push_cls0.35,churn0.66,churn_alloc0.36,push_num0.13,retain1.32,retain10.42,churn_read0.35. Everybenchmark's stdout is byte-identical to the pre-change binary.
Three changes, and they must land together
The sloppy-mode class-field route reaches boxed slots (
expr/property_set.rs).build: byte-identical source compiles to a 44x-slower object depending on where the .ts file lives; the published baseline measures the fast arm #7288 opened it for raw-f64 slots only, so
n.next = headfell through to thePutValuewrite IC whose miss isjs_put_value_set→ by-name. Theguard-free store is licensed by the same perf(method dispatch): method_calls ~290× Node — remaining cost is per-field-access shape-guard calls (plan + standby) #5093 inline precheck the raw arm
uses (it rejects frozen and descriptor-bearing receivers — the only thing
sloppy and strict
PutValuedisagree about) and the miss staysjs_put_value_set(..., strict = 0).A pointer-bearing class declares its layout at allocation (
typed_shape.rs).perf(gc): layout side tables are 34% of object construction — the construction/death half of #5094 (allocation is 7.7%) #7510 required an EMPTY pointer mask, which excluded exactly these classes —
so their descriptor arrived after every store in their constructor and none
could pass its intact-bit guard (perf:
new Klass(v,w)is 63% slower than the equivalent object literal (28.5x vs 17.4x Node) #7512's defect, still open for this shape).Obligation 2 is now discharged rather than avoided: both
newallocationpaths pre-fill every slot with
TAG_UNDEFINED, which the tracer rejects atits tag check, so a pointer-masked slot visited before its first write cannot
strand anything. Obligation 1 (no read may observe a raw-f64 slot before its
first write) is unchanged and still required of every
numberfield.The constructor prologue admits literals and pure operator trees
(
lower_call/field_init.rs).constructor(v) { this.next = null; this.v = v }truncated the prologue at statement 0 and came back EMPTY, which cost both the
dead-
undefined-store elision and (via the same set) the declaration in (2).tree_wide'sthis.b = s + 1needs the operator-tree half.Why together: (1) alone REGRESSES
tree_wide. Measured directly bycompiling the benchmarks as ESM — which already takes the class-field route —
against the pre-change compiler:
tree_wide12.21 → 14.88 s. Routing aconstructor's stores to a guard the construction path has made unsatisfiable is
slower than the inline cache it displaces. (2) is what makes the guard passable.
GC
The dangerous half. A pointer store must still reach the remembered set.
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1(depth 800), binaries compiledwith
PERRY_GC_MOVING_LOOP_POLLS=1, over pointer-cycle / linked-list /wide-tree probes: clean, and not vacuous — 20005 / 40005 / 5
[gc-fromspace-protect]retirements, i.e. the copying minor really ran. Allthree match the
node 26.5.1oracle, including a walk that sums every numericslot as well as every pointer edge.
PERRY_GC_VERIFY_EVACUATION,PERRY_GC_VERIFY_MARKandPERRY_GC_FROMSPACE_SCAN_ABORT, each under zeal: clean.PERRY_GC_TRACEdrift:churnis bit-identical (105 cycles, 0.0039 GBcopied, positive reclamation every cycle).
deeplist/tree/tree_widekeeptheir cycle counts and kinds exactly (4 / 42 / 44);
cyclescopies 10,758 → 14objects and promotes 4,746 → 2, which is the side-table bookkeeping this PR
removes no longer keeping dead objects reachable.
gc-root-dominance, both arms, exact CI invocation: 0 violations with the40-seeded-violation control catching 40/40, and
--unrooted-allocas0.One latent gate bug this exposed
scripts/gc_root_dominance_check.py'sNONCOLLECTINGset is a second copy of afact
perry-codegen/src/gc_call_effects.rsalready states, and the two haddrifted: #7510 added
js_gc_declare_typed_shape_layoutbesidejs_gc_init_typed_shape_layoutin the Rust match and not in the Python set.That stayed invisible only because the corpus then contained no class the #7510
gate admitted. Widening the gate printed 358 spurious violations, every one
js_object_alloc_class_inline_keys->js_gc_declare_typed_shape_layout. The twoentry points share a body and differ only in a
TypedShapeProofthat makesdeclaredo strictly less work, so the classification is the same one.Pre-existing, not from this PR
typed_shape_descriptors::integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrierfails identically at
6cdcd79ecwith this PR's sources reverted (verified bycheckout, not by inspection). It is an array-push contract with no class-field
involvement.
interp.tsGC canary — do not use it as the probegc-handoff/BUG-interp-silent-wrong-answer.mdreports a moving-collector bug(silently wrong answer,
1708662vs node's1708840, 6/6) and proposes addinginterp.tsto the GC-ratchet corpus as a correctness probe. Under this PRthat probe passes while the bug is fully intact.
Measured by building two compilers from the same base commit, one with this
diff and one with the codegen sources reverted:
interp.ts×6iso_miss.ts×6The bug is present in both arms. This PR perturbs allocation/promotion timing
enough that
interp.tslands on the right answer;iso_miss.ts, the tighterisolation, stays red in both. That matches the bug report's own observation that
the miss count varies with allocation timing while the wrongness does not.
Use
iso_miss.tswithmisses == 0asserted (or its checksum againstnode's
437840).interp.tswould become a probe that cannot fail — CLAUDE.md's"four ways a gate can be unable to fail", item 4.
Nothing here is caused or fixed by this PR; it is flagged because landing it
silently changes what that canary means. Possibly the same root cause as #7682
(alloc-point minor moving past the immobility guard), which would also explain
why
PERRY_GC_SCAVENGE=0is one of the four knobs that restores the answer.