perf(codegen): keep the numeric-array specialization when the array is captured (#6369)#6377
Conversation
|
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 (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe codegen preserves module receiver types separately, propagates declared binding types into native fact collection, adjusts boxed and packed-array lowering checks, fixes numeric-key method dispatch, and adds regression tests for captured numeric arrays. ChangesCaptured array specialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleCompiler
participant ClosureCompiler
participant FactCollector
participant LoopLowerer
ModuleCompiler->>ClosureCompiler: pass module_receiver_types
ClosureCompiler->>FactCollector: pass local_types as binding types
FactCollector->>LoopLowerer: provide seeded array facts
LoopLowerer->>LoopLowerer: select packed guarded loop path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
crates/perry-codegen/src/collectors/hir_facts.rs (1)
607-696: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
seed_module_bindingsseeds every module-scope array binding, not just ones this body reads.The doc comment says this seeds "a MODULE-SCOPE binding this body only reads through", but
seed_module_bindingsunconditionally seeds every id inmodule_globals ∩ binding_typesbeforecollect_stmtseven runs — it never checks whetherstmtsactually references that id. Combined withfunction.rs/method.rspassing the entire module's declared-type map asbinding_types(they need the full map for unrelated purposes — type-aware dispatch,hazardous_module_global_ids), every function/method/closure in a module re-seedslocal_kindsfor every top-level typed array in the module, not just the ones it touches. For "bundled code" with many top-levelnumber[]/typed-array consts (the scenario this PR targets), this means O(functions × module-array-bindings) extraHashMapinserts, plus wasted iteration inmark_unknown_call_escape's per-id loop, on every compile.This is not a correctness issue — unused seeded facts just sit inert (no
LocalGet/IndexGetever queries them) — but it's avoidable:collect_array_factsalready hasstmtsin scope before seeding, soseed_module_bindings(or its caller) could intersectmodule_globalswith ids actually referenced instmtsbefore seeding, without touching the separate fulllocal_typesmap thatFnCtxneeds for other purposes.♻️ Sketch of a usage-filtered seed
fn collect_array_facts( stmts: &[Stmt], params: &[perry_hir::Param], module_globals: &HashMap<u32, String>, binding_types: &HashMap<u32, perry_types::Type>, ) -> (ArrayFacts, EffectFacts, MaterializationHazardFacts) { let mut collector = ArrayFactCollector::default(); collector.seed_params(params); - collector.seed_module_bindings(module_globals, binding_types); + let referenced = collect_referenced_local_ids(stmts); // walk stmts, collect Expr::LocalGet ids + collector.seed_module_bindings(module_globals, binding_types, &referenced); collector.collect_stmts(stmts); collector.finish() }🤖 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/collectors/hir_facts.rs` around lines 607 - 696, Restrict module-binding seeding to module-global IDs actually referenced by the current statement body. Update collect_array_facts or ArrayFactCollector::seed_module_bindings to derive the referenced IDs from stmts before calling seed_declared_array_type, while retaining the full binding_types map for lookups and leaving function.rs/method.rs callers unchanged.
🤖 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/tests/issue_6369_captured_array_specialization.rs`:
- Around line 1-198: Add a cargo-test-visible unit test alongside the existing
integration coverage for the captured-array specialization fix. Focus the test
on packed_loop_array_binding_is_eligible and its receiver-type plumbing,
asserting the relevant eligible and ineligible branches or a lightweight IR-text
guard without compiling or spawning the generated binary; retain the existing
end-to-end semantics test for broader validation.
---
Nitpick comments:
In `@crates/perry-codegen/src/collectors/hir_facts.rs`:
- Around line 607-696: Restrict module-binding seeding to module-global IDs
actually referenced by the current statement body. Update collect_array_facts or
ArrayFactCollector::seed_module_bindings to derive the referenced IDs from stmts
before calling seed_declared_array_type, while retaining the full binding_types
map for lookups and leaving function.rs/method.rs callers unchanged.
🪄 Autofix (Beta)
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: b66b7ef7-5acc-47e4-8947-b70378aed3de
📒 Files selected for processing (10)
crates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/expr/literals_vars.rscrates/perry-codegen/src/stmt/loops.rscrates/perry/tests/issue_6369_captured_array_specialization.rs
…s captured (PerryTS#6369) A `number[]` got the guarded packed-numeric access path when it arrived as a parameter, but fell all the way back to the fully generic `js_dyn_index_get` when the same array was reached through a capture — a module-scope `const` read inside a function or an arrow closure. Same declared type, same loop, 27x. That is the shape bundled code is made of (a bundle is overwhelmingly module-scope `const`s captured by closures), so in practice almost nothing was reaching the fast path — a captured *typed* array was no faster than an untyped one, i.e. the annotation bought nothing once the value was captured. The declared type was there all along; it just never reached the read sites. Three seams dropped it, each fixed here: * The closure receiver oracle. `FnCtx.local_types` is what `static_type_of` / `is_array_expr` consult to pick a specialized path. `compile_function` and `compile_method` seed it from the unfiltered `module_global_types`, but closures seeded it from `module_local_types` — a map that two filters (PerryTS#5869 boxed ids, PerryTS#5982/PerryTS#6039 module globals) had already stripped. Both filters exist to protect the typed-ABI *capture representation*, not the receiver type, and PerryTS#6039 had already scoped the second one to a dedicated copy for exactly this reason. Scope the first one the same way and give closures the unfiltered map (`module_receiver_types`); the typed-ABI paths keep the filtered ones untouched. * The packed-loop array facts. PerryTS#6011 seeds a packed-numeric array *kind* for a param from its declared type, on the grounds that the loop's entry guard re-validates the real array and the matched body admits no call / await / closure. A module-scope binding is in exactly that position, so seed it the same way — and let the two packed-loop matchers version on it, instead of rejecting every module global outright. The storage test now mirrors `Expr::LocalGet`'s own precedence (capture slot -> box slot -> alloca -> global), which is what makes this safe from the module-wide boxed union that closures inherit wholesale. * `Expr::LocalGet`'s box arm did not exclude module globals, unlike `LocalSet` and `Update`. A module global's cell holds the VALUE — `Stmt::Let` stores it there directly and never allocates a box — so the deref would reinterpret an array pointer as a box pointer. It stayed latent only because a module global normally has no `ctx.locals` slot for the arm to find; the packed loop's invariant-global read cache installs exactly such a slot, and the read came back as NaN. Best of 5, 2M iterations (Node 1.1ms): | shape | before | after | |--------------------------------|--------|-------| | `number[]` as a parameter | 2.0ms | 2.0ms | | `number[]` captured by a fn | 19.0ms | 2.0ms | | `number[]` captured by a arrow | 55.0ms | 2.0ms | The captured forms now emit the identical guard as the parameter form and zero `js_dyn_index_get` calls. Soundness is unchanged: every specialized path stays a runtime-guarded fast path with the generic fallback intact. A node-vs-perry differential over heterogeneous arrays, holes, out-of-bounds, negative / fractional / non-canonical keys, rebinding, shrink/regrow under the guard, sparse arrays and stores is identical on all 42 assertions, and a captured array now behaves exactly like the same array passed as a parameter on every degraded shape.
Restoring declared local types inside closure bodies made `obj[k]()` on a plain object statically provable as a numeric-key call for the first time. `try_lower_index_get_call` bailed on *any* numeric index, handing the call to the array element lowering, which reads the slot and calls it as a bare closure -- dropping the receiver. `this` came back `undefined`, resurrecting the PerryTS#6328 defect that test_gap_6328_async_index_call guards. The bail is only sound for real arrays, which is what its comment always claimed. Gate it on `is_array_expr` so non-array receivers fall through to `js_native_call_method_value`, which binds `this` for numeric and string keys alike. Array element calls keep the specialized path, so the PerryTS#6369 perf win is unaffected.
proggeramlug
force-pushed
the
fix/6369-captured-array-specialization
branch
from
ec7e725 to
495504b
Compare
|
The What happened. Seeding
Once the key is provably numeric, if is_numeric_expr(ctx, index) && !object_is_class_ref {
return Ok(None); // -> array element lowering
}That hands the call to the array element lowering, which reads the slot and calls it as a bare closure — dropping the receiver. So Repro ( async function thisBinding() {
const obj: any = { tag: "obj", 3: function () { return this.tag; } };
let k = 3;
console.log("thisBinding", obj[k]()); // node: "obj" this PR: "undefined"
}Verified causally by an in-place A/B on one tree — reverting only this PR's The fix. The bail is only sound when the receiver really is an array, which is what its comment always claimed — it just never checked. It now gates on Array element calls still take the specialized path, so the #6369 perf win is untouched — both of this PR's own tests ( |
1 participant
Fixes #6369.
A
number[]got the guarded packed-numeric access path when it arrived as a parameter, but fell all the way back to the fully genericjs_dyn_index_getwhen the same array was reached through a capture — a module-scopeconstread inside a function or an arrow closure. Same declared type, same loop, 27x.That is the shape bundled code is made of (a large esbuild-bundled CLI app is overwhelmingly module-scope
consts captured by closures), so in practice almost nothing was reaching the fast path — a captured typed array was no faster than an untyped one, i.e. the annotation bought nothing once the value was captured.Root cause
The declared type was there all along; it just never reached the read sites. Three seams dropped it:
1. The closure receiver oracle.
FnCtx.local_typesis whatstatic_type_of/is_array_exprconsult to pick a specialized path.compile_functionandcompile_methodseed it from the unfilteredmodule_global_types— butcompile_closureseeded it frommodule_local_types, a map that two filters had already stripped:box_gets.Both hazards are about the typed-ABI capture representation, not about the receiver type — and #6039 had already scoped its filter to a dedicated copy (
typed_abi_local_types) for exactly this reason, leaving a comment explaining thatmodule_local_typesis also the receiver oracle. The boxed filter never got the same treatment. So every captured binding lost its declared type inside a closure, andarr[i]reached the lowering as an unknown receiver.Fixed by scoping the boxed filter the same way #6039 scoped the module-global one: closures now seed
local_typesfrom an unfilteredmodule_receiver_types, and every typed-ABI path keeps the filtered maps untouched.2. The packed-loop array facts. #6011 seeds a packed-numeric array kind for a param purely from its declared type, on the explicit grounds that the loop's entry guard re-validates the actual runtime array and body-observed mutations still downgrade the seed. A module-scope binding sits in exactly that position, so it is seeded the same way — and the two packed-loop matchers now version on it, instead of rejecting every module global outright.
The storage test (
packed_loop_array_binding_is_eligible) mirrorsExpr::LocalGet's own precedence — capture slot → box slot → alloca → module global — which is what makes the module-global arm safe from the module-wide boxed union thatcompile_closureinherits wholesale.3. A latent
LocalGetbug this exposed.Expr::LocalGet's box arm did not exclude module globals, unlikeLocalSetandUpdatewhich both already carry that exclusion. A module global's@perry_global_*cell holds the value —Stmt::Letstores it there directly and never allocates a box — so a box deref there reinterprets e.g. an array pointer as a box pointer. It stayed latent only because a module global normally has noctx.localsslot for the arm to find. The packed loop's invariant-global read cache installs exactly such a slot, and the read came back asNaN.Numbers
Best of 5, 2M iterations. Node: 1.1ms.
number[]as a parameternumber[]captured by a plain functionnumber[]captured by an arrow closureBoth captured forms now emit the identical guard as the parameter form, and zero
js_dyn_index_getcalls (verified viaPERRY_LLVM_KEEP_IR=1; pinned by a test).Soundness
Every specialized path remains a runtime-guarded fast path with the generic fallback intact — the change makes the existing guard reachable from a captured read, it does not assert any new fact. A wrong declared-type seed costs a guard check, never correctness: the entry guard re-validates kind, packed-ness and the whole index window, and side-exits to the generic loop otherwise; the matched loop body admits no call,
awaitor closure, so nothing can rebind the global or reshape the array between the guard and the last iteration.Verified with a node-vs-perry differential covering heterogeneous arrays (
[1,'x',{}]), holes (delete a[i]), out-of-bounds, negative / fractional / non-canonical ("01",NaN) keys, rebinding the captured binding to a different array, shrinking and regrowing it under the guard, sparse arrays, and element stores: identical on all 42 assertions. A captured array also now behaves exactly like the same array passed as a parameter on every degraded shape (string-degraded, object-degraded, shrunk, holed).New regression test
crates/perry/tests/issue_6369_captured_array_specialization.rspins both halves: the IR evidence (specialized guard present,js_dyn_index_getgone) and the spec semantics.Tests
cargo test --release --no-fail-fast -p perry-runtime -p perry-codegen -p perry-hir -- --test-threads=1→ 2213 passed, 4 failed. All 4 (every_dispatch_entry_has_manifest_counterpart,logical_property_assignment_short_circuits_the_store_4586,native_owned_uint8array_get_fallback_uses_uint8array_helper,pod_field_read_after_dynamic_materialization_uses_number_coerce) reproduce identically on a cleanorigin/maintree with the same pass/fail splits — pre-existing, unrelated to this change.Out of scope
origin/main, they do not degrade on capture — a capturedpt.xhot loop runs at 6ms vs 7ms for the same object passed as a parameter, i.e. already at parity, both reachingjs_typed_feedback_class_field_get_guard. Nothing to fix; this PR leaves them unchanged (still 7ms/7ms after).js_closure_get_capture_*call, which the raw-slot fast loop cannot host, so they'd need capture-read hoisting first. They do get the per-element guarded path from fix (1). Module-scope bindings — the bundle-relevant shape, and the one in the issue — get the full packed loop.Summary by CodeRabbit
Bug Fixes
Tests