Skip to content

repsel: admit the element-binding body form into the element-shape versioned loop clone (#7771) - #7780

Open
proggeramlug wants to merge 4 commits into
mainfrom
repsel/7771-element-fetch-clone
Open

repsel: admit the element-binding body form into the element-shape versioned loop clone (#7771)#7780
proggeramlug wants to merge 4 commits into
mainfrom
repsel/7771-element-fetch-clone

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes #7771.

Re-scoping finding (the issue's "first thing to establish")

The issue's premise — "the guarded-clone machinery is already entered here" — is false for the reproducer, in exactly the way the issue told us to check for:

  • lower_element_shape_versioned_for's matcher only admitted the single-statement acc = arr[j].field body. The reproducer's two-statement body (const r = a[i]; s += r.x + r.y) never got a clone at all — no element_shape.* block exists in its IR on main.
  • The measured per-iteration js_array_get_f64 (v0.5.1448) is already gone at runtime on current main: the fetch lowers via the bounded-index path, and fix(deforest): the caller's binding kept a growth-forwarding stub after a deforested call (#7661) #7751's producer-side store-back repairs the binding after growth, so the inline fast arm runs (timed: 50M fetches in 0.09s user vs 2.65s with the slow arm forced).

What remained of the issue's substance: the call was still emitted with a per-iteration 3-load guard diamond + hole-select, two js_number_coerce diamonds on the field reads, and a per-iteration safepoint poll — and the engine plan's third leg (element fetch under the invariant guard) was unimplemented for the shape real read loops are written in.

The fix (upstream, as the issue prescribes)

Widen the versioned-loop matcher to admit the element-binding body form:

for (let i = 0; i < a.length; i++) { const r = a[i]; s += r.x + r.y; }

The binding is virtual inside the fast clone: its Let emits nothing, and every r.field read lowers through the existing ElementShapeLoopFact to a bare element load + residual per-element check under the preheader guard. Three small mechanical pieces:

  • ElementShapeLoopFact.element_binding + a LocalGet arm in element_shape_loop_fact_for_property_get (the single entry point all three consumers already share, whose docs anticipated this widening).
  • A skip at the top of lower_let for the active fact's binding — lowering the generic IndexGet there would put a runtime-call diamond inside the clone and delete it via the call-free admission scan (fix(gc): restore evacuation at precise safepoints — the pacing half of #7682 #7690's lesson).
  • A skip in emit_shadow_slot_clear for the virtual binding's lexical-death clear: in the call-fallback shadow mode that clear is a real js_shadow_slot_set call — the clone's only call — and would silently delete the clone in that mode. Skipping can only over-root; the slow clone (lowered after the fact is popped) keeps its clear.

const-only by design: a var binding is function-scoped and observable after the loop, where the skipped Let would leave the slot stale. The slow clone still binds r generically, and the side exit re-runs the current iteration there before any effect commits.

Acceptance criteria

  1. Zero js_array_get_f64 for a[i] in the read loop — the fast clone contains no calls at all (the coerce diamonds and the poll are gone too); the generic fetch survives only in the slow clone, as it must.
  2. --opt-report still shows local r -> Ptr<Shape> (collector untouched).
  3. Guards hold, verified against node 26.5.1 byte-for-byte (test-files/test_gap_7771_*.ts, kept as permanent gap tests): hole via delete (throws like node), element set to undefined (throws like node), Array subclass behind a P[] binding (brand guard declines → 100), shrunk length (25), growth far past inline capacity (10^10), mid-loop length write (matcher declines → 9), let binding (declines → 100), escaping binding (declines → 100 + side effect), proxy-wrapped array (brand declines → 100, own file — see below).
  4. Clone entered, not merely emitted: assert_fast_clone_is_entered on both new positive IR tests, plus four decline-shape tests (mutable binding, bare-value escape, untracked field, second array). Sabotage-verified: disabling the Let skip turns both positives red.
  5. GC zeal clean: PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 on the reproducer → correct output, exit 0, [gc-zeal] forced_collections=1762 copying_minors=1762 moved_objects=400014 — the array moved 1762 times under the guard and the preheader's post-call re-derivation held.
  6. Pinned quiet mini, interleaved best-of-5, output byte-verified before timing (100k-element build + 500 read passes = 50M fetches): base 0.07–0.08s user → 0.05s user, stable across all five rounds. Existing floors exercise the single-statement shape, whose lowering is byte-asserted unchanged by the pre-existing IR tests.

Other validation

  • Full cargo test -p perry-codegen (lib + all integration suites): only failure is large_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrier, which fails identically on pristine main @ 423bb44 (known red baseline). All 44 element/clone tests green, grouped and solo.
  • 12 existing gap tests over adjacent shapes (array subclass declared-base, class-ref numeric arrays, GC rooting family, async index calls, closures) byte-identical vs node with this branch's compiler.
  • cargo fmt --check, check_file_size.sh, addr_class_inventory.py clean. Rebased onto current main.

Pre-existing bugs found and filed while building the hazard battery (both A/B-proven identical on pristine main)

No version bump per contributor convention; changelog fragment follows as changelog.d/<PR>-element-binding-clone.md.

Summary by CodeRabbit

  • New Features

    • Optimized read loops that assign array elements to a local binding before accessing their fields.
    • Preserved correct behavior for array holes, undefined elements, subclasses, proxies, large arrays, and changing lengths.
    • Automatically falls back to the general execution path when optimization is unsafe.
  • Bug Fixes

    • Prevented incorrect handling of escaped, mutable, or unsupported element bindings.
  • Tests

    • Added regression coverage for optimized and fallback loop behavior.

proggeramlug pushed a commit that referenced this pull request Aug 10, 2026
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ebbb0dec-c7cc-4d3a-a9ea-f1d48e4f7ac1

📥 Commits

Reviewing files that changed from the base of the PR and between cea661a and 86e8dc1.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • CLAUDE.md
  • Cargo.toml
📝 Walkthrough

Walkthrough

The element-shape versioned loop clone now supports immutable const r = arr[counter] bindings. Fast clones virtualize the binding and lower r.field through guarded element loads. Slow clones retain generic binding behavior. Tests cover accepted and declined loop forms.

Changes

Element-binding loop matching

Layer / File(s) Summary
Element-binding loop matching and purity analysis
crates/perry-codegen/src/stmt/element_shape_loop.rs
The matcher admits supported immutable element bindings, tracks their fields through arithmetic and math expressions, and rejects mutable, escaping, aliased, conflicting, or cross-array usage.
Virtual binding and guarded property loads
crates/perry-codegen/src/expr/mod.rs, crates/perry-codegen/src/expr/property_get/helpers.rs, crates/perry-codegen/src/expr/shadow_slot.rs, crates/perry-codegen/src/stmt/let_stmt.rs
Loop facts record the binding. Fast-clone lowering omits its initializer and shadow-slot clear. Field reads use guarded indexed loads.
Code-generation and runtime regression coverage
crates/perry-codegen/src/stmt/element_shape_loop_tests.rs, test-files/test_gap_7771_element_binding_read_loop.ts, test-files/test_gap_7771_proxy_array_read_loop.ts, changelog.d/7780-element-binding-clone.md
Tests verify fast-clone entry, call-free field loads, slow-clone generic loads, declined forms, array edge cases, proxy arrays, and documented validation results.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LoopMatcher
  participant FastClone
  participant GuardedFieldLoad
  participant SlowClone
  LoopMatcher->>FastClone: admit const r = arr[counter]
  FastClone->>GuardedFieldLoad: load r.field from tracked array element
  GuardedFieldLoad-->>FastClone: return field value
  LoopMatcher->>SlowClone: retain generic binding lowering
  SlowClone-->>SlowClone: execute generic element fetch
Loading

Possibly related issues

Possibly related PRs

  • PerryTS/perry#7612 — This PR extends the same element-shape loop cloning with virtualized element bindings.
  • PerryTS/perry#7660 — Both modify element-shape loop matching and clone handling.
  • PerryTS/perry#7669 — Both modify loop facts and specialized property-read lowering.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: admitting element-binding loops into the element-shape versioned loop clone.
Description check ✅ Passed The description provides detailed scope, implementation changes, linked issue, acceptance criteria, validation results, and known baseline failures.
Linked Issues check ✅ Passed The changes address the linked issue by specializing element-binding fetches, preserving guards and GC safety, and verifying clone entry and output correctness.
Out of Scope Changes check ✅ Passed The code, tests, and changelog fragment are directly related to the linked issue and stated objectives; no unrelated changes are evident.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch repsel/7771-element-fetch-clone

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Ralph Küpper added 4 commits August 10, 2026 18:07
…hape versioned loop clone (#7771)

The matcher for lower_element_shape_versioned_for only admitted a
single-statement accumulator body, so the shape real read loops are
written in — const r = a[i]; s += r.x + r.y — never got a clone: the
element fetch stayed on the generic bounded-index lowering with its
per-iteration guard diamond and js_array_get_f64 slow arm.

The binding is virtual inside the fast clone: its Let emits nothing,
every r.field read lowers through the fact to a bare element load under
the preheader guard, and the lexical-death shadow-slot clear is
suppressed (in the call-fallback shadow mode that clear is a runtime
call, which would fail the call-free admission scan and silently delete
the clone, #7690). The slow clone still binds r generically.

const-only by design: a var binding is function-scoped and observable
after the loop, where the skipped Let would leave the slot stale.
@proggeramlug
proggeramlug force-pushed the repsel/7771-element-fetch-clone branch from cea661a to 86e8dc1 Compare August 10, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repsel: a proven element fetch is still a runtime call — specialize a[i] inside the element-shape guarded clone

1 participant