perf(gc): make a repeat typed-shape construction a header bit-set (#7510) - #7535
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 (9)
📝 WalkthroughWalkthroughThis change adds a thread-local memo for validated typed-shape installations. It uses raw mask words, invalidates entries when layouts become ambiguous, integrates with hot TLS storage, and adds tests for validation, copying GC, and pointer tracing. ChangesTyped shape installation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Constructor
participant LayoutInstaller
participant ShapeInstallMemo
participant SHAPE_LAYOUTS
participant CopyingGC
Constructor->>LayoutInstaller: validate object fields and mask words
LayoutInstaller->>ShapeInstallMemo: lookup shape-install key
alt memo hit
ShapeInstallMemo-->>LayoutInstaller: cached layout state
LayoutInstaller-->>Constructor: write header state
else memo miss
LayoutInstaller->>SHAPE_LAYOUTS: install shared or per-object layout
SHAPE_LAYOUTS-->>LayoutInstaller: layout descriptor
LayoutInstaller->>ShapeInstallMemo: record confirmed shared installation
LayoutInstaller-->>Constructor: write layout state
end
Constructor->>CopyingGC: expose installed object
CopyingGC-->>Constructor: relocate object and pointer children
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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.
|
![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 `@changelog.d/7535-construction-shape-install.md`:
- Around line 20-21: Update the stale entry count in the description of the
gc/shape_install memoisation table from 8 to 32, keeping the references to the
thread-local direct-mapped table and its existing behavior unchanged.
🪄 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: bc3f52f3-c9cf-4b38-a6f8-bf6fc1261d9c
📒 Files selected for processing (7)
changelog.d/7535-construction-shape-install.mdcrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/shape_install.rscrates/perry-runtime/src/gc/tests/layout_trace.rscrates/perry-runtime/src/gc/tests/layout_trace/shape_install_memo.rscrates/perry-runtime/src/tls_hot.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
changelog.d/7535-construction-shape-install.md (1)
83-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the issue reference in the residual note.
Line [83] identifies the unresolved constructor-ordering issue as
#7512, but line [93] says it remains on#7510. The PR objective also identifies this issue as#7512. Update line [93] to reference the correct tracking issue.🤖 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/7535-construction-shape-install.md` around lines 83 - 93, Update the residual note so its closing tracking reference uses `#7512` instead of `#7510`, keeping the surrounding explanation and issue references unchanged.
🤖 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.
Outside diff comments:
In `@changelog.d/7535-construction-shape-install.md`:
- Around line 83-93: Update the residual note so its closing tracking reference
uses `#7512` instead of `#7510`, keeping the surrounding explanation and issue
references unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4c183fe-a45a-49ce-831b-db54773a1584
📒 Files selected for processing (1)
changelog.d/7535-construction-shape-install.md
Not taken as written — #7510's own comment folds the residual in explicitly: "Folding in the residual half of #7512, per the diagnosis there (#7515 merged the other half) … it is why #7512's remaining cost belongs here rather than in a repsel ticket." #7512 is still open, but for the broader |
) `js_gc_init_typed_shape_layout` runs on every construction of a typed object literal and every `new` of a class with a typed field layout. Since #6893 the descriptor it installs is per-SHAPE, shared through `SHAPE_LAYOUTS` and keyed by the canonical `keys_array`; the only per-object work left is two header bits. The call did not know that. For the 20-millionth `{v, w}` literal it still built a `TypedLayoutDescriptor` (two 32-byte `Vec`-carrying enums, cloned once and dropped), took a `RefCell` borrow, hashed `keys`, and compared the fresh descriptor field-by-field against the one already stored — to reach the conclusion the first construction had already reached. `gc::shape_install` memoises exactly that map answer, and nothing else: "`SHAPE_LAYOUTS[keys]` holds `Some(D)`, where `D` is what these mask globals describe". Everything the header declaration rests on — `field_count == slot_count`, mask disjointness, and the per-slot validation that each raw-f64 slot holds a plain double and no pointer-bearing slot sits outside the pointer mask — is still re-derived per object, and `POINTER_FREE` vs `SIDE_MASK` is recomputed from the pointer mask rather than read back from the memo. A stale entry can therefore only cost work, never correctness. The two soundness checks now read the caller's mask words directly instead of building a `LayoutSlotMask` per construction, which also takes the drop glue off six early-return paths and the `Vec` allocation off every construction of a shape wider than 64 slots. `LayoutSlotMask::intersects` survives as the test-only reference the word helpers are pinned against. Self-healing: an entry is falsified by one transition only — `SHAPE_LAYOUTS` poisoning a shape to ambiguous — and that branch drops the table. A relocated or recycled `keys_array` degrades to a miss, and the table is not a GC root: `keys` is compared as an integer and never dereferenced.
… shapes (#7510) Two object-literal sites with the same key names share one `keys_array` but get separate mask globals unless LLVM's constant merger folds them, so an index keyed on the shape alone would put both in one entry and let them evict each other every iteration of a loop that builds both. Any index is correct — the full tuple is still compared — so this is purely about not colliding.
Measured, not guessed. A direct-mapped table cycled round-robin by more shapes than it has slots hits ZERO percent of the time — each entry is evicted by its partner before it is read again — so it pays the probe and gets nothing back. A 16-shape churn loop against 8 slots was a reproducible 0.993x on the pinned host. At 32 slots the same loop fits. The table is 1 KiB of const-initialised thread-local, so a program with one shape pays a page it never touches.
The memo keys on the ADDRESS of the mask words. A `const` is inlined at each use site rather than having one address, so the test fixture only worked because rustc happened to promote it to a single static. Codegen emits `private unnamed_addr constant` globals, which genuinely have one address — a `static` is the faithful model as well as the stable one.
CodeRabbit caught that the fragment still said 8 entries after the sizing commit raised it to 32. Also records the headline numbers and the sizing rationale, which the fragment predated.
#7512 CodeRabbit read the paragraph as contradicting itself: it opens on #7512 and closes on #7510. The reference is right — #7510's own comment folded #7512's remainder in as its construction-path item — but the fragment never said so. It does now, and it notes that #7512 stays open for the broader class-vs-object-literal gap.
a158225 to
ab671fd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…oops The outlined per-`new`-site allocator has been the default since [#bloat]: it collapses ~145 lines of per-class-constant IR per site into one js_object_alloc_class_inline_keys call. The size half of that decision still holds — measured ~268 bytes of machine code per site, +214,656 over an 800-site program. The SPEED half has inverted. The comment reads '~17% faster on an 8M-allocation loop'; today the outlined form is 1.81x SLOWER on churn_alloc and 1.78x on push_cls. Nothing about the inline bump changed — everything around the allocation got cheaper (#7474 #7486 #7487 #7501 #7525 #7532 #7535 #7536 #7552), so the surviving FFI call and the thread-local resolutions it performs now dominate what its code bloat costs. Those resolutions cannot be made cheaper on Darwin: Mach-O has no local-exec TLS model, and building the runtime with -Ztls-model=local-exec leaves the blr through the TLV descriptor byte-identical (measured 1.02x). Only their count can be reduced. So the choice becomes per site rather than global. A `new` inside a loop takes the inline bump; everything else keeps the outlined call and adds nothing to binary size. Loop membership reuses the existing loop_targets stack — switch frames push an empty continue label, every loop pushes a real one, the same discriminator Stmt::Continue already relies on. Measured: churn_alloc 1.81x, push_cls 1.81x, churn 1.56x — the full unconditional-inline ceiling. Size +0 bytes for 800 sites none of which are in loops; equal to all-inline when every site is. tree is -1.4%.
1 participant
Item 1 of #7510 — "construction should become a header bit-set, not a side-table insert" — plus a finding on the #7512 residual that was folded into it. #7525 landed the prerequisite (the per-object side tables are now provably empty on a monomorphic workload) and named this as the next lever.
Result
mainchurn_alloc—{v, w}object literalchurn_alloc— independent repeatpush_cls—new Node(v, w)poly_shapes— 16 shapes cycled, the memo's worst caseInterleaved best-of-9 user CPU on the pinned quiet host (
perry-macos.local, M1/8 cores, load 1.6–2.4 across the runs). Arms alternate every round, so drift in machine state hits both equally.Hit rate, counted rather than reasoned about. An lldb breakpoint with an auto-continue command on
shape_install_shared, over the identical program:shape_install_sharedentriesmain#7525's first commit shipped a fast path that fired once in 40 million calls and was believed anyway. This is the same instrument pointed the other way.
The profile agrees: on the same symbolicated 2 s sample,
shape_install_shareddrops from 33 samples to below the reporting threshold (< 7), andjs_gc_init_typed_shape_layoutfrom 133 to 106.No GC regression, in the strongest available form.
PERRY_GC_DIAG=1output is byte-identical between the arms on all three benchmarks — 315 lines forchurn_alloc, 317 forpush_cls, 311 forpoly_shapes; every cycle'scopied_objects/copied_bytes/promoted_*/freed_bytesand every step decision the same. Independently, the 12gc_ratchetprobes measured back-to-back under both runtimes agree on 108 of 108 semantic metrics (retention + GC accounting), 0 differing. That is what you would expect: nothing here changes what is allocated or what the collector decides, only how the layout metadata gets published.This does not meet #7510's headline
≥1.5×bar, and the ticket already records why: the profile it was filed from is out of date, and after #7469/#7474/#7486/#7487/#7501/#7525 the wholegc::layoutfamily is ~11% ofchurn_alloc, not 33.6%. Removing the shape-install half of it entirely could not have delivered 1.5×. #7510 stays open for items 2 and 3 (layout_forget_objectis now 1.4%,layout_note_slot7.5%) and for the #7512 residual discussed at the bottom.What was costing 11%
Symbolicated leaf profile of
churn_alloc(20M{v, w}object literals; pinned quiet host at load 1.8; 1488 samples):js_gc_init_typed_shape_layoutshape_install_sharedSince #6893 the descriptor these two install is per-shape, not per-object: every same-shape object shares one canonical
TypedLayoutDescriptorinSHAPE_LAYOUTS, keyed by the sharedkeys_array. The only per-object work left is two header bits —GC_OBJ_TYPED_LAYOUT_INTACTandGC_LAYOUT_POINTER_FREE/GC_LAYOUT_SIDE_MASK.The call did not know that. For the 20-millionth
{v, w}literal it stillTypedLayoutDescriptor— 72 bytes, two 32-byteVec-carrying enums, one of them cloned, all of it dropped on the way out,RefCellborrow on the thread-localSHAPE_LAYOUTS,keys_arraypointer and probed the map,to reach the conclusion the first construction had already reached.
shape_install_shared's hit arm —Some(Some(existing)) if existing == descriptor— writes nothing at all.The design
gc/shape_install.rsmemoises exactly that map answer, in a thread-local direct-mapped table:On a hit the construction is: validate, set two header bits, done.
The memo decides nothing about the object. Everything the header declaration rests on is still re-derived per construction, ahead of the memo:
field_count == slot_count,And
POINTER_FREEvsSIDE_MASKis recomputed from the pointer mask, never read back from the memo — the entry carries no header state at all. That split is the soundness bar: a wrongPOINTER_FREEis a use-after-free factory (heap_payload_slot_selectionshort-circuits on it and skips the whole payload without consulting any mask), so that decision must not depend on a cache. A stale entry can only cost work.While reworking those checks they stopped building a
LayoutSlotMaskat all — they read the caller's mask words directly.LayoutSlotMaskis a 32-byte enum with aVecarm, so two of them meant drop glue on six early-return paths and, for any shape wider than 64 slots, two heap allocations per construction.LayoutSlotMask::intersectssurvives as the test-only reference implementation thatmask_word_helpers_agree_with_layout_slot_maskpins the three word helpers against, across trailing-zero words, bits pastslot_count, and slots past the end of the array.Self-healing
An entry is falsified by exactly one transition:
SHAPE_LAYOUTS[keys]ceasing to beSome(D).shape_install_sharedis the only writer of that map and its only such transition is the ambiguity poison (Some(Some(_)) => insert(None), two live layouts sharing one key set). That branch drops the whole table. Entries are never removed fromSHAPE_LAYOUTSand never overwritten with a differentSome, so there is no other way to go stale.Everything else already degrades to a miss:
keys_arraymoves — live objects report the new address, nothing matches, the slow path re-installs;SHAPE_LAYOUTSis keyed by that same address and its entries are never pruned;PERRY_SHAPE_LAYOUT_KEYED=0—recordis only reachable from a successfulshape_install_shared, which that knob gates, so the table stays empty and every lookup misses.This table is not a GC root, and the module says so explicitly, because a runtime-side cache of a raw heap pointer usually is one and the static root-dominance checker cannot see it.
keysis only ever compared as an integer; it is never dereferenced, never handed to the mutator, never used to keep anything alive.Two sizing decisions, both measured
The slot index mixes the two mask-global addresses in with the shape. Two object-literal sites with the same key names share one
keys_arraybut get separateprivate unnamed_addr constantmask globals unless LLVM's constant merger folds them; keying on the shape alone would put both in one entry and let them evict each other every iteration of a loop that builds both. Any index is correct — the full tuple is compared on the way out — so this is purely about not colliding.The table is 32 entries, not 8. A direct-mapped table cycled round-robin by more shapes than it has slots hits zero percent of the time: each entry is evicted by its partner before it is read again, so it pays the probe and gets nothing. A 16-shape churn loop against 8 slots measured a reproducible 0.993× — the one regression this change had. At 32 slots the same loop fits and turns into 1.059× (and 1.070× against the 8-slot build directly), with the monomorphic numbers unchanged. The whole table is 1 KiB of const-initialised thread-local, so a program with one shape pays for a page it never touches.
Testing
gc/tests/layout_trace/shape_install_memo.rsplusgc/shape_install.rs's own unit tests, 9 in total.It fires —
memo_fires_on_every_repeat_construction_of_one_shapecounts: 64 instances of one shape must produce exactly 1 install and 63 hits. This is the assertion #7525 did not have, and its absence is why that PR's first commit shipped a fast path that hit once in 40 million calls.It decides nothing —
a_memo_hit_produces_the_same_header_state_as_the_installcompares the_reservedlayout bits of an object published by the memo against one published by the install;a_contradicting_field_is_refused_even_with_the_memo_warmbuilds an instance whose raw-f64-declared slot holds a heap string and asserts it is refused with the table warm, that the hit counter did not move (validation ran first), and that both of its heap fields stay enumerable under the conservative fallback.It heals —
ambiguity_poison_invalidates_the_memoinstalls a shape, warms the memo, poisons that shape by installing a different layout under the same key names, and asserts the next construction of the original layout misses and lands correctly on the per-object path.The GC witness —
memo_installed_objects_survive_a_copying_minor_with_their_childrenbuilds six instances of a{ n: number; s: string }shape, asserts 5 of the 6 were published by the memo (subject-was-live, not merely "nothing threw"), forces an evacuating minor, and asserts the cycle actually moved ≥ 12 objects, that every instance relocated, that its raw-f64 slot survived verbatim, and that its string child relocated with its slot rewritten and its bytes intact.The sabotage arm —
a_pointer_free_declaration_on_this_shape_strands_the_childis permanent: it publishes this exact shapePOINTER_FREEby hand and asserts the collector then enumerates zero payload children. That is the use-after-free the witness is green against, and it is why the memo stores no header state.Sabotage verified by hand, three ways, each caught:
set_layout_state(POINTER_FREE)unconditionally)instance 1 must expose its one pointer field to the collector— left 0, right 1an instance that contradicts its shape's mask must not be declared intactinvalidate()the poison must have dropped the memo entryBeyond the unit tests:
cargo test -p perry-runtime— 1771 passed, 0 failed.test-files/compiled and run under both arms: byte-identical output, 20/20. Re-run against the Node oracle on the final build: 18 match byte-for-byte, 1 is skipped (node cannot run it), and 1 istest_gap_2159_defineproperty_class_prototype, an entry that is already intest-parity/known_failures.jsonas a standing gap pre-dating v0.5.1205.PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=64— the arm that faults on a stale from-space deref — produce output identical to the plain runs, 20/20. Verified not vacuous:PERRY_GC_DIAG=1prints[gc-fromspace-protect] retired_set=…five times on those runs, so copying minors really did retire and quarantine from-space pages.09_method_calls,bench_object_property,bench_numeric_array_downgrade) show no change — they run in 10–120 ms, which is below what this harness can resolve, so this is "no regression", not a measurement.raw_handle_debt.py999 (baseline 999);gc_store_site_inventory.py,addr_class_inventory.py,check_file_size.shall clean;cargo fmt --all -- --checkclean.The #7512 residual: why the reorder still does not work
The comment on #7510 folds in #7512's remaining half —
js_gc_init_typed_shape_layoutis emitted after the constructor call, so no raw-f64 class-field store inside a constructor body can pass its guard. This PR does not move it, and the reason is sharper than "the slots holdundefined":undefinedis0x7FFC_…, andlayout_raw_f64_bitsrejects any tag in[SHORT_STRING_TAG = 0x7FF9, STRING_TAG = 0x7FFF], so a pre-ctor install fails validation today. Relaxing that — acceptingundefinedin a raw-f64 slot at install time — would be safe for the collector:undefinedis not pointer-bearing, so a skipped raw-f64 slot strands nothing.It is not safe for readers.
class_field_fast_contractstates the contract the codegen-inlined path relies on: "the codegen-inlined fast path concludes slot K is raw-f64 purely from the per-object intact bit … the inline path could never read a NaN-boxed value as a raw double". Publishing the layout before the constructor runs makes exactly that false for every declared-but-not-yet-assigned field, andclass C { v: number; constructor() { console.log(this.v) } }must still printundefined.So it is not a runtime relaxation of the install-time check. It needs a codegen-side definite-assignment proof — every raw-f64-masked field written before any read of it — or a two-stage install that declares slots raw-f64 as the prologue fills them. Either is a separate change with its own soundness surface, so it stays on #7510.
Refs #7510, #7512, #7525, #6893, #5094.
Summary by CodeRabbit