perf(codegen): route typed-clone fallbacks to the Phase 5a proven-this clone (#7128) - #7141
perf(codegen): route typed-clone fallbacks to the Phase 5a proven-this clone (#7128)#7141proggeramlug wants to merge 6 commits into
this clone (#7128)#7141Conversation
…is` clone
The `{method}__pshape` clone emitted by representation-selection Phase 5a had
zero call sites across the whole measured corpus: every clone was emitted,
reachable from nothing, and dead-stripped by the linker.
Root cause is arm ordering at both routing sites. emit_guarded_direct_method_call
tried five typed-clone arms and consulted pshape_methods only in the final
`else`; the Phase 3b guard-free site had the same shape, routing to the clone on
its plain exit while its typed-receiver arm's generic fallback called the
guard-ridden public body 25 lines above. A method can only admit a proven-`this`
clone if it touches a declared field of its own chain, which is very nearly the
definition of a typed-receiver-clone candidate — so the typed arm won whenever
both were eligible.
Both sites now resolve the clone once, up front, and use it for the generic
fallback as well as the plain exit. Every rerouted block is dominated either by
the class-id + keys-token guard or by Phase 3b containment, which is exactly what
the existing `else` arm already relied on — no new proof obligation, unchanged
ABI, unchanged shadow-bound receiver slot.
|
Warning Review limit reached
Next review available in: 50 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesProven-this routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MethodLowering
participant ProvenThisClone
participant TypedFallback
participant LLVMIR
MethodLowering->>ProvenThisClone: Resolve eligible __pshape clone
MethodLowering->>TypedFallback: Pass shared generic target
TypedFallback->>LLVMIR: Emit call to proven-this clone
MethodLowering->>LLVMIR: Emit untyped direct call to shared target
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
find("@name(") matches a routed CALL site first — which appears earlier in the
module than the definition — so the slice could cover the caller's body and pass
on some other function's shadow bind.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-codegen/src/lower_call/method_override.rs (1)
183-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist
generic_body_fnonce instead of recomputing it in five arms.Each of the five typed arms repeats the same 3-line expression:
pshape_fn.clone().unwrap_or_else(|| crate::codegen::generic_method_body_name(direct_fn)).pshape_fnanddirect_fndo not change across arms, so this value is the same wherever it is computed. Compute it once next topshape_fnand reference it by borrow in each arm.♻️ Proposed refactor to hoist the shared fallback target
let pshape_fn: Option<String> = (!direct_fn.starts_with("perry_static_") && ctx .pshape_methods .contains_key(&(receiver_class_name.to_string(), property.to_string()))) .then(|| crate::collectors::pshape_method_name(direct_fn)); + let generic_body_fn: String = pshape_fn + .clone() + .unwrap_or_else(|| crate::codegen::generic_method_body_name(direct_fn));Then in each of the five typed arms (lines 272-274, 402-404, 489-491, 578-580, 680-682), drop the local recomputation and rely on the hoisted binding, for example:
if let Some((typed_fn, typed_formal_count, receiver_info)) = typed_f64_receiver_direct_fn { - let generic_body_fn = pshape_fn - .clone() - .unwrap_or_else(|| crate::codegen::generic_method_body_name(direct_fn)); let formal_args: Vec<&str> = direct_arg_slicesApply the same removal at the other four arms; the existing
&generic_body_fnusages further down each arm continue to work unchanged since they only borrow the value.Also applies to: 272-274, 402-404, 489-491, 578-580, 680-682
🤖 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/lower_call/method_override.rs` around lines 183 - 205, Hoist the shared generic body target next to the existing pshape_fn binding in the surrounding method-override function, computing it once from pshape_fn and direct_fn. Remove the repeated pshape_fn.clone().unwrap_or_else(...) expressions from all five typed arms and have their existing &generic_body_fn usages borrow the hoisted value unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/collectors/proven_this_routing_tests.rs`:
- Around line 23-29: Align the module documentation with the actual coverage in
guarded_site_module and its three emit(..., false) tests: either add a Phase 3b
fixture that declares a shape-proven new Counter() receiver and exercises its
method calls, or revise the header to claim coverage only for the guarded
method_direct.fast routing site.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/method_override.rs`:
- Around line 183-205: Hoist the shared generic body target next to the existing
pshape_fn binding in the surrounding method-override function, computing it once
from pshape_fn and direct_fn. Remove the repeated
pshape_fn.clone().unwrap_or_else(...) expressions from all five typed arms and
have their existing &generic_body_fn usages borrow the hoisted value unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a4ed4ee-d764-44b5-ae38-0399d4a5d28e
📒 Files selected for processing (6)
changelog.d/7141-pshape-routing.mdcrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/proven_this.rscrates/perry-codegen/src/collectors/proven_this_routing_tests.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs
Measurements (Raspberry Pi 5, aarch64,
|
| workload | clones emitted | call sites BASE | call sites FIXED |
|---|---|---|---|
benchmarks/repsel_census/fixtures/fixture_ptr_shape.ts |
1 | 0 | 2 |
benchmarks/repsel_census/fixtures/fixture_ptr_shape_sites.ts |
1 | 0 | 2 |
benchmarks/suite/09_method_calls.ts |
2 | 0 | 1 |
benchmarks/app-patterns/kernels/batch.ts |
1 | 0 | 0 → #7142 |
benchmarks/suite/07_object_create.ts |
0 | 0 | 0 (no clone) |
benchmarks/suite/12_binary_trees.ts |
0 | 0 | 0 (no clone) |
Every workload that emits a clone reachable through a keys-token-guarded or
Phase-3b-contained site now routes to it. batch.ts still does not, because its
r.rescore(1.5) is inside rows.map((r) => …) where r has no static class, so
the call lowers to the class-id switch tower — filed as #7142 with the
soundness argument for why this PR did not take it.
What the routed call site now enters
Point::norm2 in fixture_ptr_shape.ts, comparing the body the routed call
targets before and after:
| callee | IR lines | js_typed_feedback_class_field_*_guard |
js_object_get_field_by_name* |
opaque js_* calls |
|---|---|---|---|---|
…__Point__norm2__generic (before) |
304 | 4 | 4 | 19 |
…__Point__norm2__pshape (after) |
104 | 0 | 0 | 7 |
The 7 that remain are shadow-frame enter/pop/bind plus the cold
js_number_coerce arm. Module totals for those counters are unchanged, and
should be — the public vtable body is still emitted either way; what moved is
which body the routed call enters.
Regression checks
cargo test -p perry-codegen --lib— 409 passed, 0 failed.python3 scripts/compiler_output_regression.py census --gate— Census OK;
ptr-shape7 selected / 3 consumed,batch's consumed floor of 2 held, no
floor moved.test_gap_repsel_proven_this_frozen,test_gap_repsel_ptr_shape_barriers,
test_gap_repsel_ptr_shape_locals— compiled and run on both arms,
byte-identical stdout, rc=0 on all three.
One thing the census structurally cannot show
ptr-shape-consumed does not move, and cannot. That column counts values with
a Phase 3b local proof; a proven-this receiver is deliberately excluded from
it (the census's own coherence rule — consumed may never exceed selected,
and Phase 5a's receiver is consumed without ever being selected). The census
reports those separately, as "8 consumption(s) of a proven this receiver" —
and every one of those was, before this PR, recorded inside a body with no
caller. That is exactly why the ratchet added here reads call sites out of the
IR instead of trusting the counter.
The module doc claimed both routing sites were covered, but every test used a typed PARAMETER receiver, which only reaches emit_guarded_direct_method_call. The dynamic_dispatch.rs half of the fix had no unit test at all. Adds a shape-proven LOCAL fixture (single Let initialised by `new`, only field-accessed and method-called) whose method has a typed-receiver clone, so it takes the same shadowed fallback on the guard-free site. Fails against main's routing, passes with the fix; the receiver-bind ratchet now scans both modules. Note for the next person: returning `p.norm2()` directly disqualifies the local — the containment walk sees a LocalGet(p) inside the Return and does not distinguish a call receiver from an escape — which is why the result goes through an accumulator, exactly as fixture_ptr_shape.ts does. Also hoists generic_body_fn next to pshape_fn (CodeRabbit): both inputs are arm-invariant, so the five arms were recomputing one value.
|
Both review points applied in Coverage claim vs tested module — you were right, and it was worse than a doc Rather than narrow the header I added the missing fixture: a Phase 3b One thing worth recording, because it bit me while writing it: returning
|
1 participant
Fixes the Phase 5a defect recorded in #7128: the
{method}__pshapeproven-thisclone had zero call sites across the measured corpus. Every clone was
emitted, reachable from nothing, and dead-stripped by the linker.
Why neither existing check could see it
suite_09_method_calls'object genuinely does differ with
PERRY_PTR_SHAPE_LOCALS=0— by two deadclone bodies.
thisis a realPtr<Shape>consumption, recorded at everythis.fieldsite inside the bodynobody calls.
That is also the reconciliation of the two contradictory reports from the same
day: #7117's "7 receiver consumptions corpus-wide" and #7128's "zero call sites"
are both true, and describe the same dead code.
note_ptr_shape_consumedfilesan
Expr::Thisconsumption underPosition::Param, which the census counts asconsumed_receiverand excludes — andproven_thisisSomeonly whilelowering a
__pshapebody. Verified in IR, not inferred.Root cause
Arm ordering at both routing sites.
lower_call/method_override.rs—emit_guarded_direct_method_calltried fivetyped-clone arms and consulted
pshape_methodsonly in the finalelse.lower_call/property_get/dynamic_dispatch.rs— the Phase 3b guard-free siterouted to the clone on its plain exit, but its typed-receiver arm's own
generic fallback called the guard-ridden public body 25 lines above.
A method can only admit a proven-
thisclone if it touches a declared field ofits own chain — which is very nearly the definition of a typed-receiver-clone
candidate. So the typed arm won essentially whenever both were eligible, and the
clone was emitted with no caller at all.
Both sites now resolve the clone once, up front, and use it for the generic
fallback as well as the plain exit. The typed clone is still preferred on the
fast path; what changed is that falling off it no longer discards a receiver
proof the enclosing block had already established.
Soundness
No new proof obligation. Every rerouted block is dominated either by the
class-id + keys-token guard (
method_direct.fast) or by Phase 3b containment —exactly what the existing
elsearm already relied on. Unchanged(double this, args…)ABI; the clone still stores its receiver to a slot andjs_shadow_slot_binds it with no safepoint between, which the new tests assertdirectly rather than trusting the comment (
GC_TYPE_OBJECTmoves in the shippedconfig, #7019).
Tests
collectors/proven_this_routing_tests.rsratchets call sites, never symbolpresence:
pshape_call_targetsmatches the callee position of acall, so aptrtoint ptr @…__pshapeoperand (how the typed-feedback guard receives afunction pointer) can never be miscounted. These are
--libunit tests, so theyrun on every PR (#5960), not only nightly.
Sabotage-verified in both directions on a Raspberry Pi 5:
main, tests kept —typed_clone_fallback_routes_to_proven_this_cloneFAILS withdefined=[…__bump__pshape, …__scale__pshape] called=[…__bump__pshape], whilethe
untyped_method_routes_to_proven_this_clonecontrol still passes,confirming the control is a control and the new test targets the real defect.
Deliberately not done, filed instead
idispatch.caseN) would be unsound.delete inst.fieldcompacts packed slots while preservingclass_id(
js_object_delete_field→ keys-scan/compaction; class instances get notombstone branch), which is precisely what the keys token catches. That tower
checks class-id only.
cost-model question — it trades per-field
js_typed_feedback_class_field_get_guardcalls at the call site against a NaN-boxed
load doublein the body. Thatbelongs in the
collectors/repsel_benefit.rsgate added by perf(repsel): refuse canonical i32 when every hot consumer wants a double (#7128) #7132.Summary by CodeRabbit
Performance
Bug Fixes
Tests
Documentation