perf(codegen): packed-loop read admissions — if-conditions, clone accumulator proofs, hazard relaxation - #9084
Conversation
…umulator proofs, hazard relaxation Follow-up to PerryTS#9060/PerryTS#9063/PerryTS#9070: the packed-loop admission residuals behind the compare-only and reduce shapes. 1. If-conditions were invisible to the stable-packed matcher (PerryTS#9060's documented residual): stmt_flags / leading_read_requires_numeric matched only Let/Expr/Throw/Return, so `if (arr[i] < 0) count++` never admitted — and later reads inside If branches were invisible to the replay-safety check. Both now descend; `Compare` joins `Binary` as a numeric-consumption context (a wrong hint fails the require_numeric guard into the generic loop, never a wrong answer). 2. Numeric accumulators for the plain packed clones (versioned + range): `s += a[i]` inside a packed fast clone lowered its `+` through js_dynamic_string_or_number_add on EVERY iteration plus two root barriers — the by-construction collector runs before clone facts exist, so `s` had no proof. The packed fast preheaders now run PerryTS#9060's collect_numeric_accumulators with one Number tag test each (a non-Number accumulator takes the slow clone before anything ran), and the ids ride PackedF64LoopFact.numeric_accumulators, consulted by is_numeric_expr — the same mechanism and kill switch as the stable clone. 3. Guarded reads are numeric inside clones: has_numeric_index_fact and the boxed-fallback hazard predicate now recognize packed versioned/range facts and masked-window facts — the clone's read either produces a genuine raw double or side-exits BEFORE the value is consumed, so there is no boxed edge. This turns `if (a[i] < 0)` into a bare fcmp (was js_rel_lt per iteration) and feeds the accumulator walk. 4. Versioned-loop READ bodies take the store arm's relaxed eligibility: a call-free read body cannot invalidate what the entry guard re-proves, so the whole-function materialization hazard (tripped by the very `new Array(n).fill()` construction calls that build these buffers) no longer blocks versioning — locally-built arrays version at all. Same argument, word for word, as the existing store-arm comment; the two invalidation tests that pinned the read-side conservatism now pin the versioned-behind-guard contract their store twin already used. A stable-tier literal-bound arm was built and WITHDRAWN: plain-array literal bounds already version through the range loop, and the arm re-claimed five-field object-write bodies that nested_same_shape_object_writes deliberately keeps outside any clone. Isolated (dev box; node 26.5 in parens): count loop `if (a[i]<0) c++` 4.58 -> 1.60 ns/el (0.62) literal reduce `i<8192, s+=a[i]` 4.32 -> 4.16 (1.01) len-bound reduce (local array) 5.30 -> 4.14 (0.99) The count loop's residual is the per-iteration length IC, which PerryTS#9070's hoist removes at merge. The reduce rows are now call-free (census: fadd plus the loop poll only) and latency-bound on the accumulator's GC-root slot — true parity there needs unboxed accumulator slots in clones, scoped as the follow-on. Nine-probe differential vs node byte-identical, incl. a string accumulator (tag test -> slow clone -> concat), a literal bound past the array length (guard fail -> undefined += NaN), holey/mixed arrays, and an accumulator reassigned to a string mid-loop through a branch. perry-codegen suites 1823/0.
4c3f0a3 to
7f9d7c7
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughPacked-loop versioning now admits additional read bodies, conditional numeric reads, and numeric accumulator reductions. Fast clones receive numeric proofs, while slow clones retain fallback behavior. Regression tests verify guarded fast and slow clone generation. ChangesPacked loop admissions
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The optimization now admits more conditional packed loops, but a branch-local array mutation could potentially leave a later repeated read using stale data in affected loop shapes. The change is otherwise mergeable with explicit owner awareness and a focused regression test or follow-up fix for cache invalidation. Sequence Diagram(s)sequenceDiagram
participant LoopMatcher
participant FastPreheader
participant PackedFastClone
participant PackedSlowClone
participant NumericAnalysis
LoopMatcher->>FastPreheader: analyze reads, calls, and accumulator candidates
FastPreheader->>NumericAnalysis: validate accumulator locals as Number
FastPreheader->>PackedFastClone: pass admitted numeric facts
FastPreheader->>PackedSlowClone: branch when runtime proof fails
PackedFastClone->>NumericAnalysis: lower guarded reads and numeric additions
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, concrete change list, related issue references, benchmark results, and correctness validation. It does not reproduce the template headings or checklist, but the required information is mostly present. Full details: Docstring CoverageExplanation Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
Full host gate on |
|
Merged. I spent the most time on relaxation #5, since "same argument, word for word, as the existing store-arm comment" is a claim that has to survive the asymmetry between a store body and a read body — a store body's safety argument leans on "in-bounds stores only, cannot grow the array", which a read body doesn't need but also doesn't provide. The argument does carry: what the entry guard re-proves is the receiver and layout, and a call-free body cannot reach anything that would invalidate either, whether it stores or only reads. The Relaxation #4's soundness rests on the side-exit ordering ("produces a genuine raw double or side-exits BEFORE the value is consumed"), and the accumulator admission is the right shape for it: one Number tag test per accumulator in the fast preheader, branching to the slow preheader before the first fast iteration, so the induction base case is established while nothing has run yet. I tested that induction where it would break — an accumulator that isn't a Number, or stops being one:
28 cases, byte-identical to node v26.5.1 on both arms — so no correctness delta, which is what a perf PR should show. Then I checked the optimization is actually live rather than merely harmless, because "nothing broke" is equally consistent with "nothing happened". On a fixture matching the targets (local
2.2× and 3.5×, moving the branchy loop from 7× node to 2× node. The IR agrees: My first attempt at this measurement used a module-level array and moved the IR by one instruction, which would have let me report "engaged" off noise — worth mentioning in case anyone reuses the fixture. Validation: codegen 1347 passed, |
1 participant
Follow-up to #9060/#9063/#9070 — the remaining packed-loop admission residuals (the "compare-only" and "literal-bound" shapes documented in #9060, plus the general accumulator/read-consumption gaps behind them).
What (four admission fixes, one clone-proof extension)
If-conditions were invisible to the stable-packed matcher (perf(codegen): reduce accumulators earn the packed fast clone's numeric proof — s += arr[i] at node parity (was 5.3×) #9060's documented residual, root-caused there):
stmt_flags/leading_read_requires_numericmatched only Let/Expr/Throw/Return statements, soif (arr[i] < 0) count++never admitted — and worse, later reads inside If branches were invisible to the replay-safety check. Both now descend (condition + both branches for flags; condition for the leading numeric walk).ComparejoinsBinaryas a numeric-consumption context — a wrong hint is a failedrequire_numericguard → generic loop, never a wrong answer.Literal-bound loops (perf(codegen): reduce accumulators earn the packed fast clone's numeric proof — s += arr[i] at node parity (was 5.3×) #9060's other residual) turned out to already be served for plain arrays by the range-versioned loop —
for (i = 0; i < 8192; i++) s += a[i]versions throughpacked_f64_rangeand now gains the accumulator treatment below. A stable-tier literal-bound arm was built and then WITHDRAWN: it re-claimed five-field object-write bodies thatnested_same_shape_object_writes_version_one_through_four_fieldsdeliberately keeps outside any clone (tier-boundary creep the pinned counts caught). If subclass receivers ever measure a literal-bound gap, that arm needs a body-shape restriction first.Numeric accumulators for the plain packed clones (versioned + range):
s += a[i]inside a packed fast clone lowered its+throughjs_dynamic_string_or_number_addon EVERY iteration, plus two root barriers — the by-construction numeric collector runs before clone facts exist, soshad no proof. The packed fast preheaders now run perf(codegen): reduce accumulators earn the packed fast clone's numeric proof — s += arr[i] at node parity (was 5.3×) #9060'scollect_numeric_accumulators+ one Number tag test each (a non-Number accumulator takes the slow clone before anything ran), and the ids ridePackedF64LoopFact.numeric_accumulators, consulted byis_numeric_expr— same mechanism, same kill switch as the stable clone.Guarded reads are numeric inside clones:
has_numeric_index_fact(and the boxed-fallback hazard predicate) now recognize packed-versioned/range facts and masked-window facts — the clone's read either produces a genuine raw double or side-exits BEFORE the value is consumed, so there is no boxed edge. This is what turnsif (a[i] < 0)into a barefcmp(wasjs_rel_ltper iteration) and feeds the accumulator walk's leaf.Versioned-loop read bodies take the store arm's relaxed eligibility: a call-free read body cannot invalidate what the entry guard re-proves, so the whole-function materialization hazard (tripped by the very
new Array(n).fill()calls that build these buffers) no longer blocks versioning — locally-built arrays now version at all. Same argument, word for word, as the existing store-arm comment.Numbers (isolated, dev box; node 26.5 in parens)
if (a[i] < 0) c++count loopi < 8192, s += a[i]i < a.lengthreduce (local array)The count loop's residual is the per-iteration
a.lengthIC — #9070's length hoist removes it at merge. The reduce rows' residual is NOT calls (the clones are call-free now — census: fadd + poll only): the accumulator lives in a GC-root slot, and the store-to-load-forward + fadd dependency chain is the floor (~15 cycles/el; node keepssin a register). True reduce parity needs unboxed accumulator slots in clones (plain F64 alloca + boxed write-back at every exit) — scoped as the follow-on mechanism, not attempted here.Correctness
Nine-probe differential vs node, byte-identical: counting with negatives/-0/NaN, literal-bound over exact-length and TOO-SHORT arrays (guard fails → slow loop →
undefined+= NaN semantics), string accumulator (tag test → slow clone → concat), mixed-type array, holey array,both-branch If accumulators, and an accumulator reassigned to a string MID-LOOP through a branch.Summary by CodeRabbit
Performance
Bug Fixes