perf(repsel): resolve object-literal element types in the element-shape loop clone (#7480) - #7669
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change extends element-shape loop cloning to closed object-literal array elements. It derives validated property facts, lowers proven fields as raw doubles, rejects unsafe or ambiguous shapes, and adds codegen, runtime, and documentation coverage. ChangesElement-shape specialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LoopMatcher
participant ElementShapeFact
participant PropertyGetLowering
participant NumericAnalysis
LoopMatcher->>ElementShapeFact: resolve compatible object-literal shape
ElementShapeFact->>PropertyGetLowering: provide validated field fact
ElementShapeFact->>NumericAnalysis: prove raw-double property read
PropertyGetLowering->>LoopMatcher: emit guarded raw-f64 clone load
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/perry-codegen/src/stmt/element_shape_loop.rs (1)
333-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe field-type check is skipped when exactly one candidate matches.
candidates.retain(...)runs only whencandidates.len() > 1. A single anon shape whose field names and order match but whose field types disagree with the declared annotation is accepted without the compatibility test. This stays safe because the preheader guards on the resolved class id and the raw-f64 gate reads the resolved class's own field types, so a mismatch costs the clone and not the answer. Consider running the filter unconditionally so the accept path and the tie-break path apply the same rule.♻️ Apply the compatibility filter unconditionally
- if candidates.len() > 1 { - candidates.retain(|name| { - ctx.classes.get(*name).is_some_and(|class| { - class.fields.iter().all(|f| { - obj.properties - .get(&f.name) - .is_some_and(|p| anon_shape_field_type_is_compatible(&p.ty, &f.ty)) - }) - }) - }); - } + candidates.retain(|name| { + ctx.classes.get(*name).is_some_and(|class| { + class.fields.iter().all(|f| { + obj.properties + .get(&f.name) + .is_some_and(|p| anon_shape_field_type_is_compatible(&p.ty, &f.ty)) + }) + }) + });🤖 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 333 - 343, Run the compatibility filter in the candidate-resolution logic unconditionally, including when candidates.len() equals one. Update the retain block around anon_shape_field_type_is_compatible so every candidate is checked against its declared field types before acceptance, while preserving the existing candidate filtering behavior otherwise.
🤖 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/element_shape_loop_tests.rs`:
- Around line 255-257: Update the element-type override in the
element_shape_module setup to assert that the first initializer statement is the
expected array Stmt::Let, and panic if it is not, while preserving the
Type::Array replacement for the matching shape. Do not silently skip the
override.
- Around line 821-828: Update the post-loop IR slice in the test around the
merge-block assertion to use the existing block_def_offset for
element_shape.loop.merge rather than searching with ir.find. Keep the by-name
read assertion scoped strictly from the merge block definition so preceding
branch references cannot satisfy it.
In `@docs/engine-plan.md`:
- Around line 463-480: The campaign summary’s “Repsel stack” paragraph still
reports the closed object-literal result as 34.5× node and points to backlog
item 6. Update that summary to state that the object-literal arm now reaches
parity with node at 12 ms, and identify only the still-open element Ptr<Shape>
work as the next gap.
In `@test-files/test_gap_repsel_element_shape_loop_clone.ts`:
- Around line 439-444: Update the deletion scenario around sumRow so it removes
the v property that the loop reads instead of w, or add a separate case deleting
v. Ensure the test exercises the deleted-read-field side exit while preserving
the existing rowsDeleted setup and assertion.
---
Nitpick comments:
In `@crates/perry-codegen/src/stmt/element_shape_loop.rs`:
- Around line 333-343: Run the compatibility filter in the candidate-resolution
logic unconditionally, including when candidates.len() equals one. Update the
retain block around anon_shape_field_type_is_compatible so every candidate is
checked against its declared field types before acceptance, while preserving the
existing candidate filtering behavior otherwise.
🪄 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: 812247bf-50f3-4fdf-8172-21789b9e2cfb
📒 Files selected for processing (8)
crates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get/helpers.rscrates/perry-codegen/src/stmt/element_shape_loop.rscrates/perry-codegen/src/stmt/element_shape_loop_tests.rscrates/perry-codegen/src/type_analysis/numeric.rsdocs/engine-plan.mdtest-files/test_gap_repsel_element_shape_loop_clone.ts
9ddea01 to
3645c29
Compare
…pe loop clone (#7480) #7480's own kernel (`keep: {v, w}[]`, 200k x 50 sweeps) went from 408 ms to 12 ms on the pinned quiet mini — parity with node (12 ms) and bun (12 ms), down from 34x node. The named-class arm #7612 already covered is unchanged at 13 ms. Checksums identical across every arm and runtime. `element_class_name` resolved `Array(Named(C))` only, so an object-literal element type never reached the clone. It now also resolves the declared object type to the `__AnonShape_<hash>` class its literals allocate, by matching the declared property order against the module's anon shapes (ambiguity declines rather than guessing, so the answer does not depend on `ctx.classes` iteration order). `receiver_class_name` is deliberately NOT widened — that is the #6377 blast radius #7612 refused. The clone is made self-contained instead: its `ElementShapeLoopFact` already carried the class name and packed slot index, and the three sites that would otherwise re-derive the class from the receiver now consult that fact through one predicate. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…e matcher docs Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…er every candidate, cover a deleted read-field Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1383. Plan item 6 closes; the last one.34×, to parity with both engines. I reproduced the direction on my own build: identical checksums, The correction that matters most: the levers are not separableMy brief said "plausibly two separable levers — say which you are not taking". You took both and explained why that was forced, not greedy:
Anything gated on call-freeness has this shape, and that generalisation is worth more than the kernel. Splitting it would have produced a change that looks correct, measures nothing, and grows the binary. You were also right to decline extending The vacuous gate is the second finding, and it is a serious one
So the IR census that exists to prove the clone is call-free had been unable to fail since #7612 — on the code that then shipped the #7660 SIGBUS. That is the #7024/#7025 family exactly, in the gate that should have been the last line of defence. I verified the repair is load-bearing rather than cosmetic: reverting the slicer to the pre-fix substring form reddens the suite, so the new tests are sensitive to the slicing. Asserting the slice contains the cloned body and its element load is the right shape — it cannot pass on an empty subject again. That CodeRabbit then found the same bug twice more in your own new tests, and you fixed both, is the honest version of this. The pattern is subtle enough to recur inside a change written specifically to fix it. Correctness against the shape that just crashedNo new preheader or base derivation, and the gap test crosses Measured blast radius rather than asserted: 43 gap files gain zero clones in either arm; of 25 repsel/specabi files exactly one changes, 2 → 6, which is the liveness control. Two more corrections, both mine
Gates: 22/22 lint, fmt clean, Noted on process: the session scratchpad is shared between concurrent agents and another overwrote your |
3645c29 to
5bc5426
Compare
…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.
…aliased element types (#7480) (#7701) * perf(repsel): the element-shape loop clone fires on `arr.length` and 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. * perf(repsel): stop emitting the two calls that silently delete a guarded 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. * docs(changelog): key the fragment on PR #7701 * test(repsel): the alias-only clone test must assert the clone is ENTERED 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 * chore: bump version to 0.5.1403 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes the last piece of #7480 and
docs/engine-plan.mditem 6: elementPtr<Shape>for object-literal element types.Measured
Pinned quiet mini, release, 7 interleaved rounds, 200k elements x 50 sweeps,
runtime-derived bounds, checksums equal in every cell,
rustc/cargoat zeroand 94.6–94.9% idle before and after.
keep: {v, w}[]— #7480's own kernelkeep: Node[]— what #7612 covered{v, w}[](#7034 §3, E1–E5)34x on the object-literal arm, to parity with both engines. The
named-class arm is unchanged, and so is the region-local arm — that one was
already covered by
collectors/ptr_shape_elements.rs, which is why thischange extends the versioned-loop consumer (the parameter/global case) rather
than that pass.
Both arms link byte-identical
libperry_runtime.a/libperry_stdlib.a(sha256 equal), so this is a codegen-only change and the meaningful control is
the emitted IR, below.
What changed
stmt/element_shape_loop.rs::element_class_nameresolvedArray(Named(C))only. It now also resolves a declared object type to the
__AnonShape_<hash>class its literals allocate, by matching the declared property order against
the module's anon shapes. The hash cannot be recomputed —
mint_anon_shape_classkeys it on the literal's inferred value types (
{v: 1}tagsi, notn)while the annotation says
number— so the class is found, not derived.Ambiguity declines rather than guessing, because
ctx.classesis aHashMapand "first match wins" would make the emitted code depend oniteration order.
receiver_class_nameis not widened. That is the #6377 blast radius #7612deliberately refused; instead the clone is made self-contained. Its
ElementShapeLoopFactalready carried the class name and packed slot index, sothe three sites that would otherwise re-derive the class from the receiver now
go through one predicate,
expr::element_shape_loop_fact_for_property_get:lower_raw_f64_class_field_get_for_number_context— the interception movedabove the
receiver_class_namegate;type_analysis::is_numeric_expr'sPropertyGetarm;expr::binary's arithmetic-operand router (as a disjunct, rather than bywidening
expr_may_return_boxed_value_from_raw_f64_fallback, which wouldhave been a lie — this read has no boxed fallback).
All three are scoped to the fast clone: outside one the fact vector is empty,
so
keep[j].vanywhere else is byte-for-byte what it was.The issue's cost model was wrong, and the correction is load-bearing
#7480 records "no out-of-line guard calls, the cost is stacked inline
diamonds". The object-literal arm actually carried three calls per
iteration, the third being
js_dynamic_string_or_number_add: with noresolvable class the accumulator loses its numeric proof, so
+is not anfadd. The plan called that "a second, separable lever".It is not separable. The clone is admitted only if it is provably
call-free (
LlBlock::contains_gc_unsafe_callcounts every non-llvm.call),so resolving the element class without restoring the numeric proof emits the
clone, fails the call-free test, branches unconditionally to the slow arm and
buys exactly zero at a cost in code size. Both had to land together, and the
numeric claim inside the clone is stronger than the annotation it replaces:
the residual per-element check already proves
GC_OBJ_TYPED_LAYOUT_INTACT,i.e. that the slot holds a raw double.
IR evidence (
--trace llvm, release,PERRY_NO_AUTO_OPTIMIZE=1)sweep, object-literal kernel, before — no clone at all, zerofadd:After — the fast clone is
gep+ load, a three-load residual check,fadd:The remaining calls in the function are the preheader's
(
js_array_refresh_local_head,js_array_ensure_element_shape) and the slowclone's, which is unchanged.
Correctness against the #7660 shape
Every new gap case that reads a
{v, w}[]crossesMIN_ARRAY_CAPACITY, so thegrowth-forwarding stub the #7660 repair exists for is live on this arm too:
callee-built-and-returned, callee-filled-caller-owned, a 17-element prefix, and
a module-global array read from inside a function (the write-back's
module_globalsarm). No new preheader or base derivation was added — theelement-shape preheader is the one #7660 fixed, unchanged.
The gap test also pins the hazards specific to this arm:
rows.lengthbound, which the matcher rejects (it is aPropertyGet, not anInteger/LocalGet), so a kernel written the obviousway gets no clone and must still print the same number;
re-runs the current iteration and the accumulator turns into a string exactly
where JS says it does;
{v: number, w: number}vs{v: string, w: string}), so a mis-resolved shape would cost the clone andnever the answer;
test_gap_repsel_element_shape_loop_cloneis byte-identical to the Node 26.5.1oracle on both arms, and an IR census confirms the new sections are live:
sumRow,sumRow$spec_b_i32andsumGlobalRowsgain the clone, where onmainonlysumFieldandmainhad it.An existing gate that could not fail
fast_clone_sliceinelement_shape_loop_tests.rssliced from the firstsubstring occurrence of
for.element_shape_fast.cond— which is thebr label %…terminator of the fast preheader, four lines above the slowpreheader — and every assertion made against the result is a negative
(
!fast.contains(" call "),!fast.contains("js_array_get_f64"), …). So theIR census that exists to prove the clone is call-free had been vacuous since
#7612, on the code that then shipped a SIGBUS. It now finds the block
definition and asserts the slice contains the cloned body and its element
load, so it cannot pass on an empty subject again. Same family as #7024/#7025:
the gate ran, its subject did not.
Tests
crates/perry-codegen/src/stmt/element_shape_loop_tests.rs, 7 new (17 total inthe module, all green). One positive — #7480's kernel reaches the clone, the
clone is call-free, and the accumulate is an
fadd(the two halves assertedtogether, because either alone is inert) — and six sabotage cases: an ambiguous
shape, a shape a field-type tie can break, an optional property, a shape no
literal allocates, a reordered shape, and a read outside the clone that must
stay on the by-name path.
Gates
Run on the pinned mini against this branch (
628db10e3), each command's ownexit code, never a downstream
tail's.lint's extracted commandscargo fmt --all -- --checkcargo check --all-targetscargo test -p perry-codegen --lib --no-fail-fastcargo test -p perry-runtime --lib --no-fail-fastnative_root_coverage--moving-only --seeded-violations 40--statepoints --moving-onlyorigin/main, see belowZero
error[in any build log;Running unittestspresent in each test log.The
--statepointsarm is red onmain, not on this branch--max-unrooted 21reads 22 here. It reads 22 onorigin/main(3beef03) in the same session, on the same host, with the same corpus — and
the 22 reported fingerprints are a byte-identical set between the two arms
(
diffclean). So this change neither adds nor removes a hazard; the budget issimply one over on
maintoday, which is what open PR #7667(
gc/7664-native-lowering-unrooted-hazards) addresses. The seeded controlpassed in both arms (40 planted, 40 caught, 0 missed), so the checker was live
for both readings.
Gap-suite divergences from
test-parity/gap_snapshot.json506 tests: 490 pass, 15 parity_fail, 1 compile_fail. The harness reports 12
divergences from the committed snapshot — 1 regression
(
test_gap_zlib_4917_level: pass -> compile_fail), 10 status changes(
node_fail -> parity_fail) and 1 improvement(
test_gap_iterator_helpers_2874: parity_fail -> pass).None is attributable to this change, and that is measured rather than
argued. Every one of the 17 tests involved was compiled and run under both
arms: Perry's bytes are identical in all 17 (
TESTED=17 SAME=17 DIFFERENT=0).test_gap_zlib_4917_levelis the known host-local zlib link flake: compileddirectly with either arm's binary it exits 0 and prints the right output,
with and without auto-optimize. The failure lives in the harness's own
auto-optimized relink on this host.
node_fail -> parity_failmoves are all oracle-side classification(node still refuses e.g.
test_gap_4510_enum_forward_refwithERR_UNSUPPORTED_TYPESCRIPT_SYNTAX, exactly as its snapshot entry records) —a Perry-independent axis.
Blast radius, measured
The change can only alter code inside an element-shape fast clone, so "which
files gain one" is the blast radius. Compiling with both arms and counting
element_shape.loop.fast.preheadersites:test_gap_*files: zero clones in either arm, zerocompile-status differences — the change is inert there;
test_gap_repsel_*/test_gap_specabi_*files: exactly one filechanges,
test_gap_repsel_element_shape_loop_clone, 2 -> 6 clone sites.That file is the liveness control — a census that reports "no change" over a
corpus with zero clones in both arms proves nothing.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Documentation