perf(transform): inline small this-using methods on exact receivers (1.6x on method_calls)#5092
Merged
Merged
Conversation
Two gaps kept hot monomorphic method calls from ever being inlined, so a call like `const c = new Counter(); for (...) c.increment();` always paid full dynamic-dispatch + a non-inlined call per iteration: 1. Loop bodies were seeded with EMPTY exact-receiver facts (While/DoWhile/For in call_inliner.rs), discarding the `c -> Counter` fact established before the loop — so the receiver class was unknown inside the loop and nothing inlined. Now the body inherits the loop-invariant subset of outer facts via `loop_invariant_seed_facts`: a fact is kept only if its local is never reassigned anywhere in the loop (collect_mutated_local_ids, which recurses into closures), so a receiver rebound mid-loop correctly drops its fact and is not mis-inlined. 2. is_inlinable rejected EVERY method that references `this` (body_references_dynamic_this) — i.e. essentially all real methods — even though the method-inliner substitutes `this` for the concrete receiver (substitute_this_in_stmts). Added is_inlinable_method, which allows a direct `this` (it gets substituted) but still rejects new.target and any nested closure (whose lexical this/new.target binding is not rewritten), via method_body_blocks_this_substitution. method_lookup_safe still gates each call site, so methods on classes with dynamic/native `extends` (runtime-dependent prototype) are NOT inlined. bench_method_calls (10M monomorphic counter.increment()): 5229ms -> 3221ms (1.6x); the method body is now inlined into the loop. General win for OOP code. Validation: perry-transform + perry-codegen + perry-runtime tests pass; 29/29 suite benchmarks match Node; class-heavy correctness incl. a receiver reassigned inside a loop matches Node; full local parity shows zero regressions vs the base (all pre-existing mismatches fail identically with and without this change).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enhances method inlining by adding a this-substitution safety check, a method-specific inlinability predicate, super-usage detection utilities, and conservative loop-invariant receiver fact seeding; it wires these utilities into the inliner driver. ChangesMethod inlining with this-substitution safety, super detection, and loop optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-transform/src/inline/call_inliner.rs (1)
661-678:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFor-loop init mutations are not included in receiver-fact invalidation
Line 717 seeds
body_factsfromexact_receiver_facts, but the mutation filter currently excludesinit(Lines 661-678). Ifinitrebinds a receiver local, the old exact class fact can survive and allow unsound method inlining in the loop body. Includeinitin the mutated-local set before usingbody_facts.Proposed fix
let mut for_extra: Vec<&Expr> = Vec::new(); if let Some(c) = condition.as_ref() { for_extra.push(c); } if let Some(u) = update.as_ref() { for_extra.push(u); } let mut body_facts = loop_invariant_seed_facts(exact_receiver_facts, body, &for_extra); + if let Some(init_stmt) = init.as_ref() { + let mut init_mutated: HashSet<LocalId> = HashSet::new(); + collect_mutated_local_ids(std::slice::from_ref(init_stmt.as_ref()), &mut init_mutated); + body_facts.retain(|id, _| !init_mutated.contains(id)); + } inline_calls_in_stmts( body, func_candidates, method_candidates,Also applies to: 709-718
🤖 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/inline/call_inliner.rs` around lines 661 - 678, The loop-init handling currently runs inline_calls_in_stmts on init (using init_stmts and init_facts) but fails to add any locals rebinding in init into the mutated-local set used to derive body_facts from exact_receiver_facts, allowing stale exact-class facts to survive; update the code that processes the for-loop init (the branch that creates init_stmts/init_facts and calls inline_calls_in_stmts) to record any locals written by init into the mutated-local set (the same mutation filter used before seeding body_facts from exact_receiver_facts) so those receiver facts are invalidated before computing body_facts, and apply the same change at the other for-init handling site that mirrors this logic.
🤖 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.
Outside diff comments:
In `@crates/perry-transform/src/inline/call_inliner.rs`:
- Around line 661-678: The loop-init handling currently runs
inline_calls_in_stmts on init (using init_stmts and init_facts) but fails to add
any locals rebinding in init into the mutated-local set used to derive
body_facts from exact_receiver_facts, allowing stale exact-class facts to
survive; update the code that processes the for-loop init (the branch that
creates init_stmts/init_facts and calls inline_calls_in_stmts) to record any
locals written by init into the mutated-local set (the same mutation filter used
before seeding body_facts from exact_receiver_facts) so those receiver facts are
invalidated before computing body_facts, and apply the same change at the other
for-init handling site that mirrors this logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c095746-c4a4-4839-aee5-a456a63a3a3e
📒 Files selected for processing (4)
crates/perry-transform/src/inline/analysis.rscrates/perry-transform/src/inline/call_inliner.rscrates/perry-transform/src/inline/closure_analysis.rscrates/perry-transform/src/inline/mod.rs
The for-loop init can rebind a receiver local (`for (c = makeOther(); …)`), but loop_invariant_seed_facts only scanned body + condition + update, so a stale pre-loop exact-class fact could survive into the body and allow an unsound method inline. Also drop facts for locals mutated by init. Addresses CodeRabbit review feedback on #5092. Verified: a receiver rebound in for-init is observed as the new class in the body (matches Node).
…size gate call_inliner.rs grew past the 2000-line CI limit. Move the inline-recursion guard and lexical-`super` detection helpers into a new super_detect.rs sibling module, re-exported through mod.rs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-transform/src/inline/super_detect.rs`:
- Around line 39-50: The expr_contains_lexical_super_set function only detects
SuperPropertySet but must check for all super expression variants that reference
the implicit receiver. Modify the matches! check at the beginning of
expr_contains_lexical_super_set to include all super forms: SuperPropertySet,
SuperMethodCall, SuperCall, SuperPropertyGet, ObjectSuperPropertyGet,
ObjectSuperPropertySet, and ObjectSuperMethodCall. This ensures the function
correctly identifies all unsafe-to-inline super expressions, not just property
sets.
🪄 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: 8f59af95-1139-4a49-b487-6467b71b8392
📒 Files selected for processing (3)
crates/perry-transform/src/inline/call_inliner.rscrates/perry-transform/src/inline/mod.rscrates/perry-transform/src/inline/super_detect.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-transform/src/inline/mod.rs
- crates/perry-transform/src/inline/call_inliner.rs
expr_contains_lexical_super matched only Expr::SuperPropertySet, so a method whose body read super.prop (SuperPropertyGet), called super()/super.m() via the spread or object-literal variants, etc. could pass the inline-safety guard and be substituted onto a different receiver — re-resolving super against the wrong parent. Match every super form (reads, writes, calls, and the object-super variants). Renamed the helpers (drop the now-inaccurate _set suffix) and exposed MAX_INLINE_EXPR_RECURSION_DEPTH through mod.rs for the call_inliner unit test. Addresses CodeRabbit review on #5092.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-transform/src/inline/super_detect.rs (1)
18-18: ⚡ Quick winMake the recursion guard non-constructible outside this module.
InlineExprRecursionGuardis apub(crate)unit struct, so any crate-internal caller can instantiate/drop it without going throughenter_inline_expr_recursion(). That makes the thread-local depth counter easy to desynchronize and can accidentally punch holes in the recursion cap.Proposed fix
-pub(crate) struct InlineExprRecursionGuard; +pub(crate) struct InlineExprRecursionGuard(()); pub(crate) fn enter_inline_expr_recursion() -> Option<InlineExprRecursionGuard> { let entered = INLINE_EXPR_RECURSION_DEPTH.with(|depth| { let current = depth.get(); if current >= MAX_INLINE_EXPR_RECURSION_DEPTH { @@ }); - entered.then_some(InlineExprRecursionGuard) + entered.then_some(InlineExprRecursionGuard(())) }🤖 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/inline/super_detect.rs` at line 18, The `InlineExprRecursionGuard` struct is currently a `pub(crate)` unit struct that can be directly instantiated by any code in the crate, bypassing the thread-local depth counter management in `enter_inline_expr_recursion()`. Make this struct non-constructible outside the module by adding a private field (such as a zero-sized private type) to the struct definition, ensuring all instances can only be created through the proper `enter_inline_expr_recursion()` function and preventing the recursion counter from becoming desynchronized.
🤖 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.
Nitpick comments:
In `@crates/perry-transform/src/inline/super_detect.rs`:
- Line 18: The `InlineExprRecursionGuard` struct is currently a `pub(crate)`
unit struct that can be directly instantiated by any code in the crate,
bypassing the thread-local depth counter management in
`enter_inline_expr_recursion()`. Make this struct non-constructible outside the
module by adding a private field (such as a zero-sized private type) to the
struct definition, ensuring all instances can only be created through the proper
`enter_inline_expr_recursion()` function and preventing the recursion counter
from becoming desynchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5da1fda0-e6c5-4742-8ec9-493921dbc98e
📒 Files selected for processing (3)
crates/perry-transform/src/inline/call_inliner.rscrates/perry-transform/src/inline/mod.rscrates/perry-transform/src/inline/super_detect.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-transform/src/inline/mod.rs
- crates/perry-transform/src/inline/call_inliner.rs
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
2 participants
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.
Unlocks HIR inlining of small monomorphic methods that read/write
this.field— previously they were never inlined, so a hotobj.method()in a loop always paid full dynamic dispatch + a non-inlined call per iteration.Two gaps fixed (both in
perry-transform/src/inline/)Loop bodies were seeded with empty exact-receiver facts (
call_inliner.rs, While/DoWhile/For arms). Aconst c = new Counter()before a loop established thec → Counterfact, but the loop body got a fresh empty fact set, so the receiver class was unknown inside the loop and nothing inlined. Now the body inherits the loop-invariant subset of outer facts (loop_invariant_seed_facts): a fact is kept only if its local is never reassigned anywhere in the loop (collect_mutated_local_ids, which recurses into closures). A receiver rebound mid-loop drops its fact → not mis-inlined.is_inlinablerejected every method referencingthis(body_references_dynamic_this) — i.e. essentially all real methods — even though the method-inliner substitutesthisfor the concrete receiver (substitute_this_in_stmts). Addedis_inlinable_method, which allows a directthis(substituted) but still rejectsnew.targetand any nested closure (whose lexicalthis/new.targetis not rewritten), viamethod_body_blocks_this_substitution.Soundness
method_lookup_safestill gates each call site, so methods on classes with dynamic/nativeextends(runtime-dependent prototype chains) are not inlined.new.targetand nested closures keep the method out of the inlinable set.Result
bench_method_calls(10M monomorphiccounter.increment())incrementis now inlined into the loop. General win forthis.field-heavy OOP code. (The remaining gap to Node is the per-access field-shape guards — separate follow-up, see plan 008.)Validation
cargo test -p perry-transform -p perry-codegen -p perry-runtime— pass.node --experimental-strip-types.p = new Counter()in the loop body) matches Node — proving the loop-fact mutation guard prevents mis-inlining.Summary by CodeRabbit
thisaccessor/mutator handling.superusage and adding a recursion guard to prevent unbounded inlining work.thissubstitution during inlining.