fix(thread): take the serialized class-parent edge from the registry, not the header word - #7981
Merged
Merged
Conversation
… not the header word `ObjectHeader.parent_class_id` carries two different things. For a class instance it is the parent class id; for a plain object (`class_id == 0`) the same word is the #6759 C3c runtime ShapeId stamp, written lazily by every by-name resolve path. `serialize_object` copied the raw word, and the worker-side `deserialize` hands it to `js_object_alloc_with_parent`, which does `if parent_class_id != 0 { register_class(class_id, parent_class_id) }`. So any object literal that had been READ once and then crossed a `spawn` / `parallelMap` boundary registered `class 0 -> <a shape id>` in the process-global class-parent registry (`PARENT_DENSE[0] = shape_id + 1`, `CLASS_REGISTRY[0] = shape_id`) and bumped the store-plan epoch, once per deserialized object. Every consumer I checked guards `class_id == 0` off, so there is no known live victim — but it is registry pollution reachable from ordinary user code. The authoritative parent edge never lived in the header: every parent-chain walk in the runtime reads `get_parent_class_id(class_id)`, and each edge is registered from a compile-time constant — by `js_register_class_parent` in the module-init prelude for codegen's inline `new C()` path (which writes the header word and deliberately skips the per-alloc `register_class`), and by `register_class` inside every runtime allocator that takes a `parent_class_id` argument. Read it from there. This also removes the LAST consumer of the header word as inheritance data, which is the blocking dependency for #6759 C3's unification of class layouts and plain-object shapes into one shape-id space. Tests, all three written to fail for a stated reason rather than to pass: * the shape stamp does not reach the wire, with a vacuity assertion that the stamp is genuinely present in the fixture; * a class instance's parent still round-trips *from the registry* with the header word overwritten by a stamp — so the fix is not "always send 0"; * `object/delete_rest.rs::shape_transition_tests_6759` pins both halves of the #6759 C3 entry gate: a plain object's `delete` mints a genuinely DIFFERENT ShapeId, while a class instance's `delete` compacts its slots leaving `class_id` and `parent_class_id` untouched — so the `keys_array` pointer is the only compaction evidence `class_field_inline_guard` has, and the #7916 header shrink cannot delete it yet. Refs #6759, #7916.
📝 WalkthroughWalkthroughChangesThe runtime now derives serialized class parent IDs from the class registry. Plain objects serialize parent ID Thread serialization and shape transitions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant parallelMap
participant serialize_object
participant ClassMetadataRegistry
participant WorkerRuntime
parallelMap->>serialize_object: Serialize object
serialize_object->>ClassMetadataRegistry: Read registered parent class ID
ClassMetadataRegistry-->>serialize_object: Return parent class ID
serialize_object->>WorkerRuntime: Transfer serialized object
WorkerRuntime->>parallelMap: Reconstruct object and execute callback
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 |
added 2 commits
August 12, 2026 18:58
… split perry-only (`perry/thread` has no Node equivalent). A plain object literal is READ on the main thread first — which is what stamps a runtime ShapeId into `parent_class_id` — and crosses a `parallelMap` boundary before a 3-level `instanceof`/virtual-dispatch chain is exercised on a worker, so a serializer that replays the stamp as a class-parent edge poisons the registry BEFORE the chain that reads it. The discriminating assertion (that `get_parent_class_id(0)` stays empty) lives in the perry-runtime unit test — the class-parent registry is not observable from JS — so this file is the behavioural half only.
proggeramlug
marked this pull request as ready for review
August 12, 2026 17:09
Contributor
Author
ValidationSabotage-verify (fix committed first). Reverted The failure carries a real observed id, so the defect is measured rather than argued. The two Clean tree. End to end. Two gates that would be VACUOUS here, and are deliberately not claimed
The discriminating gate is the unit test, because the class-parent registry is not observable from JS: nothing in userland can read |
Contributor
Author
Post-fix run (closes the one item outstanding when this was undrafted)The final
The byte-identity across the two |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
changelog.d/7981-thread-parent-class-id-from-registry.md (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce this to one user-facing release-note entry.
This fragment includes internal data structures, code paths, and future header-shrink work. Replace it with a concise description of the shipped behavior, such as preventing plain-object ShapeId metadata from being treated as class inheritance during
perry/threadtransfers.Based on learnings: “For PerryTS/perry changelog fragments in changelog.d/, describe the final shipped behavior as one coherent release-note entry.”
🤖 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 `@changelog.d/7981-thread-parent-class-id-from-registry.md` around lines 1 - 45, Rewrite the changelog entry as one concise, user-facing release note describing the shipped behavior: perry/thread transfers must not treat plain-object ShapeId metadata as class inheritance. Remove internal implementation details, test rationale, registry mechanics, future work references, and issue links.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/thread_parent_class_id_tests.rs`:
- Around line 37-43: Root each raw object pointer before any key(...) allocation
or other GC-triggering operation, then reload the pointer from the root after
each allocation before using it. Apply this to the test object in
crates/perry-runtime/src/thread_parent_class_id_tests.rs:37-43, the plain-object
pointer in crates/perry-runtime/src/object/delete_rest.rs:638-658, and the
class-instance pointer in
crates/perry-runtime/src/object/delete_rest.rs:685-703; preserve the existing
field access and deletion behavior.
In `@test-files/test_issue_7981_thread_shape_stamp_parent.ts`:
- Around line 90-94: Update the instance-transfer test around Mid and spawn so
the existing inst object crosses the worker boundary instead of creating a
separate Mid instance inside chain(7). Capture inst in the spawned closure or
pass it through parallelMap, then assert the received object preserves b, m, and
kind() behavior.
- Around line 60-97: Add explicit assertions in the test flow covering the
mapped plain-object values, every worker inheritance result, the spawned chain
comparison, and the final main-thread correctness check. Keep the existing
console.log calls, and make each assertion fail the test when its corresponding
transfer or result is incorrect.
---
Nitpick comments:
In `@changelog.d/7981-thread-parent-class-id-from-registry.md`:
- Around line 1-45: Rewrite the changelog entry as one concise, user-facing
release note describing the shipped behavior: perry/thread transfers must not
treat plain-object ShapeId metadata as class inheritance. Remove internal
implementation details, test rationale, registry mechanics, future work
references, and issue links.
🪄 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: 9e5a4b9a-0dbb-4347-a262-18423e68aad9
📒 Files selected for processing (5)
changelog.d/7981-thread-parent-class-id-from-registry.mdcrates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/thread.rscrates/perry-runtime/src/thread_parent_class_id_tests.rstest-files/test_issue_7981_thread_shape_stamp_parent.ts
Comment on lines
+37
to
+43
| let obj = crate::object::js_object_alloc(0, 8); | ||
| for name in ["thr6759_a", "thr6759_b", "thr6759_c"] { | ||
| crate::object::js_object_set_field_by_name(obj, key(name), 1.0); | ||
| } | ||
| let _ = crate::object::js_object_get_field_by_name(obj, key("thr6759_b")); | ||
|
|
||
| let stamp = (*obj).parent_class_id; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Root each test object across key(...) allocations.
key(...) allocates a StringHeader while each site retains obj only as a raw Rust pointer. A collection can relocate the object before the subsequent unsafe use.
crates/perry-runtime/src/thread_parent_class_id_tests.rs#L37-L43: rootobjbefore creating keys and reload it after each key allocation.crates/perry-runtime/src/object/delete_rest.rs#L638-L658: root and reload the plain-object pointer around each key allocation.crates/perry-runtime/src/object/delete_rest.rs#L685-L703: root and reload the class-instance pointer around the deletion key allocation.
Based on learnings: “raw Rust pointer locals are neither GC roots nor reliable pins” across operations that can allocate or invoke GC.
📍 Affects 2 files
crates/perry-runtime/src/thread_parent_class_id_tests.rs#L37-L43(this comment)crates/perry-runtime/src/object/delete_rest.rs#L638-L658crates/perry-runtime/src/object/delete_rest.rs#L685-L703
🤖 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/thread_parent_class_id_tests.rs` around lines 37 -
43, Root each raw object pointer before any key(...) allocation or other
GC-triggering operation, then reload the pointer from the root after each
allocation before using it. Apply this to the test object in
crates/perry-runtime/src/thread_parent_class_id_tests.rs:37-43, the plain-object
pointer in crates/perry-runtime/src/object/delete_rest.rs:638-658, and the
class-instance pointer in
crates/perry-runtime/src/object/delete_rest.rs:685-703; preserve the existing
field access and deletion behavior.
Source: Learnings
Comment on lines
+60
to
+97
| const mapped = parallelMap([lit, lit, lit, lit], (o: Record<string, number>) => | ||
| summarize(o), | ||
| ); | ||
| console.log("plain mapped:", mapped.join(" | ")); | ||
|
|
||
| // 2. Inheritance must still resolve on a worker AFTER that deserialize — the | ||
| // edges the serializer now reads come from the same registry the pollution | ||
| // landed in. | ||
| function chain(n: number): string { | ||
| const leaf = new Leaf(n, n * 10); | ||
| const parts: string[] = [ | ||
| leaf.kind(), | ||
| String(leaf.b), | ||
| String(leaf.m), | ||
| String(leaf instanceof Mid), | ||
| String(leaf instanceof Base), | ||
| String(new Mid(n, n) instanceof Base), | ||
| ]; | ||
| return parts.join(","); | ||
| } | ||
| const expected = chain(4); | ||
| console.log("main chain:", expected); | ||
|
|
||
| const chained = parallelMap([4, 4, 4, 4], (n: number): string => chain(n)); | ||
| let allMatch = true; | ||
| for (let i = 0; i < chained.length; i++) { | ||
| if (chained[i] !== expected) allMatch = false; | ||
| } | ||
| console.log("worker chain count:", chained.length, "allMatch:", allMatch); | ||
|
|
||
| // 3. A class INSTANCE crossing the boundary keeps its fields. | ||
| const inst = new Mid(7, 70); | ||
| const back = await spawn((): string => chain(7)); | ||
| console.log("spawn chain:", back, "match:", back === chain(7)); | ||
| console.log("instance fields:", inst.b, inst.m, inst.kind()); | ||
|
|
||
| // 4. The main thread is still correct after all of it. | ||
| console.log("main again:", chain(4) === expected, summarize(lit)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make each transfer assertion fail the test.
Lines 60-97 only log mapped, allMatch, back === chain(7), and the final main-thread result. This perry-only test can exit successfully after a regression.
Add direct checks for the mapped plain-object results, worker inheritance results, spawned result, and final main-thread result. Retain the logs if the harness consumes them.
Based on learnings: “console.log output may be intentional test validation” only when it is the harness oracle; add explicit local assertions when appropriate.
🤖 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 `@test-files/test_issue_7981_thread_shape_stamp_parent.ts` around lines 60 -
97, Add explicit assertions in the test flow covering the mapped plain-object
values, every worker inheritance result, the spawned chain comparison, and the
final main-thread correctness check. Keep the existing console.log calls, and
make each assertion fail the test when its corresponding transfer or result is
incorrect.
Source: Learnings
Comment on lines
+90
to
+94
| // 3. A class INSTANCE crossing the boundary keeps its fields. | ||
| const inst = new Mid(7, 70); | ||
| const back = await spawn((): string => chain(7)); | ||
| console.log("spawn chain:", back, "match:", back === chain(7)); | ||
| console.log("instance fields:", inst.b, inst.m, inst.kind()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Transfer inst across the worker boundary.
inst does not flow into spawn. The worker creates a separate Mid instance inside chain(7). A regression in class-instance serialization, parent restoration, or field transfer will pass this section.
Capture inst in the spawned closure, or pass it through parallelMap, and validate its fields and inherited behavior on the receiving thread.
🤖 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 `@test-files/test_issue_7981_thread_shape_stamp_parent.ts` around lines 90 -
94, Update the instance-transfer test around Mid and spawn so the existing inst
object crosses the worker boundary instead of creating a separate Mid instance
inside chain(7). Capture inst in the spawned closure or pass it through
parallelMap, then assert the received object preserves b, m, and kind()
behavior.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 12, 2026
`ObjectHeader.parent_class_id` IS the shape word, but the stamp was gated on `class_id == 0`, so a CLASS INSTANCE had no shape word at all: its only header evidence of a key-set change was the `keys_array` POINTER. That asymmetry is what blocks `class_field_inline_guard` from comparing a ShapeId, which in turn blocks the #7916 header shrink. Rung 0 (#7981) removed the last reader of the header word as inheritance data, so the word is now free for every receiver kind. The rule becomes, uniformly: the word is a ShapeId <=> is_shape_id(word) with no `class_id` discriminant — which is already what all three emitted PICs test. Relaxing the runtime gates makes the runtime agree with the IR rather than introducing a new mode. Concretely: `shapes.rs` grows `object_shape_stamp` / `stamp_object_shape` / `clear_object_shape_stamp`, and the two clear sites (`set_object_keys_array`, `delete_rest`), the two mint sites (`ic_miss`, `get_field_by_name_tail`), the first-key birth stamp (`field_set_by_name/tail`) and the FIELD_CACHE key route through them. A class instance is stamped LAZILY at its first by-name resolve (eager birth stamping is rung 2), and a `delete` on it now clears + re-mints a distinct id exactly as a plain object's does. `typed_feedback::object_shape()` deliberately KEEPS its `class_id == 0` gate. Its token is not a PIC token: the guard family compares it against a codegen-supplied keys pointer (`method_direct_call_contract` requires `shape_addr == expected_keys as usize`), so returning an id there fails every class-field and direct-method-call guard closed. Migrating those nine consumers is rung 3.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 12, 2026
`ObjectHeader.parent_class_id` IS the shape word, but the stamp was gated on `class_id == 0`, so a CLASS INSTANCE had no shape word at all: its only header evidence of a key-set change was the `keys_array` POINTER. That asymmetry is what blocks `class_field_inline_guard` from comparing a ShapeId, which in turn blocks the #7916 header shrink. Rung 0 (#7981) removed the last reader of the header word as inheritance data, so the word is now free for every receiver kind. The rule becomes, uniformly: the word is a ShapeId <=> is_shape_id(word) with no `class_id` discriminant — which is already what all three emitted PICs test. Relaxing the runtime gates makes the runtime agree with the IR rather than introducing a new mode. Concretely: `shapes.rs` grows `object_shape_stamp` / `stamp_object_shape` / `clear_object_shape_stamp`, and the two clear sites (`set_object_keys_array`, `delete_rest`), the two mint sites (`ic_miss`, `get_field_by_name_tail`), the first-key birth stamp (`field_set_by_name/tail`) and the FIELD_CACHE key route through them. A class instance is stamped LAZILY at its first by-name resolve (eager birth stamping is rung 2), and a `delete` on it now clears + re-mints a distinct id exactly as a plain object's does. `typed_feedback::object_shape()` deliberately KEEPS its `class_id == 0` gate. Its token is not a PIC token: the guard family compares it against a codegen-supplied keys pointer (`method_direct_call_contract` requires `shape_addr == expected_keys as usize`), so returning an id there fails every class-field and direct-method-call guard closed. Migrating those nine consumers is rung 3.
proggeramlug
added a commit
that referenced
this pull request
Aug 12, 2026
* feat(runtime): #6759 C3 rung 1 — make the shape word uniform `ObjectHeader.parent_class_id` IS the shape word, but the stamp was gated on `class_id == 0`, so a CLASS INSTANCE had no shape word at all: its only header evidence of a key-set change was the `keys_array` POINTER. That asymmetry is what blocks `class_field_inline_guard` from comparing a ShapeId, which in turn blocks the #7916 header shrink. Rung 0 (#7981) removed the last reader of the header word as inheritance data, so the word is now free for every receiver kind. The rule becomes, uniformly: the word is a ShapeId <=> is_shape_id(word) with no `class_id` discriminant — which is already what all three emitted PICs test. Relaxing the runtime gates makes the runtime agree with the IR rather than introducing a new mode. Concretely: `shapes.rs` grows `object_shape_stamp` / `stamp_object_shape` / `clear_object_shape_stamp`, and the two clear sites (`set_object_keys_array`, `delete_rest`), the two mint sites (`ic_miss`, `get_field_by_name_tail`), the first-key birth stamp (`field_set_by_name/tail`) and the FIELD_CACHE key route through them. A class instance is stamped LAZILY at its first by-name resolve (eager birth stamping is rung 2), and a `delete` on it now clears + re-mints a distinct id exactly as a plain object's does. `typed_feedback::object_shape()` deliberately KEEPS its `class_id == 0` gate. Its token is not a PIC token: the guard family compares it against a codegen-supplied keys pointer (`method_direct_call_contract` requires `shape_addr == expected_keys as usize`), so returning an id there fails every class-field and direct-method-call guard closed. Migrating those nine consumers is rung 3. * docs: changelog fragment + rung-1 handoff notes for #7983 * test(runtime): pin the new PIC surface rung 1 opens — a compacted class instance is cacheable for the first time * docs: record the A/B clear redundancy the per-site sabotage found, and the rung-1 validation matrix * docs: record the sabotage/rebuild near-miss in RUNG1-NOTES * docs: rung-1 handoff section for rungs 2 and 3 * docs: correct cited line numbers and final cost figures in RUNG1-NOTES * docs: gap-suite triage — six crashes signature-matched to #7932, two cleared by the control-arm A/B * docs: record the control-arm A/B verdict — no gap regression is attributable to rung 1 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 12, 2026
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.
What
ObjectHeader.parent_class_idcarries two different things. For a class instance it is the parent class id. For a plain object (class_id == 0) the same word is the #6759 C3c runtime ShapeId stamp, written lazily by every by-name resolve path (typed_feedback.rs:774,get_field_by_name_tail.rs:1516,ic_miss.rs:713/756).thread.rs::serialize_objectcopied the raw word, and the worker-sidedeserializehands it tojs_object_alloc_with_parent, which does:So any object literal that has been read once and then crosses a
spawn/parallelMapboundary registersclass 0 → <a shape id>in the process-global class-parent registry (PARENT_DENSE[0] = shape_id + 1,CLASS_REGISTRY[0] = shape_id) and bumps the store-plan epoch, once per deserialized object. Every consumer I checked guardsclass_id == 0off (instanceof.rs:909,descriptor_helpers.rs:231), so there is no known live victim — but it is registry pollution reachable from ordinary user code, and it is the reason the header word could not be re-purposed.The authoritative parent edge never lived in the header. Every parent-chain walk in the runtime reads
get_parent_class_id(class_id)(object/class_meta_registry.rs:82), and each edge is registered from a compile-time constant:js_register_class_parent, emitted once per inheriting class in the module-init prelude (codegen/string_pool.rs:478-505) — precisely because codegen's inlinenew C()bump path writes the header word and skips the per-allocregister_class(class_registry/parent_static.rs:22-41);register_classinside every runtime allocator that takes aparent_class_idargument (object/alloc.rs:125, 203, 250, 398).So the serializer now reads it from there.
Why it matters beyond the fix
This removes the last consumer of
ObjectHeader.parent_class_idas inheritance data. It was the blocking dependency for #6759 Phase C3's unification of class layouts and plain-object shapes into one shape-id space — which is in turn the prerequisite for #7916's 16-byte header shrink, becauseclass_field_inline_guardcan only replace itskeys_array-identity compare with a one-word ShapeId compare once a class instance has a shape word.Tests
Three, each written to fail for a stated reason rather than to pass:
thread_parent_class_id_tests::a_plain_objects_shape_stamp_is_not_serialized_as_a_parent_class_id— asserts the stamp is genuinely present in the fixture first (so the test is not vacuous), then that it does not reach the wire.…::a_class_instances_parent_comes_from_the_registry_not_the_header— the parent still round-trips with the header word deliberately overwritten by a stamp, so the fix is not "always send 0".object/delete_rest.rs::shape_transition_tests_6759— pins both halves of the Architecture: adopt V8's object-model construction — explicit runtime state, self-describing headers, shape tree (phases A–C) #6759 C3 entry gate: a plain object'sdeletemints a genuinely different ShapeId (with a vacuity assertion), while a class instance'sdeletecompacts its slots leavingclass_idandparent_class_iduntouched. The second is written to go red when a future rung gives class instances a stamp — that is the intended signal to switch the guard.Scope note
Deliberately does not attempt the C3 unification itself. The investigation behind this (enumeration of every in-place key-set mutation, the sabotage A/B that shows which fixture shapes are vacuous as a guard gate, and a four-rung costed plan) is written up in
gc-handoff/SHAPE-NOTES.md.Refs #6759, #7916.
Summary by CodeRabbit
Bug Fixes
instanceofbehavior across worker boundaries.Tests