perf(codegen): outline class-field-SET guard-miss arm to one call (#5334 lever A)#5336
Closed
proggeramlug wants to merge 17 commits into
Closed
perf(codegen): outline class-field-SET guard-miss arm to one call (#5334 lever A)#5336proggeramlug wants to merge 17 commits into
proggeramlug wants to merge 17 commits into
Conversation
added 17 commits
June 16, 2026 17:28
…d write-scan shadow compute_closure_captures rebuilt an O(scope) membership set per closure (O(n^2) for N closures in an N-binding scope). Maintain a live id_set on Locals (insert/remove/reindex) and pass it by reference; share the fn_ctor_env write-scan Shadow instead of cloning per nested fn. cap_12000 check: 13.06s -> 0.07s. Captures/mutable-captures semantics unchanged (param/inner-decl/dayjs same-id filtering preserved).
…tics by name Several per-call/per-member HIR resolution helpers did linear scans over `Vec` registries. For a program with K classes / native bindings and M call+member expressions, every lookup walked the whole registry — including the common miss case (receiver not registered) which scanned to the end and returned `None`. That is O(M*K), quadratic on large bundles, stalling check-lower. `lookup_class` was already indexed (classes_index, #5267). This indexes the remaining Vec-scanned lookups, mirroring the proven imported_functions_index pattern, while preserving identical Option/tuple results: - native_instances (scope-stack-like: pushed on scope entry, truncated on exit): name -> Vec<usize> shadow stack, innermost (last) on top. lookup reads the top index (== old `.rev().find()` last-match-wins). New `truncate_native_instances(mark)` pops indices >= mark off each name's stack (and the two prior direct `.truncate()` sites now call it), so an inner binding stops shadowing the moment its scope pops — same shadowing as before. - module_native_instances (module-level, push-only): name -> usize, overwritten on each push (last-match-wins, matching the reverse-scan fallback arm). - func_return_native_instances + native_modules + class_statics (push-only): name -> usize keeping the FIRST entry (`or_insert`), matching the old forward `.iter().find()` first-match-wins. has_static_method/has_static_field and lookup_native_module/lookup_func_return_native_instance now O(1). Push sites for module_native_instances / func_return_native_instances routed through new register helpers so the index stays in sync. Micro-bench (20000 x3 miss lookups vs K-sized registry, release): K=2000 baseline 82ms -> fixed 0.53ms K=8000 baseline 335ms -> fixed 0.51ms K=16000 baseline 1033ms-> fixed 0.54ms (~1900x at K=16000; flat in K) Adds unit tests for native-instance shadowing+truncation and module-level last-wins, plus an #[ignore] perf gate. Builds on the closure-capture perf fix.
A 13 MB minified ESM bundle (a commander-based CLI) made `perry check` stall in the HIR `check-lower` stage forever (>1500 s, never finishing). Instrumenting `lower_expr` (env-gated `PERRY_TRACE_RELOWER`, counting lowerings per source span) showed a single ~360-byte commander builder chain — `K.name(..).description(..).argument(..).helpOption(..) .option(..).addOption(..)…` — whose receiver subtrees were lowered EXPONENTIALLY: span counts of 37M / 18.5M / 9.2M / 4.6M / 2.3M, halving once per nesting level (a clean 2^depth signature). Root cause: the chained-native-method dispatch helper `try_static_method_and_instance` (expr_call/static_and_instance.rs). `may_lower_to_native_method_call` over-approximates to `true` whenever the chain root is a native instance/module ident (here `K`, tagged commander via `new Command()`), so the helper SPECULATIVELY lowers the whole receiver prefix to inspect whether it produced a `NativeMethodCall` of a recognized fluent module. When the inner call instead lowers to a generic `Call` (or the outer method isn't one of the recognized fluent methods — `hook`/`helpOption`/`addOption`…), every fluent arm misses, the lowered receiver is discarded, and the helper returns `Err(args)`. The `lower_call_inner` fall-through tail then RE-lowers the same member callee (and thus the whole prefix) via `lower_member_inner`. Two full recursive descents into the prefix per chain level ⇒ 2^depth work. Fix: lower each receiver exactly once. When the helper lowers `member.obj` and no fluent arm consumes it, stash it in `LoweringContext::prelowered_member_receiver` keyed by the receiver's source span; `lower_member_inner` (the tail's receiver-lowering site) takes it back when re-lowering the same span instead of redoing the work. The memo is single-shot and span-keyed, any member lowering clears a stale entry, and `lower_call_inner` resets it as a safety net — so it can never leak onto a different receiver. Reuse is semantics-preserving: lowering a receiver is idempotent in the value it produces, and the fluent-success arms already reuse that very `object_expr`. Results: - Real bundle: `perry check /tmp/cli.ts` >1500 s (never finishes) → 11.9 s, prints "All checks passed! - 2 file(s) checked". - Minimal synthetic (commander chain mixing recognized/unrecognized methods), before: N=12 0.07s, N=14 0.52s, N=16 4.0s, N=18 16.3s, N≥20 >30 s timeout (exponential). After: N=20 0.01s, N=500 0.5s — the exponential re-lowering is gone (no span lowered more than ~once; `PERRY_TRACE_RELOWER` never trips its 5M-call dump even at N=2000). - `cargo test -p perry-hir --tests`: 323 passed, 0 failures (excluding the 4 pre-existing machine-specific debug-build stack-overflow tests test_lower_rejects_deep_* / nested_object_literal_lowers_in_linear_time, confirmed identical on HEAD). The `PERRY_TRACE_RELOWER` counter is left in place, fully env-gated and zero-cost when unset, as a standing diagnostic for future lowering perf work.
JS ignores surplus arguments to argless methods ("x".trim(1) is legal
and returns the trimmed string). perry's codegen bail!'d with "takes no
args, got N" for String.{toLowerCase,toUpperCase,trim,trimStart,trimEnd,
isWellFormed,toWellFormed} and Array.{pop,shift}, rejecting valid JS.
Drop the arg-count bail in all 5 sites; evaluate extra args for their
side effects (ECMA-262 evaluates arguments before the call) then discard,
matching the existing Annex B HTML-wrapper convention in the same file.
Clears the first codegen wall on the claude-code cli.js bundle.
Adds tests/argless_builtin_extra_args.rs covering "x".trim(1) and
[1].pop(99).
…rops dispatch loop
A user break/continue inside a try/catch within a loop body of an
async function* compiled to a bare Stmt::Continue with no enclosing
loop, crashing codegen with 'continue statement outside any loop'
(e.g. claude-code cli.js, the query main loop: while(k){ try{...for
await...}catch{...continue} }).
Root cause: rewrite_break_continue_in_stmts lowers a user break/continue
into [LocalSet(state, SENTINEL), Stmt::Continue], where the trailing
Stmt::Continue re-enters the state-machine dispatch while(true) loop.
For a real async function* (was_plain_async = false), the catch handler
is inlined verbatim into the separate __async_throw closure via
build_async_catch_route_body. That closure has NO dispatch loop, so:
1. the dispatch Stmt::Continue became a continue with no target, and
2. its CONTINUE_SENTINEL state number was never fixed up, because
fix_break_continue_sentinels only walks the linearized states, not
the extracted CatchRoute bodies.
Fix:
1. linearize.rs: after linearizing a while/for body, also run
fix_break_continue_sentinels on the CatchRoutes captured during
that body (new fix_break_continue_sentinels_in_catches) so the
resume state in an inlined catch points at the loop's real
cond/update/after-loop state.
2. lower.rs: in build_async_catch_route_body (async path), convert the
now-dangling dispatch Stmt::Continue/Break re-entry into a suspend
return { value: undefined, done: false } — correct async-generator
semantics: the next .next() dispatches at the resume state already
set by the preceding LocalSet.
Verified: minimal repro (async function* with while+try/catch+continue)
and a continue-in-try-body loop both compile through codegen and run
correctly (yields 1,3,5 skipping evens via continue). The claude-code
cli.js bundle now compiles PAST this error.
Note: a separate PRE-EXISTING limitation remains — synchronous/awaited
throws inside try in an async generator are not caught at runtime (fails
identically on HEAD without any loop/continue); out of scope here.
… generator no longer drops the loop target
A `break <label>` / `continue <label>` that targets an OUTER labeled loop
from inside a NESTED loop (or switch) within an `async function*` crashed
codegen with `labeled break '<label>' outside any loop` (e.g. claude-code
cli.js, a minified `q: while(...){ for(...){ try{...}catch{ break q } } }`).
Root cause: linearize.rs's labeled-loop arm rewrote `break <label>` /
`continue <label>` to the loop's targets only at the loop's own body level
(via the old `rewrite_labeled_bc_in_stmts`, which STOPPED at nested loops /
switch). A labeled break from a nested loop therefore survived as a bare
`Stmt::LabeledBreak("q")`. The nested loop's `rewrite_break_continue_in_stmts`
ignores LabeledBreak/LabeledContinue, so when that nested loop's body (or its
extracted async `.throw()` catch route) was lowered into a closure that has NO
loop, codegen hit a labeled break with no label target.
The plain single-sentinel scheme (BREAK_SENTINEL / CONTINUE_SENTINEL) can't
express "break the OUTER loop from inside an inner loop", because the inner
loop's own `fix_break_continue_sentinels` would claim those and route them to
the WRONG (inner) targets.
Fix — per-label dispatch sentinels:
1. break_continue.rs: add LABEL_BREAK/CONTINUE_SENTINEL_BASE and
`rewrite_labeled_break_continue_in_vec`, which DESCENDS into nested loops /
switch / try-catch and rewrites `break <label>` / `continue <label>` for a
given label index into a per-label `[LocalSet(state, label_sentinel),
Stmt::Continue]` dispatch re-entry. Add `fix_label_sentinels_*` to resolve
a specific label's sentinels (in states, the trailing `current` buffer, and
extracted async catch routes) to that loop's real target states.
2. break_continue.rs: teach `rewrite_break_continue_in_stmts` to SKIP a
synthesized `[LocalSet(state, <sentinel>), Continue]` dispatch re-entry so
an enclosing loop's rewrite pass doesn't re-wrap the dispatch `Continue` as
a user `continue`.
3. linearize.rs: the labeled-loop arm allocates a per-label index, runs the
descending rewrite, and hands the index to the directly-wrapped For/While
arm (thread-local PENDING_LABEL_INDEX). After computing its target states,
that arm fixes the label's sentinels: `break <label>` -> after-loop state,
`continue <label>` -> update-state (for) / cond-state (while). Indices are
reset per generator function. The catch-route path is covered: the label
sentinel is resolved before `build_async_catch_route_body` runs, and the
trailing dispatch `Continue` is then neutralized into a suspend-return by
the existing async catch-route inliner.
Verified: minimal repros (labeled break/continue from a nested loop, incl.
inside try/catch, in an async function*) compile and RUN with output identical
to Node — e.g. `q: for{ for{ continue q; break q } yield }` yields
0,1,2,3,10,11,12,13,20,30,31; nested `outer:/inner:` labels yield 0,1,10,20.
New unit tests in break_continue.rs cover the rewrite + fixup + dispatch-skip.
Note: a thrown error inside `try` in an async generator is still not caught at
runtime — a SEPARATE pre-existing limitation that reproduces identically with a
plain (non-labeled) break on HEAD; out of scope here.
A labeled non-loop statement other than a block — `a: if (...) { ... break a; ... }`
(emitted by minified bignumber.js / ajv), a labeled switch, etc. — had no break
target in codegen: only labeled blocks were desugared to a run-once
`a: do { ... } while(false)`, so `break a` from a labeled if/switch (incl. from
inside a nested loop in its body) hit 'labeled break outside any loop'. Generalize
the do-while(false) desugar to all labeled non-loop statements in both the
function-body and module-level lowering paths; labeled loops still bind the label
to the loop so `continue a` works. Verified: output matches Node for break-from-
nested-for-out-of-labeled-if.
…t sanitize alike
Distinct source class names can sanitize() to the same symbol (e.g. `$X` and
`_X` both -> `_X`; minified bundles use $/_ heavily), so two different classes
emitted `@perry_class_keys_<prefix>__<sanitized>` twice and clang rejected the IR
('redefinition of global'). The per-name dedup guard only checked the real class
name, not the sanitized symbol. Track every emitted keys-global name and
disambiguate collisions with a numeric suffix, in both the local-class and
imported-stub generation loops. The real-name-keyed class_keys_globals_map stores
the unique name, so every `new ClassName()` site still resolves correctly.
Verified: `class $X{}` + `class _X{}` compiles and runs matching Node (distinct shapes).
…ster) Every `new C()` site inlined ~50 lines of bump-allocator IR (load arena state, bump offset, fast/slow/merge, write GC+object headers, zero-fill slots) — all per-class compile-time constants, identical across sites. On a 13MB minified bundle this is a dominant source of codegen bloat (millions of IR lines). Replace the per-site inline bump with a single call to the existing runtime `js_object_alloc_class_inline_keys`, which performs the identical alloc + header init + slot zero-fill. Default on; opt back into the inline path with PERRY_INLINE_NEW=1. Measured (8M-allocation loop, -O2): inline 7030ms -> outline 5832ms (~17% FASTER), and -45 IR lines per new-site. The inline bump was a pessimization at scale — it bloated the hot loop, hurting icache/regalloc/LLVM-opt more than the saved call. perry-codegen suite green on the default (outline) path; output matches Node for fields, inheritance (super), and arrays of instances.
…each new-site
The inlined constructor body (field-init stores etc.) was the dominant per-new-site
IR after the allocator (~136 IR lines/site on a class with super+fields). Default to
CALLING the already-emitted standalone <Class>_constructor symbol instead, emitting
the ctor body once. Opt back into inlining with PERRY_INLINE_CTOR=1.
Restricted to classes with their OWN constructor AND an emitted standalone symbol:
no-own-ctor subclasses (class C extends B {}) stay on the inline path (the symbol-call
path doesn't reproduce the inline leaf-keys/shape setup); without the symbol the call
would be a no-op. Classes with super(...)/rest params round-trip correctly.
Measured win-win vs inlining (8M construct-heavy loop, new P(i,i+1) with this.x/this.y):
inline 5609ms -> call 2251ms (~2.5x FASTER), and ~136 fewer IR lines per new-site —
the inlined ctor bloated the hot loop. perry-codegen suite green on the default (call)
path; output matches the inline baseline (incl. the unrelated pre-existing no-own-ctor
by-name-read quirk, identical on both paths) for super, rest params, and arrays.
…nitize alike)
scoped_fn_name mangled `perry_fn_<mod>__<sanitize(name)>` with the non-injective
`sanitize` (every non-[A-Za-z0-9_] char -> `_`), so distinct minified function
names like `$Z5` and `_Z5` both became `perry_fn_<mod>___Z5` and clang rejected the
module with 'invalid redefinition of function'. Same root cause as the class-keys
global collision, for function symbols.
Use the injective `sanitize_member` (already used by scoped_static_method_name),
which is byte-identical for plain names and escapes others as `u_…_<hex>_…`. All
local-function references resolve through the id-keyed `func_names` map (which is
built from scoped_fn_name), so def and refs stay consistent. Updated the two
pod-layout specialization tests that hardcoded the old `$`->`_` mangling
(`layout$Tiny` -> `u_layout_24_Tiny`). perry-codegen suite green; `function $Z5(){}` +
`function _Z5(){}` now compiles and runs (was clang redefinition).
Minified code reuses short names (`function A`) across scopes, and perry lambda-
lifts nested functions to module level, so two distinct module functions can share
a name — clang then rejected the duplicate `define perry_fn_<mod>__A` with 'invalid
redefinition of function'. The per-name dedup that scoped_fn_name relies on assumed
unique names.
Disambiguate colliding function symbols with a numeric suffix in the id-keyed
func_names map (every reference resolves through it, so def/refs stay consistent).
Exported functions reserve their canonical scoped_fn_name first (referenced cross-
module by name, unique per module) and never get suffixed. Verified: two distinct
`function A(){}` in separate scopes now compile and run (1 2); perry-codegen suite green.
…ss id
Minified bundles reuse short class names (`class j`) across distinct scopes.
perry mangled instance-method / getter / setter symbols as
`perry_method_<mod>__<class>__<method>` with no class id, so two distinct
classes both named `j` produced the SAME symbol
(`perry_method_cli_js__j__getElementsByTagName`) and clang rejected the 13MB
bundle with `invalid redefinition of function`.
Fix (codegen-only):
- scoped_method_name now takes (class_id, disambiguate). When `disambiguate`
it emits the `__c<id>__` infix, mirroring scoped_static_method_name;
otherwise it keeps the historical id-less form. The id-less form is the ABI
a cross-module consumer reconstructs from import metadata, so it must stay
the default for EXPORTED (always name-unique) classes.
- duplicate_class_names(&hir.classes) computes the names borne by >1 class in
the module; only those classes get the disambiguating infix. The set is
threaded identically to the definition+dispatch-registry sites (mod.rs) and
the runtime VTABLE_REGISTRY registration (string_pool.rs) so the registered
func ptr always matches the emitted body.
- compile_method derives each body's symbol from the registry but retargets the
`__c<id>__` segment to THIS class's own id (retarget_class_id_in_symbol), so
two same-name distinct-id classes that both reach codegen (artifacts iterates
hir.classes) emit DISTINCT symbols instead of redefining one. No-op for
id-less symbols (unique classes) and for the constructor (no `__c<id>__`).
Cross-module references and the constructor keep their existing mangling, so
exported-class dispatch and `new` across modules are unchanged (verified: a
cross-module import of a class with a method + getter links and runs correctly,
which a naive unconditional-id scheme regressed via the unreliable
source_class_id=0 fallback).
Tests: static_symbol_hygiene gains duplicate_class_instance_methods_use_class_id
_in_symbols and updates the unique-name case to assert the id-less form;
retarget_class_id_in_symbol gets focused unit tests. perry-codegen --tests green.
KNOWN LIMITATION (not addressed here): two same-named classes in separate
scopes (e.g. `function mk1(){class j{...}} function mk2(){class j{...}}`) are
still COLLAPSED into one class during HIR lowering (lower_class_decl reuses the
class id by name via lookup_class; push_class_dedup drops the second), so they
share one definition and `new j()` dispatches name-keyed to the first. This
yields `1 1` where Node yields `1 2`. Making both survive with distinct ids
would require id-based (not name-based) receiver dispatch — a large rework
deliberately left out to avoid a half-fix that miscompiles. The symbol-
uniqueness fix above clears the COMPILE-time redefinition for any path that
does deliver two same-name distinct-id classes to codegen.
Minified bundles can contain two DISTINCT classes with the same name (`class j` in separate scopes) whose methods mangle to the same `perry_method_<mod>__j__<m>` symbol via multiple emission paths, so clang rejected the module with 'invalid redefinition of function'. perry's class identity is name-keyed, so every reference already resolves to ONE symbol; emit each symbol's body once (first wins) at the IR-render loop. Covers all emission paths at once. No-op for unique-named symbols (the common case). Dispatch for two same-named classes resolves to the first (pre-existing name-identity limitation), but the module compiles instead of being rejected. Verified: dup-class-name methods compile + run; perry-codegen suite green.
…E_FIELDSET (opt-in)
Replaces the ~18-line inline class-field-set diamond (guard call + raw/boxed
fast slot store + GC barrier + by-name fallback + merge) emitted at every
`obj.field = v` site on a typed class field with ONE
`call @js_class_field_set_ic(...)` — a REAL extern call (not alwaysinline), so
clang's pre-LTO IR genuinely shrinks. Targets the single biggest IR contributor
(~365K set guards + ~238K fast blocks) blocking the 13MB→1.25GB-IR clang OOM.
Helper: crates/perry-runtime/src/typed_feedback/guards.rs::js_class_field_set_ic
(L627) reproduces the inline path EXACTLY: runs the same
js_typed_feedback_class_field_set_guard; on PASS does the same store (raw-f64 →
bare `store double` into header+24 slot; boxed → js_object_set_field's slot
write + layout note + write barrier); on FAIL records the fallback and routes
through js_object_set_field_by_name with full receiver bits + masked key.
Frozen/accessor/non-writable/setter-in-chain all stay behind the guard.
Codegen: property_set.rs L336 emits the single call under the gate, keeping the
inline diamond when off (A/B on one binary). Declared in runtime_decls/objects.rs;
re-exported from typed_feedback.rs.
MEASURED (PERRY_LLVM_KEEP_IR, NO_CACHE, NO_AUTO_OPTIMIZE; per-site = 101-site vs
1-site IR delta, non-foldable sites):
IR lines/site raw-f64: 75 -> 11 (saved 64)
boxed: 41 -> 21 (saved 20)
SPEED (-O2 default, best-of-N interleaved, 30M-iter 2-raw-f64-site hot loop):
inline (off): min 2620ms / median 2690ms
outlined (on): min 2640ms / median 2730ms -> ~0.8-1.5% SLOWER, reproducibly
CORRECTNESS: outlined output identical to inline AND to node across plain /
inherited / raw-f64 / accessor fields (999500 hot-loop, accessor *2, 3.5 raw).
Tests: perry-codegen green; perry-runtime 1044/1044 green single-threaded (the 2
dynamic_props GC-mark tests fail only under parallel exec — pre-existing global-
GC-state race, pass in isolation; unrelated to this change).
DECISION: NOT defaulted. In the pessimal raw-f64 hot loop the inline bare
`store double` consistently beats the outlined call (~1%), so this fails the
strict no-slower gate. Kept as opt-in PERRY_OUTLINE_FIELDSET for the IR-bloat
escape hatch (huge IR win where clang OOMs). Follow-up direction: outline only
guard+fallback and keep the raw-f64 fast store inline to capture most of the IR
win with zero hot-loop cost.
…ine (opt-in, supersedes full-outline) Replaces the opt-in full-outline (`PERRY_OUTLINE_FIELDSET`, commit e1e5de7) with the proper inline-cache architecture: the HOT fast path stays INLINE (cheap shape compare + the byte-identical bare slot store); only the COLD miss/fallback path collapses to ONE call. This is perf-neutral by construction, which the full-outline was not (~1% slower because it outlined the hot store too). Codegen — crates/perry-codegen/src/expr/property_set.rs (lower, ~L375): %ok = inline shape compare (emit_class_field_inline_precheck) REPLACING the guard CALL → br %fast, %slow %fast: the SAME inline store as before (raw-f64: bare `store double`; boxed: slot store + write barrier) — UNCHANGED, byte-identical %slow: one `call void @js_class_field_set_slow(...)` → br %merge The inline compare is CONSERVATIVE: it falls to %slow whenever the sticky inline-enable flag is set (descriptors / typed-feedback in use). BOXED fields additionally require a compile-time proof (boxed_field_inline_safe) that no ancestor class declares a getter/setter for the key — the inline compare does not model class_setter_in_chain and the runtime flag does not cover class-vtable accessors; any uncertainty keeps the guard call. raw-f64 fields are always inline-eligible (typed-shape descriptor + intact/finite/not-frozen checks cover their fast contract). Slow helper — crates/perry-runtime/src/typed_feedback/guards.rs::js_class_field_set_slow (renamed from js_class_field_set_ic; same body): runs the real js_typed_feedback_class_field_set_guard (feedback recording, descriptor/accessor/ frozen handling, raw-f64 contract); on PASS does the same slot store; on FAIL records the fallback + routes by name. Reproduces today's semantics for every case the inline compare doesn't take %fast on (first-time shapes, typed-feedback enabled, etc.). Declared in runtime_decls/objects.rs; re-exported from typed_feedback.rs. MEASURED (PERRY_LLVM_KEEP_IR, NO_CACHE; per-site = 101-site vs 1-site delta): SPEED (-O2, 30M-iter `p.x=i; p.y=i+1` hot loop, best-of-5 interleaved): inline ON 17.74s inline OFF (orig diamond) 18.08s → FLAT (within noise). Proof the hot path is identical: the `class_field_set.fast` block is byte-identical on vs off (`inttoptr → gep i8 +24 → gep double → store double → br merge`). IR lines/site (straight-line, non-loop sites): raw-f64 26 (orig) → 71 (inline) boxed 30 → 69 The inline scheme GROWS per-site IR here, because the inline shape precheck is emitted at every site (it only collapses to a bare store after LICM hoists the compare out of a hot loop). The IR-shrink goal that motivated #5247 is therefore NOT met for straight-line code. DECISION: perf flat + correct, but IR does NOT drop → kept OPT-IN (PERRY_INLINE_FIELDSET=1). Default (unset / =0) keeps the original guard-call diamond. Not defaulted; flagged for review. Correctness: inline ON matches inline OFF on plain / raw-f64 / inherited-setter / accessor (defineProperty) / frozen, and both match Node on plain/raw/inherited/ accessor. Frozen write throws identically in both modes (pre-existing perry strict default, value uncorrupted) — not a regression. cargo test -p perry-codegen -p perry-runtime green (runtime single-threaded to avoid known parallel-GC flakes).
lever A) The DEFAULT (guard-CALL) class-field-set diamond's cold %slow arm emitted TWO inline calls per set site — `js_typed_feedback_record_fallback_call` followed by `js_object_set_field_by_name`. Since the inline guard has already run and FAILED in the entry block (that failure is what branched here), nothing is left to decide: collapse the pair into a single outlined `js_class_field_set_fallback(site_id, obj_bits, key_raw, value)` call that records the miss and routes the write by name. Distinct from `js_class_field_set_slow` (the opt-in inline-precheck path's helper), which RE-RUNS the guard — this one must not, or it would double-record. Semantics are byte-identical to the old two-call block. Perf-neutral by construction: the hot `class_field_set.fast` store is untouched, and the change is confined to the cold guard-miss arm, which never executes on a monomorphic hot path. IR shrinks by one call per class-field-set site (verified: slow arm 2 calls -> 1; full codegen suite green, 108 tests). First step of the IR-efficiency roadmap in #5334 (Tier 1, lever A: outline cold IC machinery). Establishes the outline-helper pattern reused by the larger levers (C nan-box round-trips, D non-pointer barrier elision, B adaptive full-outline).
📝 WalkthroughWalkthroughThis PR addresses LLVM symbol collisions for minified/duplicate class names by introducing class-id–disambiguated method symbol mangling, adds a gated class-field SET inline-cache path with new runtime slow/fallback helpers, outlines ChangesCodegen: LLVM Symbol Hygiene, Class-field SET IC, new/ctor Outlining, Argless Builtins
HIR Lowering: O(1) Registry Indices, Receiver Memoization, Closure-capture Refactor, Labeled Non-loop Desugar
Generator: Labeled break/continue, Per-label Sentinels, Async Catch-route Fix
Sequence DiagramssequenceDiagram
participant Codegen as compile_module
participant Helpers as helpers.rs
participant StringPool as emit_string_pool
participant LlModule as LlModule::to_ir
Codegen->>Helpers: duplicate_class_names(hir.classes)
Helpers-->>Codegen: dup_class_names: HashSet
Codegen->>StringPool: emit_string_pool(..., &dup_class_names)
StringPool->>Helpers: scoped_method_name(prefix, class_id, name, method, disambiguate)
Helpers-->>StringPool: symbol string
Codegen->>Helpers: scoped_method_name(..., c.id, ..., disambiguate)
Helpers-->>Codegen: method/getter/setter symbol
Codegen->>LlModule: functions with resolved symbols
LlModule->>LlModule: skip duplicate func.name on emit
sequenceDiagram
participant lower_new
participant Runtime as js_object_alloc_class_inline_keys
participant Ctor as *_constructor symbol
participant InlineBump as inline bump-allocator IR
lower_new->>lower_new: check PERRY_INLINE_NEW
alt PERRY_INLINE_NEW unset
lower_new->>Runtime: call js_object_alloc_class_inline_keys(cid, parent_cid, field_count, keys_ptr)
Runtime-->>lower_new: allocated object
else PERRY_INLINE_NEW=1
lower_new->>InlineBump: emit inline allocation IR
end
lower_new->>lower_new: check PERRY_INLINE_CTOR + own ctor exists
alt PERRY_INLINE_CTOR unset and own ctor present
lower_new->>Ctor: call *_constructor symbol
else PERRY_INLINE_CTOR=1
lower_new->>lower_new: inline constructor body
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
crates/perry-codegen/tests/argless_builtin_extra_args.rs (1)
94-129: ⚡ Quick winStrengthen these regressions to assert extra-arg evaluation, not only compilation.
Both tests currently pass literal extras, so they would still pass if a future change accidentally drops lowering of extra-arg expressions. Add one case with an observable extra expression and assert it is emitted before the argless builtin call.
♻️ Proposed test addition
@@ #[test] fn string_trim_with_extra_arg_compiles() { @@ } + +#[test] +fn string_trim_with_extra_arg_evaluates_extra_expression() { + let extra = Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::String("x".to_string())), + property: "toString".to_string(), + }), + args: vec![], + type_args: Vec::new(), + byte_offset: 0, + }; + let stmt = Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::String(" x ".to_string())), + property: "trim".to_string(), + }), + args: vec![extra], + type_args: Vec::new(), + byte_offset: 0, + }); + let ir = String::from_utf8(compile_module(&module_with_init(vec![stmt]), empty_opts()).unwrap()).unwrap(); + let extra_pos = ir.find("js_jsvalue_to_string").expect("extra arg should be lowered"); + let trim_pos = ir.find("js_string_trim").expect("trim call should be lowered"); + assert!(extra_pos < trim_pos, "extra args must be evaluated before trim"); +}🤖 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/tests/argless_builtin_extra_args.rs` around lines 94 - 129, The current tests for string_trim_with_extra_arg_compiles and array_pop_with_extra_arg_compiles only verify compilation with literal extra arguments, which would still pass if extra-arg evaluation were accidentally dropped in the future. Strengthen these tests by modifying at least one of them to use an observable extra expression instead of a literal (such as a function call like Expr::Call). Then add an assertion that checks the emitted IR contains evidence that this extra expression was actually evaluated before the argless builtin call, ensuring the test would catch regressions where extra-arg lowering is dropped.
🤖 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-codegen/src/codegen/helpers.rs`:
- Around line 111-119: The scoped_fn_name function uses sanitize_member which is
not injective and creates collisions: escaped names like "$Z5" produce
"u__24_Z5" while a plain function named "u__24_Z5" stays the same, creating
duplicate perry_fn_* definitions. Fix sanitize_member to be collision-free by
either reserving the "u_" namespace for escaped names (transforming or rejecting
plain names that start with "u_") or implementing a length/tagged encoding
scheme. Additionally, add a regression test that verifies the collision between
escaped "$Z5" and plain "u__24_Z5" is prevented.
In `@crates/perry-codegen/src/codegen/method.rs`:
- Around line 26-38: The retarget_class_id_in_symbol function blindly replaces
the first occurrence of the __cN__ pattern without distinguishing between
patterns that are part of the class-name component versus the actual class-id
segment. This causes incorrect retargeting when class names contain __cN__
patterns (e.g., A__c12__B). Either refactor this function to accept structured
inputs like the class name and ID separately instead of parsing the full symbol
string, or implement a more sophisticated parser that correctly identifies and
targets only the __c<digits>__ pattern representing the actual class-id segment
(which appears after the class-name component) rather than any arbitrary
occurrence within the symbol string. Add tests that cover symbols with
__c<digits>__ patterns embedded in the class-name portion.
In `@crates/perry-codegen/src/codegen/mod.rs`:
- Around line 145-151: The duplicate_class_names function incorrectly includes
nested and hoisted classes alongside exported classes, causing exported classes
to receive id-suffixed method symbols when they share names with non-exported
classes. Modify the logic to track duplicates on a per-class-id basis and
exclude exported classes from receiving the id suffix in mangled method symbols.
Exported classes should always use the id-less perry_method_*__ClassName__m form
to match what importers declare, while only nested non-exported classes sharing
names should get the id-suffixed disambiguation. This fix should also be applied
to the related code sections referenced in the comment that handle method symbol
generation.
- Around line 1924-1947: The function name deduplication logic only checks
exported function names without comparing function IDs, which causes collisions
when multiple functions share the same name (e.g., exported foo and non-exported
foo from lambda lifting) or when export aliases conflict with non-exported
functions. First, build a set of reserved symbols from all exported function
names regardless of function ID. Then, modify the is_exported check to compare
the actual function ID (f.id) with the func_id in the exported_functions tuple
entries, not just match by name. This ensures only the function whose ID matches
the export is treated as the canonical exported version, while other same-named
functions are deduplicated with unique suffixes that avoid the reserved exported
symbol names.
In `@crates/perry-codegen/src/expr/property_set.rs`:
- Around line 596-600: The fact label passed to
record_lowered_value_with_access_mode_and_facts is hardcoded as
"js_class_field_set_slow", but the helper being emitted above varies based on
the inline_fieldset condition. When inline_fieldset is false, the code emits
js_class_field_set_fallback, so the fact label should also reflect this. Make
the fact label conditional: use "js_class_field_set_fallback" when
inline_fieldset is false, and "js_class_field_set_slow" when inline_fieldset is
true. This ensures the recorded fact label matches the actual emitted helper.
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 825-861: The current code uses
std::env::var_os("PERRY_INLINE_NEW").is_none() to check if inlining should be
enabled, but this only checks for the variable's presence, not its actual value.
This means setting PERRY_INLINE_NEW=0 is treated the same as setting
PERRY_INLINE_NEW=1, causing the outline path to be skipped when it should be
used. Instead of checking if var_os() is_none(), parse the actual value of the
PERRY_INLINE_NEW environment variable and check if it equals "1" (or use a
similar explicit value check) to determine whether to take the outlined or
inline path. Apply this same fix to the similar PERRY_INLINE_CTOR environment
variable check in the 1111-1128 range as mentioned in the comment.
- Around line 1125-1130: The return value from call_local_constructor_symbol is
being discarded and the function returns obj_box instead. This skips any
constructor return-override logic. Capture the DOUBLE value returned from the
call_local_constructor_symbol function call in the branch checking
ctx.class_stack.iter().any(|active| active == class_name) ||
ctor_alias_collision || force_ctor_call, and return that value instead of
obj_box to ensure constructor return overrides are properly applied.
In `@crates/perry-hir/src/lower_decl/body_stmt.rs`:
- Around line 464-471: The issue is that the matches! check for body_is_loop
only examines the immediate body of labeled_stmt without accounting for nested
labels. When you have nested labeled statements like `outer: inner: while
(...)`, the outer label's body is `Stmt::Labeled` not the actual loop, causing
the check to incorrectly classify it as a non-loop and desugar it with the `do {
... } while(false)` wrapper. Fix this by peeling/unwrapping any nested
`Stmt::Labeled` wrappers from labeled_stmt.body before the matches! check to
expose the actual underlying statement and correctly identify if it is a loop
statement.
In `@crates/perry-hir/src/lower/expr_function.rs`:
- Around line 409-413: The `drain_from` method in the `Locals` struct
unconditionally removes each drained entry's LocalId from the `id_set`, but
LocalIds are reused across function scopes. When an inner scope declares a
binding with the same LocalId as an outer scope's binding, removing the id
entirely from `id_set` corrupts the membership set that
`compute_closure_captures` depends on for closure capture analysis. Fix
`drain_from` to either check whether each id being removed still appears in the
remaining entries before removing it from `id_set`, or rebuild `id_set` from
scratch based on the entries that remain after draining. Additionally, add test
coverage for the scenario where nested functions reuse the same LocalId across
outer and inner scopes.
In `@crates/perry-hir/src/lower/lower_expr.rs`:
- Around line 523-546: The dump function is mixing metrics from different
scopes: TOTAL is a global counter while SPANS is thread-local, causing the ratio
calculation and top spans to be misleading in multi-threaded scenarios. Move
SPANS from thread-local storage to a global synchronized map (such as a
Mutex-protected HashMap or DashMap) so that both TOTAL and SPANS aggregation use
the same unified scope. Update the SPANS accesses throughout the function to use
the global synchronized version instead of the thread-local pattern with
SPANS.with().
In `@crates/perry-hir/src/lower/stmt.rs`:
- Around line 1275-1288: Before classifying labeled_stmt.body as a non-loop in
the body_is_loop check, unwrap any stacked labels by recursively checking if the
immediate body is another labeled statement and continuing to traverse inward
until reaching the actual statement. Only after unwrapping all stacked labels
should you determine if the final statement is a loop using the matches!
pattern. This ensures that constructs like outer: inner: while(...) are
correctly identified as loops, preventing the synthetic do-while wrapper from
being incorrectly applied and preserving the correct continue target semantics.
In `@crates/perry-transform/src/generator/lower.rs`:
- Around line 1639-1670: The issue is that when rewriting Stmt::Continue and
Stmt::Break to returns with { done: false } in the
rewrite_dispatch_continue_to_suspend function, this happens before the done_id
flag is reset to false elsewhere in the code, causing the __gen_done flag to
remain true while the returned result has done: false, creating a liveness
inconsistency. Additionally, the function skips nested containers like loops,
switch, labeled blocks, and closures using the wildcard pattern at lines
1668-1670, which leaves dangling dispatch continue statements in inlined catch
and finally bodies. Fix this by ensuring the timing of done_id reset is properly
coordinated with the dispatch continue rewriting, and extend the traversal logic
to recursively handle all nested statement containers (such as Stmt::Loop,
Stmt::Switch, Stmt::Labeled, and closures) instead of skipping them, so that
break/continue statements within catch and finally handlers are properly
rewritten.
---
Nitpick comments:
In `@crates/perry-codegen/tests/argless_builtin_extra_args.rs`:
- Around line 94-129: The current tests for string_trim_with_extra_arg_compiles
and array_pop_with_extra_arg_compiles only verify compilation with literal extra
arguments, which would still pass if extra-arg evaluation were accidentally
dropped in the future. Strengthen these tests by modifying at least one of them
to use an observable extra expression instead of a literal (such as a function
call like Expr::Call). Then add an assertion that checks the emitted IR contains
evidence that this extra expression was actually evaluated before the argless
builtin call, ensuring the test would catch regressions where extra-arg lowering
is dropped.
🪄 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: 27b3a158-3166-4956-bbce-f74eaa71061c
📒 Files selected for processing (37)
crates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/string_pool.rscrates/perry-codegen/src/expr/property_set.rscrates/perry-codegen/src/lower_array_method.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/module.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/tests/argless_builtin_extra_args.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/static_symbol_hygiene.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/expr_call/static_and_instance.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower/fn_ctor_env.rscrates/perry-hir/src/lower/locals.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower/tests.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/body_stmt/nested_fn_decl.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry-runtime/src/typed_feedback.rscrates/perry-runtime/src/typed_feedback/guards.rscrates/perry-transform/src/generator/break_continue.rscrates/perry-transform/src/generator/linearize.rscrates/perry-transform/src/generator/lower.rscrates/perry-transform/src/generator/mod.rs
Comment on lines
111
to
+119
| pub(super) fn scoped_fn_name(module_prefix: &str, hir_name: &str) -> String { | ||
| format!("perry_fn_{}__{}", module_prefix, sanitize(hir_name)) | ||
| // Use the INJECTIVE sanitizer (same as scoped_static_method_name): plain | ||
| // `sanitize` maps every non-`[A-Za-z0-9_]` char to `_`, so distinct minified | ||
| // function names like `$Z5` and `_Z5` both became `perry_fn_<mod>___Z5` and | ||
| // clang rejected the module with "invalid redefinition of function". `func_names` | ||
| // is keyed by func id and every reference resolves through it, so changing the | ||
| // mangling here keeps all local-function call sites consistent. Byte-identical | ||
| // to `sanitize` for plain `[A-Za-z0-9_]` names (the overwhelming common case). | ||
| format!("perry_fn_{}__{}", module_prefix, sanitize_member(hir_name)) |
There was a problem hiding this comment.
Make the member sanitizer collision-free before using it for function symbols.
sanitize_member is not actually injective: "$Z5" escapes to u__24_Z5, while a plain function named "u__24_Z5" stays u__24_Z5. That can still emit duplicate perry_fn_* definitions. Reserve the u_ namespace for escaped names, or switch to a length/tagged encoding, and add a regression for the plain-vs-escaped collision.
Possible localized fix
pub(super) fn sanitize_member(name: &str) -> String {
let is_plain = name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_');
- if is_plain {
+ if is_plain && !name.starts_with("u_") {
// Byte-identical to `sanitize` for plain names (incl. leading-digit fix).
return sanitize(name);
}🤖 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/codegen/helpers.rs` around lines 111 - 119, The
scoped_fn_name function uses sanitize_member which is not injective and creates
collisions: escaped names like "$Z5" produce "u__24_Z5" while a plain function
named "u__24_Z5" stays the same, creating duplicate perry_fn_* definitions. Fix
sanitize_member to be collision-free by either reserving the "u_" namespace for
escaped names (transforming or rejecting plain names that start with "u_") or
implementing a length/tagged encoding scheme. Additionally, add a regression
test that verifies the collision between escaped "$Z5" and plain "u__24_Z5" is
prevented.
Comment on lines
+26
to
+38
| fn retarget_class_id_in_symbol(symbol: &str, class_id: u32) -> String { | ||
| // Find `__c` followed by one or more ASCII digits followed by `__`. | ||
| let bytes = symbol.as_bytes(); | ||
| let mut i = 0usize; | ||
| while i + 3 < bytes.len() { | ||
| if &bytes[i..i + 3] == b"__c" { | ||
| let mut j = i + 3; | ||
| while j < bytes.len() && bytes[j].is_ascii_digit() { | ||
| j += 1; | ||
| } | ||
| // Require at least one digit AND a closing `__`. | ||
| if j > i + 3 && j + 1 < bytes.len() && &bytes[j..j + 2] == b"__" { | ||
| return format!("{}__c{}{}", &symbol[..i], class_id, &symbol[j..]); |
There was a problem hiding this comment.
Do not retarget the first __cN__ blindly.
A class name like A__c12__B produces perry_method_m__A__c12__B__c5__m; this helper rewrites the class-name segment and leaves the real class-id segment unchanged. That emits a symbol no reference site can reconstruct. Please retarget from structured inputs instead of parsing the symbol string, or add a parser/test that covers __c<digits>__ inside the class-name component.
🤖 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/codegen/method.rs` around lines 26 - 38, The
retarget_class_id_in_symbol function blindly replaces the first occurrence of
the __cN__ pattern without distinguishing between patterns that are part of the
class-name component versus the actual class-id segment. This causes incorrect
retargeting when class names contain __cN__ patterns (e.g., A__c12__B). Either
refactor this function to accept structured inputs like the class name and ID
separately instead of parsing the full symbol string, or implement a more
sophisticated parser that correctly identifies and targets only the
__c<digits>__ pattern representing the actual class-id segment (which appears
after the class-name component) rather than any arbitrary occurrence within the
symbol string. Add tests that cover symbols with __c<digits>__ patterns embedded
in the class-name portion.
Comment on lines
+145
to
+151
| // Names borne by more than one class in this module (the minified-bundle | ||
| // `class j` reused across scopes). Methods of these classes are mangled with | ||
| // a disambiguating class-id infix so two distinct classes can't collide into | ||
| // one `perry_method_…` symbol (clang `invalid redefinition of function`). | ||
| // Every other class — including all exported / cross-module classes, which | ||
| // are name-unique — keeps the id-less, cross-module-stable symbol. | ||
| let dup_class_names = duplicate_class_names(&hir.classes); |
There was a problem hiding this comment.
Keep exported class method symbols id-less even when a nested class has the same name.
duplicate_class_names(&hir.classes) includes hoisted/nested classes, so an exported Foo plus a nested non-exported Foo makes the source emit perry_method_*__Foo__c<id>__m. Importers always declare the id-less perry_method_*__Foo__m here, causing cross-module undefined symbols. Track disambiguation per class id and exclude the exported class from the id-suffixed form.
Also applies to: 1672-1677, 1794-1806
🤖 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/codegen/mod.rs` around lines 145 - 151, The
duplicate_class_names function incorrectly includes nested and hoisted classes
alongside exported classes, causing exported classes to receive id-suffixed
method symbols when they share names with non-exported classes. Modify the logic
to track duplicates on a per-class-id basis and exclude exported classes from
receiving the id suffix in mangled method symbols. Exported classes should
always use the id-less perry_method_*__ClassName__m form to match what importers
declare, while only nested non-exported classes sharing names should get the
id-suffixed disambiguation. This fix should also be applied to the related code
sections referenced in the comment that handle method symbol generation.
Comment on lines
+1924
to
+1947
| let mut used_fn_symbols: HashMap<String, u32> = HashMap::new(); | ||
| for f in &hir.functions { | ||
| if hir.exported_functions.iter().any(|(exp, _)| exp == &f.name) { | ||
| used_fn_symbols | ||
| .entry(scoped_fn_name(&module_prefix, &f.name)) | ||
| .or_insert(1); | ||
| } | ||
| } | ||
| for f in &hir.functions { | ||
| func_names.insert(f.id, scoped_fn_name(&module_prefix, &f.name)); | ||
| let base = scoped_fn_name(&module_prefix, &f.name); | ||
| let is_exported = hir.exported_functions.iter().any(|(exp, _)| exp == &f.name); | ||
| let sym = if is_exported { | ||
| base | ||
| } else { | ||
| let n = used_fn_symbols.entry(base.clone()).or_insert(0); | ||
| let s = if *n == 0 { | ||
| base.clone() | ||
| } else { | ||
| format!("{base}__dup{n}") | ||
| }; | ||
| *n += 1; | ||
| s | ||
| }; | ||
| func_names.insert(f.id, sym); |
There was a problem hiding this comment.
Use function ids, and reserve alias export symbols, when deduplicating function names.
This checks exports by name only. If one foo is exported and another lambda-lifted/non-exported foo exists, both are assigned the canonical perry_fn_*__foo. Similarly, export { foo as bar } can collide with a non-exported function named bar before the alias wrapper is emitted. Build reserved symbols from exported names, but treat a function as canonical-exported only when the export’s func_id == f.id.
Possible direction
- for f in &hir.functions {
- if hir.exported_functions.iter().any(|(exp, _)| exp == &f.name) {
- used_fn_symbols
- .entry(scoped_fn_name(&module_prefix, &f.name))
- .or_insert(1);
- }
+ for (exported_name, _) in &hir.exported_functions {
+ used_fn_symbols
+ .entry(scoped_fn_name(&module_prefix, exported_name))
+ .or_insert(1);
}
for f in &hir.functions {
let base = scoped_fn_name(&module_prefix, &f.name);
- let is_exported = hir.exported_functions.iter().any(|(exp, _)| exp == &f.name);
+ let is_exported = hir
+ .exported_functions
+ .iter()
+ .any(|(exp, id)| *id == f.id && exp == &f.name);🤖 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/codegen/mod.rs` around lines 1924 - 1947, The
function name deduplication logic only checks exported function names without
comparing function IDs, which causes collisions when multiple functions share
the same name (e.g., exported foo and non-exported foo from lambda lifting) or
when export aliases conflict with non-exported functions. First, build a set of
reserved symbols from all exported function names regardless of function ID.
Then, modify the is_exported check to compare the actual function ID (f.id) with
the func_id in the exported_functions tuple entries, not just match by name.
This ensures only the function whose ID matches the export is treated as the
canonical exported version, while other same-named functions are deduplicated
with unique suffixes that avoid the reserved exported symbol names.
Comment on lines
596
to
600
| ctx.record_lowered_value_with_access_mode_and_facts( | ||
| "ClassFieldSet", | ||
| None, | ||
| "js_object_set_field_by_name", | ||
| "js_class_field_set_slow", | ||
| &fallback, |
There was a problem hiding this comment.
Keep the fallback fact label aligned with the emitted helper.
Line 599 always records "js_class_field_set_slow", but the default path above emits js_class_field_set_fallback when inline_fieldset is false. This will mislabel raw-f64 fallback diagnostics/facts for the default guard-miss path.
Proposed fix
+ let fallback_helper = if inline_fieldset {
+ "js_class_field_set_slow"
+ } else {
+ "js_class_field_set_fallback"
+ };
ctx.record_lowered_value_with_access_mode_and_facts(
"ClassFieldSet",
None,
- "js_class_field_set_slow",
+ fallback_helper,🤖 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/expr/property_set.rs` around lines 596 - 600, The
fact label passed to record_lowered_value_with_access_mode_and_facts is
hardcoded as "js_class_field_set_slow", but the helper being emitted above
varies based on the inline_fieldset condition. When inline_fieldset is false,
the code emits js_class_field_set_fallback, so the fact label should also
reflect this. Make the fact label conditional: use "js_class_field_set_fallback"
when inline_fieldset is false, and "js_class_field_set_slow" when
inline_fieldset is true. This ensures the recorded fact label matches the actual
emitted helper.
Comment on lines
+464
to
+471
| let body_is_loop = matches!( | ||
| &*labeled_stmt.body, | ||
| ast::Stmt::For(_) | ||
| | ast::Stmt::While(_) | ||
| | ast::Stmt::DoWhile(_) | ||
| | ast::Stmt::ForIn(_) | ||
| | ast::Stmt::ForOf(_) | ||
| ); |
There was a problem hiding this comment.
Peel nested labels before desugaring as non-loop.
outer: inner: while (...) { continue outer; } is still a labeled loop target, but this immediate matches! sees outer’s body as Stmt::Labeled and rewrites it to outer: do { inner: while (...) ... } while(false). That changes continue outer from continuing the inner loop to exiting the run-once wrapper.
Proposed fix
- let body_is_loop = matches!(
- &*labeled_stmt.body,
- ast::Stmt::For(_)
- | ast::Stmt::While(_)
- | ast::Stmt::DoWhile(_)
- | ast::Stmt::ForIn(_)
- | ast::Stmt::ForOf(_)
- );
+ fn labeled_target_is_loop(stmt: &ast::Stmt) -> bool {
+ match stmt {
+ ast::Stmt::For(_)
+ | ast::Stmt::While(_)
+ | ast::Stmt::DoWhile(_)
+ | ast::Stmt::ForIn(_)
+ | ast::Stmt::ForOf(_) => true,
+ ast::Stmt::Labeled(inner) => labeled_target_is_loop(inner.body.as_ref()),
+ _ => false,
+ }
+ }
+ let body_is_loop = labeled_target_is_loop(labeled_stmt.body.as_ref());🤖 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-hir/src/lower_decl/body_stmt.rs` around lines 464 - 471, The
issue is that the matches! check for body_is_loop only examines the immediate
body of labeled_stmt without accounting for nested labels. When you have nested
labeled statements like `outer: inner: while (...)`, the outer label's body is
`Stmt::Labeled` not the actual loop, causing the check to incorrectly classify
it as a non-loop and desugar it with the `do { ... } while(false)` wrapper. Fix
this by peeling/unwrapping any nested `Stmt::Labeled` wrappers from
labeled_stmt.body before the matches! check to expose the actual underlying
statement and correctly identify if it is a loop statement.
Comment on lines
+409
to
+413
| // The closure's own scope has been popped, so `ctx.locals.id_set()` is now | ||
| // exactly the enclosing scope's live locals — the membership view capture | ||
| // analysis needs. (Previously rebuilt per closure from a cloned snapshot.) | ||
| let (captures, mutable_captures) = | ||
| compute_closure_captures(ctx, &body, ctx.locals.id_set(), ¶ms); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Locals id_set maintenance and the capture call sites that now rely on it.
fd -p 'crates/perry-hir/src/lower/locals.rs' -x sed -n '1,260p' {}
rg -n -C4 'struct Locals|id_set|fn push|drain_from|reindex' crates/perry-hir/src/lower/locals.rs
rg -n -C3 'id_set\(\)|compute_closure_captures' crates/perry-hir/src/lower/expr_function.rs crates/perry-hir/src/lower_decl/body_stmt/nested_fn_decl.rs
rg -n -C3 'duplicate.*LocalId|id_set|drain_from|closure.*capture' crates/perry-hir/src/lower/tests.rsRepository: PerryTS/perry
Length of output: 19465
🏁 Script executed:
rg -n 'fresh_local' crates/perry-hir/src/ -A 5 -B 2Repository: PerryTS/perry
Length of output: 38931
🏁 Script executed:
rg -n 'compute_closure_captures|closure.*capture|capture.*test' crates/perry-hir/src/lower/tests.rs -A 3 -B 2 | head -100Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Check if LocalIds can be duplicated across scopes or if they're truly unique
rg -n 'LocalId|fresh_local' crates/perry-types/src/ -B 2 -A 5Repository: PerryTS/perry
Length of output: 487
🏁 Script executed:
rg -n 'test.*closure|test.*capture|nested.*closure|reuse.*id' crates/perry-hir/src/lower/tests.rs -B 2 -A 8 | head -150Repository: PerryTS/perry
Length of output: 39
🏁 Script executed:
# Check if there's a test for the dayjs scenario mentioned in expr_function.rs
rg -n 'dayjs|parseDate|inner.*outer.*id|same.*id.*outer' crates/perry-hir/src/ -B 3 -A 3Repository: PerryTS/perry
Length of output: 18272
Fix Locals::drain_from to preserve id_set entries that remain in outer scopes.
The current implementation unconditionally removes each drained entry's LocalId from id_set, but LocalIds are reused across function scopes (as documented in expr_function.rs lines 1101–1107 for the dayjs case). When an inner scope declares var i with the same LocalId as an outer scope's var i, calling id_set.remove(id) removes the id entirely, even though it still exists in the remaining entries for the outer scope. This corrupts the membership set that closure capture analysis depends on.
Verify that drain_from only removes ids that do not appear elsewhere in entries after draining, or rebuild id_set from the remaining entries. Also ensure tests cover the nested-function LocalId reuse scenario (where outer and inner scopes both declare bindings with the same generated id).
🤖 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-hir/src/lower/expr_function.rs` around lines 409 - 413, The
`drain_from` method in the `Locals` struct unconditionally removes each drained
entry's LocalId from the `id_set`, but LocalIds are reused across function
scopes. When an inner scope declares a binding with the same LocalId as an outer
scope's binding, removing the id entirely from `id_set` corrupts the membership
set that `compute_closure_captures` depends on for closure capture analysis. Fix
`drain_from` to either check whether each id being removed still appears in the
remaining entries before removing it from `id_set`, or rebuild `id_set` from
scratch based on the entries that remain after draining. Additionally, add test
coverage for the scenario where nested functions reuse the same LocalId across
outer and inner scopes.
Comment on lines
+523
to
+546
| let n = TOTAL.fetch_add(1, Ordering::Relaxed) + 1; | ||
| SPANS.with(|m| { | ||
| *m.borrow_mut().entry((lo, hi)).or_insert(0) += 1; | ||
| }); | ||
| if n % 5_000_000 == 0 { | ||
| dump(&format!("periodic@{n}")); | ||
| } | ||
| } | ||
|
|
||
| fn dump(tag: &str) { | ||
| SPANS.with(|m| { | ||
| let m = m.borrow(); | ||
| let total = TOTAL.load(Ordering::Relaxed); | ||
| let distinct = m.len(); | ||
| let mut v: Vec<_> = m.iter().map(|(k, c)| (*c, *k)).collect(); | ||
| v.sort_unstable_by(|a, b| b.0.cmp(&a.0)); | ||
| eprintln!( | ||
| "RELOWER[{tag}] total={total} distinct={distinct} ratio={:.2}", | ||
| total as f64 / distinct.max(1) as f64 | ||
| ); | ||
| for (c, (lo, hi)) in v.into_iter().take(20) { | ||
| eprintln!("RELOWER span {lo}..{hi} count={c}"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Keep relower totals and span stats in the same aggregation scope.
Line 535 uses a process-global TOTAL, while Line 536-545 reads only thread-local SPANS, so the printed ratio/top spans can be misleading under multi-threaded lowering.
Suggested direction
- let total = TOTAL.load(Ordering::Relaxed);
- let distinct = m.len();
+ let total: u64 = m.values().map(|c| *c as u64).sum();
+ let distinct = m.len();If you want cross-thread totals, move SPANS to a global synchronized map so both metrics share the same scope.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let n = TOTAL.fetch_add(1, Ordering::Relaxed) + 1; | |
| SPANS.with(|m| { | |
| *m.borrow_mut().entry((lo, hi)).or_insert(0) += 1; | |
| }); | |
| if n % 5_000_000 == 0 { | |
| dump(&format!("periodic@{n}")); | |
| } | |
| } | |
| fn dump(tag: &str) { | |
| SPANS.with(|m| { | |
| let m = m.borrow(); | |
| let total = TOTAL.load(Ordering::Relaxed); | |
| let distinct = m.len(); | |
| let mut v: Vec<_> = m.iter().map(|(k, c)| (*c, *k)).collect(); | |
| v.sort_unstable_by(|a, b| b.0.cmp(&a.0)); | |
| eprintln!( | |
| "RELOWER[{tag}] total={total} distinct={distinct} ratio={:.2}", | |
| total as f64 / distinct.max(1) as f64 | |
| ); | |
| for (c, (lo, hi)) in v.into_iter().take(20) { | |
| eprintln!("RELOWER span {lo}..{hi} count={c}"); | |
| } | |
| }); | |
| let n = TOTAL.fetch_add(1, Ordering::Relaxed) + 1; | |
| SPANS.with(|m| { | |
| *m.borrow_mut().entry((lo, hi)).or_insert(0) += 1; | |
| }); | |
| if n % 5_000_000 == 0 { | |
| dump(&format!("periodic@{n}")); | |
| } | |
| } | |
| fn dump(tag: &str) { | |
| SPANS.with(|m| { | |
| let m = m.borrow(); | |
| let total: u64 = m.values().map(|c| *c as u64).sum(); | |
| let distinct = m.len(); | |
| let mut v: Vec<_> = m.iter().map(|(k, c)| (*c, *k)).collect(); | |
| v.sort_unstable_by(|a, b| b.0.cmp(&a.0)); | |
| eprintln!( | |
| "RELOWER[{tag}] total={total} distinct={distinct} ratio={:.2}", | |
| total as f64 / distinct.max(1) as f64 | |
| ); | |
| for (c, (lo, hi)) in v.into_iter().take(20) { | |
| eprintln!("RELOWER span {lo}..{hi} count={c}"); | |
| } | |
| }); |
🤖 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-hir/src/lower/lower_expr.rs` around lines 523 - 546, The dump
function is mixing metrics from different scopes: TOTAL is a global counter
while SPANS is thread-local, causing the ratio calculation and top spans to be
misleading in multi-threaded scenarios. Move SPANS from thread-local storage to
a global synchronized map (such as a Mutex-protected HashMap or DashMap) so that
both TOTAL and SPANS aggregation use the same unified scope. Update the SPANS
accesses throughout the function to use the global synchronized version instead
of the thread-local pattern with SPANS.with().
Comment on lines
+1275
to
+1288
| let body_is_loop = matches!( | ||
| &*labeled_stmt.body, | ||
| ast::Stmt::For(_) | ||
| | ast::Stmt::While(_) | ||
| | ast::Stmt::DoWhile(_) | ||
| | ast::Stmt::ForIn(_) | ||
| | ast::Stmt::ForOf(_) | ||
| ); | ||
| if !body_is_loop { | ||
| let body = if let ast::Stmt::Block(block) = &*labeled_stmt.body { | ||
| lower_block_stmt_scoped(ctx, block)? | ||
| } else { | ||
| lower_body_stmt(ctx, &labeled_stmt.body)? | ||
| }; |
There was a problem hiding this comment.
Unwrap stacked labels before classifying the body as non-loop.
outer: inner: while (...) { continue outer; } is a labeled loop, but this immediate check treats outer as non-loop and wraps it in a synthetic run-once do-while, changing continue outer to target the synthetic loop instead of the real loop.
🐛 Proposed fix
+ fn labeled_body_targets_loop(stmt: &ast::Stmt) -> bool {
+ match stmt {
+ ast::Stmt::For(_)
+ | ast::Stmt::While(_)
+ | ast::Stmt::DoWhile(_)
+ | ast::Stmt::ForIn(_)
+ | ast::Stmt::ForOf(_) => true,
+ ast::Stmt::Labeled(inner) => labeled_body_targets_loop(&inner.body),
+ _ => false,
+ }
+ }
+
- let body_is_loop = matches!(
- &*labeled_stmt.body,
- ast::Stmt::For(_)
- | ast::Stmt::While(_)
- | ast::Stmt::DoWhile(_)
- | ast::Stmt::ForIn(_)
- | ast::Stmt::ForOf(_)
- );
+ let body_is_loop = labeled_body_targets_loop(&labeled_stmt.body);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let body_is_loop = matches!( | |
| &*labeled_stmt.body, | |
| ast::Stmt::For(_) | |
| | ast::Stmt::While(_) | |
| | ast::Stmt::DoWhile(_) | |
| | ast::Stmt::ForIn(_) | |
| | ast::Stmt::ForOf(_) | |
| ); | |
| if !body_is_loop { | |
| let body = if let ast::Stmt::Block(block) = &*labeled_stmt.body { | |
| lower_block_stmt_scoped(ctx, block)? | |
| } else { | |
| lower_body_stmt(ctx, &labeled_stmt.body)? | |
| }; | |
| fn labeled_body_targets_loop(stmt: &ast::Stmt) -> bool { | |
| match stmt { | |
| ast::Stmt::For(_) | |
| | ast::Stmt::While(_) | |
| | ast::Stmt::DoWhile(_) | |
| | ast::Stmt::ForIn(_) | |
| | ast::Stmt::ForOf(_) => true, | |
| ast::Stmt::Labeled(inner) => labeled_body_targets_loop(&inner.body), | |
| _ => false, | |
| } | |
| } | |
| let body_is_loop = labeled_body_targets_loop(&labeled_stmt.body); | |
| if !body_is_loop { | |
| let body = if let ast::Stmt::Block(block) = &*labeled_stmt.body { | |
| lower_block_stmt_scoped(ctx, block)? | |
| } else { | |
| lower_body_stmt(ctx, &labeled_stmt.body)? | |
| }; |
🤖 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-hir/src/lower/stmt.rs` around lines 1275 - 1288, Before
classifying labeled_stmt.body as a non-loop in the body_is_loop check, unwrap
any stacked labels by recursively checking if the immediate body is another
labeled statement and continuing to traverse inward until reaching the actual
statement. Only after unwrapping all stacked labels should you determine if the
final statement is a loop using the matches! pattern. This ensures that
constructs like outer: inner: while(...) are correctly identified as loops,
preventing the synthetic do-while wrapper from being incorrectly applied and
preserving the correct continue target semantics.
Comment on lines
+1639
to
+1670
| fn rewrite_dispatch_continue_to_suspend(stmts: &mut Vec<Stmt>) { | ||
| for stmt in stmts.iter_mut() { | ||
| match stmt { | ||
| Stmt::Continue | Stmt::Break => { | ||
| *stmt = Stmt::Return(Some(make_iter_result(Expr::Undefined, false))); | ||
| } | ||
| Stmt::If { | ||
| then_branch, | ||
| else_branch, | ||
| .. | ||
| } => { | ||
| rewrite_dispatch_continue_to_suspend(then_branch); | ||
| if let Some(eb) = else_branch.as_mut() { | ||
| rewrite_dispatch_continue_to_suspend(eb); | ||
| } | ||
| } | ||
| Stmt::Try { | ||
| body, | ||
| catch, | ||
| finally, | ||
| } => { | ||
| rewrite_dispatch_continue_to_suspend(body); | ||
| if let Some(c) = catch.as_mut() { | ||
| rewrite_dispatch_continue_to_suspend(&mut c.body); | ||
| } | ||
| if let Some(f) = finally.as_mut() { | ||
| rewrite_dispatch_continue_to_suspend(f); | ||
| } | ||
| } | ||
| // Nested loops / switch / labeled / closures own their own | ||
| // break/continue — leave them untouched. | ||
| _ => {} |
There was a problem hiding this comment.
Preserve async catch-route liveness when replacing dispatch continues.
Line 1643 returns before the done_id = false reset at Line 1766, so a break/continue path suspends with { done: false } while the captured __gen_done flag remains true. Also, label rewrites can place the synthesized LocalSet(state_id, target) + Continue pair inside nested loops/switch/labeled bodies, but Lines 1668-1670 skip those containers and leave the dangling dispatch continue in an inlined catch body.
🐛 Proposed fix
-fn rewrite_dispatch_continue_to_suspend(stmts: &mut Vec<Stmt>) {
- for stmt in stmts.iter_mut() {
- match stmt {
- Stmt::Continue | Stmt::Break => {
- *stmt = Stmt::Return(Some(make_iter_result(Expr::Undefined, false)));
- }
+fn rewrite_dispatch_continue_to_suspend(
+ stmts: &mut Vec<Stmt>,
+ state_id: LocalId,
+ done_id: LocalId,
+) {
+ let mut i = 0;
+ while i < stmts.len() {
+ let is_dispatch_reentry = i > 0
+ && matches!(
+ (&stmts[i - 1], &stmts[i]),
+ (Stmt::Expr(Expr::LocalSet(id, val)), Stmt::Continue | Stmt::Break)
+ if *id == state_id && matches!(val.as_ref(), Expr::Number(_))
+ );
+ if is_dispatch_reentry {
+ stmts[i] = Stmt::Expr(Expr::LocalSet(done_id, Box::new(Expr::Bool(false))));
+ stmts.insert(
+ i + 1,
+ Stmt::Return(Some(make_iter_result(Expr::Undefined, false))),
+ );
+ i += 2;
+ continue;
+ }
+
+ match &mut stmts[i] {
Stmt::If {
then_branch,
else_branch,
..
} => {
- rewrite_dispatch_continue_to_suspend(then_branch);
+ rewrite_dispatch_continue_to_suspend(then_branch, state_id, done_id);
if let Some(eb) = else_branch.as_mut() {
- rewrite_dispatch_continue_to_suspend(eb);
+ rewrite_dispatch_continue_to_suspend(eb, state_id, done_id);
}
}
Stmt::Try {
body,
catch,
@@
finally,
} => {
- rewrite_dispatch_continue_to_suspend(body);
+ rewrite_dispatch_continue_to_suspend(body, state_id, done_id);
if let Some(c) = catch.as_mut() {
- rewrite_dispatch_continue_to_suspend(&mut c.body);
+ rewrite_dispatch_continue_to_suspend(&mut c.body, state_id, done_id);
}
if let Some(f) = finally.as_mut() {
- rewrite_dispatch_continue_to_suspend(f);
+ rewrite_dispatch_continue_to_suspend(f, state_id, done_id);
}
}
- // Nested loops / switch / labeled / closures own their own
- // break/continue — leave them untouched.
+ Stmt::While { body, .. } | Stmt::DoWhile { body, .. } | Stmt::For { body, .. } => {
+ rewrite_dispatch_continue_to_suspend(body, state_id, done_id);
+ }
+ Stmt::Switch { cases, .. } => {
+ for case in cases.iter_mut() {
+ rewrite_dispatch_continue_to_suspend(&mut case.body, state_id, done_id);
+ }
+ }
+ Stmt::Labeled { body, .. } => {
+ let mut wrapped = vec![std::mem::replace(body.as_mut(), Stmt::Break)];
+ rewrite_dispatch_continue_to_suspend(&mut wrapped, state_id, done_id);
+ **body = wrapped.into_iter().next().unwrap();
+ }
_ => {}
}
+ i += 1;
}
}
@@
if !fall_through {
- rewrite_dispatch_continue_to_suspend(&mut rewritten);
+ rewrite_dispatch_continue_to_suspend(&mut rewritten, state_id, done_id);
}Also applies to: 1704-1727, 1766-1779
🤖 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-transform/src/generator/lower.rs` around lines 1639 - 1670, The
issue is that when rewriting Stmt::Continue and Stmt::Break to returns with {
done: false } in the rewrite_dispatch_continue_to_suspend function, this happens
before the done_id flag is reset to false elsewhere in the code, causing the
__gen_done flag to remain true while the returned result has done: false,
creating a liveness inconsistency. Additionally, the function skips nested
containers like loops, switch, labeled blocks, and closures using the wildcard
pattern at lines 1668-1670, which leaves dangling dispatch continue statements
in inlined catch and finally bodies. Fix this by ensuring the timing of done_id
reset is properly coordinated with the dispatch continue rewriting, and extend
the traversal logic to recursively handle all nested statement containers (such
as Stmt::Loop, Stmt::Switch, Stmt::Labeled, and closures) instead of skipping
them, so that break/continue statements within catch and finally handlers are
properly rewritten.
Contributor
Author
|
Superseded by #5351, which contains lever A as a single clean commit off |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
First step of the IR-efficiency roadmap (#5334, Tier 1 / lever A): outline the cold guard-miss arm of the class-field-SET inline-cache diamond.
The DEFAULT (guard-CALL) diamond's
%slowarm emitted two inline calls per set site:The inline guard has already run and failed in the entry block (that failure is what branched control here), so nothing is left to decide. Collapse the pair into one outlined call:
js_class_field_set_fallbackrecords the miss and routes the write by name — byte-identical to the two-call block.Why it's safe
js_class_field_set_slow(the opt-in inline-precheck path's helper), which re-runs the guard. This helper must not, or it would double-record. It's a pure record-then-by-name fallback.class_field_set.fastslot store is untouched; the change is confined to the cold guard-miss arm, which never executes on a monomorphic hot path.Measurement
Per class-field-SET site, the
%slowarm drops from 2 calls to 1 (verified on emitted IR for aPoint-field-churn test). On a large minified bundle there are ~117K such fallback arms, so this removes ~117K call sites' worth of IR — a small but pure-win slice, and it establishes the outline-helper pattern reused by the larger levers in #5334 (C: nan-box round-trips, D: non-pointer barrier elision, B: adaptive full-outline for oversized modules).Tests
typed_feedback_guards_direct_class_field_specializationto assert the single outlined call on the default path (and that the SET-sitejs_object_set_field_by_nameis gone;record_fallback_calllegitimately remains from the always-on class-field-GET fallback block).perry-codegensuite green (108 + the rest).Refs #5334.
Summary by CodeRabbit
Bug Fixes
Array.pop(extra))—now properly evaluated for side effects rather than causing errors.New Features
breakandcontinuestatements in generator functions.Performance
SEToperations with configurable inline caching and slow-path fallbacks.Tests