perf(gc): bind scalar-replacement root slots once per frame, not once per store - #7013
Conversation
📝 WalkthroughWalkthroughScalar-replaced slot rooting now binds shadow slots once in function entry, emits a persistent root barrier after each eligible store, initializes rooted allocas to ChangesScalar-replaced slot rooting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScalarSlotRoot
participant EntrySetup
participant ShadowSlot
participant LLVMIRTests
ScalarSlotRoot->>EntrySetup: schedule one js_shadow_slot_bind
EntrySetup->>ShadowSlot: bind initialized alloca
ScalarSlotRoot->>ShadowSlot: emit per-store root barrier
LLVMIRTests->>ScalarSlotRoot: inspect generated LLVM IR
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/scalar_replaced_slot_roots.rs (1)
441-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the requested benchmark matrix.
The PR objective asks for measurements and matrix results in a comment; this section only states an approximate per-iteration cost. Include the measured configurations and before/after results.
🤖 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-codegen/tests/scalar_replaced_slot_roots.rs` around lines 441 - 450, Expand the comment above the per-slot binding test to include the requested benchmark matrix, listing each measured configuration and its before/after results. Replace the approximate “~4 ns per iteration” statement with the actual measured data while retaining the explanation that the bind is loop-invariant and scoped per slot.
🤖 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-codegen/tests/scalar_replaced_slot_roots.rs`:
- Around line 441-450: Expand the comment above the per-slot binding test to
include the requested benchmark matrix, listing each measured configuration and
its before/after results. Replace the approximate “~4 ns per iteration”
statement with the actual measured data while retaining the explanation that the
bind is loop-invariant and scoped per slot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 938f083d-6cc4-46cc-a298-f94c50de6c3b
📒 Files selected for processing (5)
crates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/scalar_slot_root.rscrates/perry-codegen/src/expr/shadow_slot.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/tests/scalar_replaced_slot_roots.rs
Hoists the per-store
js_shadow_slot_bindthat #7007 emits for scalar-replacedobject fields and array elements out to the function-entry setup, leaving at the
store only the incremental-mark shading barrier — emitted inline and guarded, so
the common path is a load, a compare and a not-taken branch instead of a
TLS-touching call.
Why the bind is loop-invariant. A bind sets
slot_ptrs[idx] = alloca,mirrors
stack[idx] = *alloca, setsactive[idx], and runs the root barrier.The alloca is entry-hoisted and never moves, and both readers of a bound slot
(
visit_shadow_stack_root_slots,js_shadow_slot_get) dereferenceslot_ptrs[idx]in preference to thestack[idx]mirror — so the mirror is deadstorage and only the barrier is per-store work. Same treatment
enable_persistent_shadow_slot_for_array_aliasalready gives aconst item = arr[i]alias.The hoist does not move when the rooted value is read. The collector reads
the alloca at collection time, exactly as it did when the bind sat at the store.
Nothing is snapshotted into a register and re-read later, so the value a
safepoint observes is unchanged — what disappears is a redundant copy, not an
observation point.
Measured on a quiet Mac mini (load 1.9), pinned Node 26.5.0, 10 M iterations
of
{ s: mkStr(i), y: (i>>3) & 1023 }, best-of-5, arms interleaved:41 ms of rooting cost → ~3 ms, 93 % recovered (~4.1 ns/store → ~0.3 ns).
Binary size byte-identical. The #6997 numeric gate is untouched: a proven-numeric
field still takes no slot, no call, no frame growth — and now no entry bind.
The one soundness obligation the hoist creates. Binding at entry makes the
slot
activefrom function entry, so the collector dereferences the allocabefore any store reaches it and on paths where none ever does. An uninitialized
alloca would hand the root-word decoder stack garbage that can pass
is_plausible_heap_addr. The object-literal, anonymous-shape andsplit()pathsalready stored
undefinedinto their slots inentry_allocas; thearray-element and fused-uppercase-receiver paths did not, and now do.
Correctness.
gc_repsel_matrix.sh --arms all --pressure 8vs pinned Node26.5.0: PASS=229 UNVER=190 XFAIL=1 FAIL=20 over 440 cells (22 rows,
post-#7011) — main's published 460-cell baseline (240/199/1/20) minus exactly
the row #7011 de-duplicated, which contributed 11 PASS and 9 UNVER. No cell
changed state. Both FAIL files are the #6981 pair.
test_gap_repsel_scalar_replaced_localsis PASS on all 20 arms including the evacuating ones, where the arm was
confirmed live (
copied_objects261 251–379 072, output byte-identical to theoracle) — the hoisted binding is still rewritten through
slot_ptrs[idx]whenthe collector relocates its referent.
Tests. Four new codegen-contract tests, each verified to FAIL against the
pre-hoist compiler: one bind per slot rather than per store; the bind in the
entry block ahead of the storing loop and after
js_shadow_frame_push(binding before the push would write the caller's frame); a shading barrier at
every store; and the entry initialization the hoist depends on.
Changeset fragment follows in #7014 — this PR was merged from a WIP commit
before the fragment was attached.