perf(repsel): return-shape facts — Ptr<Shape> survives the return escape (#7034 §4) - #7107
Conversation
391ab9a to
5c21542
Compare
|
Warning Review limit reached
Next review available in: 2 minutes 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 (12)
📝 WalkthroughWalkthroughAdds producer- and caller-side return-shape analysis for ChangesReturn-shape representation selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleDispatchFacts
participant collect_return_shape_functions
participant ptr_shape
participant RuntimeTest
ModuleDispatchFacts->>collect_return_shape_functions: provide module functions and barrier facts
collect_return_shape_functions->>ModuleDispatchFacts: store function ID to class mappings
ptr_shape->>ModuleDispatchFacts: query return_shape_class
ModuleDispatchFacts-->>ptr_shape: return inferred class
RuntimeTest->>ptr_shape: execute returned-object scenarios
ptr_shape-->>RuntimeTest: produce promoted representation results
Possibly related issues
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs (1)
344-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister class
Dso this test isolates the class-disagreement guard.
facts_forpushes only classCinto the module. Thenew D()return refers to a class that the module does not declare. The fact can then be denied becauseDis unknown, not because the two returns disagree on the class. AddDto the module to pin the intended guard.♻️ Proposed change: declare `D` in the fixture module
Add a variant of the module builder that accepts extra classes:
fn facts_for_classes(extra: Vec<Class>, functions: Vec<Function>) -> (ModuleDispatchFacts, Class) { let mut hir = Module::new("t"); hir.classes.push(class_c()); hir.classes.extend(extra); hir.functions = functions; (super::super::collect_module_dispatch_facts(&hir), class_c()) }Then build
Das a copy ofclass_c()withid: 1andname: "D", and usefacts_for_classes(vec![class_d()], vec![...])in this test.🤖 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/collectors/ptr_shape_returns_tests.rs` around lines 344 - 366, Register class D in the disagreeing_return_classes_get_no_fact fixture so the assertion specifically exercises disagreement between known return classes. Add or reuse a module-builder helper that accepts extra classes, create class_d from class_c with id 1 and name "D", and invoke that helper with D and the existing test function.crates/perry-codegen/src/collectors/ptr_shape_returns.rs (1)
137-148: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the non-pointer predicate out of
ptr_shape_returns.rs.
is_definitely_non_pointerrepeatspointer_locals.rs’sis_definitely_non_pointer_type. The current test only checksptr_shape_returns.rscases, so a new non-pointerTypevariant can be added without updating this duplicate. A wrong caller-side non-pointer result can let aPtr<Shape>binding stay in an unrooted alloca. Move or delegate this check to a single shared predicate.🤖 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/collectors/ptr_shape_returns.rs` around lines 137 - 148, Replace the local is_definitely_non_pointer check in ptr_shape_returns.rs with the shared is_definitely_non_pointer_type predicate from pointer_locals.rs, or move the predicate into a common module and reuse it from both locations. Remove the duplicate implementation and ensure the existing return-type filtering in the surrounding collector preserves the same behavior for all Type variants.
🤖 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 `@test-files/test_gap_repsel_return_shape.ts`:
- Around line 188-194: Update maybeRec so its false branch reaches the end of
the function without an explicit return, while preserving the Rec return when b
is true. If bare return behavior also needs coverage, add a separate test case
or helper containing an explicit return statement.
- Around line 164-176: Update returnThenFinally to return the local Rec object o
instead of a computed string, while preserving the try/finally mutations. Move
the field reads and expected-value checks to the caller so they occur after
finally completes and verify that the returned object reflects the final score
mutation.
---
Nitpick comments:
In `@crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs`:
- Around line 344-366: Register class D in the
disagreeing_return_classes_get_no_fact fixture so the assertion specifically
exercises disagreement between known return classes. Add or reuse a
module-builder helper that accepts extra classes, create class_d from class_c
with id 1 and name "D", and invoke that helper with D and the existing test
function.
In `@crates/perry-codegen/src/collectors/ptr_shape_returns.rs`:
- Around line 137-148: Replace the local is_definitely_non_pointer check in
ptr_shape_returns.rs with the shared is_definitely_non_pointer_type predicate
from pointer_locals.rs, or move the predicate into a common module and reuse it
from both locations. Remove the duplicate implementation and ensure the existing
return-type filtering in the surrounding collector preserves the same behavior
for all Type variants.
🪄 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: 6e5ac264-e235-4637-9c67-1c1e16f6efda
📒 Files selected for processing (11)
benchmarks/repsel_census/baseline.jsonchangelog.d/7107-repsel-return-shape-facts.mdcrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/ptr_shape.rscrates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rscrates/perry-codegen/src/collectors/ptr_shape_report.rscrates/perry-codegen/src/collectors/ptr_shape_returns.rscrates/perry-codegen/src/collectors/ptr_shape_returns_tests.rscrates/perry-codegen/src/collectors/scalar_method_dispatch.rstest-files/test_gap_repsel_return_shape.tstest-parity/gc_repsel_corpus.txt
| // 6. `finally` runs after the return value is computed but before the caller | ||
| // resumes — the ordering the return exemption's soundness argument rests on. | ||
| function returnThenFinally(): string { | ||
| const o = new Rec(1, "fin", 10); | ||
| const seen: string[] = []; | ||
| try { | ||
| o.score = 20; | ||
| return o.name + ":" + o.score + ":" + seen.length; | ||
| } finally { | ||
| seen.push("ran"); | ||
| o.score = 999; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the local object from this test.
returnThenFinally returns a computed string. The return-shape fact cannot apply because the local o does not escape through return o.
Return o from this function. Read its fields in the caller after the finally block. This tests the bare-return exemption and verifies that finally mutates the returned object before the caller resumes.
🤖 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 `@test-files/test_gap_repsel_return_shape.ts` around lines 164 - 176, Update
returnThenFinally to return the local Rec object o instead of a computed string,
while preserving the try/finally mutations. Move the field reads and
expected-value checks to the caller so they occur after finally completes and
verify that the returned object reflects the final score mutation.
| // 7b. A producer that can fall through to `undefined`. | ||
| function maybeRec(b: boolean): Rec | undefined { | ||
| if (b) { | ||
| return new Rec(5, "maybe", 5); | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a real fall-through case.
maybeRec explicitly executes return undefined. It does not test a control-flow path that reaches the end of the function.
Remove the final return to test fall-through. Add a separate return; case if bare returns require separate coverage.
🤖 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 `@test-files/test_gap_repsel_return_shape.ts` around lines 188 - 194, Update
maybeRec so its false branch reaches the end of the function without an explicit
return, while preserving the Rec return when b is true. If bare return behavior
also needs coverage, add a separate test case or helper containing an explicit
return statement.
5c21542 to
6817665
Compare
…ape (#7034 §4) `Ptr<Shape>` promoted ZERO locals on batch.ts, the object/property-heavy workload the representation exists for: rule 2 (containment) listed `return` as an outright disqualifier and every real record escapes its producing scope, so the proof died at the first escape. `PERRY_PTR_SHAPE_LOCALS=0` vs default produced an identical `__text` — there was nothing to switch off. Producer side: `return <the local>` no longer disqualifies. A `return` is a terminator — every use of the local in that body either precedes it on that path or is unreachable from it, the sole exception being a `finally` block, which still runs before the caller resumes and whose uses the same walk checks anyway. Only the bare form is exempt; a `return` inside a nested closure body is not, because that value escapes at an unbounded later time. Caller side (new collectors/ptr_shape_returns.rs): a module function whose every return path hands back a freshly allocated, unaliased object of one class carries a return-shape fact, and a direct call to it is rule-1 provenance of `new C(...)` strength. Freshness is discharged by re-running the full Phase 3b proof over the producer's body, not by a weaker approximation of it. No ABI change, no cloning, no cross-call-site agreement. Measured: batch.ts 0 -> 2 promoted locals; app-patterns kernels 2 -> 4; benchmarks/suite unchanged at 4. The #7104 census reports the same delta and its batch ptr-shape floor is ratcheted 0 -> 2, so the gain is now gated. The A/B stops being vacuous: __text moves 1,532 bytes between the arms and guard-gate volatile loads drop 26 -> 22. Size only; nothing was timed (the box was under load 40-135 throughout). A call-seeded candidate never claims numeric_fields: the producer's stores are outside the caller's region, so no exhaustive-reachable-store proof is available. Same stand-down, same reason, as collectors/proven_this.rs. GC: no new site holds an object pointer. The caller's binding is the same NaN-boxed slot it always was, shadow-bound by collect_pointer_typed_locals / js_shadow_slot_bind; verified in the IR that the returned register is stored and bound with no intervening safepoint, and that each access re-derives the pointer from the slot. TaPtr's callee-side no-bind shortcut is not copied — it is sound only for non-movable typed-array storage (#6990, #7019). New guard: a producer annotated with a definitely-non-pointer return type would cost the caller's binding its shadow slot, so it carries no fact. That check calls pointer_locals' own predicate, which this change hoists to module scope so there is exactly one definition — a copy drifting by one Type variant would leave a value unrooted there while proven movable here.
6817665 to
e74bfda
Compare
Does promotion actually make code faster? Measured — Q-A yes, Q-B no, and both at onceThis PR's changelog deliberately declined to make a speed claim ("the box was under load 40–135 throughout and nothing was timed"). This supplies that number, on a quiet host, and keeps the two questions apart:
A null on Q-B is not evidence against Q-A. On Method
Controls, all of which behaved as required:
Q-A — promotion is worth a lot where it is consumedMac mini, 9 interleaved reps, median instructions retired. IR columns are guard-gate volatile loads and by-name field fallbacks,
The table is self-validating: every workload whose IR changed moved, every workload whose IR did not change read 0.00%. Promotion count alone predicts nothing; IR consumption predicts everything. Cross-checked on a second microarchitecture and OS — Raspberry Pi 5 (Cortex-A76, Linux,
The effect is not a microarchitectural artifact. On Linux the For scale, against Node 26.5.1 on the same host (instructions retired):
Consumption evidence — from the IR, not from a counterPer-function guard census on
On ★ A promotion can go unconsumed in three ways, not oneThis PR documents the first. I found a second, and it is the reason the obvious benchmark choice would have produced a false negative.
The boundary between (2) and a real promotion is sharp and I probed it: adding one in-loop field store to Consequence for how this campaign picks benchmarks: the census's three promoting suite workloads — Q-B — what #7107 buys on
|
| arm | median instructions | vs parent.on |
within-arm spread |
|---|---|---|---|
| pre-#7107, knob default | 3,800,130,645 | — | 0.95% |
| pre-#7107, knob off | 3,802,984,004 | +0.075% | 0.94% |
| #7107, knob default | 3,800,942,119 | +0.021% | 0.90% |
| #7107, knob off | 3,802,744,104 | +0.069% | 0.92% |
Every between-arm difference is an order of magnitude below the within-arm spread. The effect is not resolvable; I can only bound it at roughly |Δ| < 0.1%.
This is exactly what the structure predicts. totalsRow — the only function whose code changed — runs a 4-iteration loop, once (summaries.length === BUCKETS.length === 4), i.e. ~16 field operations, against buildRows/shape/ranked/summarize each processing 40,000 rows. The promoted site is ~0.002% of the program's field traffic.
__text reproduces the size claim's direction, at a smaller magnitude than the changelog's 1,532 bytes (I used PERRY_NO_AUTO_OPTIMIZE=1, so the absolute __text differs too):
pre-#7107 knob=on 8,827,532 knob=off 8,827,532 <- identical: nothing to switch off
#7107 knob=on 8,826,884 knob=off 8,827,532 <- -648 bytes
Recommendation: keep the size number, and state plainly that the runtime effect on batch.ts is below measurement. Do not soften it — batch.ts is a promotion-coverage fixture, not a performance one, and it is honest for it to be the former.
Caveat on the −84.4%, stated rather than buried
The magnitude is real but it is not "a 7-load shape check removed". On w5's baseline arm, forcing every access to the out-of-line guard with PERRY_DISABLE_CLASS_FIELD_INLINE=1 made it 0.8% faster (111.63G → 110.73G) — i.e. the inline guarded diamond is earning nothing on that access shape. The same knob on an array-element receiver (a[i].x, no promotion available) costs +47.6% (23.54G → 34.74G), so the inline path is healthy in general and the sticky gate is not tripped (typed feedback is off by default — verified in typed_feedback.rs).
So Ptr<Shape> is currently the only thing making plain-local field traffic fast, because the existing fast path does not cover that shape. Why the inline diamond misses on a monomorphic class Vec3 local deserves its own investigation — if it were fixed, the baseline would improve and this delta would shrink. I did not chase it.
(I also confirmed the 84% is not a NaN-boxing/int32 artifact: a variant storing non-integral doubles gives the identical 111.63G → 17.38G.)
Assessment of the campaign's central assumption
It holds, conditionally, and the conditions are the finding.
Unboxing a value to a proven static representation makes real code substantially faster — 6.4× on field-dominated work, 1.24× on this PR's own idiom — when and only when the promotion is consumed at an access site that actually executes. Neither half is automatic today:
- Consumption is not implied by promotion. Three distinct mechanisms silently drop it, and on
batch.tshalf the reported promotions are dropped. Any future "N promotions" number is uninterpretable without an IR check. - Coverage, not per-site win, is the binding constraint. The per-site win is large and now measured. What is small is how often a promoted site is hot: on
batch.tsand on the micro-benchmarks, essentially never.
The campaign's next marginal engineering is therefore worth more spent on (a) making module-init consume Ptr<Shape> (mechanism 1 — it is a flag, and it is what strands totals) and (b) reaching escape positions that real code actually uses on hot paths, than on further per-site sharpening. #7107 is the right kind of change — it opens an escape position, which is a coverage change — and its -19.4% on the hot form of its own idiom is the honest measure of what the return position is worth once something hot uses it.
Suggested gate discipline, in the spirit of "a gate must assert its subject was live": the census floor should be a floor on consumed promotions — cheaply approximated by asserting the arms' IR differs — not on reported ones. As it stands, w1/w3/w6 would satisfy a promotion floor while emitting byte-identical code.
What I could not measure
- Whether perf(repsel): return-shape facts — Ptr<Shape> survives the return escape (#7034 §4) #7107 helps any real program, as opposed to a constructed hot form of its idiom.
batch.tsis the only committed workload exercising the return position and its promoted site is cold. This needs a workload where a record-producing function is called in a loop — I built one (w4, −19.4%) but it is synthetic. - Why the inline class-field guard misses on plain-local receivers (above).
- The
PERRY_PTR_SHAPE_LOCALS=0arm is not "pre-representation-selection" — it also disables Phase 5a proven-this. Onbatch.tsthe pre-perf(repsel): return-shape facts — Ptr<Shape> survives the return escape (#7034 §4) #7107 knob A/B changes IR (~191 lines) without changing any guard count, which is consistent with a proven-thisclone that__textthen absorbs. - Wall-clock on the primary host is reported but the mini idles at load ~1.2–1.5 (WindowServer, screen-saver). The instruction counts are the claim; the Pi's wall times (idle at load 1.0) are the trustworthy wall numbers and they track the instruction deltas.
Oracle-staleness note, since it was raised: .node-version pins 26.5.1 and no host had it. I installed it on all three and re-ran. On this corpus — including this PR's test_gap_repsel_return_shape.ts and batch.ts — Node 26.5.0 and 26.5.1 produce byte-identical output, so earlier oracle-exactness claims made against 26.5.0 are not invalidated for these files. That is a statement about this corpus only, not a licence to keep using a stale oracle.
Unrelated correctness bug found along the way
w4 initially failed the oracle in every arm — including pre-#7107 and with PERRY_PTR_SHAPE_LOCALS=0 — so it is not a representation-selection defect. Minimised: a string-literal handle is loaded before an allocating call in the same expression and reused after it, so an evacuating GC rewrites the handle global while the register holds the stale pointer.
console.log("acc:" + run(10000000)); // perry prints an EMPTY line; node prints acc:74999992500000
const v = run(10000000);
console.log("hoisted:" + v); // correct%r1 = load double, ptr @…_.str.2.handle ; literal handle read FIRST
%r2 = call double @…__run__spec_i32(i32 10000000) ; 10M allocations -> GC evacuates
%r3 = bitcast double %r1 to i64 ; STALE — no js_gc_temp_root_push
%r5 = call i64 @js_string_concat_value(i64 %r4, double %r2)This is precisely the hazard expr/temp_root.rs (#6951) exists to close — "an already-evaluated operand waiting for its sibling" — with the literal-handle load not covered. Filed as #7114; it should not ride on this PR.
The promotion census (#7104, #7113) counted `select()` calls. That is the wrong quantity: a promotion can be selected, reported as a win, and produce literally nothing. `batch.ts` reports two Ptr<Shape> promotions and applies one. `totals` is proven, counted as a win, and keeps the guarded diamond at every access site; #7107's entire 1,532-byte saving came from `acc`. That was found by reading emitted IR, never by the report. The three census workloads that promote (07_object_create, 09_method_calls, 12_binary_trees) were proposed as the cleanest available experiment for measuring what a promotion is worth. All three would have measured 0.00% -- each declares its promoted local at module top level, and 07/12 are additionally scalar-replaced. With PERRY_PTR_SHAPE_LOCALS=0 the objects for 07/12 are byte-identical to the default; 09 differs only by two __pshape clones with zero call sites. Corpus-wide, Ptr<Shape> is now 6 selected / 2 consumed. Four proven and thrown away: two by the module-init context gate (#7109), two by scalar replacement (#7115, filed by this work -- it was undocumented). No unexplained residue. Consumption is recorded at the six codegen sites that COMMIT to the guard-free lowering, never at a select()-adjacent site, so the count stays checkable against IR rather than against another counter. `outcome` (and, for consumed entries, the consuming site) joins `Entry::dedup_key`: without it every consumption record collapsed into its own selection and the tally was pinned at zero. --opt-report schema goes to 2. `selected` keeps its meaning but explicitly stops implying emitted bytes, so this is a meaning change, not an additive field. The census gains a ptr-shape-consumed column with its own floors, its own LIVENESS_FLOORS minimum, and three new failure modes: consumption recorded outside the selected population, consumption counted per access site rather than per value, and wasted promotions that name no mechanism -- the last is what makes deleting a drop-recorder visible. CONSUMPTION_INSTRUMENTED lives in the script, not the regenerable baseline; only ptr-shape is instrumented and the other keys report no consumption data rather than a zero. No existing floor was lowered; batch's ratcheted ptr-shape: 2 is untouched. Byte-neutral: 23/23 workloads identical with the report off vs on, and 23/23 between the pre-change and post-change compilers with the report off. Sabotage-verified in both directions (harness exit codes, not a wrapper shell's): dropping `outcome` from dedup_key, removing all six consumption recorders, removing either mechanism recorder, counting per access site, folding proven-`this` consumption into the local column, and deleting the consumed liveness minimum each turn the gate red; the unmodified tree is green. The five pre-existing PERRY_*_LOCALS=0 sabotages still go red, and CI's sabotage step now also asserts ptr-shape-consumed tracks the compiler. ptr_shape.rs's number-by-construction proof moves to ptr_shape_numeric.rs to stay under the 2000-line gate. Refs #7106, #7107, #7109, #7115
The promotion census (#7104, #7113) counted `select()` calls. That is the wrong quantity: a promotion can be selected, reported as a win, and produce literally nothing. `batch.ts` reports two Ptr<Shape> promotions and applies one. `totals` is proven, counted as a win, and keeps the guarded diamond at every access site; #7107's entire 1,532-byte saving came from `acc`. That was found by reading emitted IR, never by the report. The three census workloads that promote (07_object_create, 09_method_calls, 12_binary_trees) were proposed as the cleanest available experiment for measuring what a promotion is worth. All three would have measured 0.00% -- each declares its promoted local at module top level, and 07/12 are additionally scalar-replaced. With PERRY_PTR_SHAPE_LOCALS=0 the objects for 07/12 are byte-identical to the default; 09 differs only by two __pshape clones with zero call sites. Corpus-wide, Ptr<Shape> is now 6 selected / 2 consumed. Four proven and thrown away: two by the module-init context gate (#7109), two by scalar replacement (#7115, filed by this work -- it was undocumented). No unexplained residue. Consumption is recorded at the six codegen sites that COMMIT to the guard-free lowering, never at a select()-adjacent site, so the count stays checkable against IR rather than against another counter. `outcome` (and, for consumed entries, the consuming site) joins `Entry::dedup_key`: without it every consumption record collapsed into its own selection and the tally was pinned at zero. --opt-report schema goes to 2. `selected` keeps its meaning but explicitly stops implying emitted bytes, so this is a meaning change, not an additive field. The census gains a ptr-shape-consumed column with its own floors, its own LIVENESS_FLOORS minimum, and three new failure modes: consumption recorded outside the selected population, consumption counted per access site rather than per value, and wasted promotions that name no mechanism -- the last is what makes deleting a drop-recorder visible. CONSUMPTION_INSTRUMENTED lives in the script, not the regenerable baseline; only ptr-shape is instrumented and the other keys report no consumption data rather than a zero. No existing floor was lowered; batch's ratcheted ptr-shape: 2 is untouched. Byte-neutral: 23/23 workloads identical with the report off vs on, and 23/23 between the pre-change and post-change compilers with the report off. Sabotage-verified in both directions (harness exit codes, not a wrapper shell's): dropping `outcome` from dedup_key, removing all six consumption recorders, removing either mechanism recorder, counting per access site, folding proven-`this` consumption into the local column, and deleting the consumed liveness minimum each turn the gate red; the unmodified tree is green. The five pre-existing PERRY_*_LOCALS=0 sabotages still go red, and CI's sabotage step now also asserts ptr-shape-consumed tracks the compiler. ptr_shape.rs's number-by-construction proof moves to ptr_shape_numeric.rs to stay under the 2000-line gate. Refs #7106, #7107, #7109, #7115 Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…lers read them as (#7170 R0) Report-only. Three defects in the `--opt-report` instrumentation, each of which made a scheduler-facing number say something other than what it looked like, and two of which have now mis-scheduled work twice (#7152, #7170). 1. Per-function alloc-site dedup masked rows. Every anonymous literal renders as `object literal { ... }` and carries `byte_offset: 0`, so every unbound literal in one function collapsed onto a single `Entry::dedup_key`. An `alloc_ordinal` (the site's index in the region's deterministic walk) discriminates them; a function lowered twice still walks the same body and still collapses, and what the drain DID collapse is now reported as `summary.masked_by_dedup` rather than left implicit. 2. `constructor argument` conflated two mechanisms. A closed-shape literal lowers to `new __AnonShape_N(v0, ...)` whose constructor arguments ARE its property values, so `{a: {b: 1}}` filed its inner literal as a constructor argument. Split into `constructor argument` and `object literal property value` — measured 5 and 418 on the dependency corpus, so the old label was 98.8 % not-constructor-arguments. 3. The `return` bucket counted syntactic sites, not opportunities. `ptr_shape_returns.rs` (#7107) already admits a bare `return new C(...)` as a producer, but `deny_alloc_site` fires before any seeding. Such sites now carry their own rule and a new `Tier::Served`, so they leave the rule-1 bucket instead of inflating it. The buckets are also computed by the compiler now (`summary.by_rule`, `summary.by_alloc_context`) instead of being reconstructed downstream with `jq` — the reduction that could not see defects 2 and 3 by construction. Census: `alloc_contexts` per workload, plus `ALLOC_BUCKET_FLOORS` and `ALLOC_RULE_FLOORS` held in code, and `fixture_alloc_buckets.ts` written to land one allocation in each bucket. The rule floor is the only layer that can catch the served-return wiring dying, because that wiring lives in `codegen/function.rs` and every compiler unit test sets the report scope by hand. No census floor moved on the existing corpus and no existing workload's candidate count moved: the distortion is invisible on hand-written benchmarks and doubles the row count on real dependency JS, which is why it survived.
…lers read them as (#7170 R0) (#7176) * perf(repsel): make the Ptr<Shape> alloc-site buckets mean what schedulers read them as (#7170 R0) Report-only. Three defects in the `--opt-report` instrumentation, each of which made a scheduler-facing number say something other than what it looked like, and two of which have now mis-scheduled work twice (#7152, #7170). 1. Per-function alloc-site dedup masked rows. Every anonymous literal renders as `object literal { ... }` and carries `byte_offset: 0`, so every unbound literal in one function collapsed onto a single `Entry::dedup_key`. An `alloc_ordinal` (the site's index in the region's deterministic walk) discriminates them; a function lowered twice still walks the same body and still collapses, and what the drain DID collapse is now reported as `summary.masked_by_dedup` rather than left implicit. 2. `constructor argument` conflated two mechanisms. A closed-shape literal lowers to `new __AnonShape_N(v0, ...)` whose constructor arguments ARE its property values, so `{a: {b: 1}}` filed its inner literal as a constructor argument. Split into `constructor argument` and `object literal property value` — measured 5 and 418 on the dependency corpus, so the old label was 98.8 % not-constructor-arguments. 3. The `return` bucket counted syntactic sites, not opportunities. `ptr_shape_returns.rs` (#7107) already admits a bare `return new C(...)` as a producer, but `deny_alloc_site` fires before any seeding. Such sites now carry their own rule and a new `Tier::Served`, so they leave the rule-1 bucket instead of inflating it. The buckets are also computed by the compiler now (`summary.by_rule`, `summary.by_alloc_context`) instead of being reconstructed downstream with `jq` — the reduction that could not see defects 2 and 3 by construction. Census: `alloc_contexts` per workload, plus `ALLOC_BUCKET_FLOORS` and `ALLOC_RULE_FLOORS` held in code, and `fixture_alloc_buckets.ts` written to land one allocation in each bucket. The rule floor is the only layer that can catch the served-return wiring dying, because that wiring lives in `codegen/function.rs` and every compiler unit test sets the report scope by hand. No census floor moved on the existing corpus and no existing workload's candidate count moved: the distortion is invisible on hand-written benchmarks and doubles the row count on real dependency JS, which is why it survived. * docs: changelog fragment for PR 7176 * docs(repsel): state the measured size of the served-return correction * refactor(opt-report): one body behind deny and deny_alloc * fix(repsel): return is a POSITION, not anywhere under a returned expression Review of #7176. Three fixes and a bucket split: - `RETURN` was set once at `Stmt::Return` and `scan_expr` propagated it through the fallback arm, so `return cond ? new C() : new D()`, `return flag && new C()`, `return await new C()` and `return new C().x` all filed their allocations as return positions. That over-counted the `return` bucket -- 323 of which was published on #7170 as R1's ceiling -- and would have handed Tier::Served to operands the return-shape fact does not cover as soon as the producer side widened. `scan_return` is now the only place a return position is produced, and servedness reads `NewSite::is_return_position` rather than comparing a label, so renaming a report bucket cannot silently disable it. Nested allocations get their own honest bucket, `returned expression operand`. - The census floored allocation position and denial rule independently, so a report that filed a `return` under the unserved rule while emitting the served rule on another position satisfied both. One composite table keyed by (analysis, context, rule) replaces both, matching the tuple the renderer already uses. - render_text/render_json read a process-global dedup counter while taking an entries snapshot; `render_text_with`/`render_json_with` take it explicitly and are what tests use, so an absence assertion is order-independent. - rule_buckets kept the first tier seen per rule; it now debug_asserts the 1:1 invariant instead of resolving it silently. * test(opt-report): exercise the rule-to-tier invariant assertion * chore(census): regenerate baseline with the composite allocation buckets * chore(census): regenerate baseline on the pinned base 5a06970 * docs: update the changelog fragment for the review fixes --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
… CJS IIFE (#7170 R1) `cjs_wrap` puts every CommonJS module body in an IIFE, so a module-level `function` declaration never reaches `hir.functions` — it lowers to `Stmt::Let { init: Expr::Closure }`, and a call to it to `Call { callee: LocalGet(id) }`. #7107's producer walked `hir.functions` and its caller-side seed accepted only `Expr::FuncRef`, so the return-shape mechanism was structurally unreachable across CommonJS: 91.6% of dependency-JS allocation sites sit in `closure` regions (#7170 §2/§6). Both halves are extended, because both missed it: * producer — every `Expr::Closure` in the module is a candidate body, keyed by the `FuncId` it already carries (one module-wide counter with `hir.functions`). The two arms are projected onto one `ProducerBody` view so the closure arm cannot prove something weaker than the function arm. * consumer — a `LocalGet` callee resolves through a module-wide binding proof (`single_binding_closure_locals`): exactly one `Stmt::Let` with a closure init, never reassigned at any depth in any body, never also a parameter or `catch` binding. That is the same statement `Expr::FuncRef` makes, and it is the only property the seed needs of a callee. Freshness is unchanged: the full Phase 3b proof still re-runs over the producer's body. Also fixes a latent hole the callee proof would have inherited: `Expr::WithSet` carries its fallback `LocalId` outside any child expression, so `spec_abi_sites::record_expr_use` never recorded `with (o) { x = v }` as a reassignment.
… CJS IIFE (#7170 R1) `cjs_wrap` puts every CommonJS module body in an IIFE, so a module-level `function` declaration never reaches `hir.functions` — it lowers to `Stmt::Let { init: Expr::Closure }`, and a call to it to `Call { callee: LocalGet(id) }`. #7107's producer walked `hir.functions` and its caller-side seed accepted only `Expr::FuncRef`, so the return-shape mechanism was structurally unreachable across CommonJS: 91.6% of dependency-JS allocation sites sit in `closure` regions (#7170 §2/§6). Both halves are extended, because both missed it: * producer — every `Expr::Closure` in the module is a candidate body, keyed by the `FuncId` it already carries (one module-wide counter with `hir.functions`). The two arms are projected onto one `ProducerBody` view so the closure arm cannot prove something weaker than the function arm. * consumer — a `LocalGet` callee resolves through a module-wide binding proof (`single_binding_closure_locals`): exactly one `Stmt::Let` with a closure init, never reassigned at any depth in any body, never also a parameter or `catch` binding. That is the same statement `Expr::FuncRef` makes, and it is the only property the seed needs of a callee. Freshness is unchanged: the full Phase 3b proof still re-runs over the producer's body. Also fixes a latent hole the callee proof would have inherited: `Expr::WithSet` carries its fallback `LocalId` outside any child expression, so `spec_abi_sites::record_expr_use` never recorded `with (o) { x = v }` as a reassignment.
… CJS IIFE (#7170 R1) (#7233) * perf(repsel): make Ptr<Shape> return-shape facts reachable inside the CJS IIFE (#7170 R1) `cjs_wrap` puts every CommonJS module body in an IIFE, so a module-level `function` declaration never reaches `hir.functions` — it lowers to `Stmt::Let { init: Expr::Closure }`, and a call to it to `Call { callee: LocalGet(id) }`. #7107's producer walked `hir.functions` and its caller-side seed accepted only `Expr::FuncRef`, so the return-shape mechanism was structurally unreachable across CommonJS: 91.6% of dependency-JS allocation sites sit in `closure` regions (#7170 §2/§6). Both halves are extended, because both missed it: * producer — every `Expr::Closure` in the module is a candidate body, keyed by the `FuncId` it already carries (one module-wide counter with `hir.functions`). The two arms are projected onto one `ProducerBody` view so the closure arm cannot prove something weaker than the function arm. * consumer — a `LocalGet` callee resolves through a module-wide binding proof (`single_binding_closure_locals`): exactly one `Stmt::Let` with a closure init, never reassigned at any depth in any body, never also a parameter or `catch` binding. That is the same statement `Expr::FuncRef` makes, and it is the only property the seed needs of a callee. Freshness is unchanged: the full Phase 3b proof still re-runs over the producer's body. Also fixes a latent hole the callee proof would have inherited: `Expr::WithSet` carries its fallback `LocalId` outside any child expression, so `spec_abi_sites::record_expr_use` never recorded `with (o) { x = v }` as a reassignment. * docs(changelog): #7233 fragment for the #7170 R1 CJS-IIFE return-shape reach * fix(repsel): enforce FuncId key uniqueness for return-shape facts (#7170 R1) * docs(repsel): correct two stale doc lines in ptr_shape_returns.rs --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Phase P2 of the #7034 scoping plan: make the
Ptr<Shape>proof survive the return escape.The problem
Ptr<Shape>promoted zero locals onbenchmarks/app-patterns/kernels/batch.ts— the object/property-heavy program the representation exists for.collectors/ptr_shape.rsrule 2 (containment) listedreturnas an outright disqualifier, and every real record escapes its producing scope, so the proof died at the first escape. Independently:PERRY_PTR_SHAPE_LOCALS=0versus default produced an identical__text. There was nothing to switch off.Returns were identified in #7034 §4/§5 as the cheapest of the three escape positions — no ABI change, no function cloning, no cross-call-site agreement — which is why they go first.
What landed
Producer side (
ptr_shape.rs, rule 2).return <the local>no longer disqualifies. Areturnis a terminator: every use of the local in that body either precedes it on that path or is unreachable from it. The sole exception is afinallyblock, which still runs before the caller resumes and whose uses this same walk checks anyway — so the caller cannot have reshaped the object at any access this pass licenses. Deliberately narrow:return [o],return {a: o},return f(o),return c ? o : xall still escape.UseWalk::in_closure): that value escapes at an unbounded later time, so it is not a terminator for the enclosing function's local. The guard holds even where capture analysis has not marked the reference.Caller side (new
collectors/ptr_shape_returns.rs). A module function whose every return path hands back a freshly allocated, unaliased object of one class carries a return-shape fact; a directExpr::FuncRefcall to it is then rule-1 provenance of exactlynew C(...)strength, and the result binding becomes an ordinaryPtr<Shape>candidate subject to rules 2–5 as usual.Rule 1's
new C(...)seed carries two facts — exact dynamic class, and no other reference exists yet. A call site gets neither for free (function get() { return CACHE; }also "returns a C"). Freshness is therefore discharged by re-running the full Phase 3b proof over the producer's body, not by a second, weaker approximation of it: a local that proof promotes has, by rule 2, no alias anywhere. No fact is issued forreturn CACHE, a fall-through-to-undefinedpath, a barereturn;, disagreeing return classes, an async/generator producer, or an indirect callee.A call-seeded candidate never claims
numeric_fields— the producer's stores are outside the caller's region, so no exhaustive-reachable-store proof is available. Same stand-down, same reason, ascollectors/proven_this.rs.Gated by the existing
PERRY_PTR_SHAPE_LOCALS. No new env knob, so no new unexercised off-state.Numbers
batch.tsaccintotalsRow;totalsat module scope)benchmarks/app-patterns/kernels/(12 files)benchmarks/suite/(30 files)test_gap_repsel_return_shape.tsThe #7104 promotion census reports the same delta independently, and its
batchptr-shapefloor is ratcheted 0 → 2 here, so the gain is gated: withPERRY_PTR_SHAPE_LOCALS=0the census goes red onbatch 0 (floor 2)(verified).The A/B stops being vacuous. On
batch.ts,PERRY_PTR_SHAPE_LOCALS=0vs default:__text(this PR)__text(before)LOCALS=1LOCALS=0— a 1,532-byte move where there was none. In the emitted IR, guard-gate volatile loads drop 26 → 22 and by-name field fallbacks 53 → 51.
Size and speed are reported separately. Everything above is a static count or a section size. Nothing was timed: the box carried load 40–135 throughout (concurrent agent builds), and this campaign has twice recorded bogus levers from measuring under load. No speed claim is made.
Known limitation (stated, not papered over)
Module-init contexts set
repsel_context_allows_canonical_i32: false— a pre-existing Phase 1 decision incodegen/entry.rs— andFnCtx::ptr_shape_receiver_factgates on that flag. Sototals, a module-scope binding, is proven and reported as a win but its access sites keep the guarded lowering. One of the twobatch.tspromotions is currently unconsumed, and the 1,532-byte__textdelta is attributable toaccalone. Making module-init consumePtr<Shape>is a separate, independently-measurable change and is not attempted here.GC contract
No new site holds an object pointer. The caller's binding is the same NaN-boxed local slot it always was, shadow-bound by
collect_pointer_typed_locals/js_shadow_slot_bindlike any other object local. Verified in the emitted IR forbatch.ts:— the returned register is stored and bound with no safepoint between the return and the bind, and it is dead immediately after. Every access re-derives the raw pointer from the slot inside one region (
ptr_shape.rs's tagged-at-rest contract); because the slot address escapes to the shadow registry, LLVM cannot CSE the reload across a safepoint.TaPtr's callee-side no-bind shortcut is explicitly not copied — it is sound only for non-movable typed-array storage, andGC_TYPE_OBJECTmoves (#6990; #7019 ships evacuating young-gen scavenge default-on).One new guard the original sketch did not have.
collect_pointer_typed_localsdrops a local's shadow slot when it can prove the value non-pointer, and for a call it proves that from the callee's declared return type. A producer annotated: numberthat actually returns an object would leave the caller with a promotedPtr<Shape>local in an unrooted alloca — an evacuating minor would move the object without rewriting the slot. Perry does not check annotations, so such a producer carries no fact (is_definitely_non_pointer(&f.return_type)).Observed directly under the corpus run:
root_sources.compiled_shadow.rewritten_slots = 3per copying minor — the collector is rewriting the bound slots while output stays oracle-exact.Verification
GC × repsel matrix —
scripts/gc_repsel_matrix.sh --arms all --pressure 8, oracle Node 26.5.1:against the 440-cell baseline of
PASS=339 UNVER=100 XFAIL=1 FAIL=0. The new corpus file adds 20 cells and all 20 are PASS, none UNVER — it was live on every arm that requires liveness.XFAILis the known #6984 (PERRY_PTR_SHAPE_LOCALS=0breakstest_gap_repsel_ptr_shape_locals), untouched. The per-arm liveness table shows everyrequires=movearm actually moving objects (22/23 collected, 22/23 moved, 22/23 copy-minor).Rest:
ptr_shape_returns_tests.rs); 326perry-codegenlib tests green.in_closuretracking, the freshness body-proof, the non-pointer-return-type guard, or the fall-through check each makes exactly the test that names it fail; the tree was verified restored afterwards.test-files/test_gap_repsel_return_shape.ts, registered intest-parity/gc_repsel_corpus.txt(the matrix exits 3 on an unregisteredtest_gap_repsel_*). Byte-exact against the pinned Node 26.5.1 oracle on default,PERRY_PTR_SHAPE_LOCALS=0,PERRY_GC_HEAP_LIMIT=8,PERRY_GC_FORCE_EVACUATE=1, and conservative-scan-off.console.timewall-clock output (which differs from itself between two runs of the same binary), six are an identical pre-existingperry-ext-http/src/server/server.rs:911panic present in both arms, differing only in thread-id and ASLR addresses.Pre-existing failures on
main, unrelated to this PRFlagging these because they will appear on this PR's checks and are not caused by it — each verified at the base commit:
scripts/check_file_size.shexits 1 on 15 over-limit files (e.g.codegen/typed_abi.rs, already 2,094 lines at the base commit). None are in this diff; the script is unmodified.collectors/ptr_shape.rsis at 1,998 after this change — under the gate, but with almost no headroom left.scripts/addr_class_inventory.pyexits 1 onobject/native_module/constants.rs:2050(O_SYMLINKband literal).perry-codegen'smanifest_consistencyintegration test fails on 3 missingdecimal.js/bignumber.jsmanifest rows.Not measured
--releasebuild ofperryplus the-staticwrappers, and free disk fell from 36 GB to 25 GB during this session under concurrent agent builds; starting a release build risked the ~15 GB working floor. This diff touchescrates/, so the workflow's relevance filter will run it on this PR and produce the authoritative gated and ungated columns.--profile perry-dev --no-buildagainst a locally built compiler rather than--profile release; arms and corpus are identical, but absolute cell counts are not directly comparable to a release-profile baseline.Rec.bumpin the new gap test is denied by rule 3 (this-flow) for reasons unrelated to the return position — athis-flow walk that appears to have the same return-position blindness rule 2 had. Noted, not investigated.Advances #7034 (P2 of P0–P6). Follows #7104 (P1, the census this PR ratchets).
Summary by CodeRabbit
New Features
Bug Fixes
finallyblocks.Tests