perf(repsel): element-shape loop clone serves the element-binding form through function boundaries (#7766) - #7778
Conversation
📝 WalkthroughWalkthroughThe PR enables call-free element-shape cloning for parameter-bound arrays. It changes synthesized ChangesElement-shape parameter binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ForOfLowering
participant ElementShapeLoop
participant PropertyGetHelpers
participant OptReport
ForOfLowering->>ElementShapeLoop: create integer loop counter
ElementShapeLoop->>ElementShapeLoop: verify reachable call-free clone
ElementShapeLoop->>OptReport: record Ptr<Shape> selection
PropertyGetHelpers->>OptReport: record PtrShape field-load consumption
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 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 |
Validation results (criteria 4 + 5, plus the #7780-family hazard check)Protected floors (criterion 5) — pinned quiet mini, interleaved best-of-5, outputs byte-verified vs node before timing
The uniform 1.000 is verified honest, not vacuous: the two arms' binaries differ ( Dependency-JS corpus (criterion 4) — #7152/#7170 methodology, every third
|
| ptr-shape bucket | base | fix |
|---|---|---|
| rule 1 (provenance) | 2606 | 2606 |
| rule 1 — already served by return-shape | 30 | 30 |
| rule 2 (containment) | 42 | 42 |
| rule 5 (module-wide barrier) | 151 | 151 |
| class admission | 6 | 6 |
| selected / consumed | 1 / 56 | 1 / 56 |
Zero per-module differences. Two readings, both load-bearing:
- The dep-JS rule-1 wall does not move — as the PR body predicted from repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's tier census, it is unbound-allocation territory (nested-literal components,
definePropertydescriptors, closure regions) that no read-side mechanism can serve. The first-party half — the case repsel: rule-1 provenance does not survive a function boundary — a typedP[]parameter still reads fields by name (first-party half of #7152/#7170) #7766 was filed on — is what this PR moves (to node parity). The dep-JS half stays with repsel: dependency JS is walled by rule 1 (unbound allocations), not containment — 506 of 746 candidates #7152/repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's own mechanisms. - It is also a 393-real-module no-regression proof for the for-of desugar change: not one ptr-shape outcome shifted anywhere in the corpus.
(Absolute numbers are ~2× #7170's because this selection is 409 modules vs its 197 — same corpus, denser slice.)
The #7780-family hazard (third clone-deletion mode), checked here
PR #7780's session found that the element binding's lexical-death shadow-slot clear is a real js_shadow_slot_set call in the call-fallback shadow mode, which silently deletes a call-free clone. This PR's approach is structurally immune: the fast clone lowers &body[1..], so the binding never enters clear_loop_body_shadow_slots' statement walk — no clear is emitted in any mode. Verified empirically under PERRY_INLINE_SHADOW_SLOT=0 (the call-fallback mode): the deref block still cond_brs INTO the fast clone and the fast blocks contain zero calls, for both the binding and for…of spellings.
Overlap with #7780
#7780 (branch repsel/7771-element-fetch-clone) independently implements the same matcher/fact core. Unique to this PR: the for-of desugar counter fix (Integer(0) — the 9–11× for…of win), the --opt-report select/consume wiring (criterion 1), the #7766 parameter-boundary soundness gap test, and the corpus A/B. Unique to #7780: the emit_shadow_slot_clear defensive skip (moot under this PR's body-slicing but harmless belt-and-braces), and the #7775/#7776 side-find gap tests. The two need reconciling before either merges — maintainer's call which lands first; the other rebases to its unique parts.
…7782) * fix(gc): the seeded schedule arms the poll word, like zeal (#7778) PERRY_GC_SCHEDULE_RATE=1 saw 6 safepoints against zeal's 9,648 loop polls on the same reproduction: nothing kept the poll word armed for the schedule mode, so its loop-safepoint bypass sat behind a gate that never opened. resolve_poll_seed keeps the seed when the schedule is enabled, and ScheduleGuard mirrors ZealGuard's arm/release pair. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * docs: key the poll-word fix's comments to the filed issue #7781 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * docs(changelog): fragment for the poll-word fix Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1451 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * fix(gc): ScheduleGuard's arm bookkeeping must be asymmetric — disarm_poll saturates off()-disarm-then-Drop-rearm leaks +1 permanently when the disarm lands on a zero word (saturation loses the decrement, the paired arm does not). The leak pinned the poll armed for the rest of the test binary: the timing test slowed and the generation-gate contract took a safepoint drain mid-stage, 2/2 consistently. Only set() arms; only its own Drop releases. 3/3 full-suite runs clean after. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
Status: ready for review. The gap suite ( |
7b2267b to
4a33721
Compare
…m through function boundaries (#7766) The versioned clone's matcher admitted only a single-statement accumulator body, so the element-binding spelling — 'const r = ps[i]; s += r.x' and the for…of desugar that emits exactly that shape — kept by-name field reads through a typed parameter array while the direct 'ps[i].x' spelling was already served. Three changes: 1. The matcher admits a leading 'const r = arr[counter]' element binding whose every use is a tracked r.field read; the fast clone never materializes the binding (its Let is skipped — lowering it would emit the element-read tier's calls and silently delete the clone, #7690's shape), and the fact answers for both read spellings. 2. Both for-of desugars mint their counter as Integer(0) instead of Number(0.0) — the literal kind the integer-local collector seeds on. With Number(0.0) the desugared counter never joined integer_locals, never got a canonical i32 slot, and every i32-counter loop optimization silently declined the for…of spelling of loops it served in indexed form. 3. The clone records a Ptr<Shape> selection (and per-read consumption) in --opt-report when — and only when — the deref block cond_brs INTO the fast clone, so a parameter-array loop no longer reads as an unserved rule-1 wall. Probes (dev profile, 200k elements x 200 passes): binding form through a parameter 0.22s -> 0.07s, for…of 0.78s -> 0.07s — both at node parity.
Registers this PR's test_gap_repsel_element_shape_param_binding plus #7774's test_gap_repsel_element_group_numeric, and deletes the latter's registration exclusion: gc_repsel_matrix.sh has its own manifest pre-check that does not consult check_test_registration.py exclusions, so the excluded fixture broke every matrix invocation (exit 3) from the moment #7774 merged. One registry, everything registered; the matrix's arm-level liveness gate (#7255) is the vacuity guard for low-allocation cells. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
4a33721 to
ed6be3c
Compare
![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 `@test-parity/gc_repsel_corpus.txt`:
- Line 749: Update the comment near “Low-allocation read loop” to hyphenate the
compound modifier as “zero-copying minors,” without changing the surrounding
description.
🪄 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: d56a647a-388b-4565-bbb0-ae4cec5b6fe2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CLAUDE.mdCargo.tomlscripts/check_test_registration.pytest-parity/gc_repsel_corpus.txt
💤 Files with no reviewable changes (1)
- scripts/check_test_registration.py
| test_gap_repsel_element_shape_param_binding | ||
|
|
||
| # #7770 (PR #7774, registered by #7778): the element-group numeric-field | ||
| # proof. Low-allocation read loop — its cell can run zero copying minors on |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change zero copying minors to zero-copying minors to fix the grammar warning and clarify the description.
🧰 Tools
🪛 LanguageTool
[grammar] ~749-~749: Use a hyphen to join words.
Context: ...cation read loop — its cell can run zero copying minors on # a filtered arm (the ...
(QB_NEW_EN_HYPHEN)
🤖 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 `@test-parity/gc_repsel_corpus.txt` at line 749, Update the comment near
“Low-allocation read loop” to hyphenate the compound modifier as “zero-copying
minors,” without changing the surrounding description.
Source: Linters/SAST tools
|
Audit complete — merging. This PR raced #7780 (both implemented the element-binding matcher independently); the rebase keeps main's landed matcher and grafts this PR's unique delta, which is the part that actually carries the clone across function boundaries: What survives (and was verified):
Closes #7766. |
…sion sites (#7766 follow-up) #7778 made both for-of desugar sites mint the synthetic __idx_* counter as Expr::Integer(0) — the literal kind collect_integer_let_ids seeds on, and thus the difference between a counter that gets a canonical i32 slot and one that is invisible to every i32-counter loop optimization. Nothing pinned it: the desugar is correct either way and prints identical output, so a revert to Number(0.0) would silently un-optimize every for-of loop with no test failing. Verdict tests on the lowered HIR, one per emission site (module-init and function-body have drifted independently before), both sabotage-verified.
1 participant
Closes #7766.
The verified framing (the issue asked for this first)
The issue's mechanism 1 said the clone "is admitted but its body does not specialize element field reads" and asked that framing to be verified before building on it. Measured on current
main(423bb44):s += ps[i].x + ps[i].ythrough a typedP[]parameter gets a versioned clone whose deref blockcond_brs INTO the fast preheader (the perf(repsel): the element-shape loop clone fires onarr.lengthand aliased element types (#7480) #7701 entry test), with offset loads and no calls in the fast body. Runtime confirms: 0.10s vs node 0.07s on 200k×200 (dev profile). The by-name/guard/coerce calls the issue counted in (A)'s IR belong to the slow clone — mandatory fallback IR, dead at runtime for an honest caller.{ const r = ps[i]; s += r.x + r.y }— andfor (const p of ps), which desugars to exactly that shape — got no clone at all: 2.2× and 7.8× slower than the direct spelling respectively. Two independent causes:Number(0.0), whichcollect_integer_let_idsdoes not seed, so the desugared counter never joinedinteger_locals, never got a canonical i32 slot, and every i32-counter loop optimization silently declined thefor…ofspelling of loops it served in indexed form.Mechanism decision (all three evaluated)
1. Extend the element-shape guarded clone — chosen. The guard establishes the fact at runtime (
js_array_ensure_element_shapeverifies every element's class; the declared type only names the class id to check against), so it is sound for parameters by construction — exactly the "establish, don't assume" bar the issue sets. With the framing corrected, the remaining work was matcher coverage, not read specialization, which makes this by far the smallest correct change (~120 lines, no new ABI, one clone per loop as today).2. Clone-and-route at the call site — needs per-signature callee clones (code size), call-site proof plumbing across modules, and serves only calls whose arguments already carry proofs — which #7170 measured as rare in dependency JS (91.6% of its rule-1 population sits in closures where no proof exists). A runtime guard is still needed for unproven callers, i.e. mechanism 1 is a prerequisite of 2, not an alternative.
3. Entry guard + specialized body — pays the guard on every call even when the loop is cold, duplicates the whole function body instead of one loop, and the guard's O(n) first-scan can be wasted work if the loop is never reached. The loop-preheader placement already implemented is strictly better positioned.
What this PR changes
stmt/element_shape_loop.rs): admits an optional leadingconst r = arr[counter]element binding whose every use is a trackedr.fieldread. The binding's source array participates in the one-array-per-loop rule, and any other use ofr— bare reference, mutable binding, non-counter index, second array — declines (each shape has a red test).Letentirely — lowering it would emit the element-read tier's calls, fail the call-free scan, and silently delete the clone (fix(gc): restore evacuation at precise safepoints — the pacing half of #7682 #7690's failure shape).r.fieldreads are answered by the loop fact (element_binding_localonElementShapeLoopFact); the slow clone keeps the full body, so the side-exit protocol (re-execute the current iteration, nothing committed) is unchanged.lower/stmt_loops.rsmodule-init,lower_decl/body_stmt.rsfunction bodies): the minted counter init isInteger(0)instead ofNumber(0.0)— the literal kind the integer-local collector seeds on, and the same shape a user-writtenlet i = 0produces. (for-in deliberately untouched.)--opt-report: the clone records aPtr<Shape>selection — and per-read consumption — when and only when the deref block branches into the fast clone (an emitted-but-deleted clone records nothing: a gate must assert its subject was live).Acceptance criteria, measured
--opt-reporton the binding case now printsPtr<Shape> 1 selected … 1 of those selections were CONSUMED, naming the binding local; the fast clone emits nojs_object_get_field_by_name_f64, nojs_typed_feedback_class_field_get_guard, nojs_number_coerce(asserted by IR census with the perf(repsel): the element-shape loop clone fires onarr.lengthand aliased element types (#7480) #7701 entered-not-just-emitted test).ptr_shape_elementsroute unchanged; full element_shape suite green — 28 tests).test-files/test_gap_repsel_element_shape_param_binding.tsruns mixed arrays, a subclass with extra fields, same-shape plain literals, a hole, mid-loop mutation, growth between calls, and a fake array-like through the parameter boundary in both spellings — byte-identical to node 26.5.1 (the pinned oracle).Perf (probes, dev profile, 200k elements × 200 passes, local M1)
const r = ps[i]; s += r.x + r.y(param)for (const p of ps) s += p.x + p.y(param)s += ps[i].x + ps[i].y(param, already served)Known-unrelated red
large_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrierfails identically on cleanmain@ 423bb44 (verified in a pristine worktree); the fixture is hand-built HIR with nofor…ofand no element-shape loop, so it is structurally unreachable by this diff. Integration suites don't run per-PR (#5960), which is how it sits red on main.Heads-up
#7771 (
wt-7771, in flight) specializes barea[i]fetches inside the same clone and touches the same three files — whichever lands second will need a small rebase.Summary by CodeRabbit
Performance
Bug Fixes
Tests