-
-
Notifications
You must be signed in to change notification settings - Fork 150
perf(codegen): repsel Phase 5a β Ptr<Shape> proven this in methods
#6925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
81bb397
perf(codegen): repsel Phase 5a β Ptr<Shape> proven `this` in methods
c43e325
perf(codegen): repsel Phase 5a β Ptr<Shape> proven `this` in methods
49de992
docs(changelog): key the Phase 5a fragment to PR #6925
e3dc9d2
perf(codegen): carry the perry_static_ exclusion into the Phase 5a guβ¦
c4ce9a7
fix(codegen): stand down proven-`this` clones whose symbol collides wβ¦
948861d
docs(changelog): record the #6927 collision guard and the IR-verifiedβ¦
6e8b4d1
docs(changelog): don't present the FORCE_EVACUATE arm as evacuation-sβ¦
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| **Representation-selection Phase 5a β `Ptr<Shape>` proven `this` in methods (RFC Β§5.4/Β§5.6/Β§5.7):** two call sites already prove a receiver's exact shape and then throw the proof away by calling the guard-ridden PUBLIC method body, where every `this.field` re-enters the per-access guarded diamond (`expr/class_field_inline_guard.rs`) that LLVM provably cannot hoist (volatile gate + the fallback arm's opaque call block LICM). Phase 5a emits an additive `internal` proven-`this` clone of the method and routes both sites to it: `lower_call/method_override.rs`'s `method_direct.fast` arm (entered only after `js_method_direct_shape_guard` / `js_typed_feedback_method_direct_call_guard` matched class id **and** keys token) and `lower_call/property_get/dynamic_dispatch.rs`'s Phase-3b guard-free `Ptr<Shape>` receiver arm. Net new proof work, GC work and layout work: zero β the clone keeps the identical `(double this, double argsβ¦)` ABI and the identical shadow-bound tagged-at-rest receiver slot (`GC_TYPE_OBJECT` is movable, so the `TaPtr` no-shadow shortcut does **not** transfer), and field indexes come from the existing chain-global `class_field_global_index`. | ||
|
|
||
| Structural result (`test_gap_repsel_ptr_shape_locals.ts`, pre-opt β clone; verified to persist post-`opt -O3`): `Vec5a.lenSq` 4 volatile gates / 4 `js_typed_feedback_class_field_get_guard` / 4 by-name gets (297 lines) β **0 / 0 / 0** (97 lines); `Vec5a.normalizeTo` 5 / 7 / 5 (786 lines) β **0 / 0 / 0** (489); `Counter5a.increment` and `.bump` 1 / 1 / 1 β **0 / 0 / 0**. The generic bodies retain every diamond post-`-O3`, which is the direct evidence that the per-access cost is unhoistable and has to be removed structurally. Small clones are then fully inlined into their call sites by LLVM (they are `internal` with one caller). | ||
|
|
||
| Eligibility (class level): the Phase 3b `chain_admissible` walk (modeled, accessor-free, computed-free, statically-extended chain), `ModuleDispatchFacts::prototype_is_stable`, and the module-wide Β§5.2 shape-barrier kill. Method level: not async/generator/`was_plain_async`, no captures, no rest/default/`arguments` params, `this` never used as a value (reuses `ThisFlowAnalysis` β `Expr::This` in value position and closures mentioning `this` both reject), every `this.f = v` write to a declared chain field, and every internally-invoked `this.m()`/`super.m()` vetted transitively. **Subclass safety is enforced at the routing sites, not in the collector:** both only route when the receiver's proven exact class *declares* the method (`method_registry.rs` never inserts inherited entries), so the clone's `this` is exactly the class it was compiled for; inherited dispatch keeps today's lowering. | ||
|
|
||
| `numeric_fields` is deliberately **not** claimed for a proven `this`. A Phase 3b local can claim `JsNumber` under an exhaustive reachable-store proof because containment proves no alias exists; a proven `this` is caller-owned and aliased by construction, so an `obj.f = "s"` store elsewhere β which downgrades the slot's raw-f64 layout at runtime β is not enumerable. Instead the number context keeps a 2-instruction inline plain-finite check on the *loaded bits* with a cold `js_number_coerce` arm, which still retires the volatile gate, the 7-header-load shape check, the guard call and the by-name fallback lookup. | ||
|
|
||
| **`Object.freeze`/`seal`/`preventExtensions` (soundness item 1) β took option (a), the module-wide kill for write-containing clones.** These are not in the Β§5.2 barrier list; Phase 3b needs no rule there because containment proves no alias and a freeze of the local disqualifies it directly. A proven `this` has no containment, so `Object.freeze(c)` followed by `c.m()` would let a guard-free raw store silently succeed where strict-mode class code must throw a `TypeError`. A new `ModuleDispatchFacts::freeze_barrier_sites` fact therefore disables proven-`this` clones **that contain a `this.field` write** whenever the module contains any freeze-family site; read-only clones and all of Phase 3b are unaffected. Option (b) (reads-only first increment) was rejected because it would have excluded the single most common method shape (`this.value = this.value + 1`). Covered by a dedicated gap test (`test_gap_repsel_proven_this_frozen.ts` β separate file precisely because the kill is module-wide). | ||
|
|
||
| **Vtable reachability (soundness item 2):** the clone is `internal` and is never passed to `js_register_class_method`, so it has no indirect route via `js_native_call_method`, `emit_collapsed_instance_dispatch`, the own-property override probe, or `.call`/`.apply`/`.bind`. A **separate** ratchet test (`collectors::proven_this::tests::pshape_symbol_reachability`) pins a four-file allowlist for the symbol fragment rather than widening `spec_abi_symbol_reachability`'s allowlist, which would have un-ratcheted the Phase 2 guarantee. | ||
|
|
||
| Two deliberate deviations from the plan, both documented inline: (1) the per-field `js_typed_feedback_class_field_get_guard` loop at the guarded call site is **kept**, because it guards the `__typed_f64_recv` clone's bare `load double` field access and the whole-object shape guard does *not* subsume it β an external `obj.f = "s"` preserves class id and key set while downgrading the slot layout; the proven-`this` clone needs no such guard because it never claims `JsNumber`. (2) the typed-receiver arm keeps priority over the new routing at the guard-free site, to avoid regressing an already-tuned path. | ||
|
|
||
| Also included, the adjacent `type_analysis/predicates.rs` fix: the `Named(_)` exclusion that made `const o: SomeIface = new C()` lose its Phase 3b proof is narrowed to "declared `Named(n)` where `n` is a known class". A TS `Named` annotation is just as often an interface or alias, which names no layout and is strictly weaker than the provenance proof; explicit class annotations still win, and `Generic { .. }` keeps its blanket exclusion (#6040). | ||
|
|
||
| **Clone-symbol collision guard (#6927).** `pshape_method_name` appends a suffix to the public symbol, so a class declaring both `foo` and `foo__pshape` would give `foo`'s clone the same LLVM symbol as `foo__pshape`'s PUBLIC entry β two definitions of one name. Cross-class shapes collide identically (a class literally named `C__foo` with a method `pshape`), because the `__` separators are not escaped either, so the check compares composed **symbols** from the method registry rather than member names. Colliding clones stand down to today's guarded lowering, which is correct for any receiver. Pruning happens right after the registry is built and before `emit_module_artifacts` reads `cross_module.pshape_methods`, so emission and call-site routing stay in lockstep and a call site can never route to a clone that was declined. This is a local mitigation for this phase's suffix only β the hazard is pre-existing and shared by the whole generated-clone family (`foo` + `foo__generic` already collides today); the family-wide fix is tracked in **#6927**. | ||
|
|
||
| **GC contract under a moving collector, verified in IR (relevant to #6910).** `GC_TYPE_OBJECT` is movable, so the `TaPtr` no-shadow shortcut does not transfer. Two properties were checked rather than assumed: (1) the clone keeps `js_shadow_slot_bind` on the receiver β the shadow-frame setup in `compile_method` is unconditional, and this change touches only `llvm_name`, `linkage`, and public-trampoline re-emission; (2) no raw receiver pointer outlives a safepoint. Pre-`opt`, all 9 raw-pointer derivations in `Vec5a.normalizeTo`'s clone are fed by a fresh load of the shadow-bound slot occurring after the last preceding call β including across the internal `this.lenSq()` call. Post-`opt -O3`, LLVM inlines `lenSq` (removing that safepoint outright) and reloads the receiver from the slot after the remaining `js_dynamic_div` call; a dominance-aware scan finds all 7 surviving derivations in the same basic block as their defining load with no intervening call. The escaped shadow-bound alloca is what defeats CSE. | ||
|
|
||
| The gap corpus additionally places allocation and call safepoints inside hot loops over proven receivers, and passes byte-exact with `PERRY_GC_FORCE_EVACUATE=1` and `PERRY_GC_VERIFY_EVACUATION=1` set. **That green arm is not evidence the proven-`this` receiver survives a live evacuation.** #6950 measured `FORCE_EVACUATE` to be inert on every reachable path (45 completed cycles, all manual full mark-sweeps, `moved_objects=0`), so what the arm establishes is byte-exact behavioural agreement *with the flags set* β nothing more. The actual soundness argument for a moving collector is the static/IR one above (retained `js_shadow_slot_bind`, no raw receiver pointer live across a safepoint), not the flag run. | ||
|
|
||
| Gated by `PERRY_PTR_SHAPE_THIS` (default on, `0`/`off`/`false` disables), keyed into the object cache. | ||
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.