perf(runtime): #6759 C3c-r — header-stamped stable shape ids, id-keyed FIELD_CACHE, PIC ABA fix#6803
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 (8)
📝 WalkthroughWalkthroughThe runtime now allocates process-global stable shape IDs, stamps plain-object headers, keys field caches by shape identity, invalidates stale stamps after mutations, and restricts PIC priming to shape-shared keys arrays. Tests, changelog text, and shape-tree documentation are updated. ChangesC3c-r runtime shape identity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ObjectLookup
participant ShapeTable
participant FieldCache
participant PICMiss
ObjectLookup->>ShapeTable: Ensure stable shape ID
ShapeTable-->>ObjectLookup: Return shape ID
ObjectLookup->>FieldCache: Lookup by shape key
FieldCache-->>ObjectLookup: Return field index
PICMiss->>PICMiss: Validate shape-shared keys array
PICMiss->>FieldCache: Prime PIC entry
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 review |
✅ Action performedReview finished.
|
3340446 to
0043552
Compare
…d FIELD_CACHE, PIC ABA fix (WIP)
…n-split rationale in object_shape
647018a to
c054111
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/delete_rest.rs (1)
384-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant clear — already handled by
set_object_keys_array, and the comment is inaccurate.
set_object_keys_array(obj, keys_cloned)is called earlier in this same function (Line 320) with a genuinely different pointer (keys_clonedis a fresh allocation, never the same address askeys). Its own pointer-change check inmod.rs((*obj).keys_array != keys_array && shapes::is_shape_id(...)) already clearsparent_class_idat that point, so by the time this block runs,(*obj).parent_class_idis already0andis_shape_id(0)is always false — this is dead code. The comment also states the compaction happens "under the SAME keys address," which contradicts the clone-based approach documented just above (Lines 298-304: "clone the keys_array before mutating it").Either remove this block, or if it's intended as defense-in-depth against a future change to
set_object_keys_array, fix the comment to say so instead of claiming a same-address scenario that doesn't occur here.♻️ Proposed cleanup
- // `#6759` C3c: the compaction changed the layout under the SAME keys - // address, so the stamped shape id no longer describes this - // object. Ids are never reused, so clearing here (plus the - // record drop above) makes every stale id-keyed cache entry a - // permanent miss; the next resolve stamps a fresh id. - if (*obj).class_id == 0 && crate::object::shapes::is_shape_id((*obj).parent_class_id) { - (*obj).parent_class_id = 0; - }🤖 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/delete_rest.rs` around lines 384 - 391, Remove the redundant class_id/parent_class_id clearing block and its inaccurate comment from the function containing the earlier set_object_keys_array(obj, keys_cloned) call; that helper already handles the pointer-change case, so no additional defense-in-depth logic is requested.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/object/delete_rest.rs`:
- Around line 384-391: Remove the redundant class_id/parent_class_id clearing
block and its inaccurate comment from the function containing the earlier
set_object_keys_array(obj, keys_cloned) call; that helper already handles the
pointer-change case, so no additional defense-in-depth logic is requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d9578a2-00e6-44d6-857a-c9a648e61b6d
📒 Files selected for processing (8)
changelog.d/6803-header-shape-id-c3c-r.mdcrates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/field_get_set/ic_miss.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/shapes.rscrates/perry-runtime/src/typed_feedback.rsdocs/shape-tree-plan.md
1 participant
Phases C3c-r (+ the C3b consumer that's sound today) of #6759 / #6798. Stacked on #6802 (C5a). Runtime-only — the codegen PIC remainder is explicitly deferred to its own review, per the C3 gate.
What changed
0x8000_0000..0xC000_0000), superseding C3a's per-thread counter. Two properties are load-bearing: the range is disjoint from every real and builtin class id (a stamp can never be mistaken for inheritance data), and ids are globally unique + never reused — the worker serializer replaysparent_class_idverbatim, so a deep-copied object's stamp arriving on another thread must never alias that thread's ids. Exhaustion (2^30 shape births) parks the allocator and degrades to no-acceleration, never to reuse.class_id == 0objects carry their shape id in the otherwise-deadparent_class_idword — stamped lazily when a read resolves through the slow path, cleared exactly when the invariant would break: a keys-pointer change (set_object_keys_array, pointer-change only — in-place appends keep the stamp because slots are append-only) and in-place delete compaction (next to the existing eagershape_drop).js_object_get_field_ic_missnow primes the per-site cache only forGC_FLAG_SHAPE_SHAREDkeys arrays (literal shapes and class-keys arrays — both shape-cache-resident, process-rooted, address-immortal). An OWNED keys array can die and have its address recycled under a different shape, and the emitted PIC hit path is the one unvalidated compare-and-load in the system — a stale hit reads the wrong slot. Owned/wide receivers are served by the (validated, now id-stable) FIELD_CACHE instead.Explicitly deferred (with rationale in
docs/shape-tree-plan.md)typed_feedback::object_shape()keeps the keys-address token: lazy stamping would split one logical shape into two tokens per site (pre-stamp vs post-stamp observations) and read as spurious polymorphism. Canonicalizing on ids needs eager stamping at allocation — the codegen-assisted remainder.parent_class_idis real inheritance data) — same rung.Validation
Full
cargo test -p perry-runtime --lib -- --test-threads=1green (1450 passed), including three new tests: id range/stability/disjointness, the end-to-end stamp lifecycle on a real object through the real write/read paths, and the PIC shared-only predicate.Summary by CodeRabbit