feat(codegen/runtime): stamp class ShapeIds at birth - #8009
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughCompiled class instances now receive canonical ChangesClass ShapeId stamping
Mergeability Score: ⚪ Minimal · up to The change eagerly assigns class ShapeIds before publication while preserving the existing fallback behavior, with the supplied validation covering codegen, runtime, formatting, and output parity. No actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant ModuleInit
participant Codegen
participant ShapeRegistry
participant Allocator
participant Object
ModuleInit->>Codegen: initialize class keys and ShapeId globals
Codegen->>ShapeRegistry: js_object_shape_id_for_keys(keys_ptr, field_count)
ShapeRegistry-->>Codegen: canonical ShapeId
Codegen->>Allocator: allocate class instance with ShapeId
Allocator->>Object: store ShapeId in object header
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Reviewed against the rung-1 handoff ( What I verified
Gap 1 — no assertion that the fast path still fires18 lines of test, none asserting the emitted hit path still serves a stamped receiver. Rung 1 shipped This rung widens exactly that population — every class instance now carries an id from birth instead of lazily. A regression in which eagerly-stamped instances prime the wrong token would produce correct output, green tests, and a permanently dead fast path. That is the failure mode this ladder has hit twice. Please add the equivalent assertion for an eagerly-stamped instance. That is the discriminating quantity here; Gap 2 — the epoch-guard bypass becomes universal and should be restatedRung 1 flagged this explicitly for the rungs after it:
Rung 1 argued safety because a class instance's keys array is the process-rooted canonical one — immortal — with the only mortal case being the post-delete clone, identical in kind to a plain object's. That argument still holds for this rung, but eager stamping makes it universal rather than incidental, and it is currently inherited silently. Please restate it in the PR body or a code comment, so the next reader knows the load-bearing assumption is "class keys arrays are immortal" and not something weaker. Request — hold the merge briefly
A bisect over Nothing here suggests this PR is the cause — it is not in the regressed range. This is purely about not widening the search while it is in progress. Happy to merge as soon as the bisect names the commit. |
…ate the split population (#8010) * fix(runtime): birth-stamp the class allocators #8009 left lazy, and gate the split #8009 (C3 rung 2) stamps a class instance's ShapeId at birth on the COMPILED path — `js_object_alloc_class_inline_keys_stamped`, called from the inline `new C(…)` lowering. Three other class-instance allocators were left on rung 1's lazy self-heal, which its own doc comment states: js_object_alloc_class_with_keys js_object_alloc_class_dynamic_parent js_object_alloc_class_inline_keys (the compatibility entry point) For any class that lands on one of those, the shape's population is still SPLIT — and a split population is not a slow start, it is a permanent 0% PIC hit rate. The emitted read PIC derives its entire cache token from the header shape word: is_stamp = (parent_class_id - 0x8000_0000) u< 0x4000_0000 token = is_stamp ? (parent_class_id | 1<<62) : keys_array so instance #1 misses, is stamped, primes the id token; instance #2 is newborn, computes the keys pointer, misses; the handler re-primes the same id; instance #3 misses. Forever. That is exactly #7983's defect, which this bisected: on instructions retired, isolated against its own parent, `cycles` +54.3%, `deeplist` +45.2%, `interp` +28.3%, `pipeline` +23.9%, `iso_miss` +22.9%, while the object-literal benchmarks (`churn` +1.2%, `retain` +0.2%) and `fib40` (+0.04%) did not move — literals have been birth-stamped since #6804. All three now stamp at birth. The two shape-cached allocators read the id out of the `ShapeCacheEntry::runtime_shape_id` their existing probe already returns (`shape_cache_get` -> `shape_cache_get_with_id`): one extra u32 from a loaded cache line, a compare and a store. The compatibility entry point mints from its canonical keys array — one shape-table probe, and it is not the compiled hot path. THE GATE. `a_fresh_class_instance_computes_the_token_the_miss_handler_primed` asserts the token the miss handler PRIMES equals the token a freshly-allocated sibling COMPUTES, using `emitted_pic_token` — the emitted IR's formula transcribed into the test. It FAILS on `main` as of #8009 and passes here, which is the whole point: #8009's own test asserts that a newborn CARRIES a stamp, and that is a presence check. Both-stamped and both-unstamped each satisfy it; only the MIXTURE is the bug, and only a test that compares the two sides can see it. This one also passes in either uniform state, so it does not need rewriting if the policy ever flips back. Two premise assertions in `delete_rest::shape_transition_tests_6759` said a fresh instance is unstamped. They are updated, not deleted, and the parent-chain test is strictly stronger for it: the word is now clobbered from birth, so there is no window in which it was ever valid inheritance data. * docs(changelog): fragment for #8010 (class-shape uniformity gate) * docs(gc-handoff): bisect notes for the #7983 shape-population regression --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Summary
ShapeIdonce at module initialization, beside its rooted keys globalThis is #6759 C3 rung 2, the next prerequisite for #7916's remaining header shrink. It closes the lazy-stamp window so the guard-migration rung can rely on a uniform shape word. It does not close #7916; the guard migration and header-layout change remain follow-up work.
Measurements
retainremains 168 MB written for 48 MB of numeric payload (3.5x amplification)No version files are changed and there is no version bump.
Validation
cargo fmt --all -- --checkcargo test -p perry-codegen --lib— 935 passedcargo test -p perry-runtime --lib— 2,278 passed, 4 ignoredmaintest-files/test_edge_objects_records.tswith the built Perry binary and verified byte-for-byte stdout parity with Nodescripts/check_file_size.shgit diff --check origin/main...HEADRefs #6759
Refs #7916
Summary by CodeRabbit
Improvements
Bug Fixes