repsel: admit the element-binding body form into the element-shape versioned loop clone (#7771) - #7780
repsel: admit the element-binding body form into the element-shape versioned loop clone (#7771)#7780proggeramlug wants to merge 4 commits into
Conversation
|
Warning Review limit reached
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 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 (2)
📝 WalkthroughWalkthroughThe element-shape versioned loop clone now supports immutable ChangesElement-binding loop matching
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
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
…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.
cea661a to
86e8dc1
Compare
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-statementacc = arr[j].fieldbody. The reproducer's two-statement body (const r = a[i]; s += r.x + r.y) never got a clone at all — noelement_shape.*block exists in its IR on main.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_coercediamonds 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:
The binding is virtual inside the fast clone: its
Letemits nothing, and everyr.fieldread lowers through the existingElementShapeLoopFactto a bare element load + residual per-element check under the preheader guard. Three small mechanical pieces:ElementShapeLoopFact.element_binding+ aLocalGetarm inelement_shape_loop_fact_for_property_get(the single entry point all three consumers already share, whose docs anticipated this widening).lower_letfor the active fact's binding — lowering the genericIndexGetthere 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).emit_shadow_slot_clearfor the virtual binding's lexical-death clear: in the call-fallback shadow mode that clear is a realjs_shadow_slot_setcall — 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: avarbinding is function-scoped and observable after the loop, where the skippedLetwould leave the slot stale. The slow clone still bindsrgenerically, and the side exit re-runs the current iteration there before any effect commits.Acceptance criteria
js_array_get_f64fora[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.--opt-reportstill showslocal r -> Ptr<Shape>(collector untouched).test-files/test_gap_7771_*.ts, kept as permanent gap tests): hole viadelete(throws like node), element set toundefined(throws like node), Array subclass behind aP[]binding (brand guard declines → 100), shrunk length (25), growth far past inline capacity (10^10), mid-looplengthwrite (matcher declines → 9),letbinding (declines → 100), escaping binding (declines → 100 + side effect), proxy-wrapped array (brand declines → 100, own file — see below).assert_fast_clone_is_enteredon both new positive IR tests, plus four decline-shape tests (mutable binding, bare-value escape, untracked field, second array). Sabotage-verified: disabling theLetskip turns both positives red.PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800on 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.Other validation
cargo test -p perry-codegen(lib + all integration suites): only failure islarge_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.cargo fmt --check,check_file_size.sh,addr_class_inventory.pyclean. Rebased onto currentmain.Pre-existing bugs found and filed while building the hazard battery (both A/B-proven identical on pristine main)
new Proxy(arr, {})makes an unrelated function's indexed read loop run zero iterations #7775 — an uncalled function containingnew Proxy(arr, {})makes an unrelated function's indexed read loop run zero iterations (module-wide miscompile; why the proxy gap test lives in its own file).as any—s += r.x + r.yyields NaN where node string-concatenates #7776 — heterogeneous element viaas any:s += r.x + r.yyieldsNaNwhere node string-concatenates (slow/generic path; this PR's guard correctly declines that array, and its slow clone reproduces main's behavior exactly).No version bump per contributor convention; changelog fragment follows as
changelog.d/<PR>-element-binding-clone.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests