perf(repsel): the element-shape loop clone fires on arr.length and aliased element types (#7480) - #7701
Conversation
|
Warning Review limit reached
Next review available in: 8 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 (3)
📝 WalkthroughWalkthroughElement-shape loops now accept matching ChangesElement-shape loop specialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ElementShapeLoopMatcher
participant ElementShapeGuard
participant ElementShapeFastClone
participant LoopLowering
ElementShapeLoopMatcher->>ElementShapeGuard: select Bound or ArrayLength
ElementShapeGuard-->>ElementShapeFastClone: return validated i32 trip count
ElementShapeFastClone->>LoopLowering: lower specialized loop
LoopLowering-->>ElementShapeFastClone: omit redundant length loads and GC polls
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 |
![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
🧹 Nitpick comments (3)
crates/perry-codegen/src/stmt/element_shape_loop.rs (1)
718-723: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the trip-count variant from
matched.bound, not fromOption::is_some.
materialized_boundencodes two facts: the materialized value, and which trip-count mode the guard must use. TodayNonemeans exactlyArrayLength. A future bound arm that materializes nothing would silently take theArrayLengthpath and make the guard skip thelength >= boundproof.Match on
matched.boundso the mode has one source of truth.♻️ Proposed refactor
- let trip_count = match &materialized_bound { - Some(bound) => { - crate::expr::element_shape_guard::ElementShapeLoopTripCount::Bound(bound.as_str()) - } - None => crate::expr::element_shape_guard::ElementShapeLoopTripCount::ArrayLength, - }; + let trip_count = match (matched.bound, &materialized_bound) { + (ElementShapeLoopBound::ArrayLength(_), _) => { + crate::expr::element_shape_guard::ElementShapeLoopTripCount::ArrayLength + } + (_, Some(bound)) => { + crate::expr::element_shape_guard::ElementShapeLoopTripCount::Bound(bound.as_str()) + } + (_, None) => return Ok(false), + };🤖 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/src/stmt/element_shape_loop.rs` around lines 718 - 723, Update the trip-count selection near ElementShapeLoopTripCount to match on matched.bound rather than testing materialized_bound with Some/None. Preserve the materialized bound string for bound cases and select ArrayLength only when matched.bound represents that mode, keeping the guard mode independent of whether a value was materialized.crates/perry-codegen/src/stmt/class_field_loop_tests.rs (1)
258-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fast_clone_slicestill uses span selection; the changelog says otherwise.
changelog.d/7701-element-shape-loop-applicability.mdstates the class-field tests gained "the same by-name slice fix". This helper still slices everything betweenfor.class_field_fast.condandfor.class_field_slow.cond.The span is correct today because
ClassFieldLoopBoundadmits onlyConstantandLocal, so noplen.*blocks land in the gap. It carries the same fragility the element-shape helper just removed: any future neighbour emitted between the two cond blocks is attributed to the fast clone.Either apply the by-name selection used in
element_shape_loop_tests.rs, or correct the changelog claim.🤖 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/src/stmt/class_field_loop_tests.rs` around lines 258 - 267, Update fast_clone_slice to use the same by-name IR block selection as the element-shape loop tests, selecting only the class-field fast-clone block instead of slicing through the slow-condition boundary. Keep the helper’s existing fast-clone output contract, and leave the changelog claim accurate rather than weakening or removing it.crates/perry-codegen/src/stmt/loops.rs (1)
4784-4802: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSuppression key is broader than the stated justification. Confirm the wider scope is intended.
The comment justifies the suppression with "the caller has ALREADY materialized the trip count and passed it in
precomputed_i32_bound". The condition does not testprecomputed_i32_bound. It tests whether any fact scope is active, so a nested loop lowered inside a fast clone body also loses its length hoist while receivingprecomputed_i32_bound == None.For the element-shape clone this cannot happen: the matcher admits a single
Stmt::Expr(Expr::LocalSet(...))body. For the class-field clone the body shape is wider.The result is a slower nested loop, not a wrong one, and the bounded-index facts you keep remain valid. If you want the condition to match the comment, gate on
precomputed_i32_bound.is_some()as well.Also applies to: 4852-4852
🤖 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/src/stmt/loops.rs` around lines 4784 - 4802, Narrow the length-hoist suppression in the loop-lowering logic around hoisted_length_slot and the corresponding path near the second occurrence so it requires an active precomputed_i32_bound, rather than any element_shape_loop_facts or class_field_loop_facts scope alone. Preserve the existing suppression when a bound was materialized and passed by the caller, while allowing nested loops with precomputed_i32_bound == None to hoist their own lengths.
🤖 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-codegen/src/stmt/class_field_loop_tests.rs`:
- Around line 233-239: Update the class-field fast-clone assertion around
assert_fast_clone_is_entered to inspect only the block_slice for
class_field.loop.preheader.deref, adding the local block_slice helper equivalent
to element_shape_loop_tests.rs. Within that scoped block, assert only the branch
target class_field.loop.fast.preheader; remove the whole-IR check and alternate
fast condition label.
In `@crates/perry-codegen/src/stmt/element_shape_loop_tests.rs`:
- Around line 970-974: Update the assertion in the relevant element-shape loop
test to verify that the arr.length bound is materialized once in the preheader,
rather than asserting js_array_length is absent from the fast clone. Preserve
the existing call-free assertion, and rename or adjust the assertion message to
clearly identify the redundant per-iteration load it detects.
---
Nitpick comments:
In `@crates/perry-codegen/src/stmt/class_field_loop_tests.rs`:
- Around line 258-267: Update fast_clone_slice to use the same by-name IR block
selection as the element-shape loop tests, selecting only the class-field
fast-clone block instead of slicing through the slow-condition boundary. Keep
the helper’s existing fast-clone output contract, and leave the changelog claim
accurate rather than weakening or removing it.
In `@crates/perry-codegen/src/stmt/element_shape_loop.rs`:
- Around line 718-723: Update the trip-count selection near
ElementShapeLoopTripCount to match on matched.bound rather than testing
materialized_bound with Some/None. Preserve the materialized bound string for
bound cases and select ArrayLength only when matched.bound represents that mode,
keeping the guard mode independent of whether a value was materialized.
In `@crates/perry-codegen/src/stmt/loops.rs`:
- Around line 4784-4802: Narrow the length-hoist suppression in the
loop-lowering logic around hoisted_length_slot and the corresponding path near
the second occurrence so it requires an active precomputed_i32_bound, rather
than any element_shape_loop_facts or class_field_loop_facts scope alone.
Preserve the existing suppression when a bound was materialized and passed by
the caller, while allowing nested loops with precomputed_i32_bound == None to
hoist their own lengths.
🪄 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: 7acdc808-d1eb-43d3-b078-6f8de179419a
📒 Files selected for processing (6)
changelog.d/7701-element-shape-loop-applicability.mdcrates/perry-codegen/src/expr/element_shape_guard.rscrates/perry-codegen/src/stmt/class_field_loop_tests.rscrates/perry-codegen/src/stmt/element_shape_loop.rscrates/perry-codegen/src/stmt/element_shape_loop_tests.rscrates/perry-codegen/src/stmt/loops.rs
| assert!( | ||
| ir.contains("label %for.class_field_fast.cond") | ||
| || ir.contains("label %class_field.loop.fast.preheader"), | ||
| "{what}: the guard must branch INTO the fast clone. If it ends in an \ | ||
| unconditional branch to the slow clone, the call-free proof failed and \ | ||
| the clone is dead code that every label assertion above still accepts" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This assertion does not detect the state it describes. Scope it to the guard's deref block.
The check searches the whole IR. When the call-free proof fails, the lowering terminates the deref block with br label %class_field.loop.slow.preheader and leaves the fast blocks in place. Those fast blocks still contain their own branches, so label %for.class_field_fast.cond remains in the emitted IR and the first disjunct still matches. The assertion passes in exactly the state the comment says it catches.
The element-shape twin, assert_fast_clone_is_entered in element_shape_loop_tests.rs, first slices element_shape.loop.preheader.deref and then asserts the branch target inside that block. Apply the same scoping here against class_field_loop.preheader.deref, and assert only the branch into class_field.loop.fast.preheader.
💚 Proposed fix
- assert!(
- ir.contains("label %for.class_field_fast.cond")
- || ir.contains("label %class_field.loop.fast.preheader"),
- "{what}: the guard must branch INTO the fast clone. If it ends in an \
- unconditional branch to the slow clone, the call-free proof failed and \
- the clone is dead code that every label assertion above still accepts"
- );
+ let deref = block_slice(ir, "class_field_loop.preheader.deref");
+ assert!(
+ deref.contains("label %class_field.loop.fast.preheader"),
+ "{what}: the guard must branch INTO the fast clone. The deref block \
+ ends in an unconditional branch to the slow clone, which means the \
+ call-free proof failed and the clone is dead code that every label \
+ assertion above still accepts:\n{deref}"
+ );This needs a block_slice helper in this file, equivalent to the one in element_shape_loop_tests.rs. Do you want me to generate it?
🤖 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/src/stmt/class_field_loop_tests.rs` around lines 233 -
239, Update the class-field fast-clone assertion around
assert_fast_clone_is_entered to inspect only the block_slice for
class_field.loop.preheader.deref, adding the local block_slice helper equivalent
to element_shape_loop_tests.rs. Within that scoped block, assert only the branch
target class_field.loop.fast.preheader; remove the whole-IR check and alternate
fast condition label.
…aliased element types (#7480) The clone landed in #7612 and learned object-literal element types in #7669. Neither moved churn_read.ts at all, because the matcher declined before the lowering was ever reached: 1. `j < arr.length` is a PropertyGet, which the bound match did not admit — and that is the bound form every #7480 kernel is written in. 2. `type Node = {v: number}` makes receiver_class_name answer "Node", a name no class owns; element_class_name returned it and skipped the anon-shape resolver #7669 had just added for exactly this case. Admit `ElementShapeLoopBound::ArrayLength` for the array the body reads (a foreign array's length is declined — the preheader relates the two not at all), take the trip count from the length word the guard already loads, and resolve `type` aliases on both the array and element level. Also fixes the IR census: fast_clone_slice sliced a SPAN between the two clones, so the slow clone's `.length` hoist blocks were attributed to the fast clone. It now selects blocks by name.
…ded fast clone (#7480) Both versioned-loop clones build the fast body first and prove it call-free second; a failed proof branches unconditionally to the slow clone and leaves the fast blocks as unreachable code. A call emitted into the clone therefore DELETES it rather than slowing it, with every IR-census label still present. Two lowerings were doing that to the element-shape clone: - #7690's back-edge poll, in the element-load block. churn_read.ts: 0.03s with polls off, 0.54s with polls on -- same compiler, clone dead. - the loop-invariant arr.length hoist, which re-derived a bound the caller had already passed as precomputed_i32_bound. Only the load is skipped; the bounds proofs and the i32 counter slot stay. The class-field clone is NOT affected today -- checked, not assumed: with the suppression removed its three IR tests stay green, because loop_may_allocate already proves an obj.field-only body inert. It is covered anyway, since the two clones rest on the identical argument. Both test files gained assert_*_is_entered: the guard must cond_br INTO the clone, not merely emit its labels. Sabotage-tested -- removing the suppression turns the two .length-bound tests red.
CLONE_LABELS is satisfied by a preheader whose deref block ends in an unconditional branch to the slow clone -- the exact shape a failed call-free proof emits. Verified the gap was real: with the pre-fix assertions and `if false && fast_clone_call_free`, the test still passed. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
0c0b42a to
b8edb1b
Compare
Audit — merging as v0.5.1403, with one added assertionI tried hard to break the widening and could not. The one thing I did find was a vacuous test, and I fixed it on the branch rather than hand it back. The soundness question, traced rather than assumedBoth new admission surfaces reach the same, unmodified call-free proof: let fast_clone_call_free = !ctx.func.blocks()[fast_pre_idx].contains_gc_unsafe_call()
&& (fast_scan_start..fast_scan_end).all(|i| !ctx.func.blocks()[i].contains_gc_unsafe_call());
What I tried to break it with:
Worth noting "aliased element types" here means TS The vacuous test, and the proof it was vacuous
Not a hypothesis — measured. With the pre-fix assertions and With Your Gates26/26. Full |
1 participant
Part 1 of P2 (#7480 step 4 / repsel #5093): make the element-shape versioned
loop clone actually fire, and stop two lowerings from silently deleting it.
Why churn_read.ts never moved
#7612 landed the clone and #7669 taught it object-literal element types. Neither
moved
churn_read.tsby a millisecond — it sat at 0.35 s across four measurementrounds while everything around it improved 4–11×. The lowering was never the
problem. The matcher declined before reaching it, for two independent
reasons, each on its own fatal:
for (let j = 0; j < arr.length; j++)failed the bound match. The boundadmitted a literal or a loop-invariant local;
keep.lengthis aPropertyGet, so the match returnedNoneat the condition, before the classresolver was ever consulted. That is the bound form every one of repsel: element-shape proofs through arrays — measured 6.2× vs node; route = invariant bit → versioned-loop consumer → element Ptr<Shape> #7480's own
kernels — and
churn_read,churn,retain— is written in.type Node = { v: number; w: number }shadowed the anon-shape resolver.element_class_namereturnsreceiver_class_name's answer when it has one,and for an alias-typed array that answer is
"Node"— a name noctx.classesentry owns, because the literals allocate an
__AnonShape_…. The early returnmeant the resolver perf(repsel): resolve object-literal element types in the element-shape loop clone (#7480) #7669 added for exactly this case was never reached for any
alias-typed array.
A four-compile probe matrix localized it: inline element type + constant bound
fires; alias + constant declines; inline +
.lengthdeclines; inline +loop-invariant local bound fires.
And then: two calls that delete the clone rather than slow it
Both
lower_element_shape_versioned_forandlower_class_field_versioned_forbuild the fast clone first and prove it call-free second. On a failed proof they
terminate the guard with an unconditional branch to the slow clone and leave
the fast blocks as unreachable code. So a call emitted into one of these clones
does not make it slower — it removes it, silently, with every block label still
present for an IR census to find.
putting a
js_gc_loop_safepoint()in the clone's element-load block. Samecompiler,
churn_read.ts: polls off → clone runs, 0.03 s user; polls on →br label %element_shape.loop.slow.preheader, 0.54 s. Skipping the poll insidea clone is not a new licence — a poll exists so an allocating loop can defer
a collection, which is why
loop_may_allocatealready gates it. That predicateanswers from the HIR body, before specialization, so it cannot see that
arr[j].flowers to a bare load here.arr.lengthhoist. With a.lengthbound the clone gota second
plendiamond (js_value_length_f64) whose result nothing reads,because the caller had already handed the trip count in as
precomputed_i32_bound. Only the load is skipped; the bounded-index andbuffer-width facts and the i32 counter slot are proofs and storage the clone's
other lowering may depend on.
This is the failure mode
stmt/element_shape_loop.rs's own module docs named inadvance — "a silent loss of the optimization … with no test failing" — and every
perry-codegentest was green throughout.The class-field clone is not affected today, and that was checked rather than
assumed: with the suppression removed its three IR tests stay green, because
loop_may_allocatealready proves anobj.field-only body inert. Thesuppression covers it anyway (identical argument), and its tests gained the same
liveness assertion.
Results
Quiet M1 mini, best-of-3 interleaved. Both arms are rebased onto the #7690
stack (
20fce6daa):origin/mainis currently GC-livelocked — #7687 shippedonly part 1 of the #7682 fix, and
churn_allocruns 0.43 s vs 8.7 s there — soany allocation-touching measurement on plain
mainis meaningless.All 13 benchmark stdouts byte-identical; peak RSS byte-identical (retain 367 MB
both arms).
retainimproves by only 0.035 s, and it cannot do better from the read side:by amplification (
retain_read10= build + 10 read passes vsretain= build +1), one read pass costs 40.0 ms → 3.9 ms (10.3×) and is 2.8% of retain's
1.42 s. Zeroing it entirely would save 0.040 s. The rest is 3M allocations +
GC.
Tests
The IR census is the regression gate for this optimization, and it gained the
assertion it was missing:
assert_fast_clone_is_entered— the guard mustcond_brinto the clone.Label-presence assertions cannot distinguish "optimized" from "optimization
silently deleted". Added to all five positive tests, and to the class-field
suite. Sabotage-tested: with the suppression removed, the two
.length-bound tests go red with "the deref block ends in an unconditionalbranch to the slow clone", so a green run is evidence rather than decoration.
fast_clone_slicenow selects blocks by name instead of slicing a spanbetween the fast and slow cond blocks. An
arr.lengthbound makes the slowclone hoist its own length read, and those
plen.*blocks landed in the gap —so a call belonging to the slow clone was attributed to the fast one and the
census failed on a fast clone that was, and still is, bare. This is the mirror
image of the repsel: element-shape proofs through arrays — measured 6.2× vs node; route = invariant bit → versioned-loop consumer → element Ptr<Shape> #7480 step 3 bug the function's own doc records; a span that
depends on what a neighbour emits can report wrong in either direction.
.lengthbound fires and stays call-free; a foreign array's.lengthis declined (the preheader relates the two lengths not at all, sothat would be an out-of-range read rather than a slow clone); an aliased
element type resolves; an unresolvable
Namedelement type with no class andno alias is declined rather than guessed; and
churn_read.ts's exact shape(alias and
.lengthtogether) reaches the clone.Validation
cargo test -p perry-codegen: 783/0 lib, all integration suites green exceptlarge_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrier,which is red on the clean baseline commit with no local changes (verified).
It slices a span between
apush.inbounds.andapush.realloc.— the samefragile pattern fixed above. Reported separately; not touched here.
cargo test -p perry-runtime --release: 1930/0.9864 root stores, with
--self-testconfirming the checker can still fail.cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/addr_class_inventory.py: clean.snapshot flags — six network crashes and
zlib_3285_params— reproduceidentically on the base compiler, and the ten
node_fail -> parity_failstatus changes are the oracle running tests it previously could not. One
improvement:
test_gap_iterator_helpers_2874: parity_fail -> pass.Notes for review
changelog.d/fragment is keyed on a guessed PR number and should berenamed to the real one.
lower_object_array_write_versioned_for,has the same call-free scan and no fact scope to key off. It is a write-path
loop, is not covered by the benchmarks here, and is left for a follow-up rather
than changed blind.
Summary by CodeRabbit
New Features
.length.Bug Fixes