perf(class): stop disarming every dispatch guard when a class prototype is materialized - #7800
Conversation
📝 WalkthroughWalkthroughThe PR adds a runtime helper for class-and-keys shape checks, preserves guards during declared-class prototype materialization, and extends shape-only direct method calls with bounded subclass dispatch arms. ChangesClass dispatch and prototype materialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant emit_guarded_direct_method_call
participant js_method_direct_shape_class
participant Subclass_method_target
participant Dynamic_fallback
emit_guarded_direct_method_call->>js_method_direct_shape_class: Probe receiver class and keys
js_method_direct_shape_class-->>emit_guarded_direct_method_call: Return class ID and keys
emit_guarded_direct_method_call->>Subclass_method_target: Call matching declared or subclass target
emit_guarded_direct_method_call->>Dynamic_fallback: Continue when no arm matches
Possibly related PRs
Suggested reviewers: 🚥 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 |
83b778e to
6cfeba9
Compare
|
Gap suite ( |
…pe is materialized `class_decl_prototype_value()` lazily materializes a declared class's prototype object the first time anything demands it — `instanceof`, `Object.getPrototypeOf`, a `super` chain. It called `invalidate_class_prototype_fast_guards()`, which trips a process-global, MONOTONIC latch that makes every `js_method_direct_shape_guard` / `js_typed_feedback_method_direct_call_guard` answer "miss" for the rest of the run, retires every element-shape record (`invalidate_all_element_shapes`, #7480), and bumps `VTABLE_GEN`, retiring the `vtable_ic` / `obj_dispatch_ic` caches (#7769). That latch is for prototype SURGERY (`Class.prototype.m = fn`) — the two call sites in `class_registry/prototype_methods.rs`, which keep it. Materialization changes nothing about which member `recv.m()` resolves to: the object is fresh and unobserved, and the writes below it install `constructor` plus exactly the methods the class already declares. But because any demand lands there, an ordinary class-hierarchy program disarmed its own speculation during startup and then ran every method call and every array element read on the slow path. Measured on `gc-handoff/apps/shapes.ts` with per-precondition counters on the guard: 384,000 of 384,000 probes failed on this latch and on nothing else. Also adds `js_method_direct_shape_class`, the class-id half of `js_method_direct_shape_guard` (which is now defined in terms of it, so its single-pair semantics are unchanged by construction), and uses it to widen the shape-guarded direct call from one arm — the declared receiver class — to the declared class plus its subclass closure, capped at 8 arms. For a base-typed collection the single-arm bet loses on every element. shapes 0.2256 -> 0.1976 s on the quiet mini (best-of-5, output byte-identical to node, exit 0). Four allocation-heavy programs regress 3.4-4.2%; see the PR body — this is a draft for that reason.
6cfeba9 to
87911ac
Compare
Gap suite: complete. 522/522 run, exit 1, zero regressions attributable to this change.
Every row is identical between the two builds. Eight fail on clean This is the documented phantom-regression shape for a fresh worktree — the harness Corroborating: the run also reports 10 Two caveats stated plainly rather than papered over:
|
![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
🤖 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/lower_call/property_get/dynamic_dispatch.rs`:
- Around line 938-980: Before adding a subclass dispatch arm in the loop
resolving `sub_name`, reject it when `class_chain_has_field_named(ctx, sub_name,
property)` is true, so an own or inherited class-field override is not bypassed
by `target_fn`. Preserve the existing method/static/rest/arity eligibility
checks, and add a regression covering a base-typed reference whose subclass
defines a same-named class field.
🪄 Autofix
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: 91927f27-f070-4882-b83f-5a1312302517
📒 Files selected for processing (7)
changelog.d/7800-class-dispatch-prototype-latch.mdcrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/typed_feedback/guards.rs
| let Some(keys_global) = ctx.class_keys_globals.get(sub_name).cloned() else { | ||
| continue; | ||
| }; | ||
| // Resolve through the SUBCLASS's own chain, and remember | ||
| // where it landed: the rest-param shape is a property of | ||
| // the declaring class, and a rest-bearing target cannot be | ||
| // called with this site's flat, base-arity argument list. | ||
| let mut cur = Some(sub_name.clone()); | ||
| let mut resolved: Option<(String, String)> = None; | ||
| while let Some(c) = cur { | ||
| let key = (c.clone(), property.to_string()); | ||
| if let Some(fname) = ctx.methods.get(&key).cloned() { | ||
| resolved = Some((c, fname)); | ||
| break; | ||
| } | ||
| cur = ctx.classes.get(&c).and_then(|c| c.extends_name.clone()); | ||
| } | ||
| let Some((decl_class, target_fn)) = resolved else { | ||
| continue; | ||
| }; | ||
| if target_fn.starts_with("perry_static_") { | ||
| continue; | ||
| } | ||
| if matches!( | ||
| ctx.method_has_rest | ||
| .get(&(decl_class.clone(), property.to_string())), | ||
| Some(&true) | ||
| ) { | ||
| continue; | ||
| } | ||
| if ctx | ||
| .method_param_counts | ||
| .get(&(decl_class, property.to_string())) | ||
| .is_some_and(|&n| n > max_explicit_arity) | ||
| { | ||
| continue; | ||
| } | ||
| seen_ids.push(sub_id); | ||
| subclass_arms.push(SubclassDispatchArm { | ||
| class_id: sub_id, | ||
| keys_global, | ||
| target_fn, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude subclass arms that can have an own class-field override.
shape_only_guard checks fields only on class_name. A subclass can declare property as a class field. Its canonical keys token then passes this arm, but arm.target_fn bypasses the own field and calls the inherited method.
For example, const value: Base = new Sub() must call Sub's method = () => ... field, not Base.method().
Reject an arm when class_chain_has_field_named(ctx, sub_name, property) is true. Add a regression for a base-typed reference to a subclass with a same-named class field.
Proposed eligibility guard
if !is_subclass {
continue;
}
+ if class_chain_has_field_named(ctx, sub_name, property) {
+ continue;
+ }
let Some(keys_global) = ctx.class_keys_globals.get(sub_name).cloned() else {
continue;
};🤖 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/property_get/dynamic_dispatch.rs` around
lines 938 - 980, Before adding a subclass dispatch arm in the loop resolving
`sub_name`, reject it when `class_chain_has_field_named(ctx, sub_name,
property)` is true, so an own or inherited class-field override is not bypassed
by `target_fn`. Preserve the existing method/static/rest/arity eligibility
checks, and add a regression covering a base-typed reference whose subclass
defines a same-named class field.
1 participant
What this found
gc-handoff/apps/shapes.tswas believed to be a class-dispatch problem — 5.87xscriptc, 2.86x node. The dispatch guard was never running at all.
class_decl_prototype_value()— the lazy materializer that creates a declaredclass's prototype object the first time anything demands it — called
invalidate_class_prototype_fast_guards(). That is not a hint. It trips aprocess-global, monotonic latch that
js_method_direct_shape_guard/js_typed_feedback_method_direct_call_guardanswer "miss" for every receiver for the rest of the run, so every
recv.m()on a declared class falls into thejs_native_call_methodtower;crate::array::invalidate_all_element_shapes(), retiring everyelement-shape record (repsel: element-shape proofs through arrays — measured 6.2× vs node; route = invariant bit → versioned-loop consumer → element Ptr<Shape> #7480), so
arr[i]degrades to the genericjs_require_object_coercible+js_is_symbol+js_object_get_index_polymorphicpath;
VTABLE_GEN, retiring thevtable_ic/obj_dispatch_iccaches (perf(runtime): class dispatch and instanceof stop consulting locked hash maps — shapes.ts 0.28s → 0.23s #7769).The latch exists for prototype surgery (
Class.prototype.m = fn) — the twocall sites in
class_registry/prototype_methods.rs, which keep it. Materializationchanges none of that: the object is fresh and unobserved, and the writes
immediately below install
constructorplus exactly the methods the class alreadydeclares.
What actually reaches the materializer (measured with a name-printing probe on
the materializer itself, not inferred):
class A {};new A()class B extends A {};new B()B,Aclass C extends B extends A;new C()C,B,Anew B()B,Ax instanceof SomeClassObject.getPrototypeOf(x)arr instanceof ArraySo the trigger is
newon any class thatextendssomething — instantiating asubclass materializes its whole prototype ancestor chain. It is not
instanceofand not
getPrototypeOf; an earlier revision of this description said it was, andthat was inferred rather than measured. In
shapes.tsthe three areRect,Shape,Node2D— the ancestor chain of the first subclassbuild()constructs.Evidence — temporary per-precondition counters on the guard,
shapes.ts:384,000 of 384,000 probes failed on this latch and on nothing else.
inval_sites[0]isclass_decl_prototype_value. A probe containing noinstanceofat all shows the same 100%.The two halves are only worth anything together
js_method_direct_shape_classfactors the class-id half out ofjs_method_direct_shape_guard(which is now defined in terms of it, so itssingle-pair semantics are unchanged by construction). Codegen uses it to widen the
shape-guarded direct call from ONE arm — the declared receiver class — to the
declared class plus its subclass closure, each paired with the body the method
resolves to when walked from that class, capped at 8 arms.
Measured separately, each half is a no-op:
The reason is that they gate each other. With the latch stuck, no guard of any
width ever passes. With the latch fixed but only one arm, the guard still
speculates the declared class —
Node2D— which is never the runtime class ofanything in the array, so it still misses on every element. Neither is worth
landing without the other.
Measurements (quiet M1 mini, best-of-5, all three arms interleaved rep-by-rep)
Baseline
origin/main@0a2bf15bd(perry 0.5.1455), corpusgc-handoff/m0810/pr/.Every cell exit 0, every output byte-identical to
node --experimental-strip-types.Re-confirmed at 9 reps for the four programs an earlier, contaminated two-arm run
had flagged: asyncpipe 0.7220 -> 0.7196 (0.997), interp 1.8874 -> 1.8886 (1.001),
iso_miss 2.3671 -> 2.3673 (1.000), pipeline 0.5520 -> 0.5613 (1.017), shapes
0.2232 -> 0.1866 (0.836). That first run had been taken while another agent's
benchmarks were running on the mini and its ~4% "regressions" did not reproduce.
shapes is still 2.4x node (0.078) and 4.9x scriptc (0.038). This closes a third
of the gap, not the gap. See the probes below for where the rest is.
Correctness
gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0, also underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800and underPERRY_GC_VERIFY_EVACUATION=1(both exit 0).shapes.tslikewise.test-files/*.tsmatching class / inherit / extend / method / proto / super /instanceof: identical pass/fail set to clean
main— the same 7 pre-existingfailures, each individually A/B'd against the reference build.
Review question I could not settle
Is prototype materialization really not surgery? The writes below the removed
call install the class's own declared methods on its own fresh prototype, which
cannot change what
recv.m()resolves to. The residual risk is a later write tothat now-existing prototype object that does not route through
js_register_prototype_method/class_prototype_method_root_store— those twostill invalidate, and
Object.definePropertyis covered bydescriptors_in_use()— but I did not enumerate every path that can reach a materialized prototype
object's fields.
Probes
gc-handoff/bench/shapes_{build,describe,dispatch,dispatch_static}.tsdecomposeapps/shapes.ts, each annotated with its measured seconds. Onmainthey recordthat
build()is 0.1035 s (46% of the program) and thatdescribe()'s"lit" + this.stringFieldconcatenation is 0.074 s (33%, ~620 ns/call, throughjs_dynamic_string_or_number_add— the NaN-boxed field read does not carry itsdeclared
stringtype forward). Those two, not dispatch, are where the remaininggap to node's 0.083 s lives.
Blast radius of the latch, measured
The latch is monotonic in production — the only
store(false)is#[cfg(test)](
class_registry/gc_roots.rs:495). Since almost every class-hierarchy programtrips it, the obvious worry is that it silently disarms the element-shape repsel
work (#7770, #7771, #7766, #7702) process-wide. It does not, and the measured
cost elsewhere is ~0. Quiet mini, best-of-9, one statement added before an
otherwise identical hot loop:
instanceofgetPrototypeOfchurn_readshape, object literalsTwo different mechanisms, and only one of them is monotonic:
invalidate_all_element_shapes()bumpsCLASS_SHAPE_GENERATION; each record carries the generation it was installedunder and
ensure_element_shapere-establishes it on the next query(
array/element_shape.rs:204,:388). One bump costs at most onere-establishment per array — the repsel element-shape work is not disarmed by
this.
shapes.tspaid for — but on its own it is worth only 1.0% there (0.2228 ->0.2205). It becomes worth 16.6% only in combination with the multi-arm widening,
because a single-arm guard bets on the declared class and misses on a
base-typed collection whether or not the latch is set.
Summary by CodeRabbit
Performance
Documentation