perf(repsel): per-array homogeneous element-shape invariant (#7480) - #7496
Merged
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe runtime adds per-array homogeneous object-shape proofs. It maintains proofs across stores, layout changes, prototype invalidation, allocation cleanup, and copying garbage collection. New C APIs expose proof establishment, class identity, versions, epochs, and guard checks. ChangesArray element-shape invariant
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-runtime/src/array/element_shape_tests.rs (1)
355-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
forget_element_shapeandprune_dead_element_shape_owners.The suite covers the store, rebuild, numeric-layout, prototype, and transfer paths. Two lifecycle hooks have no test:
forget_element_shape, reached fromlayout_clear_for_ptron object death and address recycling. It is the only path that removes the record rather than only clearing the bit. It is what prevents a recycled address from inheriting a stale per-arrayepoch.prune_dead_element_shape_owners, reached from the dead-owner fan-out incrates/perry-runtime/src/gc/dead_owner.rsline 220.A test for each would assert that
test_element_shape_record_existsreturnsfalseafter the hook runs.Based on learnings from PR 7227: for GC-rooted thread-local cache scanners and their reset helpers, add independent tests covering the cleanup hooks rather than assuming shared teardown exists.
🤖 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-runtime/src/array/element_shape_tests.rs` around lines 355 - 368, Add independent tests in the element-shape test suite for both lifecycle hooks: invoke forget_element_shape through the layout_clear_for_ptr/object-death or address-recycling path, and invoke prune_dead_element_shape_owners through the dead-owner cleanup path. In each test, create an element-shape record first, run the hook, and assert test_element_shape_record_exists returns false; do not rely on shared teardown to provide coverage.Source: Learnings
🤖 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-runtime/src/array/element_shape_tests.rs`:
- Around line 285-299: Introduce one shared serialization guard in a module
reachable by both test suites, protecting all tests that read or mutate
ELEMENT_SHAPE_EPOCH or CLASS_SHAPE_GENERATION. Apply it to the test
the_global_epoch_advances_on_a_clear_and_holds_still_otherwise and
prototype_surgery_retires_every_outstanding_proof, plus proof-survival tests
such as matching_pushes_extend_the_verified_prefix in
crates/perry-runtime/src/array/element_shape_tests.rs; in
crates/perry-runtime/src/gc/tests/layout_trace/element_shape.rs, acquire the
same guard in proven_array before its first push and hold it throughout both
copying-minor tests. Ensure the guard serializes both files’ tests against each
other.
In `@crates/perry-runtime/src/array/element_shape.rs`:
- Around line 455-484: Update transfer_element_shape so that when the
destination fails closed because moved is true but had_bit is false, it also
removes the record stored at new_user. Preserve the existing bit-clearing
behavior and ensure subsequent installs at new_user cannot inherit the
transferred array’s epoch.
---
Nitpick comments:
In `@crates/perry-runtime/src/array/element_shape_tests.rs`:
- Around line 355-368: Add independent tests in the element-shape test suite for
both lifecycle hooks: invoke forget_element_shape through the
layout_clear_for_ptr/object-death or address-recycling path, and invoke
prune_dead_element_shape_owners through the dead-owner cleanup path. In each
test, create an element-shape record first, run the hook, and assert
test_element_shape_record_exists returns false; do not rely on shared teardown
to provide coverage.
🪄 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: d41a210d-1722-4e75-a11f-37a873178c80
📒 Files selected for processing (12)
changelog.d/7496-element-shape-invariant.mdcrates/perry-runtime/src/array/element_shape.rscrates/perry-runtime/src/array/element_shape_tests.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/gc/dead_owner.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/tests/layout_trace.rscrates/perry-runtime/src/gc/tests/layout_trace/element_shape.rscrates/perry-runtime/src/gc/types.rscrates/perry-runtime/src/object/class_registry/prototype_methods.rscrates/perry-runtime/src/object/prototype_chain.rs
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
…heritance hole Three review findings from #7496: 1. The two process-wide counters made the tests order-dependent — the `ELEMENT_SHAPES` table is thread-local, but `ELEMENT_SHAPE_EPOCH` and `CLASS_SHAPE_GENERATION` are not, so one test's clear was another's observation. Both test files now take a shared, poison-tolerant `ELEMENT_SHAPE_TEST_LOCK` as their FIRST statement, so unwind order drops the state-restoring guards while the lock is still held. 2. `transfer_element_shape` moved the record before reading `had_bit`, so a fail-closed transfer could leave an ORPHAN record at the destination for a later establishment to inherit an identity from — silently continuing a different array's proof identity, which is the versioning a consumer is supposed to guard on. Fixed at the root: proof identities now come from a monotone `ELEMENT_SHAPE_PROOF_SEQ` rather than being read back from whatever record sits at the address, establishing is separated from extending, a clear drops the record outright, and the fail-closed transfer leaves nothing behind. 3. Added coverage for the two lifecycle hooks that stop a recycled address inheriting a stale identity: `forget_element_shape` (object death) and `prune_dead_element_shape_owners` (collection hook).
Contributor
Author
|
Done in 1ae6551 — and agreed that these are the same failure mode as the previous finding, not a separate nitpick: #[test]
fn forget_element_shape_removes_the_record_on_address_recycling() {
let _serialized = test_serialize();
let arr = built_from_pushes(CLASS_A, 3);
let retired = proof(arr).expect("proven").epoch;
assert!(test_element_shape_record_exists(arr as usize));
forget_element_shape(arr as usize); // gc::layout_clear_for_ptr
assert!(!test_element_shape_record_exists(arr as usize));
unsafe { assert!(!test_element_shape_bit_set(arr)) };
assert!(proof(arr).is_none());
let reused = unsafe { ensure_element_shape(arr) }.expect("still homogeneous");
assert_ne!(reused.epoch, retired);
}
#[test]
fn pruning_dead_owners_removes_their_records() {
let _serialized = test_serialize();
let dead = built_from_pushes(CLASS_A, 2);
let live = built_from_pushes(CLASS_A, 2);
let dead_key = dead as usize;
prune_dead_element_shape_owners(&|owner| owner == dead_key); // gc::dead_owner::fan_out
assert!(!test_element_shape_record_exists(dead as usize));
assert!(test_element_shape_record_exists(live as usize));
assert!(proof(live).is_some());
}Both assert the recycling consequence as well as the removal, so they fail if the hook stops removing or if the identity discipline regresses. The prune test also pins the other direction — a live owner's record and proof must survive — so a prune that over-collects cannot pass either. |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
added 8 commits
August 6, 2026 10:35
The shared prerequisite both #7480 routes need: an O(1), array-level answer to "are all the elements the same shape?". Invariant layer only — no consumer, no emitted-code change. Storage mirrors Phase 4a's dense bit: a `GcHeader._reserved` bit (11, shared with the object-only `OBJ_FLAG_HAS_DESCRIPTORS`) that the copying collector carries for free, plus an address-keyed record moved by `layout_transfer` exactly as `TYPED_LAYOUTS` is. The bit is the authority, so a stale record at a recycled address is unreachable and a missing record fails closed. Maintained from `gc::layout_note_slot` — the one funnel both the runtime's element-store helpers and codegen's inline element stores already reach, since `array_store_needs_layout_note` elides the note only for arrays statically proven numeric and pointer-free, which an element-shape array can never be. The record holds four integers and no heap pointer, so it is deliberately not a `gc_register_mutable_root_scanner` entry; dead keys are dropped on the same collection hook that prunes `ARRAY_NAMED_PROPS`.
…ssor `try_read_gc_header` additionally rejects small-buffer slab addresses, which pass `is_plausible_heap_addr` but carry no GcHeader at all — a bare `addr - GC_HEADER_SIZE` read there sees the previous slab entry's data bytes as a header.
`keepalive-anchors` is a DEFAULT feature, so a `#[used]` static would pin five uncalled functions into every shipped binary — the dead-strip defeat the hello-size campaign traced its regression to. The consumer PR anchors whichever symbol it emits.
…heritance hole Three review findings from #7496: 1. The two process-wide counters made the tests order-dependent — the `ELEMENT_SHAPES` table is thread-local, but `ELEMENT_SHAPE_EPOCH` and `CLASS_SHAPE_GENERATION` are not, so one test's clear was another's observation. Both test files now take a shared, poison-tolerant `ELEMENT_SHAPE_TEST_LOCK` as their FIRST statement, so unwind order drops the state-restoring guards while the lock is still held. 2. `transfer_element_shape` moved the record before reading `had_bit`, so a fail-closed transfer could leave an ORPHAN record at the destination for a later establishment to inherit an identity from — silently continuing a different array's proof identity, which is the versioning a consumer is supposed to guard on. Fixed at the root: proof identities now come from a monotone `ELEMENT_SHAPE_PROOF_SEQ` rather than being read back from whatever record sits at the address, establishing is separated from extending, a clear drops the record outright, and the fail-closed transfer leaves nothing behind. 3. Added coverage for the two lifecycle hooks that stop a recycled address inheriting a stale identity: `forget_element_shape` (object death) and `prune_dead_element_shape_owners` (collection hook).
proggeramlug
force-pushed
the
perf/7480-element-shape-invariant
branch
from
August 6, 2026 08:35
4cf2f94 to
4a5f9a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds the shared prerequisite decided in #7480. Invariant layer only — no consumer, and no emitted-code change.
#7480's scoping finding was that its two candidate routes are not "A vs B": both need the same missing fact first — a per-array homogeneous element-shape invariant, "every element of this array is an object of class
C". The only array-level invariants that existed were the numeric ones (GC_ARRAY_RAW_F64_LAYOUT/_HOLES). This PR adds the pointer sibling and stops there, because #6377's lesson is that every added proof un-gates latent fast paths its own microbench never exercises: the proof lands and is tested on its own before anything reads it.Storage — deliberately the same shape as Phase 4a's dense bit
4a's dense bit works because the collector copies the whole
_reservedword when it moves an object, so the invariant survives a copying minor with no side-table walk and no per-move bookkeeping. This mirrors it point for point:RAW_F64_LAYOUT)ELEMENT_SHAPE)_reservedbit 7_reservedbit 11_reservedis copied)ensure_array_numeric_raw_f64ensure_element_shapetransfer_array_numeric_layouttransfer_element_shapeclear_array_numeric_layoutclear_element_shapeBit 11 is shared with the object-only
OBJ_FLAG_HAS_DESCRIPTORS— the same disjoint-by-obj_typereuseGC_ARRAY_RAW_F64_HOLES(bit 12) already makes againstGC_OBJ_TYPED_LAYOUT_INTACT._reservedhad no free bit left.The one thing 4a does not need is a payload: "raw f64" is the whole fact, so its bit is the whole record. A shape id does not fit in a bit, so it lives in an address-keyed thread-local record moved by
transfer_element_shapefrom insidelayout_transfer— the same call site, and the same split, asTYPED_LAYOUTS. The bit stays the authority: a fresh allocation's_reservedis zero, so a stale record left at a recycled address is unreachable, and a bit with no record fails closed (and clears itself).The record is
{ class_id: u32, verified_len: u32, epoch: u64, generation: u64 }— four integers, no heap pointer. It is therefore deliberately not agc_register_mutable_root_scannerentry: there is nothing in it for the collector to mark or rewrite, theclass_idis a registry index, and the key is only ever compared, never dereferenced. Dead keys are dropped byprune_dead_element_shape_ownerson the same collection hook that prunesARRAY_NAMED_PROPS(footprint only).Where it is maintained, and why that one place is enough
The single element-store hook hangs off
gc::layout_note_slot. That is the one funnel both the runtime's element-store helpers (note_array_slot,note_array_slot_layout_only,store_array_slot,note_array_hole_fill_slot) and codegen's inline element stores already pass through —array_store_needs_layout_noteelides the note only when the array is statically proven numeric and pointer-free, which an element-shape array can never be. The call sits ahead oflayout_note_slot'sGC_LAYOUT_UNKNOWNearly return (an all-pointer array is marked unknown on its first generic write) and costs aGC_TYPE_ARRAYcompare plus a bit test on the header word the next line reads anyway. An array without the bit exits on one predictable-not-taken branch.verified_lenis the structural half, and it is what keeps the matrix small enough to enumerate: the record pins thelengthit was verified against, and a query requires the currentlengthto still match. Every operation that changeslengthoutside the store funnels —pop,shift,splice,length = n, sparse extend, codegen's inline append — is therefore invalidated automatically on the next query, with no call site of its own.Invalidation matrix as implemented
layout_note_slothook,index == 0 && length == 0verified_len)JSON.parse,map)ensure_element_shaperescan — self-healing, like 4adelete arr[i]TAG_HOLEstore through the same hookarr.length = n(truncate and extend)TAG_HOLEstores +verified_lenmismatchpop, and any length change behind the runtime's backverified_len != length, fail-closedshift/unshift/splice/fill/copyWithin/reverse/sortrebuild_array_layout— the post-hoc funnel they all already usejs_array_fill_f64_*,js_array_alloc)set_array_numeric_layout; the two invariants are mutually exclusiveC.prototype.m = …,Object.defineProperty(C.prototype, …)invalidate_class_prototype_fast_guards, the existing single latch all three prototype-write entries funnel throughObject.setPrototypeOf(o, …)/__proto__ =object_set_static_prototype_impl, inside theinstance_overridegate (its quiet sibling fires on everynew F())_reserved; record moved bytransfer_element_shapeAn element object gaining an expando is deliberately not an invalidation:
class_idis unchanged, so "every element is an object of classC" still holds. Layout stability within a class is the existingGC_OBJ_TYPED_LAYOUT_INTACTbit's job, and a consumer composes the two.Versioning
Three
AtomicU64counters, all starting at 1 (the crate's convention for invalidation counters —PROP_PLAN_EPOCH,PERRY_IC_EPOCH— not a per-threadCell, since the class registry a generation bump answers to is process-wide):js_array_element_shape_epoch()— bumped on every clear or invalidation, of any array. This is the word a hoisted guard re-reads. One relaxed load, one compare, no side-table probe, no rescan. Deliberately coarse: an unrelated array's clear deopts a running loop, which errs in the safe direction.CLASS_SHAPE_GENERATION— bumped only by prototype surgery. Records carry the generation they were installed under and fail their next query, so one prototype write retires every outstanding record at O(1) without enumerating arrays.ELEMENT_SHAPE_PROOF_SEQ— not an invalidation signal but an identity source. Every established proof takes the next value and carries it unchanged while it is kept or extended, so a consumer that pinned(class_id, epoch)can never be fooled by the same array being re-proven after a break, nor by a different array being established at a recycled address. Identities are never read back out of the table, which is what makes a record that outlives its array harmless rather than a donor (review finding 2).Design note: how #5093's versioned-loop clone consumes this
lower_class_field_versioned_for(crates/perry-codegen/src/stmt/loops.rs) already has exactly the skeleton the element-shape consumer needs, and the extension is mechanical:Match. The array analogue of
match_class_field_versioned_loop:for (let i = 0; i < A.length; i++) { … A[i].f … }over a region-localA.collectors/ptr_shape_elements.rs's E1–E5 already computes this candidate set (empty-literal provenance, push-only mutation, in-bounds induction-variable reads, noIndexSet/splice/lengthwrite), so the matcher reuses those facts rather than re-deriving them.Preheader guard. Where the class-field version emits
emit_class_field_loop_preheader_check(class id +keys_array+ intact bit), the array version emits:<expected_class_id>is the compile-time class the E2 producers push, already available as aclass_keys_global. Theensurecall must be placed before the array head is reloaded into the fast clone's cached register, for the same reason the class-field version keeps "receiver load → check → loop entry" call-free: the pointer the check validated has to be the pointer the clone uses.Clone + entry condition. Reuse
lower_for_after_init_with_i32_boundfor the fast clone andlower_for_after_initfor the slow one, and keep the existingfast_clone_call_freeverification verbatim. That check is what makes an epoch re-read unnecessary in this first consumer: a call-free clone cannot allocate, cannot collect, and — because E3 admits no mutator onA— cannot retire the invariant, so the preheader guard holds for the whole trip count.js_array_element_shape_epoch()is the back-edge re-check for the later, weaker form whose clone is allowed to contain calls: reload it and side-exit toslow.preheaderon change.js_array_element_shape_check(arr, class_id, epoch)is the same test pinned to one array's proof identity, for a consumer that would rather not deopt on an unrelated array's clear.Body. Inside the fast clone,
A[i]yields a value whose class is<expected_class_id>by the guard, so it is rule-1new C(...)-strength provenance forptr_shape.rs—element_read_classalready issues exactly that fact — and everyr.fieldlowers guard-free through 3b's existing proven-local machinery. Nothing new is needed on the read side.Deopt. Guard false →
slow.preheader, the unmodified clone, exactly as today. There is no state to unwind: the fast clone is entered only from the preheader.Group integrity, the
numeric_fieldsstand-down and the rooting contract are all unchanged fromptr_shape_elements.rs— this adds a runtime witness for a fact that pass could previously only assert statically.Verification
29 new tests — 27 in
array/element_shape_tests.rscovering every row of the matrix above plus the two lifecycle hooks (forget_element_shape,prune_dead_element_shape_owners), and 2 ingc/tests/layout_trace/element_shape.rsfor GC survival. The GC pair asserts its subject was live before it asserts a verdict —assert_copied_minor_trace(…)plus "the array must actually have moved" — so a run with zero copying minors cannot pass. One of them additionally pushes after the move and asserts the proof both extends on a match and retires on a mismatch, which is what proves the record is reachable at the new key rather than merely readable once. Both files serialize on a shared poison-tolerant lock taken ahead of any state-restoring guard (review finding 1).cargo test -p perry-runtime element_shape--test-threads=1Full runtime suite, 6 runs at default parallelism: 4 green, 2 red — both reds
promise::keyed_table::tests::settling_many_keys_is_not_quadratic, never an element-shape test. Pre-existing: pristinemain(f05ae3b), same host, same 6-run protocol, is also 4 green / 2 red, once on that same wall-clock scaling assertion and once ontui::tree::tests::register_increments_handle. Same rate, same tests, neither in a module this PR touches.python3 scripts/raw_handle_debt.py— 999 (baseline 999); the new module has zero bare reads and takes no raw pointer across an allocation point.python3 scripts/addr_class_inventory.py— passed; element headers are read throughaddr_class::try_read_gc_header, which also rejects the small-buffer slab addresses that pass a bare plausibility check but carry noGcHeader.bash scripts/check_file_size.sh— OK.cargo fmt --all -- --check— clean.git diff --name-only main...HEADtouchescrates/perry-runtimeonly, soperry-codegen/perry-hir/perry-transform/perryare byte-identical tomainand the emitted IR is unchanged by construction; and a--trace llvmprobe over an array-of-new C(…)workload (build-by-push, indexed field reads, an inline object-literal array) emits 0 occurrences ofelement_shapein its.lland links 0 such symbols into the binary. The same probe emits 6js_gc_note_slot_layoutcalls, which is the funnel this PR's maintenance hook rides — the design claim is exercised, not assumed. Nokeepalive-anchors#[used]statics were added: that feature is on by default, so an anchor would pin five uncalled functions into every shipped binary, the exact dead-strip defeat the hello-size campaign traced its regression to. The consumer PR anchors whichever symbol it emits.Refs #7480.