fix(repsel): resolve the growth-forwarding chain before the element-shape clone derives its base (#7480) - #7660
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe element-shape loop preheader repairs forwarded array heads, stores refreshed bindings, validates representations, and reloads handles before shape queries. Closure-captured arrays are excluded. Tests cover IR ordering, fast-clone boundaries, growth, and callee mutation. ChangesElement-shape repair
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant LoopMatcher
participant RepairPreheader
participant ArrayRuntime
participant ShapeGuard
participant FastClone
LoopMatcher->>RepairPreheader: emit repair path for eligible array
RepairPreheader->>ArrayRuntime: refresh forwarded array head
ArrayRuntime-->>RepairPreheader: return live head
RepairPreheader->>LoopMatcher: store and reload binding
LoopMatcher->>ShapeGuard: query element shape with refreshed handle
ShapeGuard->>FastClone: branch to fast clone
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/engine-plan.md`:
- Around line 487-490: Update the named-class arm description to use the stable
phrase “before `#7660`” or cite the exact pre-fix revision, replacing the
time-dependent “on main” reference while preserving the SIGBUS and
growth-forwarding context.
- Around line 474-475: Clarify the benchmark ratio in docs/engine-plan.md by
renaming the ratio column or adding a statement before the table that defines it
as perry/node. Ensure the examples and all existing ratio values remain
consistent with that denominator.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| | `keep: Node[]` — what #7612 covers | 13 ms‡ | 57 ms† | 15 ms | **0.23×** | | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define the benchmark ratio.
The table contains both Node and Bun, but ratio does not identify its denominator. The values indicate perry/node (414 / 12 = 34.5; 13 / 57 = 0.23). Rename the column or state the formula before the table.
🤖 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 `@docs/engine-plan.md` around lines 474 - 475, Clarify the benchmark ratio in
docs/engine-plan.md by renaming the ratio column or adding a statement before
the table that defines it as perry/node. Ensure the examples and all existing
ratio values remain consistent with that denominator.
| ‡That cell did not exist before #7660: on `main` the named-class arm | ||
| *SIGBUSes*, because the versioned-loop consumer derived its elements base | ||
| from an unresolved growth-forwarding stub for any array grown outside the | ||
| scope that reads it. Step 2's win was real but gated behind a crash. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a stable pre-fix reference instead of “on main”.
After #7660 lands, “on main the named-class arm SIGBUSes” becomes false. Refer to “before #7660” or identify the exact pre-fix revision used for the comparison.
🤖 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 `@docs/engine-plan.md` around lines 487 - 490, Update the named-class arm
description to use the stable phrase “before `#7660`” or cite the exact pre-fix
revision, replacing the time-dependent “on main” reference while preserving the
SIGBUS and growth-forwarding context.
…hape clone derives its base (#7480) The element-shape versioned loop clone (#7612) derived `length` and the elements base from the array binding's RAW pointer. `js_array_grow` moves the array and leaves a forwarding stub whose first payload word (length‖capacity) is overwritten with the new head, so on a stale binding `length` read the low half of a heap pointer — passing the bound check — while the base still addressed the pre-growth buffer. Correct up to MIN_ARRAY_CAPACITY (16), SIGBUS at 17. Repairs the binding with repsel 4a.2's `js_array_refresh_local_head` before the guard call (the refresh can allocate, so it cannot go after the base is derived), and writes the live head back so the existing post-call re-loads pick it up.
Audit — merging as v0.5.1375. This is a live SIGBUS on
|
aa21ad8 to
6eeec68
Compare
1 participant
Re-measuring #7480 to decide its remaining step found a shipped SIGBUS in
#7612's element-shape versioned loop clone. The perf work #7480 describes is
not what this PR does — it fixes the crash that consumer has on
main, andre-measures the issue so its next step is chosen against real numbers. Details
of the measurement are in the issue comment; the fix is below.
The bug
js_array_growdoes not grow in place. It allocates the larger arrayelsewhere, copies, and leaves a forwarding stub at the old address —
and the stub's first payload word (
length‖capacity) is overwritten withthe new head, which is how
clean_arr_ptrfollows the chain. Every runtimeentry point resolves that chain, so a binding holding a stale head still
behaves correctly through the runtime. Only bindings the growing code itself
wrote through get re-pointed; a callee that grows a caller's array, or a
function that grows a local and returns it, leaves stale heads behind. That is
repsel 4a.2's (#6904) canonical case, and it already has a fix:
js_array_refresh_local_head.Every other consumer of a raw array head calls it. The element-shape preheader
did not, and derived both the bound check and the elements base straight from
the raw pointer:
On a stub those two facts fail in the worst possible combination.
lengthreads the low 32 bits of a heap pointer — a huge number, so
len_okpasses— while the base still addresses the pre-growth buffer. Meanwhile the guard
call one block earlier resolved the chain internally and answered truthfully
about the live array, so the class-id test passed too.
The clone then reads correct elements up to the old capacity and runs off the
end of the block after it.
MIN_ARRAY_CAPACITY == 16, so:Reproducing it — two ingredients, and one of them is not obvious
A first attempt to reproduce this failed, and the reason is worth stating up
front because it defines the blast radius.
match_element_shape_versioned_loopaccepts a loop bound that is
Expr::IntegerorExpr::LocalGetand nothingelse. An inline
arr.lengthbound is anExpr::PropertyGet, so the matcherdeclines and the binary contains no clone.
for (let j = 0; j < keep.length; j++)therefore cannot fault; hoisting the identical bound into a local(
const len = keep.length) is enough to flip it.MIN_ARRAY_CAPACITY(16) inside a callee that returned it. Minimal N = 17.Not required, each checked on
main@384eba78b:--profile perry-devand--releaseproduce the sametable, same minimal N;
PERRY_NO_AUTO_OPTIMIZE=1— it faults with auto-optimize on too;suppressor is
PERRY_GC_MOVING_LOOP_POLLS=1at compile time, which makesthe clone stand down by construction — that is how I localised it);
a hoisted bound faults identically. I originally guessed this was
load-bearing; the isolation arms below disprove it, and it is corrected here
and in array growth forwarding: build()-and-return leaves a stale head in the binding (minimal N = 17) #7661.
nkeep.lengthlenkeep.lengthpush/popwrite-backlenThe bound form is load-bearing; the loop's location is not. The last row is the
stub proof: a
keep.push(x); keep.pop();that changes no contents but routesthrough the runtime push helper — which resolves the chain and writes the live
head back — exits 0 with the right answer at exactly the N where the same file
without it faults.
Minimal reproducer, exit 138 (
EXC_BAD_ACCESS/SIGBUS) onmain:Note
sweep's bound is the parametern(anExpr::LocalGet). Change it toj < keep.lengthand the same file exits 0 — see ingredient (A).PERRY_PTR_SHAPE_LOCALS=0,PERRY_GEN_GC=0,PERRY_GEN_GC_EVACUATE=0andPERRY_WRITE_BARRIERS=0all still crash: it is not GC movement and not 3b, itis growth forwarding.
That
keepreally is a stub rather than a live head is established twoways, not assumed. Directly: adding a module-scope
keep.push(…); keep.pop();after the
build()— which forces a write-back of the resolved head into theglobal — makes the same pre-fix compiler print the right answer and exit 0.
And by construction:
js_array_refresh_local_headreturns its input untouchedwhen there is nothing to follow, so if the binding were already live this PR
would be a no-op and the crash would survive it.
Note for a follow-up, out of scope here: something on the producer side is
leaving that stub reachable —
expr/array_push.rsdoes write the reallocatedhead back to the pushing scope's own slot, so the value that reaches
keepislosing it somewhere between
build's slot and the caller's binding. This PRfixes the consumer, which is the right layer regardless (a stale head can
arrive from any of several routes, which is why every runtime entry point
resolves the chain), but the producer gap is tracked as #7661.
Why every existing test missed it. Both the gap test and the codegen census
build their arrays in the same scope that reads them, so the binding always
held the live head, and the largest was 64 elements pushed at module scope —
where each
push's write-back updates the global. The raw-pointer derivationwas never handed a stub.
The fix
A
element_shape.loop.preheader.repairblock between the brand test and theguard call: follow the chain once with
js_array_refresh_local_head, write thelive head back into the binding, re-derive the tag/band predicate from it.
Ordering is the substance, and it is why the repair is a new block rather than
two lines in the existing deref block:
from here to the end of the clone" — that is the revocation argument. The
refresh can allocate (
clean_arr_ptrforce-materialises a lazy array), so arefresh there would reintroduce exactly the "base derived across an
allocating call" hazard step (4) exists to prevent.
both re-read the binding — deliberately, because the guard call can move
the array. Refreshing without storing back would leave both re-reads pulling
the stub straight out again.
The write-back also lands the durable half of #6904's self-heal: after the
first visit the binding holds the live head, so every later loop entry, and the
slow clone, address the current array directly.
stmt/element_shape_loop.rsadditionally declines closure-captured arrays,which have a capture cell a plain slot store would not update.
Tests
Gap suite —
test_gap_repsel_element_shape_loop_clone.tsgains case 10with both stale-head shapes above 16 elements: callee-builds-and-returns, and
callee-grows-the-caller's-array. Both bound forms are covered — a parameter
bound (
sumField(returned, returned.length), whose loop readsn) and amodule-scope loop with a literal bound — so a future narrowing of ingredient
(A) cannot silently drop the coverage. Verified in both directions:
main(ec675f2fe, and384eba78b)node --experimental-strip-typesStandalone reproducers, both profiles, live at
/tmp/s7480-real/with arun_all.shthat prints the full N-scan per arm.Codegen census — three tests in
element_shape_loop_tests.rs. Theload-bearing one asserts the stored-back value is the refresh's result, not
merely that a store exists; sabotage-checked by deleting the write-back arm,
which fails that test and only that test.
CLONE_LABELSgains the repairblock, so a silently-dropped repair fails the census.
Gates
22/22 lint commands (extracted from
test.yml),cargo fmt --all --check,cargo test -p perry-codegen --lib728 passed,cargo test -p perry-runtime --lib1917 passed,cargo check --all-targets,native_root_coverage14passed. Codegen changed, so
gc_root_dominance_check.py --moving-only --seeded-violations 40over a freshly built 149-module corpus: 0 violations,40/40 seeded caught, 9828 root stores — the checker was live.
The repair introduces a call into the preheader, so the gap test was also run
under seven GC configurations, all byte-identical to node:
PERRY_GC_ZEAL=1,PERRY_GC_FORCE_EVACUATE=1,PERRY_GEN_GC=0,PERRY_GEN_GC_EVACUATE=0,PERRY_GC_VERIFY_EVACUATION=1,PERRY_GC_FROMSPACE_SCAN_ABORT=1,PERRY_WRITE_BARRIERS=0.Gap-suite subset as the #6377 check (
repsel/array/class/object/new/computed/prop— 76 tests, node 26.5.1): 75 pass, 0 fail, 1node_fail(test_gap_prop_plan_cache_invalidation; node itself exitsnon-zero, so not reachable from a compiler change).
Measured (pinned quiet mini, 7 interleaved rounds, checksums equal everywhere)
200k elements × 50 sweeps, runtime-derived bounds (
process.argv), releasebuild,
PERRY_NO_AUTO_OPTIMIZE=1,PERRY_RUNTIME_DIRpinned per arm.mainkeep: Node[]— the clone's own shapekeep: {v,w}[]— #7480's kernel, out of the clone's reachTwo controls, because "the guard now declines" would look identical to "fixed"
on the crash test alone:
down via
PERRY_GC_MOVING_LOOP_POLLS=1: 49–66 ms against 12–13 ms withit. The fast path is doing the work.
k_literal's emitted LLVMIR is byte-identical between the two arms (
cmp, not a filename compare— build: the object cache doesn't key on codegen env vars — PERRY_WRITE_BARRIERS=0 silently does nothing with a warm cache #6394 keys the object cache on every codegen env var), so 414 vs 414 is
identity rather than a lucky measurement.
k_class's IR differs, as it must.The
{v,w}[]row is #7480's remaining work and is untouched here:element_class_nameresolvesArray(Named(C))only, so an object-literalelement type never reaches the clone. The issue's recorded 93 ms / 6.2× is
stale in the optimistic direction; engine-plan item 6 is updated with the
current table, including the reason node's own baseline differs 5× between the
two kernels (
v: number;survives type-stripping as a class field declaration,pinning the field to tagged representation).