perf(runtime): #6759 Phase C3a — stable shape identity: owned-grow migration + GC rekey#6801
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughShape records now receive monotonic stable IDs and migrate with their keys arrays during owned growth and GC evacuation. Shape-table scanning is registered with GC, and tests cover both relocation paths. ChangesStable shape identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ObjectSetter
participant ShapeTable
participant GarbageCollector
ObjectSetter->>ShapeTable: migrate record after owned keys growth
ShapeTable-->>ObjectSetter: retain shape_id
GarbageCollector->>ShapeTable: rekey metadata after keys evacuation
ShapeTable-->>GarbageCollector: retain shape_id at new address
Possibly related issues
🚥 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 |
|
Gap suite vs |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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/object/shapes.rs`:
- Around line 58-61: Update ShapeRegistry::alloc_shape_id so the shape counter
cannot wrap and reuse an existing ID after u32::MAX. Use a wider non-wrapping
counter, preferably u64, and update the associated shape ID storage/types as
needed; otherwise fail explicitly before allocation would reuse an ID. Preserve
monotonic, unique IDs for all successful allocations.
🪄 Autofix (Beta)
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: 11e820aa-7185-43fe-8a05-6866d2e5667f
📒 Files selected for processing (5)
changelog.d/6801-stable-shape-identity-phase-c3a.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/dead_owner_side_tables.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/shapes.rs
| fn alloc_shape_id(&self) -> u32 { | ||
| let id = self.next_id.get(); | ||
| self.next_id.set(id.wrapping_add(1).max(1)); | ||
| id |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent shape_id reuse on counter wrap.
After u32::MAX, this wraps and allocates 1 again. That breaks the promised monotonic, unique shape identity and would alias future identity-keyed consumers. Use a non-wrapping wider ID (preferably u64) or fail explicitly before reuse.
Proposed fix
- shape_id: u32,
+ shape_id: u64,
...
- next_id: std::cell::Cell<u32>,
+ next_id: std::cell::Cell<u64>,
...
- fn alloc_shape_id(&self) -> u32 {
+ fn alloc_shape_id(&self) -> u64 {
let id = self.next_id.get();
- self.next_id.set(id.wrapping_add(1).max(1));
+ self.next_id
+ .set(id.checked_add(1).expect("shape id space exhausted"));
id
}📝 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.
| fn alloc_shape_id(&self) -> u32 { | |
| let id = self.next_id.get(); | |
| self.next_id.set(id.wrapping_add(1).max(1)); | |
| id | |
| fn alloc_shape_id(&self) -> u64 { | |
| let id = self.next_id.get(); | |
| self.next_id | |
| .set(id.checked_add(1).expect("shape id space exhausted")); | |
| id | |
| } |
🤖 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/object/shapes.rs` around lines 58 - 61, Update
ShapeRegistry::alloc_shape_id so the shape counter cannot wrap and reuse an
existing ID after u32::MAX. Use a wider non-wrapping counter, preferably u64,
and update the associated shape ID storage/types as needed; otherwise fail
explicitly before allocation would reuse an ID. Preserve monotonic, unique IDs
for all successful allocations.
…gration + GC rekey (WIP)
… in scoped-registry tests
f2c5148 to
182695f
Compare
|
Legitimate against this PR's snapshot — and already resolved on |
1 participant
Phase C3a of #6759 / #6798 (design: #6798 comment): stable shape identity, runtime-only. Stacked on #6800 (Phase C2).
Problem
ArrayHeaderstores elements inline, so every append growth reallocates an owned keys array — and every consumer keyed on the keys address treats that as a brand-new shape. For the C1 shape records specifically, a wide object's slot map was orphaned at the old address on each capacity doubling and rebuilt O(key_count) at the new one. (Small shapes ≤64 keys ride the shared-transition system with stable per-shape addresses; it's exactly the wide regime — the one C1's records exist for — that churned.)What changed
Shaperecords gain a stableshape_id: u32(monotonic, 1-based, allocated at record birth). Not yet consumed — it's the identity C3b re-keys FIELD_CACHE/typed_feedback exactness on, and C3c makes header-resident (parent_class_idslot forclass_id == 0objects, per the design).js_object_set_field_by_name's slow paths callshape_keys_grown(prev, new)whenjs_array_pushreallocated a NON-GC_FLAG_SHAPE_SHAREDkeys array — the record (slot map,indexed_len,shape_id) moves to the new address instead of being dropped. A shared array's fork deliberately does NOT migrate: the old address still describes the siblings' live shape; the clone is a genuine transition edge.scan_shape_table_rekey_mut): when evacuation moves a live keys array, the shape record follows it (metadata-rewrite phase only; the records hold no heap references and mark nothing — same pattern as the descriptor-table owner rekey).Validation
cargo test -p perry-runtime --lib -- --test-threads=1green (1445 passed), including two new tests: owned-grow migration preserves the record +shape_id; copied-minor move rekeys the record to the to-space address.node --experimental-strip-typeson this tip (C2+C3a stacked) — results in PR comment.Runtime-only; no codegen, header, or semantic change.
Summary by CodeRabbit
Performance
Reliability
Tests