From 26429d2175c046dc38ecce4a036cd36b94ef2cd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 10:31:09 +0200 Subject: [PATCH 1/9] perf(repsel): make the Ptr alloc-site buckets mean what schedulers read them as (#7170 R0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- benchmarks/repsel_census/baseline.json | 113 +++++-- .../fixtures/fixture_alloc_buckets.ts | 84 +++++ crates/perry-codegen/src/codegen/function.rs | 14 +- .../src/collectors/cjs_scaffolding.rs | 9 +- .../collectors/ptr_shape_opt_report_tests.rs | 266 ++++++++++++++++ .../src/collectors/ptr_shape_report.rs | 131 ++++++-- crates/perry-codegen/src/opt_report/mod.rs | 248 +++++++++++++++ crates/perry-codegen/src/opt_report/render.rs | 286 +++++++++++++++++- .../compiler_output_harness/repsel_census.py | 195 ++++++++++++ 9 files changed, 1294 insertions(+), 52 deletions(-) create mode 100644 benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts diff --git a/benchmarks/repsel_census/baseline.json b/benchmarks/repsel_census/baseline.json index 69275442a3..362121606b 100644 --- a/benchmarks/repsel_census/baseline.json +++ b/benchmarks/repsel_census/baseline.json @@ -28,7 +28,8 @@ "ptr_shape_get_number": 2, "ptr_shape_set": 1, "ptr_shape_method": 1 - } + }, + "alloc_contexts": {} }, { "name": "fixture_ptr_shape_sites", @@ -59,7 +60,8 @@ "ptr_shape_set": 1, "ptr_shape_update": 1, "ptr_shape_method": 1 - } + }, + "alloc_contexts": {} }, { "name": "fixture_ptr_shape_elements", @@ -89,6 +91,39 @@ "ptr_shape_method": 1, "class_field_get.shape_proven_load": 1, "ptr_shape_set": 2 + }, + "alloc_contexts": {} + }, + { + "name": "fixture_alloc_buckets", + "role": "liveness", + "source": "benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts", + "floors": { + "ptr-shape": 3, + "ptr-shape-consumed": 0, + "ptr-numarray": 0, + "canonical-i32": 0, + "canonical-u32": 0, + "canonical-str": 0, + "int-valued-ta": 0, + "spec-abi-entry": 0, + "spec-abi-taptr-slot": 0 + }, + "candidates": { + "ptr-shape": 6, + "ptr-numarray": 0, + "canonical-slot": 0, + "int-valued-ta": 0, + "spec-abi": 0 + }, + "unconsumed_mechanisms": { + "scalar_replaced": 3 + }, + "consumption_sites": {}, + "alloc_contexts": { + "return": 1, + "constructor argument": 1, + "object literal property value": 1 } }, { @@ -114,7 +149,8 @@ "spec-abi": 2 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "fixture_canonical_slots", @@ -139,7 +175,8 @@ "spec-abi": 3 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "fixture_int_valued_ta", @@ -164,7 +201,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "fixture_spec_abi_taptr", @@ -189,7 +227,8 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "fixture_loop_bounded_i32", @@ -214,7 +253,8 @@ "spec-abi": 5 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "fixture_module_init_canonical", @@ -239,7 +279,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "batch", @@ -270,6 +311,9 @@ "ptr_shape_get_number": 1, "ptr_shape_set": 2, "class_field_get.shape_proven_load": 2 + }, + "alloc_contexts": { + "return": 1 } }, { @@ -295,7 +339,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_02_loop_overhead", @@ -320,7 +365,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_03_array_write", @@ -345,7 +391,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_04_array_read", @@ -370,7 +417,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_05_fibonacci", @@ -395,7 +443,8 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_06_math_intensive", @@ -420,7 +469,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_07_object_create", @@ -447,7 +497,8 @@ "unconsumed_mechanisms": { "scalar_replaced": 1 }, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_08_string_concat", @@ -472,7 +523,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_09_method_calls", @@ -503,7 +555,8 @@ "class_field_get.shape_proven_load": 1, "ptr_shape_get_number": 1, "ptr_shape_set": 1 - } + }, + "alloc_contexts": {} }, { "name": "suite_10_nested_loops", @@ -528,7 +581,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_11_prime_sieve", @@ -553,7 +607,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_12_binary_trees", @@ -580,7 +635,8 @@ "unconsumed_mechanisms": { "scalar_replaced": 1 }, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_13_factorial", @@ -605,7 +661,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_14_closure", @@ -630,7 +687,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_15_mandelbrot", @@ -655,7 +713,8 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_16_matrix_multiply", @@ -680,7 +739,8 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} }, { "name": "suite_17_loop_data_dependent", @@ -705,8 +765,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {} } ], - "generated_at": "2026-07-31T21:18:40.428173Z" + "generated_at": "2026-08-01T08:24:43.033789Z" } diff --git a/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts new file mode 100644 index 0000000000..4625014468 --- /dev/null +++ b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts @@ -0,0 +1,84 @@ +// Liveness fixture for the `Ptr` ALLOC-SITE BUCKETS (#7170 R0). +// +// The other `ptr_shape` fixtures assert that a promotion can be observed. This +// one asserts something the census could not see at all before R0: that the +// three *denial* buckets the report files unbound allocations into are still +// being told apart. +// +// They were not. #7170 measured two merges, each of which made a +// scheduler-facing number mean something other than what it looked like: +// +// * `constructor argument` conflated a genuine `new C(arg)` (2.0% of sites +// on the dependency corpus) with the property values of anonymous-shape +// object literals (24.5%) — because `{a: {b: 1}}` lowers to +// `new __AnonShape_N()`, so the inner literal really is an +// argument of a `new`, just not of a constructor anybody wrote. +// * the `return` bucket counted syntactic sites rather than opportunities: +// `collectors/ptr_shape_returns.rs` (#7107) already admits a bare +// `return new C(...)` as a return-shape producer, but `deny_alloc_site` +// fires before any seeding and could not know. +// +// Each of the three shapes below lands in exactly one bucket, and +// `ALLOC_BUCKET_FLOORS` / `ALLOC_RULE_FLOORS` require all three to be +// reported. A classification arm that stops firing re-merges its bucket into a +// neighbour, the count for that bucket goes to zero, and the census goes red — +// instead of the merge being invisible, which is the state R0 found. +// +// This is also the only layer that exercises the WIRING. The unit tests set the +// report scope by hand, so `codegen/function.rs` could pass `false` for every +// function's return-shape fact and every one of them would still pass. Here the +// fact has to travel from `collect_return_shape_functions` through +// `ModuleDispatchFacts` into the region scope for `makePoint`'s row to carry +// the served rule. +// +// Do not "tidy" this file: +// * giving `makePoint` a second return of a different class, or an early +// `return;`, revokes its return-shape fact and takes the served bucket to +// zero; +// * binding the inner literal of `nested` to its own `const` moves it out of +// the anonymous-shape-component bucket entirely (it becomes rule 1's own +// seed); +// * adding `Object.defineProperty` / `delete` / `setPrototypeOf` / `Proxy` +// anywhere arms the rule-5 module barrier, which kills every return-shape +// fact in the module and silently empties the served bucket. + +class Point { + x: number; + y: number; + constructor(x: number, y: number) { + this.x = x; + this.y = y; + } +} + +class Boxed { + v: Point; + constructor(v: Point) { + this.v = v; + } +} + +// Bucket 1 — `return`, in a function that DOES carry a return-shape fact: +// every return is a bare fresh `new` of one declared, admissible class, the +// body cannot fall off its end, and the declared return type is a pointer. +// So this row must be filed under the served rule, not the rule-1 wall. +export function makePoint(i: number): Point { + return new Point(i, i + 1); +} + +// Bucket 2 — a genuine `new C(arg)` constructor argument. `Boxed` is a class +// the developer declared, so `new Point(1, 2)` here is what the label says. +const boxed = new Boxed(new Point(1, 2)); + +// Bucket 3 — an anonymous-shape component. `{ inner: ..., k: 3 }` is a closed +// shape, so it lowers to `new __AnonShape_N(, 3)` and the inner +// literal arrives as an argument of that synthetic constructor. It is a field +// value of a parent allocation that is itself unbound: proving its shape +// licenses nothing on its own, which is exactly why it must not share a bucket +// with the line above. +const nested = { inner: { a: 1, b: 2 }, k: 3 }; + +const p = makePoint(3); +console.log( + "alloc_buckets:" + (boxed.v.x + nested.inner.a + nested.k + p.x + p.y), +); diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index a48c35b494..b755bf5b10 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -594,8 +594,18 @@ pub(super) fn compile_function( .unwrap_or_default(); // `--opt-report` (#6952): attribute every representation decision the // collectors below make to this function. No-op when the report is off. - let _opt_report_scope = - crate::opt_report::enter_region(&f.name, crate::opt_report::RegionKind::Function); + // + // #7170 R0: this is the only region opener holding both the `FuncId` and + // `ModuleDispatchFacts`, so it is the only one that can say whether this + // body's `return new C(...)` sites are already served by #7107's + // return-shape mechanism. Read by nothing but the report. + let _opt_report_scope = crate::opt_report::enter_function_region( + &f.name, + cross_module + .module_dispatch + .return_shape_class(f.id) + .is_some(), + ); let native_facts = crate::collectors::collect_native_region_fact_graph_with_spec_lens( &f.body, &f.params, diff --git a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs index 9fe1855c37..cb0f89538d 100644 --- a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs +++ b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs @@ -1325,8 +1325,15 @@ mod tests { let base = super::super::ptr_shape_report::unbound_new_sites(®ion, &CjsPreamble::default()); let contexts: Vec<&str> = base.iter().map(|s| s.context).collect(); + // #7170 §5.1 / R0 renamed this bucket, and the rename is measured on + // exactly this statement: `const __cjs_module = { exports: {} }` is an + // anonymous-shape allocation whose constructor arguments ARE its + // property values, so the inner `{}` is a *component of a literal*, not + // an argument of a `new C(...)`. It was 188 of the 194 rows the old + // `constructor argument` label carried (PR #7171 §1) — which is why + // that label could not be read as "constructor arguments". assert!( - contexts.contains(&"constructor argument"), + contexts.contains(&"object literal property value"), "the `{{ exports: {{}} }}` inner literal is no longer reported \ unsuppressed; this test's premise is gone: {contexts:?}" ); diff --git a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs index fcfb1d6350..4eecb47be8 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs @@ -332,3 +332,269 @@ fn nothing_is_recorded_when_the_report_is_off() { "the sink must stay empty when the report is off" ); } + +// ── #7170 R0: what the alloc-site buckets MEAN ───────────────────────────── +// +// Three defects, measured on 197 dependency modules, each of which made a +// scheduler-facing number say something other than what it looked like. The +// tests below are grouped by defect and each one is written so that reverting +// its arm in the compiler takes it — and only it — red. + +fn anon_shape(args: Vec) -> Expr { + Expr::New { + class_name: "__AnonShape_1f2e".to_string(), + args, + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + } +} + +fn alloc_rows(entries: &[crate::opt_report::Entry]) -> Vec<&crate::opt_report::Entry> { + entries + .iter() + .filter(|e| e.position == Position::AllocSite) + .collect() +} + +/// Defect 2 (#7170 §5.1). A closed-shape object literal lowers to +/// `new __AnonShape_N(v0, …)` whose constructor arguments ARE its property +/// values, so `{a: {b: 1}}` filed its inner literal under +/// `constructor argument` — the label for a genuine `new C(arg)`. On the +/// dependency corpus the two are 24.5% and 2.0% of sites: one bucket was 92% +/// the other thing. +#[test] +fn a_nested_object_literal_is_not_filed_as_a_constructor_argument() { + let classes = HashMap::new(); + let stmts = vec![Stmt::Let { + id: 1, + name: "outer".to_string(), + ty: Type::Any, + mutable: false, + init: Some(anon_shape(vec![anon_shape(Vec::new())])), + }]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1, "the inner literal is the only unbound site"); + assert_eq!( + rows[0].alloc_context.as_deref(), + Some("object literal property value"), + "a property value of an anonymous shape is not a constructor argument" + ); +} + +/// The other direction: a genuine `new C(arg)` argument must KEEP the +/// `constructor argument` bucket. Without this the split is satisfiable by a +/// rule that simply renames everything. +#[test] +fn a_real_constructor_argument_keeps_its_own_bucket() { + let c = class_with_fields("C", &["x"]); + let boxc = class_with_fields("Box", &["v"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + classes.insert("Box".to_string(), &boxc); + let stmts = vec![Stmt::Let { + id: 1, + name: "b".to_string(), + ty: Type::Any, + mutable: false, + init: Some(Expr::New { + class_name: "Box".to_string(), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }), + }]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].alloc_context.as_deref(), + Some("constructor argument"), + "`new Box(new C())` really is a constructor argument" + ); +} + +/// Defect 3 (#7170 §5.2). `deny_alloc_site` fires unconditionally at the top +/// of the collector, before any seeding, so a `return new C(...)` in a +/// function that ALREADY carries a return-shape fact (#7107) was counted as a +/// rule-1 miss. It is not one: the class reaches every +/// `const r = producer(...)` call site. +#[test] +fn a_return_in_a_return_shape_producer_is_reported_as_served() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].rule.as_deref(), + Some("rule 1 (provenance) — already served by return-shape"), + "a served return must leave the rule-1 bucket, not merely annotate it" + ); + assert_eq!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// The same site in a function WITHOUT a return-shape fact stays an ordinary +/// rule-1 denial. This is the anti-vacuity half: a classifier that answers +/// "served" unconditionally passes the test above and fails this one. +#[test] +fn a_return_in_a_plain_function_is_still_a_rule_1_denial() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", false); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].rule.as_deref(), Some("rule 1 (provenance)")); + assert_eq!( + rows[0].tier, + Some(crate::opt_report::Tier::CompilerLimitation) + ); +} + +/// #7170 §6, as a test rather than a comment: **91.6% of dependency-JS +/// allocation sites are in closure regions, and none of them is served.** +/// `collect_return_shape_functions` issues facts only for `hir.functions` +/// entries and the caller-side seed only fires on a bare `Expr::FuncRef` +/// callee, which a closure call never is. A classifier that keyed servedness +/// on the syntax of the returns alone — rather than on the fact — would mark +/// this whole population served and delete the wall it is supposed to measure. +#[test] +fn a_closure_region_never_reports_a_served_return() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_closure("mk", 7); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].rule.as_deref(), + Some("rule 1 (provenance)"), + "a closure body cannot carry a return-shape fact" + ); +} + +/// Only the RETURN position is served. `return f({a: 1})` puts the literal in +/// a call-argument position of a function that is not a producer at all, and +/// even inside a real producer a nested argument is not one of the returns. +#[test] +fn only_the_return_position_is_marked_served() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + // `return new C(new C())` — outer is the return, inner is an argument. + let stmts = vec![Stmt::Return(Some(Expr::New { + class_name: "C".to_string(), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 2, "outer return plus inner argument"); + let served: Vec<_> = rows + .iter() + .filter(|e| e.tier == Some(crate::opt_report::Tier::Served)) + .collect(); + assert_eq!(served.len(), 1, "exactly the return position is served"); + assert_eq!(served[0].alloc_context.as_deref(), Some("return")); +} + +/// Defect 1 (PR #7171 §3). Every anonymous literal renders as +/// `object literal { ... }` and carries `byte_offset: 0`, so +/// `(module, function, name, position, analysis, outcome, rule)` was the same +/// tuple for every unbound literal in a function and all but one vanished. +/// PR #7171 measured the consequence directly: suppressing 379 scaffolding +/// rows REVEALED 62 real user rows the dedup had been hiding. +#[test] +fn two_unbound_literals_in_one_function_are_two_rows() { + let classes = HashMap::new(); + let stmts = vec![ + Stmt::Expr(anon_shape(Vec::new())), + Stmt::Expr(anon_shape(Vec::new())), + ]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!( + rows.len(), + 2, + "two syntactic allocations are two rows, not one: {rows:#?}" + ); + let mut ordinals: Vec<_> = rows.iter().filter_map(|e| e.alloc_ordinal).collect(); + ordinals.sort_unstable(); + assert_eq!(ordinals, vec![0, 1]); +} + +/// ...and the de-duplication that motivated the key still works: a function +/// lowered twice (a boxed entry plus a typed clone) walks the same body and +/// produces the same ordinals, so its rows still collapse — and the count of +/// what collapsed is reported rather than left implicit. +#[test] +fn a_region_lowered_twice_still_collapses_and_says_how_much() { + let classes = HashMap::new(); + let stmts = vec![ + Stmt::Expr(anon_shape(Vec::new())), + Stmt::Expr(anon_shape(Vec::new())), + ]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("twice", false); + let _ = run(&stmts, &classes); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + assert_eq!( + alloc_rows(&entries).len(), + 2, + "the second lowering collapses" + ); + assert_eq!( + crate::opt_report::masked_by_dedup(), + 2, + "and the report says how many rows it folded away" + ); +} diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index 628fbae0c8..305afa20f2 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -46,6 +46,11 @@ pub(super) struct ShapeDenial { } const RULE1: &str = "rule 1 (provenance)"; +/// #7170 R0. A **separate rule string**, not a flavour of [`RULE1`]: the rule +/// name is what every scheduler-facing tally buckets on (#7152 and #7170 both +/// counted `rule 1` rows straight off the report), so a site that an existing +/// mechanism already serves has to leave that bucket, not merely annotate it. +const RULE1_SERVED: &str = "rule 1 (provenance) — already served by return-shape"; const RULE2: &str = "rule 2 (containment)"; const RULE3: &str = "rule 3 (this-flow containment)"; const RULE4: &str = "rule 4 (dispatch stability)"; @@ -64,6 +69,32 @@ pub(super) const UNBOUND_ALLOC: ShapeDenial = ShapeDenial { issue: Some("#7034 §4 (return-shape facts)"), }; +/// #7170 §5.2, R0: the same syntactic site as [`UNBOUND_ALLOC`], in the return +/// position of a function that **already carries a return-shape fact**. +/// +/// `report::deny_alloc_site` runs at the top of +/// `collect_shape_proven_ptr_locals`, before any seeding — it cannot see that +/// `collectors/ptr_shape_returns.rs` admits a bare `return new C(...)` with no +/// local at all as a producer, so the class does reach every +/// `const r = producer(…)` caller. Left under [`UNBOUND_ALLOC`] these rows +/// counted as rule-1 misses, and #7170 measured the resulting 506/963 headline +/// inflated by exactly them. +/// +/// **What this asserts, precisely:** the fact was *issued* for the enclosing +/// function. Whether any caller consumed it is a different question with a +/// different counter (`Outcome::Consumed`), and this row makes no claim about +/// it. +pub(super) const UNBOUND_ALLOC_SERVED_RETURN: ShapeDenial = ShapeDenial { + rule: RULE1_SERVED, + reason: "returned from a function that carries a return-shape fact, so its \ + class already reaches every `const r = producer(...)` call site \ + (#7107). The allocation itself is still unbound — rule 1 cannot \ + anchor to it — but this is NOT an unserved position, and counting \ + it as one over-states the rule-1 wall.", + tier: Tier::Served, + issue: Some("#7107 (return-shape facts, shipped)"), +}; + pub(super) const LET_INIT_NOT_NEW: ShapeDenial = ShapeDenial { rule: RULE1, reason: "the binding is re-declared with an initializer that is not the \ @@ -365,23 +396,44 @@ pub(super) fn deny_local( } /// Record an allocation site that never became a candidate (rule 1). +/// +/// #7170 R0 changed two things here and nothing else: +/// +/// 1. The denial is [`UNBOUND_ALLOC_SERVED_RETURN`] when the site is in return +/// position of a function that already carries a return-shape fact, so the +/// served population leaves the rule-1 bucket instead of inflating it. +/// 2. The syntactic position and the site's walk ordinal are recorded as +/// first-class fields, which is what lets `Entry::dedup_key` tell two +/// allocations in one function apart. Every object literal renders as +/// `object literal { ... }` and carries `byte_offset: 0`, so before this +/// they collapsed onto one row. pub(super) fn deny_alloc_site(site: &NewSite) { if !opt_report::enabled() || suppressed() { return; } - opt_report::deny(Denial { - position: Position::AllocSite, - name: &site.display, - local_id: None, - analysis: Analysis::PtrShape, - rule: UNBOUND_ALLOC.rule, - reason: UNBOUND_ALLOC.reason, - tier: UNBOUND_ALLOC.tier, - issue: UNBOUND_ALLOC.issue, - loop_depth: site.loop_depth, - detail: Some(format!("allocation position: {}", site.context)), - byte_offset: (site.byte_offset != 0).then_some(site.byte_offset), - }); + let served = site.context == RETURN && opt_report::region_is_return_shape_producer(); + let d = if served { + UNBOUND_ALLOC_SERVED_RETURN + } else { + UNBOUND_ALLOC + }; + opt_report::deny_alloc( + Denial { + position: Position::AllocSite, + name: &site.display, + local_id: None, + analysis: Analysis::PtrShape, + rule: d.rule, + reason: d.reason, + tier: d.tier, + issue: d.issue, + loop_depth: site.loop_depth, + detail: Some(format!("allocation position: {}", site.context)), + byte_offset: (site.byte_offset != 0).then_some(site.byte_offset), + }, + site.context, + site.ordinal, + ); } // ── Auxiliary walks (only run under the report flag) ─────────────────────── @@ -456,6 +508,41 @@ fn walk_lets(stmts: &[Stmt], depth: u32, f: &mut impl FnMut(u32, &str, u32)) { } } +// ── Allocation-position vocabulary ───────────────────────────────────────── +// +// Named constants rather than inline literals, because #7170 §5.1 is a bug +// about exactly one of these strings meaning two different things. `RETURN` is +// additionally compared against in `deny_alloc_site`, and a typo there would +// silently disable the served-return classification. + +const RETURN: &str = "return"; +/// A genuine `new C(arg)` argument — the developer wrote a constructor call. +const CTOR_ARG: &str = "constructor argument"; +/// A property value of an anonymous-shape object literal. +/// +/// #7170 §5.1: a closed-shape literal lowers to +/// `new __AnonShape_N(v0, v1, …)` whose constructor arguments *are* its +/// property values (`perry-hir/src/lower/expr_object.rs`), so `{a: {b: 1}}` +/// filed its inner literal under [`CTOR_ARG`]. Measured on 197 dependency +/// modules the two are 2.0% and 24.5% of sites respectively — one bucket was +/// 92% the other thing. They are not the same opportunity: a nested literal is +/// a field value of a parent allocation that is itself unbound, so proving its +/// shape licenses nothing on its own. +const ANON_SHAPE_COMPONENT: &str = "object literal property value"; + +/// The allocation-position label for the arguments of `class_name`. +fn arg_context(class_name: &str) -> &'static str { + if is_anon_shape(class_name) { + ANON_SHAPE_COMPONENT + } else { + CTOR_ARG + } +} + +fn is_anon_shape(class_name: &str) -> bool { + class_name.starts_with("__AnonShape") +} + /// An object allocation that is not the initializer of a `Stmt::Let`. pub(super) struct NewSite { /// `new Row(...)` / `{ key, value }` — what the developer wrote. @@ -463,6 +550,9 @@ pub(super) struct NewSite { /// Where it sits: `return`, `call argument`, `array element`, … pub context: &'static str, pub loop_depth: u32, + /// Index of this site in the region's walk. A de-duplication discriminant; + /// see [`crate::opt_report::Entry::alloc_ordinal`]. + pub ordinal: u32, /// Byte offset of the `new` expression in its module's source. `Expr::New` /// is the one HIR node that already carries a source position (#5253, /// captured for constructor TypeErrors), so allocation sites — which have @@ -503,11 +593,14 @@ fn scan_stmts( // The Let init IS the provenance site rule 1 accepts; skip it and // scan only its arguments. Stmt::Let { - init: Some(Expr::New { args, .. }), + init: Some(Expr::New { + class_name, args, .. + }), .. } => { + let ctx = arg_context(class_name); for a in args { - scan_expr(a, depth, "constructor argument", out); + scan_expr(a, depth, ctx, out); } } Stmt::Let { init, .. } => { @@ -517,7 +610,7 @@ fn scan_stmts( } Stmt::Expr(e) => scan_expr(e, depth, ctx, out), Stmt::Throw(e) => scan_expr(e, depth, "throw", out), - Stmt::Return(Some(e)) => scan_expr(e, depth, "return", out), + Stmt::Return(Some(e)) => scan_expr(e, depth, RETURN, out), Stmt::Return(None) => {} Stmt::If { condition, @@ -608,10 +701,12 @@ fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { display: display_class(class_name), context: ctx, loop_depth: depth, + ordinal: out.len() as u32, byte_offset: *byte_offset, }); + let arg_ctx = arg_context(class_name); for a in args { - scan_expr(a, depth, "constructor argument", out); + scan_expr(a, depth, arg_ctx, out); } } Expr::Call { callee, args, .. } => { @@ -633,7 +728,7 @@ fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { /// "__AnonShape_…" }`; render them as the object literals the developer /// actually wrote rather than leaking the synthetic class name. fn display_class(class_name: &str) -> String { - if class_name.starts_with("__AnonShape") { + if is_anon_shape(class_name) { String::from("object literal { ... }") } else { format!("new {class_name}(...)") diff --git a/crates/perry-codegen/src/opt_report/mod.rs b/crates/perry-codegen/src/opt_report/mod.rs index 9b5a08442b..cc57f4933c 100644 --- a/crates/perry-codegen/src/opt_report/mod.rs +++ b/crates/perry-codegen/src/opt_report/mod.rs @@ -100,6 +100,9 @@ pub(crate) mod test_support { .unwrap_or_else(|e| e.into_inner()); FORCED.store(true, Ordering::Relaxed); let _ = super::take_entries(); + // The drain above may itself have collapsed leftovers; zero the + // counter so a hand-built render test never reads a neighbour's. + super::MASKED_BY_DEDUP.store(0, Ordering::Relaxed); Session { _guard: guard } } @@ -112,6 +115,7 @@ pub(crate) mod test_support { .unwrap_or_else(|e| e.into_inner()); FORCED.store(false, Ordering::Relaxed); let _ = super::take_entries(); + super::MASKED_BY_DEDUP.store(0, Ordering::Relaxed); Session { _guard: guard } } @@ -299,14 +303,43 @@ pub enum Tier { /// Perry's limitation, not the program's. This tier doubles as a roadmap /// generator: `issue` names the tracking issue where one exists. CompilerLimitation, + /// The value is denied *at this position* but a different, already-shipped + /// mechanism carries its shape onward, so it is **not** a missed + /// opportunity and must not be counted as one (#7170 R0). + /// + /// This tier exists because the other three all answer "who should fix + /// it?", and for these rows the answer is "nobody — it is already fixed + /// elsewhere". Filing them under `CompilerLimitation` (which is where a + /// rule-1 alloc-site denial lands by default) put them in the roadmap + /// bucket a scheduler reads, and #7152/#7170 were both scheduled off a + /// number inflated by exactly that. + /// + /// The explicit rename keeps the serialized spelling identical to + /// [`Tier::as_str`], for the reason `Analysis::PtrNumArray` documents: + /// serde's kebab-case derive would spell this `served` while every + /// heading and text row says `served-elsewhere`, so one tier would appear + /// under two names in a single document and a consumer keying on either + /// would see half the rows. + #[serde(rename = "served-elsewhere")] + Served, } impl Tier { + /// Every tier, in report order. The text renderer enumerates *this* rather + /// than a hand-written list, so a new tier cannot be silently unrendered. + pub const ALL: [Tier; 4] = [ + Tier::Fixable, + Tier::CompilerLimitation, + Tier::Served, + Tier::InherentlyPolymorphic, + ]; + pub fn as_str(self) -> &'static str { match self { Tier::Fixable => "fixable", Tier::InherentlyPolymorphic => "inherently-polymorphic", Tier::CompilerLimitation => "compiler-limitation", + Tier::Served => "served-elsewhere", } } @@ -315,6 +348,7 @@ impl Tier { Tier::Fixable => "Fixable in your source", Tier::InherentlyPolymorphic => "Inherently polymorphic (correctly boxed — no action)", Tier::CompilerLimitation => "Perry limitation (not your code)", + Tier::Served => "Already served by another mechanism (not a missed opportunity)", } } } @@ -396,6 +430,31 @@ pub struct Entry { /// it is a site nobody has ever watched work. Two of the six were in /// exactly that state when this field was added. pub site: Option, + /// For a [`Position::AllocSite`] entry: the syntactic position the + /// allocation sits in (`return`, `constructor argument`, `object literal + /// property value`, …). + /// + /// A first-class field rather than prose inside `detail`, for the same + /// reason `site` is one: this is the bucket a scheduler counts. #7170 §5.1 + /// found the `constructor argument` label conflating genuine `new C(arg)` + /// arguments (2.0% of sites) with the property values of anonymous-shape + /// object literals (24.5%) — two different opportunities needing two + /// different fixes, merged into one number by the label alone. + pub alloc_context: Option, + /// For a [`Position::AllocSite`] entry: the site's index in its region's + /// deterministic report walk. + /// + /// **A de-duplication discriminant, not a source location.** Anonymous + /// object literals all render as `object literal { ... }` and all carry + /// `byte_offset: 0` (`perry-hir/src/lower/expr_object.rs` never populates + /// one for them), so before this field every unbound literal in a function + /// collapsed onto a single row in [`Entry::dedup_key`] — PR #7171 measured + /// suppressing 379 scaffolding rows *revealing* 62 previously-masked real + /// ones, and suppressing a single record moving its row between buckets + /// while changing the total by zero. The walk is a plain `Vec` traversal + /// with no map iteration, so a function lowered twice (a boxed entry plus + /// a typed clone) produces the same ordinals and still de-duplicates. + pub alloc_ordinal: Option, } impl Entry { @@ -438,6 +497,17 @@ impl Entry { /// It is kept because a dedup key should carry the full discriminant: the /// cost is one tuple element, and the failure mode if a future rule name /// collides is silent data loss in the report that feeds the census. + /// - **`alloc_ordinal`** separates the several unbound allocation sites of + /// one function from each other. It is `None` for every other position, + /// so no other tally moves. See [`Entry::alloc_ordinal`] for why the + /// pre-existing key could not tell them apart, and + /// [`masked_by_dedup`] for what is still being collapsed. + /// + /// `alloc_context` is deliberately **not** in this key. It is a function + /// of the site, so the ordinal already implies it, and a second + /// enforcement point that no sabotage can kill is how #7171 ended up + /// with four green holes in its first pass. One discriminant, one test. + #[allow(clippy::type_complexity)] fn dedup_key( &self, ) -> ( @@ -449,6 +519,7 @@ impl Entry { Outcome, Option, Option, + Option, ) { ( self.module.clone(), @@ -468,6 +539,7 @@ impl Entry { // and selection tallies (and therefore the baseline's `candidates` // column) are unchanged. self.site.clone(), + self.alloc_ordinal, ) } } @@ -480,6 +552,20 @@ struct Scope { function: String, region: RegionKind, invoked_per_element: Option, + /// #7170 R0: this region is a function that carries a **return-shape fact** + /// (`collectors/ptr_shape_returns.rs`, #7107), so its `return new C(...)` + /// sites already feed an existing mechanism. + /// + /// Set by exactly one caller — [`enter_function_region`], from + /// `codegen/function.rs`, which is the only place that holds both the + /// `FuncId` and `ModuleDispatchFacts`. Every other region (method, closure, + /// module-init) leaves it `false`, and that is CORRECT rather than + /// conservative: `collect_return_shape_functions` issues facts only for + /// `hir.functions` entries, and the caller-side seed + /// (`find_return_shape_candidates`) only fires on a bare `Expr::FuncRef` + /// callee — which a closure call never is. #7170 §6 is precisely the + /// measurement that closures are *not* served. + return_shape_producer: bool, } thread_local! { @@ -523,6 +609,48 @@ pub(crate) fn enter_region(function: &str, region: RegionKind) -> ScopeGuard { enter(&module, function, region) } +/// [`enter_region`] for a top-level function body, additionally recording +/// whether this function carries a **return-shape fact** (#7107). +/// +/// Report-only: the flag is read by nothing but +/// [`region_is_return_shape_producer`], which only the `Ptr` alloc-site +/// recorder consults. Like every other entry point here it is inert — and +/// allocation-free — when the report is off. +pub(crate) fn enter_function_region(function: &str, return_shape_producer: bool) -> ScopeGuard { + if !enabled() { + return ScopeGuard { + previous: None, + active: false, + }; + } + let scope = Scope { + module: current_module(), + function: function.to_string(), + region: RegionKind::Function, + invoked_per_element: None, + return_shape_producer, + }; + let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); + ScopeGuard { + previous, + active: true, + } +} + +/// Whether the region currently being lowered carries a return-shape fact. +/// +/// `false` with no scope at all, which is the safe direction: an unattributed +/// site is reported as an ordinary rule-1 denial rather than silently written +/// off as served. +pub(crate) fn region_is_return_shape_producer() -> bool { + SCOPE.with(|s| { + s.borrow() + .as_ref() + .map(|sc| sc.return_shape_producer) + .unwrap_or(false) + }) +} + fn current_module() -> String { SCOPE.with(|s| { s.borrow() @@ -564,6 +692,7 @@ pub(crate) fn enter(module: &str, function: &str, region: RegionKind) -> ScopeGu function: function.to_string(), region, invoked_per_element: None, + return_shape_producer: false, }; let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); ScopeGuard { @@ -587,6 +716,10 @@ pub(crate) fn enter_closure(function: &str, func_id: u32) -> ScopeGuard { function: function.to_string(), region: RegionKind::Closure, invoked_per_element: per_element_role(Some(func_id)), + // #7170 §6: a closure is never a return-shape producer — the caller-side + // seed requires a bare `Expr::FuncRef` callee. This `false` is the + // measurement, not a default. + return_shape_producer: false, }; let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); ScopeGuard { @@ -702,6 +835,57 @@ pub(crate) fn deny(d: Denial<'_>) { detail: d.detail, byte_offset: d.byte_offset, site: None, + alloc_context: None, + alloc_ordinal: None, + }); +} + +/// [`deny`] for a [`Position::AllocSite`] row, carrying the two fields that +/// make an unbound allocation identifiable: the syntactic position it sits in +/// and its ordinal in the region's report walk. +/// +/// A separate entry point rather than two more fields on [`Denial`], because +/// every other denial site would then have to spell `None` twice for a pair of +/// fields only this one can populate. +pub(crate) fn deny_alloc(d: Denial<'_>, alloc_context: &str, alloc_ordinal: u32) { + if !enabled() { + return; + } + let (module, function, region, per_element) = SCOPE.with(|s| match s.borrow().as_ref() { + Some(sc) => ( + sc.module.clone(), + sc.function.clone(), + sc.region, + sc.invoked_per_element.clone(), + ), + None => ( + String::from(""), + String::from(""), + RegionKind::Function, + None, + ), + }); + push(Entry { + module, + function, + region, + position: d.position, + name: d.name.to_string(), + local_id: d.local_id, + analysis: d.analysis, + outcome: Outcome::Denied, + rep: String::from("Boxed"), + rule: Some(d.rule.to_string()), + reason: Some(d.reason.to_string()), + tier: Some(d.tier), + issue: d.issue.map(str::to_string), + loop_depth: d.loop_depth, + invoked_per_element: per_element, + detail: d.detail, + byte_offset: d.byte_offset, + site: None, + alloc_context: Some(alloc_context.to_string()), + alloc_ordinal: Some(alloc_ordinal), }); } @@ -732,6 +916,8 @@ pub(crate) fn deny_named(function: &str, region: RegionKind, d: Denial<'_>) { detail: d.detail, byte_offset: d.byte_offset, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -790,6 +976,8 @@ pub(crate) fn select( detail, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -826,6 +1014,8 @@ pub(crate) fn select_explicit( detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -895,6 +1085,8 @@ pub(crate) fn consume( detail: Some(format!("consumed at {site}")), byte_offset: None, site: Some(site.to_string()), + alloc_context: None, + alloc_ordinal: None, }); } @@ -943,9 +1135,29 @@ pub(crate) fn unconsumed(u: Unconsumed<'_>) { detail: u.detail, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } +/// How many entries the most recent [`take_entries`] collapsed. +/// +/// #7170 R0: de-duplication is not free information hygiene — it is a lossy +/// step between "what the compiler recorded" and "what a scheduler counts", +/// and PR #7171 measured it changing which bucket a row lands in while leaving +/// the total at zero. The count is therefore reported rather than left +/// implicit, so a reader can see how much of the population is still being +/// folded away instead of having to assume it is none. +/// +/// Set (not accumulated) by each drain; `0` before the first one. +static MASKED_BY_DEDUP: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// Entries collapsed by the most recent [`take_entries`]. See +/// [`MASKED_BY_DEDUP`]. +pub fn masked_by_dedup() -> usize { + MASKED_BY_DEDUP.load(std::sync::atomic::Ordering::Relaxed) +} + /// Drain every recorded entry, de-duplicated and ranked. Called once by the /// CLI after module codegen finishes. pub fn take_entries() -> Vec { @@ -953,8 +1165,10 @@ pub fn take_entries() -> Vec { Ok(mut v) => std::mem::take(&mut *v), Err(_) => Vec::new(), }; + let before = entries.len(); let mut seen = std::collections::HashSet::new(); entries.retain(|e| seen.insert(e.dedup_key())); + MASKED_BY_DEDUP.store(before - entries.len(), std::sync::atomic::Ordering::Relaxed); entries.sort_by(|a, b| a.rank().cmp(&b.rank())); entries } @@ -983,6 +1197,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -1020,6 +1236,38 @@ mod tests { } } + /// One tier, one spelling — the same trap `serialized_analysis_matches_as_str` + /// guards for analyses. A tier whose serde name differs from its `as_str` + /// name appears twice in one document and a consumer keying on either sees + /// half the rows. + #[test] + fn serialized_tier_matches_as_str() { + for tier in Tier::ALL { + let serialized = serde_json::to_string(&tier).unwrap(); + assert_eq!( + serialized.trim_matches('"'), + tier.as_str(), + "serde and as_str disagree for {tier:?}" + ); + } + } + + /// `Tier::ALL` must actually enumerate the enum. The text renderer loops + /// over it, so a tier missing here is a tier that is in the JSON and not in + /// the human report. + #[test] + fn tier_all_lists_every_variant() { + for tier in [ + Tier::Fixable, + Tier::InherentlyPolymorphic, + Tier::CompilerLimitation, + Tier::Served, + ] { + assert!(Tier::ALL.contains(&tier), "{tier:?} missing from Tier::ALL"); + } + assert_eq!(Tier::ALL.len(), 4); + } + #[test] fn dedup_key_collapses_a_function_lowered_twice() { let a = entry("f", 0, None); diff --git a/crates/perry-codegen/src/opt_report/render.rs b/crates/perry-codegen/src/opt_report/render.rs index 2347217a10..fd5de74901 100644 --- a/crates/perry-codegen/src/opt_report/render.rs +++ b/crates/perry-codegen/src/opt_report/render.rs @@ -134,8 +134,37 @@ pub fn render_text(entries: &[Entry]) -> String { " {:<16} {total_selected} values unboxed, {total_denied} left Boxed", "TOTAL", ); + let masked = super::masked_by_dedup(); + if masked > 0 { + let _ = writeln!( + out, + " {:<16} {masked} duplicate row(s) collapsed by de-duplication", + "", + ); + } out.push('\n'); + // ── Unbound allocation sites, by position ────────────────────────────── + // #7170 R0. These rows are the majority of the `Ptr` denial + // population on real code, and their positions are what a scheduler reads + // to decide which escape to open next. Printed as an explicit table so + // that reading is not a `jq` reduction over `entries[]` whose bucket + // definitions live nowhere. + let alloc = alloc_buckets(entries); + if !alloc.is_empty() { + let total: usize = alloc.iter().map(|b| b.denied).sum(); + let _ = writeln!(out, "Unbound allocation sites by position ({total})"); + out.push_str("-------------------------------------\n"); + for b in &alloc { + let _ = writeln!( + out, + " {:<34} {:>5} {} / {}", + b.context, b.denied, b.analysis, b.rule, + ); + } + out.push('\n'); + } + // ── Headline: call out any representation that promoted nothing ──────── // This is the #7034 §0 finding, stated without having to read IR. let mut zeros: Vec<(&Analysis, &AnalysisTally)> = tallies @@ -218,11 +247,7 @@ pub fn render_text(entries: &[Entry]) -> String { ); } - for tier in [ - Tier::Fixable, - Tier::CompilerLimitation, - Tier::InherentlyPolymorphic, - ] { + for tier in Tier::ALL { let rows: Vec<&&Entry> = denials .iter() .filter(|e| e.tier == Some(tier)) @@ -256,6 +281,9 @@ pub fn render_text(entries: &[Entry]) -> String { if let Some(detail) = &e.detail { let _ = writeln!(out, " {detail}"); } + if let Some(ordinal) = e.alloc_ordinal { + let _ = writeln!(out, " allocation site #{ordinal} in this region"); + } if let Some(offset) = e.byte_offset { let _ = writeln!(out, " source byte offset {offset}"); } @@ -320,13 +348,87 @@ struct JsonAnalysis<'a> { records_consumption: bool, } +/// One `(analysis, rule)` denial bucket. +/// +/// #7170 R0: the bucket counts are computed **here**, by the compiler that +/// owns the rule vocabulary, rather than reconstructed downstream with `jq` +/// over `entries[]`. Every scheduling decision in the #7152/#7170 chain was +/// taken off a hand-rolled reduction of those rows, and two of the three +/// defects R0 fixes were invisible in that reduction by construction: it could +/// not see that one label meant two mechanisms, nor that a bucket contained +/// already-served sites. +#[derive(Debug, serde::Serialize)] +struct JsonRuleBucket<'a> { + analysis: &'a str, + rule: &'a str, + tier: Option, + denied: usize, +} + +/// One `(allocation position, rule)` bucket, over `alloc-site` denials. +/// +/// Separate from [`JsonRuleBucket`] because the position is the discriminant +/// that #7170 §5.1 found merged: `constructor argument` and `object literal +/// property value` share a rule and are different opportunities. +#[derive(Debug, serde::Serialize)] +struct JsonAllocBucket<'a> { + analysis: &'a str, + context: &'a str, + rule: &'a str, + denied: usize, +} + #[derive(Debug, serde::Serialize)] struct JsonSummary<'a> { selected: usize, denied: usize, consumed: usize, unconsumed: usize, + /// Entries the drain collapsed. See + /// [`crate::opt_report::masked_by_dedup`] — a nonzero value here is the + /// count of rows a consumer is NOT seeing. + masked_by_dedup: usize, by_analysis: Vec>, + by_rule: Vec>, + by_alloc_context: Vec>, +} + +fn rule_buckets(entries: &[Entry]) -> Vec> { + let mut out: BTreeMap<(&str, &str), (Option, usize)> = BTreeMap::new(); + for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) { + let rule = e.rule.as_deref().unwrap_or(""); + let slot = out + .entry((e.analysis.as_str(), rule)) + .or_insert((e.tier, 0)); + slot.1 += 1; + } + out.into_iter() + .map(|((analysis, rule), (tier, denied))| JsonRuleBucket { + analysis, + rule, + tier, + denied, + }) + .collect() +} + +fn alloc_buckets(entries: &[Entry]) -> Vec> { + let mut out: BTreeMap<(&str, &str, &str), usize> = BTreeMap::new(); + for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) { + let Some(context) = e.alloc_context.as_deref() else { + continue; + }; + let rule = e.rule.as_deref().unwrap_or(""); + *out.entry((e.analysis.as_str(), context, rule)).or_insert(0) += 1; + } + out.into_iter() + .map(|((analysis, context, rule), denied)| JsonAllocBucket { + analysis, + context, + rule, + denied, + }) + .collect() } #[derive(Debug, serde::Serialize)] @@ -348,6 +450,7 @@ pub fn render_json(entries: &[Entry]) -> String { denied: tallies.values().map(|t| t.denied).sum(), consumed: tallies.values().map(|t| t.consumed).sum(), unconsumed: tallies.values().map(|t| t.unconsumed).sum(), + masked_by_dedup: super::masked_by_dedup(), // Enumerate `Analysis::ALL`, not the analyses that happen to have // entries: an analysis that recorded nothing must appear with an // explicit `0`. A consumer cannot tell an absent key from a zero @@ -369,6 +472,8 @@ pub fn render_json(entries: &[Entry]) -> String { } }) .collect(), + by_rule: rule_buckets(entries), + by_alloc_context: alloc_buckets(entries), }, entries, }; @@ -400,6 +505,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -423,6 +530,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -722,3 +831,170 @@ mod tests { ); } } + +#[cfg(test)] +mod r0_bucket_tests { + use super::*; + use crate::opt_report::{Position, RegionKind}; + + fn alloc_site(context: &str, rule: &str, tier: Tier, ordinal: u32) -> Entry { + Entry { + module: "dep.js".into(), + function: "mk".into(), + region: RegionKind::Function, + position: Position::AllocSite, + name: "object literal { ... }".into(), + local_id: None, + analysis: Analysis::PtrShape, + outcome: Outcome::Denied, + rep: "Boxed".into(), + rule: Some(rule.into()), + reason: Some("unbound".into()), + tier: Some(tier), + issue: None, + loop_depth: 0, + invoked_per_element: None, + detail: Some(format!("allocation position: {context}")), + byte_offset: None, + site: None, + alloc_context: Some(context.into()), + alloc_ordinal: Some(ordinal), + } + } + + const RULE1: &str = "rule 1 (provenance)"; + const SERVED: &str = "rule 1 (provenance) — already served by return-shape"; + + /// #7170 R0's primary deliverable, at the schema level: the bucket counts a + /// scheduler reads are computed by the compiler that owns the rule + /// vocabulary, not reconstructed with `jq` over `entries[]`. Two of the + /// three defects R0 fixes were invisible to such a reduction by + /// construction — it could not see that one label meant two mechanisms. + #[test] + fn json_reports_the_alloc_buckets_separately() { + let entries = vec![ + alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 1, + ), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 2, + ), + alloc_site("return", SERVED, Tier::Served, 3), + ]; + let json: serde_json::Value = serde_json::from_str(&render_json(&entries)).unwrap(); + let buckets = json["summary"]["by_alloc_context"].as_array().unwrap(); + let find = |ctx: &str| { + buckets + .iter() + .find(|b| b["context"] == ctx) + .unwrap_or_else(|| panic!("missing {ctx} bucket in {buckets:#?}")) + .clone() + }; + assert_eq!(find("constructor argument")["denied"], 1); + assert_eq!(find("object literal property value")["denied"], 2); + assert_eq!(find("return")["denied"], 1); + assert_eq!(find("return")["rule"], SERVED); + } + + /// The wall number itself: `by_rule` must separate the served population + /// from the rule-1 one, because "506 of 963" was a count of the merged + /// bucket and every scheduling decision in the #7152/#7170 chain was taken + /// off it. + #[test] + fn json_by_rule_separates_served_sites_from_the_rule_1_wall() { + let entries = vec![ + alloc_site("return", RULE1, Tier::CompilerLimitation, 0), + alloc_site("return", SERVED, Tier::Served, 1), + alloc_site("return", SERVED, Tier::Served, 2), + ]; + let json: serde_json::Value = serde_json::from_str(&render_json(&entries)).unwrap(); + let rules = json["summary"]["by_rule"].as_array().unwrap(); + let row = |rule: &str| { + rules + .iter() + .find(|r| r["rule"] == rule) + .unwrap_or_else(|| panic!("missing rule {rule} in {rules:#?}")) + .clone() + }; + assert_eq!(row(RULE1)["denied"], 1, "the wall is 1, not 3"); + assert_eq!(row(SERVED)["denied"], 2); + assert_eq!(row(SERVED)["tier"], "served-elsewhere"); + } + + /// A served row must not be filed under the roadmap tier. That heading is + /// what a scheduler reads as "Perry's outstanding work", and #7170 R0 + /// exists because already-shipped mechanisms were being counted there. + #[test] + fn a_served_site_renders_under_its_own_heading_not_the_roadmap_one() { + let text = render_text(&[alloc_site("return", SERVED, Tier::Served, 0)]); + assert!( + text.contains("Already served by another mechanism"), + "got:\n{text}" + ); + assert!( + !text.contains("Perry limitation (not your code)"), + "a served row must not appear in the roadmap tier; got:\n{text}" + ); + } + + /// Every tier must be renderable. The loop enumerates `Tier::ALL`, so a + /// future tier cannot be added and silently left unprinted — which is how + /// a row disappears from the human report while still being in the JSON. + #[test] + fn every_tier_has_a_heading_and_is_rendered() { + for (i, tier) in Tier::ALL.into_iter().enumerate() { + let text = render_text(&[alloc_site("return", RULE1, tier, i as u32)]); + assert!( + text.contains(tier.heading()), + "{:?} is not rendered; got:\n{text}", + tier + ); + } + } + + /// The allocation-position table is the human-readable half of the same + /// fix, and it must state the position rather than leaving it to prose. + #[test] + fn text_reports_the_allocation_positions_as_a_table() { + let text = render_text(&[ + alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 1, + ), + ]); + assert!( + text.contains("Unbound allocation sites by position (2)"), + "got:\n{text}" + ); + assert!( + text.contains("object literal property value"), + "got:\n{text}" + ); + } + + /// An entry that is not an allocation site contributes to `by_rule` and to + /// nothing else — the alloc table must not grow rows for ordinary locals. + #[test] + fn a_local_denial_does_not_appear_in_the_alloc_table() { + let mut local = alloc_site("return", RULE1, Tier::Fixable, 0); + local.position = Position::Local; + local.alloc_context = None; + local.alloc_ordinal = None; + let json: serde_json::Value = serde_json::from_str(&render_json(&[local])).unwrap(); + assert!(json["summary"]["by_alloc_context"] + .as_array() + .unwrap() + .is_empty()); + assert_eq!(json["summary"]["by_rule"].as_array().unwrap().len(), 1); + } +} diff --git a/scripts/compiler_output_harness/repsel_census.py b/scripts/compiler_output_harness/repsel_census.py index 7799bc8913..f176731035 100644 --- a/scripts/compiler_output_harness/repsel_census.py +++ b/scripts/compiler_output_harness/repsel_census.py @@ -278,6 +278,61 @@ } +#: Minimum number of unbound allocations each **position bucket** must be seen +#: in, per workload. **Held in code, never in the baseline**, for the same +#: reason as [`LIVENESS_FLOORS`], and gated separately. +#: +#: #7170 R0 split one alloc-site label into three that mean different things: +#: +#: * `constructor argument` — a genuine `new C(arg)` argument (2.0%) +#: * `object literal property value` — a property value of an anonymous-shape +#: literal, i.e. `{a: {b: 1}}` (24.5%) +#: * `return` — with the rule naming whether the +#: enclosing function's return-shape fact +#: already serves it +#: +#: Every one of those classifications can fail silently to **zero**, and a zero +#: bucket is indistinguishable from "the classifier is dead" — CLAUDE.md failure +#: mode 4, in the instrument that R0 exists to repair. `fixture_alloc_buckets` +#: is written to put at least one allocation in each, so a classification arm +#: that stops firing takes this check red instead of quietly re-merging the +#: buckets it separated. +#: +#: The `return` bucket carries a RULE floor as well ([`ALLOC_RULE_FLOORS`]), +#: because the position alone cannot distinguish a served return from an +#: unserved one — that distinction lives in the rule string, and it is the one +#: the corrected wall number is derived from. +ALLOC_BUCKET_FLOORS: dict[str, dict[str, int]] = { + "fixture_alloc_buckets": { + "constructor argument": 1, + "object literal property value": 1, + "return": 1, + }, +} + +#: Minimum number of times a named DENIAL RULE must be reported, per workload — +#: the half of the #7170 R0 classification that the position cannot express. +#: **Held in code, never in the baseline.** +#: +#: `return` is one position and two opportunities. A `return new C(...)` in a +#: function that carries a return-shape fact (#7107) is already served: the +#: class reaches every `const r = producer(...)` call site, and counting it as a +#: rule-1 miss is what inflated #7170's 506/963 headline. A `return` in a +#: function WITHOUT the fact — every closure in CommonJS, which is 91.6% of the +#: dependency-corpus population — is a real miss. Only the rule string tells +#: them apart, so only a rule floor can assert the distinction is still live. +#: +#: This floor is what makes the served classification falsifiable end to end: +#: hard-coding `false` in `codegen/function.rs`, or dropping the fact conjunct +#: in `deny_alloc_site`, takes it red while every unit test still passes, +#: because the unit tests set the report scope by hand. +ALLOC_RULE_FLOORS: dict[str, dict[str, int]] = { + "fixture_alloc_buckets": { + "rule 1 (provenance) — already served by return-shape": 1, + }, +} + + #: Workloads allowed to produce **zero candidates** — no analysis considered any #: value in them. Held in code for the same reason as [`LIVENESS_FLOORS`]: it is #: an assertion about the compiler, and `--update` must not be able to widen it. @@ -408,6 +463,11 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: consumed_receiver = 0 unconsumed_mechanisms: dict[str, int] = {} denial_rules: dict[str, int] = {} + #: #7170 R0. Unbound-allocation denials keyed by the syntactic position the + #: allocation sits in, which is the bucket a scheduler reads. Kept separate + #: from `denial_rules` because two of the three buckets R0 separated share a + #: rule name and differ only here. + alloc_contexts: dict[str, int] = {} # Keyed by analysis as well as by rule. `unconsumed_mechanisms` alone is # rule-keyed, and with a second instrumented analysis a gap in analysis A # would be excused by a mechanism recorded for analysis B. @@ -460,6 +520,9 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: denial_rules[str(entry.get("rule") or "")] = ( denial_rules.get(str(entry.get("rule") or ""), 0) + 1 ) + context = entry.get("alloc_context") + if context is not None: + alloc_contexts[str(context)] = alloc_contexts.get(str(context), 0) + 1 if unknown_sites: raise HarnessError( f"--opt-report recorded consumption at unregistered site(s) " @@ -494,6 +557,7 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: "candidates": candidates, "unconsumed_mechanisms": unconsumed_mechanisms, "denial_rules": denial_rules, + "alloc_contexts": alloc_contexts, "unconsumed_by_analysis": unconsumed_by_analysis, "consumed_receiver": consumed_receiver, "consumption_sites": consumption_sites, @@ -757,6 +821,61 @@ def check_refusal_floors(observed: dict[str, dict[str, Any]]) -> list[str]: return failures +def check_alloc_bucket_floors(observed: dict[str, dict[str, Any]]) -> list[str]: + """Every alloc-site position bucket must still be observed somewhere. + + Independent of the baseline file on purpose — see [`ALLOC_BUCKET_FLOORS`]. + + This is the third layer of #7104's pattern for the #7170 R0 classifications: + unit tests prove the classifier picks the right bucket, this proves the + classifier is still *wired into a real compile*, and the sabotage matrix + proves each arm can go red. Layer 2 is the one the unit tests cannot give: + `codegen/function.rs` could pass `false` for every function and every unit + test would still pass, because they set the scope by hand. + """ + failures: list[str] = [] + for name, minimums in ALLOC_BUCKET_FLOORS.items(): + if name not in observed: + failures.append( + f"alloc-bucket workload {name!r} did not run; the census cannot " + "claim to observe a bucket it never measured" + ) + continue + buckets = observed[name].get("alloc_contexts", {}) + for context, minimum in minimums.items(): + seen = int(buckets.get(context, 0)) + if seen < minimum: + failures.append( + f"{name}: allocation position {context!r} was reported " + f"{seen} time(s), and must be reported at least {minimum}. " + "Either the classification in " + "collectors/ptr_shape_report.rs stopped distinguishing it " + "(re-merging buckets #7170 §5.1 separated) or the fixture " + f"changed shape. Observed buckets: {sorted(buckets)}" + ) + for name, minimums in ALLOC_RULE_FLOORS.items(): + if name not in observed: + failures.append( + f"alloc-rule workload {name!r} did not run; the census cannot " + "claim to observe a rule it never measured" + ) + continue + rules = observed[name].get("denial_rules", {}) + for rule, minimum in minimums.items(): + seen = int(rules.get(rule, 0)) + if seen < minimum: + failures.append( + f"{name}: denial rule {rule!r} was reported {seen} time(s), " + f"and must be reported at least {minimum}. The served-return " + "classification is no longer reaching a real compile — " + "either codegen/function.rs stopped passing the " + "return-shape fact into the report scope, or " + "deny_alloc_site stopped consulting it. " + f"Observed rules: {sorted(rules)}" + ) + return failures + + def check_instrument_liveness(observed: dict[str, dict[str, Any]]) -> list[str]: """No census key may read zero across the ENTIRE corpus. @@ -1086,6 +1205,7 @@ def census(args: argparse.Namespace) -> int: improvements += imp liveness = check_liveness_fixtures(observed) if not partial else [] refusals = check_refusal_floors(observed) if not partial else [] + alloc_buckets = check_alloc_bucket_floors(observed) if not partial else [] dead = check_instrument_liveness(observed) if not partial else [] unreached = check_analysis_reach(observed) if not partial else [] # Always checked, even for a --workload subset: it is an internal @@ -1111,6 +1231,7 @@ def census(args: argparse.Namespace) -> int: ("REGRESSION", regressions), ("DEAD INSTRUMENT", liveness + dead), ("REFUSAL NO LONGER FIRING", refusals), + ("ALLOC-SITE BUCKET NO LONGER DISTINGUISHED", alloc_buckets), ("UNREACHED BY EVERY ANALYSIS", unreached), ("CONSUMPTION COUNTER IS INCOHERENT", invariant), ("WASTED PROMOTION WITH NO NAMED MECHANISM", unexplained), @@ -1169,6 +1290,13 @@ def _update( # Context, never gated: which mechanism ate each wasted promotion. workload["unconsumed_mechanisms"] = observed[name].get("unconsumed_mechanisms", {}) workload["consumption_sites"] = observed[name].get("consumption_sites", {}) + # #7170 R0, also context and also never gated: where this workload's + # unbound allocations sit. Recorded per workload so the rule-1 wall can + # be re-derived from a checked-in artifact instead of from a `jq` line + # in an issue comment — which is how the 506/963 headline outlived the + # measurement it came from. The gated assertion is + # ALLOC_BUCKET_FLOORS / ALLOC_RULE_FLOORS, held in code. + workload["alloc_contexts"] = observed[name].get("alloc_contexts", {}) baseline["generated_at"] = utc_now() path.write_text(json.dumps(baseline, indent=2) + "\n", encoding="utf-8") print(f"Wrote {path.relative_to(REPO_ROOT)} ({len(workloads)} workload(s)).") @@ -1233,6 +1361,73 @@ def self_test(_args: argparse.Namespace) -> int: dead = check_instrument_liveness({"w": {"counts": counts}}) assert any("ptr-shape" in d for d in dead), dead + # #7170 R0: the alloc-site bucket extraction and its floors. Asserted in + # the FAILING direction first, for the reason stated at the top of this + # function — and once per defect, because the three classifications can + # each die on their own. + bucket_report = { + "schema_version": 2, + "summary": { + "selected": 0, + "denied": 3, + "by_analysis": [ + { + "analysis": a, + "target_rep": a, + "rule_source": "x", + "selected": 0, + "denied": 3 if a == "ptr-shape" else 0, + "consumed": 0, + "unconsumed": 0, + } + for a in EXPECTED_ANALYSES + ], + }, + "entries": [ + { + "analysis": "ptr-shape", + "outcome": "denied", + "rep": "Boxed", + "position": "alloc-site", + "rule": rule_name, + "alloc_context": context, + } + for context, rule_name in ( + ("constructor argument", "rule 1 (provenance)"), + ("object literal property value", "rule 1 (provenance)"), + ("return", "rule 1 (provenance) — already served by return-shape"), + ) + ], + } + buckets = census_from_report(bucket_report) + assert buckets["alloc_contexts"] == { + "constructor argument": 1, + "object literal property value": 1, + "return": 1, + }, buckets["alloc_contexts"] + + fixture = next(iter(ALLOC_BUCKET_FLOORS)) + good = {fixture: {"counts": buckets["counts"], **buckets}} + assert not check_alloc_bucket_floors(good), check_alloc_bucket_floors(good) + + # Re-merging the two literal buckets — the #7170 §5.1 defect — is red. + merged = json.loads(json.dumps(good)) + merged[fixture]["alloc_contexts"] = {"constructor argument": 2, "return": 1} + failures = check_alloc_bucket_floors(merged) + assert any("object literal property value" in f for f in failures), failures + + # Losing the served-return classification — the #7170 §5.2 defect, and the + # only one a unit test in the compiler cannot catch, because it lives in + # codegen/function.rs rather than in the classifier — is red. + unserved = json.loads(json.dumps(good)) + unserved[fixture]["denial_rules"] = {"rule 1 (provenance)": 3} + failures = check_alloc_bucket_floors(unserved) + assert any("already served by return-shape" in f for f in failures), failures + + # And a workload that did not run at all cannot pass by being absent. + failures = check_alloc_bucket_floors({}) + assert failures, "a missing alloc-bucket fixture must be a failure" + # #7128: the refusal check must go red when the rule stops firing, and # green only when it fires at least as often as it was measured to. target = next(iter(REFUSAL_FLOORS)) From d29ba29f81880620be015e290dbd550446688af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 10:34:58 +0200 Subject: [PATCH 2/9] docs: changelog fragment for PR 7176 --- changelog.d/7176-repsel-alloc-site-buckets.md | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 changelog.d/7176-repsel-alloc-site-buckets.md diff --git a/changelog.d/7176-repsel-alloc-site-buckets.md b/changelog.d/7176-repsel-alloc-site-buckets.md new file mode 100644 index 0000000000..2e7a39b214 --- /dev/null +++ b/changelog.d/7176-repsel-alloc-site-buckets.md @@ -0,0 +1,56 @@ +**`--opt-report`: the `Ptr` allocation-site buckets now mean what they +are read as (#7170 R0).** Report-only — emitted LLVM IR is identical in both +arms over 30 dependency modules, each with a same-compiler control run, with +both arms run *under* `--opt-report` (the only mode in which any of this code +executes). Three defects, each of which made a scheduler-facing number say +something other than what it looked like: + +- **De-duplication was masking rows, by a factor of 2.2.** Every anonymous + object literal renders as `object literal { ... }` and carries + `byte_offset: 0`, so every unbound literal in a function collapsed onto one + `Entry::dedup_key`. Over 196 real `__esModule` dependency modules the + `ptr-shape` allocation-site denial count goes **837 → 1842**. A new + `alloc_ordinal` (the site's index in the region's deterministic walk) + discriminates them; a function lowered twice walks the same body, produces + the same ordinals, and still collapses. What the drain *did* collapse is now + reported as `summary.masked_by_dedup` instead of left implicit. +- **`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` (5 on the corpus) and + `object literal property value` (418) — the old label was 98.8 % the latter, + and the two need different fixes: a nested literal is a field value of a + parent allocation that is itself unbound, so proving its shape licenses + nothing on its own. +- **The `return` bucket counted syntactic sites, not opportunities.** + `collectors/ptr_shape_returns.rs` (#7107) already admits a bare + `return new C(...)` as a producer, but `deny_alloc_site` fires before any + seeding. Those sites now carry their own rule string and a new `Tier::Served` + so they leave the rule-1 bucket rather than inflating it. + +The buckets are computed by the compiler (`summary.by_rule`, +`summary.by_alloc_context`) instead of being reconstructed downstream, which is +what made the second and third defects invisible: a `jq` reduction over +`entries[]` cannot see that one label means two mechanisms. + +**The re-derived dependency-JS rule-1 wall is 1838 unserved allocation sites of +2028 `Ptr` candidates (90.6 %)**, replacing #7152's `506 / 746` and +#7170's `963 / 1352`. The correction #7170 §5.2 expected to be material is not +— served returns are **4** sites — because only 62 of 1842 unbound allocations +are in a `function` region at all; 1711 (92.9 %) are in closures, which cannot +carry a return-shape fact. The instrument's real distortion was the dedup, and +it ran the other way: the wall is larger than reported, not smaller. + +Census: `alloc_contexts` per workload (context, never gated), 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 — +it lives in `codegen/function.rs`, and every compiler unit test sets the report +scope by hand. No existing census floor moved 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. + +Still open, measured but deliberately not fixed here: `statement` is now the +largest bucket (455 of 1842, 24.7 %) and is a **catch-all** — the report walk's +default context, holding property stores, reassignments, conditional branches +and default parameters indistinguishably. From 3940c8c06718aef97ade99783ff0c01a13b7699d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 10:41:13 +0200 Subject: [PATCH 3/9] docs(repsel): state the measured size of the served-return correction --- .../perry-codegen/src/collectors/ptr_shape_report.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index 305afa20f2..a9068e77f3 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -77,8 +77,15 @@ pub(super) const UNBOUND_ALLOC: ShapeDenial = ShapeDenial { /// `collectors/ptr_shape_returns.rs` admits a bare `return new C(...)` with no /// local at all as a producer, so the class does reach every /// `const r = producer(…)` caller. Left under [`UNBOUND_ALLOC`] these rows -/// counted as rule-1 misses, and #7170 measured the resulting 506/963 headline -/// inflated by exactly them. +/// counted as rule-1 misses. +/// +/// **How big this correction actually is: 4 sites of 1842**, measured over 196 +/// real dependency modules. #7170 §5.2 expected it to be a material share of +/// the rule-1 wall and it is not — because only 62 of those 1842 allocations +/// are in a `function` region at all (1711 are in closures, which cannot carry +/// a return-shape fact; #7170 §6). The bucket is separated anyway: 4 is the +/// honest number only once the classification exists, and "small" is a result +/// rather than a reason to leave two mechanisms merged. /// /// **What this asserts, precisely:** the fact was *issued* for the enclosing /// function. Whether any caller consumed it is a different question with a From 3dddfda89f73cab33339c61c98f375b7b230821e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 10:42:59 +0200 Subject: [PATCH 4/9] refactor(opt-report): one body behind deny and deny_alloc --- crates/perry-codegen/src/opt_report/mod.rs | 54 ++++++---------------- 1 file changed, 13 insertions(+), 41 deletions(-) diff --git a/crates/perry-codegen/src/opt_report/mod.rs b/crates/perry-codegen/src/opt_report/mod.rs index cc57f4933c..771f958358 100644 --- a/crates/perry-codegen/src/opt_report/mod.rs +++ b/crates/perry-codegen/src/opt_report/mod.rs @@ -799,45 +799,7 @@ pub(crate) struct Denial<'a> { /// `detail` costs anything: the arguments are evaluated before this function /// can early-return. pub(crate) fn deny(d: Denial<'_>) { - if !enabled() { - return; - } - let (module, function, region, per_element) = SCOPE.with(|s| match s.borrow().as_ref() { - Some(sc) => ( - sc.module.clone(), - sc.function.clone(), - sc.region, - sc.invoked_per_element.clone(), - ), - None => ( - String::from(""), - String::from(""), - RegionKind::Function, - None, - ), - }); - push(Entry { - module, - function, - region, - position: d.position, - name: d.name.to_string(), - local_id: d.local_id, - analysis: d.analysis, - outcome: Outcome::Denied, - rep: String::from("Boxed"), - rule: Some(d.rule.to_string()), - reason: Some(d.reason.to_string()), - tier: Some(d.tier), - issue: d.issue.map(str::to_string), - loop_depth: d.loop_depth, - invoked_per_element: per_element, - detail: d.detail, - byte_offset: d.byte_offset, - site: None, - alloc_context: None, - alloc_ordinal: None, - }); + deny_in_scope(d, None, None); } /// [`deny`] for a [`Position::AllocSite`] row, carrying the two fields that @@ -848,6 +810,16 @@ pub(crate) fn deny(d: Denial<'_>) { /// every other denial site would then have to spell `None` twice for a pair of /// fields only this one can populate. pub(crate) fn deny_alloc(d: Denial<'_>, alloc_context: &str, alloc_ordinal: u32) { + if !enabled() { + return; + } + deny_in_scope(d, Some(alloc_context.to_string()), Some(alloc_ordinal)); +} + +/// The one body behind [`deny`] and [`deny_alloc`]: build a `Denied` entry from +/// the ambient scope. Kept single so the two entry points cannot drift in what +/// they attribute. +fn deny_in_scope(d: Denial<'_>, alloc_context: Option, alloc_ordinal: Option) { if !enabled() { return; } @@ -884,8 +856,8 @@ pub(crate) fn deny_alloc(d: Denial<'_>, alloc_context: &str, alloc_ordinal: u32) detail: d.detail, byte_offset: d.byte_offset, site: None, - alloc_context: Some(alloc_context.to_string()), - alloc_ordinal: Some(alloc_ordinal), + alloc_context, + alloc_ordinal, }); } From caefafda6ee3c88a496fa4d87089979d804f4f0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 11:43:01 +0200 Subject: [PATCH 5/9] 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. --- .../fixtures/fixture_alloc_buckets.ts | 15 +- .../collectors/ptr_shape_opt_report_tests.rs | 181 +++++++++++++- .../src/collectors/ptr_shape_report.rs | 90 ++++++- crates/perry-codegen/src/opt_report/render.rs | 80 ++++-- .../compiler_output_harness/repsel_census.py | 235 +++++++++++------- 5 files changed, 474 insertions(+), 127 deletions(-) diff --git a/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts index 4625014468..317031e565 100644 --- a/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts +++ b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts @@ -78,7 +78,20 @@ const boxed = new Boxed(new Point(1, 2)); // with the line above. const nested = { inner: { a: 1, b: 2 }, k: 3 }; +// Bucket 4 — a returned expression OPERAND. What this function returns is the +// conditional, not either allocation, so #7107's return-shape fact covers +// neither and `pickPoint` gets no fact at all (`producer_return_class` admits +// only a bare `Expr::New` or a proven local as a return). Before the #7176 +// review both arms inherited the `return` label from `Stmt::Return` and were +// counted as return positions — which is what over-stated the `return` bucket +// published on #7170 as R1's ceiling. +export function pickPoint(flag: boolean): Point { + return flag ? new Point(5, 6) : new Point(7, 8); +} + const p = makePoint(3); +const q = pickPoint(true); console.log( - "alloc_buckets:" + (boxed.v.x + nested.inner.a + nested.k + p.x + p.y), + "alloc_buckets:" + + (boxed.v.x + nested.inner.a + nested.k + p.x + p.y + q.x + q.y), ); diff --git a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs index 4eecb47be8..2e399ac711 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs @@ -506,9 +506,14 @@ fn a_closure_region_never_reports_a_served_return() { ); } -/// Only the RETURN position is served. `return f({a: 1})` puts the literal in -/// a call-argument position of a function that is not a producer at all, and -/// even inside a real producer a nested argument is not one of the returns. +/// Only the RETURN position is served, across both nesting shapes a `return` +/// can put an allocation in: a constructor argument of the returned `new`, and +/// an argument of a returned *call*. +/// +/// Both are in the body. The doc used to cite `return f(...)` while the body +/// built only `return new C(new C())` — a different code path, since the +/// `Expr::New` arm overrides the context and the `Expr::Call` arm is what +/// handles the other. #[test] fn only_the_return_position_is_marked_served() { let c = class_with_fields("C", &["x"]); @@ -537,6 +542,30 @@ fn only_the_return_position_is_marked_served() { .collect(); assert_eq!(served.len(), 1, "exactly the return position is served"); assert_eq!(served[0].alloc_context.as_deref(), Some("return")); + + // `return f(new C())` — the allocation is a CALL ARGUMENT. Nothing here is + // a return position, so nothing is served even in a producer region. + // + // Reuses the SAME `session`: `Session::start` takes a process-global, + // non-reentrant lock, so opening a second one while the first is alive + // deadlocks. `entries()` has already drained the sink, so the second phase + // starts clean. + let called = vec![Stmt::Return(Some(Expr::Call { + callee: Box::new(Expr::FuncRef(9)), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + }))]; + + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&called, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].alloc_context.as_deref(), Some("call argument")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); } /// Defect 1 (PR #7171 §3). Every anonymous literal renders as @@ -598,3 +627,149 @@ fn a_region_lowered_twice_still_collapses_and_says_how_much() { "and the report says how many rows it folded away" ); } + +// ── #7176 review, Major 1: `return` is a POSITION, not "anywhere under a +// returned expression" ───────────────────────────────────────────────── +// +// `RETURN` is set once, at `Stmt::Return(Some(e))`, and `scan_expr` propagates +// `ctx` unchanged through its fallback arm — only `New`, `Call` arguments and +// `Array` override it. So every allocation nested inside a returned expression +// inherited the `return` label and, in a region flagged as a return-shape +// producer, `Tier::Served` with it. +// +// The return-shape fact covers the function's RETURN VALUE. It says nothing +// about an operand of a conditional, a `&&`, an `await` or a member base that +// happens to sit inside the returned expression. Two consequences, both live: +// +// * the `return` bucket over-counts — and 323 of it was published as R1's +// ceiling on #7170; +// * servedness is decided by a string that means the wrong thing. Today no +// production region can hit that (`producer_return_class` admits only a +// bare `Expr::New` or `Expr::LocalGet` return, so a ternary/await/binary +// return yields no fact at all) — but that is a distant invariant in +// another file, and R2 widening the producer side would silently turn this +// into a wrong `Served` row. These tests force the producer flag on so the +// classifier is tested on its own terms rather than on that invariant. + +fn ternary(a: Expr, b: Expr) -> Expr { + Expr::Conditional { + condition: Box::new(Expr::Bool(true)), + then_expr: Box::new(a), + else_expr: Box::new(b), + } +} + +/// Run with the region flagged as a return-shape producer, which is the only +/// state in which the served classification can fire at all. +fn run_as_producer( + stmts: &[Stmt], + classes: &HashMap, +) -> Vec { + let session = Session::start(); + let guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(stmts, classes); + drop(guard); + session.entries() +} + +fn c_classes() -> Class { + class_with_fields("C", &["x"]) +} + +/// `return cond ? new C() : new C()` — two operands of a conditional, neither +/// of which is the function's return value. +#[test] +fn a_conditional_arm_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(ternary(new_c(), new_c())))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 2); + for e in &rows { + assert_ne!( + e.alloc_context.as_deref(), + Some("return"), + "a conditional arm is not the returned value" + ); + assert_ne!( + e.tier, + Some(crate::opt_report::Tier::Served), + "the return-shape fact does not cover a conditional arm" + ); + } +} + +/// `return flag && new C()` — a binary operand. +#[test] +fn a_logical_operand_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::And, + left: Box::new(Expr::Bool(true)), + right: Box::new(new_c()), + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// `return await new C()` — the awaited operand is not the returned value +/// either; the promise's resolution is. +#[test] +fn an_awaited_operand_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::Await(Box::new(new_c()))))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// `return new C().x` — the allocation is a member-access BASE. What is +/// returned is a field of it, which is not even an object. +#[test] +fn a_member_base_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::PropertyGet { + object: Box::new(new_c()), + property: "x".to_string(), + byte_offset: 0, + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// The anti-vacuity half: a BARE `return new C()` — the one shape the +/// return-shape fact actually covers — must still be `return` and still be +/// served. Without this, "never mark anything served" passes all four above. +#[test] +fn a_bare_return_is_still_a_return_position_and_still_served() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].alloc_context.as_deref(), Some("return")); + assert_eq!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index a9068e77f3..46546380e1 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -418,7 +418,7 @@ pub(super) fn deny_alloc_site(site: &NewSite) { if !opt_report::enabled() || suppressed() { return; } - let served = site.context == RETURN && opt_report::region_is_return_shape_producer(); + let served = site.is_return_position && opt_report::region_is_return_shape_producer(); let d = if served { UNBOUND_ALLOC_SERVED_RETURN } else { @@ -518,11 +518,26 @@ fn walk_lets(stmts: &[Stmt], depth: u32, f: &mut impl FnMut(u32, &str, u32)) { // ── Allocation-position vocabulary ───────────────────────────────────────── // // Named constants rather than inline literals, because #7170 §5.1 is a bug -// about exactly one of these strings meaning two different things. `RETURN` is -// additionally compared against in `deny_alloc_site`, and a typo there would -// silently disable the served-return classification. +// about exactly one of these strings meaning two different things. +// +// None of these strings is load-bearing: servedness is decided by +// [`NewSite::is_return_position`], set at the one site that knows it. A label +// here can be renamed without silently disabling a classification. +/// The allocation IS the function's return value: `return new C(...)`. const RETURN: &str = "return"; +/// The allocation sits *inside* a returned expression but is not the returned +/// value — a conditional arm, a `&&` operand, an awaited operand, a member +/// access base. +/// +/// Split out in review of #7176. `RETURN` was set once, at +/// `Stmt::Return(Some(e))`, and `scan_expr` propagates its context unchanged +/// through the fallback arm, so `return cond ? new C() : new D()` filed both +/// arms 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 the moment +/// the producer side widened. +const RETURNED_OPERAND: &str = "returned expression operand"; /// A genuine `new C(arg)` argument — the developer wrote a constructor call. const CTOR_ARG: &str = "constructor argument"; /// A property value of an anonymous-shape object literal. @@ -560,6 +575,15 @@ pub(super) struct NewSite { /// Index of this site in the region's walk. A de-duplication discriminant; /// see [`crate::opt_report::Entry::alloc_ordinal`]. pub ordinal: u32, + /// This allocation **is** the expression of a `Stmt::Return` — the value the + /// function hands back — rather than something nested inside it. + /// + /// The only input to the served-return classification, and set in exactly + /// one place ([`scan_return`]) together with the `context` label, so a + /// sabotage cannot kill one without the other. Deriving servedness from the + /// label string instead would make a cosmetic rename of a report bucket + /// silently disable it. + pub is_return_position: bool, /// Byte offset of the `new` expression in its module's source. `Expr::New` /// is the one HIR node that already carries a source position (#5253, /// captured for constructor TypeErrors), so allocation sites — which have @@ -617,7 +641,7 @@ fn scan_stmts( } Stmt::Expr(e) => scan_expr(e, depth, ctx, out), Stmt::Throw(e) => scan_expr(e, depth, "throw", out), - Stmt::Return(Some(e)) => scan_expr(e, depth, RETURN, out), + Stmt::Return(Some(e)) => scan_return(e, depth, out), Stmt::Return(None) => {} Stmt::If { condition, @@ -694,6 +718,52 @@ fn scan_stmts( } } +/// Scan the expression of a `Stmt::Return`. +/// +/// **The direct expression of a `return` is the function's return value; +/// anything nested inside it is an operand.** `return cond ? new C() : new D()` +/// returns the *conditional*, not either allocation, and #7107's return-shape +/// fact says nothing about them. +/// +/// This is the only place `RETURN` and [`NewSite::is_return_position`] are set, +/// and they are set together, so no sabotage can leave the label and the +/// classification disagreeing. +fn scan_return(e: &Expr, depth: u32, out: &mut Vec) { + match e { + Expr::New { + class_name, + args, + byte_offset, + .. + } => { + push_new_site(out, class_name, RETURN, depth, *byte_offset, true); + let arg_ctx = arg_context(class_name); + for a in args { + scan_expr(a, depth, arg_ctx, out); + } + } + _ => scan_expr(e, depth, RETURNED_OPERAND, out), + } +} + +fn push_new_site( + out: &mut Vec, + class_name: &str, + context: &'static str, + depth: u32, + byte_offset: u32, + is_return_position: bool, +) { + out.push(NewSite { + display: display_class(class_name), + context, + loop_depth: depth, + ordinal: out.len() as u32, + byte_offset, + is_return_position, + }); +} + fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { match e { // A closure body is its own lowering region; it reports separately. @@ -704,13 +774,9 @@ fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { byte_offset, .. } => { - out.push(NewSite { - display: display_class(class_name), - context: ctx, - loop_depth: depth, - ordinal: out.len() as u32, - byte_offset: *byte_offset, - }); + // Never a return position: `scan_return` handles the one expression + // that is, and it does not route through here. + push_new_site(out, class_name, ctx, depth, *byte_offset, false); let arg_ctx = arg_context(class_name); for a in args { scan_expr(a, depth, arg_ctx, out); diff --git a/crates/perry-codegen/src/opt_report/render.rs b/crates/perry-codegen/src/opt_report/render.rs index fd5de74901..611068d567 100644 --- a/crates/perry-codegen/src/opt_report/render.rs +++ b/crates/perry-codegen/src/opt_report/render.rs @@ -73,7 +73,18 @@ fn hotness(e: &Entry) -> String { } /// Human-readable report. +/// +/// Reads the process-global de-duplication counter. **Tests must use +/// [`render_text_with`]**: the renderers take a snapshot (`entries`) while that +/// counter is live, so a test asserting on the "duplicate row(s) collapsed" +/// line without holding a `test_support::Session` would observe whatever a +/// concurrently-running collector test last stored. pub fn render_text(entries: &[Entry]) -> String { + render_text_with(entries, super::masked_by_dedup()) +} + +/// [`render_text`] with the de-duplication count supplied explicitly. +pub fn render_text_with(entries: &[Entry], masked: usize) -> String { let mut out = String::new(); let tallies = tally(entries); @@ -134,7 +145,6 @@ pub fn render_text(entries: &[Entry]) -> String { " {:<16} {total_selected} values unboxed, {total_denied} left Boxed", "TOTAL", ); - let masked = super::masked_by_dedup(); if masked > 0 { let _ = writeln!( out, @@ -400,6 +410,15 @@ fn rule_buckets(entries: &[Entry]) -> Vec> { let slot = out .entry((e.analysis.as_str(), rule)) .or_insert((e.tier, 0)); + // Rule-to-tier is 1:1 because both come from one `ShapeDenial` + // constant — but nothing HERE enforces that, and silently keeping the + // first tier seen would let a scheduler-facing bucket mean two things + // at once, which is the defect this PR exists to remove. Assert the + // invariant rather than resolving it. + debug_assert_eq!( + slot.0, e.tier, + "rule {rule} carries two tiers; the bucket would report only one" + ); slot.1 += 1; } out.into_iter() @@ -442,6 +461,12 @@ struct JsonReport<'a> { /// builds and catch a representation regression (a `selected` count silently /// going to zero — exactly what #7034 found by hand). pub fn render_json(entries: &[Entry]) -> String { + render_json_with(entries, super::masked_by_dedup()) +} + +/// [`render_json`] with the de-duplication count supplied explicitly. See +/// [`render_text_with`] for why tests use this form. +pub fn render_json_with(entries: &[Entry], masked: usize) -> String { let tallies = tally(entries); let report = JsonReport { schema_version: SCHEMA_VERSION, @@ -450,7 +475,7 @@ pub fn render_json(entries: &[Entry]) -> String { denied: tallies.values().map(|t| t.denied).sum(), consumed: tallies.values().map(|t| t.consumed).sum(), unconsumed: tallies.values().map(|t| t.unconsumed).sum(), - masked_by_dedup: super::masked_by_dedup(), + masked_by_dedup: masked, // Enumerate `Analysis::ALL`, not the analyses that happen to have // entries: an analysis that recorded nothing must appear with an // explicit `0`. A consumer cannot tell an absent key from a zero @@ -888,7 +913,7 @@ mod r0_bucket_tests { ), alloc_site("return", SERVED, Tier::Served, 3), ]; - let json: serde_json::Value = serde_json::from_str(&render_json(&entries)).unwrap(); + let json: serde_json::Value = serde_json::from_str(&render_json_with(&entries, 0)).unwrap(); let buckets = json["summary"]["by_alloc_context"].as_array().unwrap(); let find = |ctx: &str| { buckets @@ -914,7 +939,7 @@ mod r0_bucket_tests { alloc_site("return", SERVED, Tier::Served, 1), alloc_site("return", SERVED, Tier::Served, 2), ]; - let json: serde_json::Value = serde_json::from_str(&render_json(&entries)).unwrap(); + let json: serde_json::Value = serde_json::from_str(&render_json_with(&entries, 0)).unwrap(); let rules = json["summary"]["by_rule"].as_array().unwrap(); let row = |rule: &str| { rules @@ -933,7 +958,7 @@ mod r0_bucket_tests { /// exists because already-shipped mechanisms were being counted there. #[test] fn a_served_site_renders_under_its_own_heading_not_the_roadmap_one() { - let text = render_text(&[alloc_site("return", SERVED, Tier::Served, 0)]); + let text = render_text_with(&[alloc_site("return", SERVED, Tier::Served, 0)], 0); assert!( text.contains("Already served by another mechanism"), "got:\n{text}" @@ -950,7 +975,7 @@ mod r0_bucket_tests { #[test] fn every_tier_has_a_heading_and_is_rendered() { for (i, tier) in Tier::ALL.into_iter().enumerate() { - let text = render_text(&[alloc_site("return", RULE1, tier, i as u32)]); + let text = render_text_with(&[alloc_site("return", RULE1, tier, i as u32)], 0); assert!( text.contains(tier.heading()), "{:?} is not rendered; got:\n{text}", @@ -963,15 +988,18 @@ mod r0_bucket_tests { /// fix, and it must state the position rather than leaving it to prose. #[test] fn text_reports_the_allocation_positions_as_a_table() { - let text = render_text(&[ - alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), - alloc_site( - "object literal property value", - RULE1, - Tier::CompilerLimitation, - 1, - ), - ]); + let text = render_text_with( + &[ + alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 1, + ), + ], + 0, + ); assert!( text.contains("Unbound allocation sites by position (2)"), "got:\n{text}" @@ -982,6 +1010,26 @@ mod r0_bucket_tests { ); } + /// The de-duplication count is reported only when it is nonzero, and it is + /// supplied explicitly here rather than read from the process-global that + /// `render_text` consults — otherwise this ABSENCE assertion would observe + /// whatever a concurrently-running collector test last stored. + #[test] + fn the_masked_row_count_is_printed_only_when_nonzero() { + let rows = [alloc_site("return", RULE1, Tier::CompilerLimitation, 0)]; + assert!( + !render_text_with(&rows, 0).contains("collapsed by de-duplication"), + "a zero must not print a line" + ); + let text = render_text_with(&rows, 7); + assert!( + text.contains("7 duplicate row(s) collapsed by de-duplication"), + "got:\n{text}" + ); + let json: serde_json::Value = serde_json::from_str(&render_json_with(&rows, 7)).unwrap(); + assert_eq!(json["summary"]["masked_by_dedup"], 7); + } + /// An entry that is not an allocation site contributes to `by_rule` and to /// nothing else — the alloc table must not grow rows for ordinary locals. #[test] @@ -990,7 +1038,7 @@ mod r0_bucket_tests { local.position = Position::Local; local.alloc_context = None; local.alloc_ordinal = None; - let json: serde_json::Value = serde_json::from_str(&render_json(&[local])).unwrap(); + let json: serde_json::Value = serde_json::from_str(&render_json_with(&[local], 0)).unwrap(); assert!(json["summary"]["by_alloc_context"] .as_array() .unwrap() diff --git a/scripts/compiler_output_harness/repsel_census.py b/scripts/compiler_output_harness/repsel_census.py index f176731035..d63c12c9b8 100644 --- a/scripts/compiler_output_harness/repsel_census.py +++ b/scripts/compiler_output_harness/repsel_census.py @@ -278,18 +278,36 @@ } -#: Minimum number of unbound allocations each **position bucket** must be seen -#: in, per workload. **Held in code, never in the baseline**, for the same +#: Minimum number of unbound allocations each **allocation bucket** must be +#: seen in, per workload. **Held in code, never in the baseline**, for the same #: reason as [`LIVENESS_FLOORS`], and gated separately. #: -#: #7170 R0 split one alloc-site label into three that mean different things: +#: The key is the FULL bucket identity — `(analysis, allocation position, +#: denial rule)` — and not the position and the rule counted independently. +#: That was the first shape of this table and CodeRabbit was right to reject it +#: (#7176 review): with two separate maps, a report that filed a `return` under +#: the unserved rule *and* emitted the served rule on some other position +#: satisfied both floors while describing a compiler that had stopped +#: classifying anything correctly. The renderer already defines an allocation +#: bucket by all three (`opt_report::render::alloc_buckets`); the census now +#: floors the same tuple, so the two cannot describe different things. #: -#: * `constructor argument` — a genuine `new C(arg)` argument (2.0%) +#: #7170 R0 split one alloc-site label into buckets that mean different things: +#: +#: * `constructor argument` — a genuine `new C(arg)` argument #: * `object literal property value` — a property value of an anonymous-shape -#: literal, i.e. `{a: {b: 1}}` (24.5%) -#: * `return` — with the rule naming whether the +#: literal, i.e. `{a: {b: 1}}` +#: * `return` — the allocation IS the returned value, +#: with the RULE saying whether the #: enclosing function's return-shape fact #: already serves it +#: * `returned expression operand` — nested inside a returned expression +#: (a conditional arm, a `&&` operand, an +#: awaited operand, a member base). Added +#: in review: these were being filed as +#: return positions, which over-counted +#: the `return` bucket published on #7170 +#: as R1's ceiling. #: #: Every one of those classifications can fail silently to **zero**, and a zero #: bucket is indistinguishable from "the classifier is dead" — CLAUDE.md failure @@ -298,39 +316,32 @@ #: that stops firing takes this check red instead of quietly re-merging the #: buckets it separated. #: -#: The `return` bucket carries a RULE floor as well ([`ALLOC_RULE_FLOORS`]), -#: because the position alone cannot distinguish a served return from an -#: unserved one — that distinction lives in the rule string, and it is the one -#: the corrected wall number is derived from. -ALLOC_BUCKET_FLOORS: dict[str, dict[str, int]] = { +#: The served row is also the only end-to-end assertion on the WIRING: hard- +#: coding `false` in `codegen/function.rs`, or dropping the fact conjunct in +#: `deny_alloc_site`, takes it red while every compiler unit test still passes, +#: because those set the report scope by hand. +ALLOC_BUCKET_FLOORS: dict[str, dict[tuple[str, str, str], int]] = { "fixture_alloc_buckets": { - "constructor argument": 1, - "object literal property value": 1, - "return": 1, + ("ptr-shape", "constructor argument", "rule 1 (provenance)"): 1, + ("ptr-shape", "object literal property value", "rule 1 (provenance)"): 1, + ("ptr-shape", "returned expression operand", "rule 1 (provenance)"): 1, + ( + "ptr-shape", + "return", + "rule 1 (provenance) — already served by return-shape", + ): 1, }, } -#: Minimum number of times a named DENIAL RULE must be reported, per workload — -#: the half of the #7170 R0 classification that the position cannot express. -#: **Held in code, never in the baseline.** -#: -#: `return` is one position and two opportunities. A `return new C(...)` in a -#: function that carries a return-shape fact (#7107) is already served: the -#: class reaches every `const r = producer(...)` call site, and counting it as a -#: rule-1 miss is what inflated #7170's 506/963 headline. A `return` in a -#: function WITHOUT the fact — every closure in CommonJS, which is 91.6% of the -#: dependency-corpus population — is a real miss. Only the rule string tells -#: them apart, so only a rule floor can assert the distinction is still live. -#: -#: This floor is what makes the served classification falsifiable end to end: -#: hard-coding `false` in `codegen/function.rs`, or dropping the fact conjunct -#: in `deny_alloc_site`, takes it red while every unit test still passes, -#: because the unit tests set the report scope by hand. -ALLOC_RULE_FLOORS: dict[str, dict[str, int]] = { - "fixture_alloc_buckets": { - "rule 1 (provenance) — already served by return-shape": 1, - }, -} + +def alloc_bucket_key(analysis: str, context: str, rule: str) -> str: + """The one spelling of an allocation bucket's identity. + + A string rather than a tuple because the observed map is serialized into the + baseline JSON as context. Built in exactly one place so the extractor and + the floors cannot key on different things. + """ + return f"{analysis} | {context} | {rule}" #: Workloads allowed to produce **zero candidates** — no analysis considered any @@ -463,11 +474,12 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: consumed_receiver = 0 unconsumed_mechanisms: dict[str, int] = {} denial_rules: dict[str, int] = {} - #: #7170 R0. Unbound-allocation denials keyed by the syntactic position the - #: allocation sits in, which is the bucket a scheduler reads. Kept separate - #: from `denial_rules` because two of the three buckets R0 separated share a - #: rule name and differ only here. - alloc_contexts: dict[str, int] = {} + #: #7170 R0. Unbound-allocation denials keyed by the FULL bucket identity — + #: analysis, allocation position and denial rule together, via + #: [`alloc_bucket_key`]. Not three independent tallies: two of the buckets + #: R0 separated share a rule and differ only by position, and two share a + #: position and differ only by rule, so only the tuple identifies a bucket. + alloc_buckets: dict[str, int] = {} # Keyed by analysis as well as by rule. `unconsumed_mechanisms` alone is # rule-keyed, and with a second instrumented analysis a gap in analysis A # would be excused by a mechanism recorded for analysis B. @@ -522,7 +534,12 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: ) context = entry.get("alloc_context") if context is not None: - alloc_contexts[str(context)] = alloc_contexts.get(str(context), 0) + 1 + key = alloc_bucket_key( + str(analysis), + str(context), + str(entry.get("rule") or ""), + ) + alloc_buckets[key] = alloc_buckets.get(key, 0) + 1 if unknown_sites: raise HarnessError( f"--opt-report recorded consumption at unregistered site(s) " @@ -557,7 +574,7 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: "candidates": candidates, "unconsumed_mechanisms": unconsumed_mechanisms, "denial_rules": denial_rules, - "alloc_contexts": alloc_contexts, + "alloc_buckets": alloc_buckets, "unconsumed_by_analysis": unconsumed_by_analysis, "consumed_receiver": consumed_receiver, "consumption_sites": consumption_sites, @@ -841,37 +858,23 @@ def check_alloc_bucket_floors(observed: dict[str, dict[str, Any]]) -> list[str]: "claim to observe a bucket it never measured" ) continue - buckets = observed[name].get("alloc_contexts", {}) - for context, minimum in minimums.items(): - seen = int(buckets.get(context, 0)) - if seen < minimum: - failures.append( - f"{name}: allocation position {context!r} was reported " - f"{seen} time(s), and must be reported at least {minimum}. " - "Either the classification in " - "collectors/ptr_shape_report.rs stopped distinguishing it " - "(re-merging buckets #7170 §5.1 separated) or the fixture " - f"changed shape. Observed buckets: {sorted(buckets)}" - ) - for name, minimums in ALLOC_RULE_FLOORS.items(): - if name not in observed: - failures.append( - f"alloc-rule workload {name!r} did not run; the census cannot " - "claim to observe a rule it never measured" - ) - continue - rules = observed[name].get("denial_rules", {}) - for rule, minimum in minimums.items(): - seen = int(rules.get(rule, 0)) + buckets = observed[name].get("alloc_buckets", {}) + for (analysis, context, rule), minimum in minimums.items(): + key = alloc_bucket_key(analysis, context, rule) + seen = int(buckets.get(key, 0)) if seen < minimum: failures.append( - f"{name}: denial rule {rule!r} was reported {seen} time(s), " - f"and must be reported at least {minimum}. The served-return " - "classification is no longer reaching a real compile — " - "either codegen/function.rs stopped passing the " - "return-shape fact into the report scope, or " - "deny_alloc_site stopped consulting it. " - f"Observed rules: {sorted(rules)}" + f"{name}: allocation bucket {key!r} was reported {seen} " + f"time(s), and must be reported at least {minimum}. The " + "position and the rule are floored as ONE tuple on purpose: " + "either collectors/ptr_shape_report.rs stopped telling this " + "bucket apart from a neighbour (#7170 §5.1), or the " + "served-return classification stopped reaching a real " + "compile — codegen/function.rs no longer passing the " + "return-shape fact into the report scope, or deny_alloc_site " + "no longer consulting it. Every compiler unit test stays " + "green for that last one, because they set the scope by " + f"hand. Observed buckets: {sorted(buckets)}" ) return failures @@ -1295,8 +1298,8 @@ def _update( # be re-derived from a checked-in artifact instead of from a `jq` line # in an issue comment — which is how the 506/963 headline outlived the # measurement it came from. The gated assertion is - # ALLOC_BUCKET_FLOORS / ALLOC_RULE_FLOORS, held in code. - workload["alloc_contexts"] = observed[name].get("alloc_contexts", {}) + # ALLOC_BUCKET_FLOORS, held in code. + workload["alloc_buckets"] = observed[name].get("alloc_buckets", {}) baseline["generated_at"] = utc_now() path.write_text(json.dumps(baseline, indent=2) + "\n", encoding="utf-8") print(f"Wrote {path.relative_to(REPO_ROOT)} ({len(workloads)} workload(s)).") @@ -1363,20 +1366,26 @@ def self_test(_args: argparse.Namespace) -> int: # #7170 R0: the alloc-site bucket extraction and its floors. Asserted in # the FAILING direction first, for the reason stated at the top of this - # function — and once per defect, because the three classifications can - # each die on their own. + # function — and once per defect, because the classifications can each die + # on their own. + bucket_rows = ( + ("constructor argument", "rule 1 (provenance)"), + ("object literal property value", "rule 1 (provenance)"), + ("returned expression operand", "rule 1 (provenance)"), + ("return", "rule 1 (provenance) — already served by return-shape"), + ) bucket_report = { "schema_version": 2, "summary": { "selected": 0, - "denied": 3, + "denied": len(bucket_rows), "by_analysis": [ { "analysis": a, "target_rep": a, "rule_source": "x", "selected": 0, - "denied": 3 if a == "ptr-shape" else 0, + "denied": len(bucket_rows) if a == "ptr-shape" else 0, "consumed": 0, "unconsumed": 0, } @@ -1392,19 +1401,13 @@ def self_test(_args: argparse.Namespace) -> int: "rule": rule_name, "alloc_context": context, } - for context, rule_name in ( - ("constructor argument", "rule 1 (provenance)"), - ("object literal property value", "rule 1 (provenance)"), - ("return", "rule 1 (provenance) — already served by return-shape"), - ) + for context, rule_name in bucket_rows ], } buckets = census_from_report(bucket_report) - assert buckets["alloc_contexts"] == { - "constructor argument": 1, - "object literal property value": 1, - "return": 1, - }, buckets["alloc_contexts"] + assert buckets["alloc_buckets"] == { + alloc_bucket_key("ptr-shape", c, r): 1 for c, r in bucket_rows + }, buckets["alloc_buckets"] fixture = next(iter(ALLOC_BUCKET_FLOORS)) good = {fixture: {"counts": buckets["counts"], **buckets}} @@ -1412,17 +1415,59 @@ def self_test(_args: argparse.Namespace) -> int: # Re-merging the two literal buckets — the #7170 §5.1 defect — is red. merged = json.loads(json.dumps(good)) - merged[fixture]["alloc_contexts"] = {"constructor argument": 2, "return": 1} + merged[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", "constructor argument", "rule 1 (provenance)"): 2, + alloc_bucket_key( + "ptr-shape", + "returned expression operand", + "rule 1 (provenance)", + ): 1, + alloc_bucket_key( + "ptr-shape", + "return", + "rule 1 (provenance) — already served by return-shape", + ): 1, + } failures = check_alloc_bucket_floors(merged) assert any("object literal property value" in f for f in failures), failures - # Losing the served-return classification — the #7170 §5.2 defect, and the - # only one a unit test in the compiler cannot catch, because it lives in - # codegen/function.rs rather than in the classifier — is red. - unserved = json.loads(json.dumps(good)) - unserved[fixture]["denial_rules"] = {"rule 1 (provenance)": 3} - failures = check_alloc_bucket_floors(unserved) - assert any("already served by return-shape" in f for f in failures), failures + # Filing a returned-expression OPERAND as a return position — the review + # defect this bucket was split to catch — is red. + operand_as_return = json.loads(json.dumps(good)) + operand_as_return[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", c, r): 1 + for c, r in bucket_rows + if c != "returned expression operand" + } + operand_as_return[fixture]["alloc_buckets"][ + alloc_bucket_key("ptr-shape", "return", "rule 1 (provenance)") + ] = 1 + failures = check_alloc_bucket_floors(operand_as_return) + assert any("returned expression operand" in f for f in failures), failures + + # ★ The hole CodeRabbit found in the first shape of this table: the served + # RULE present, but on the wrong POSITION. With the rule and the context + # floored independently this passed; keyed as one tuple it cannot. + swapped = json.loads(json.dumps(good)) + swapped[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", c, r): 1 + for c, r in bucket_rows + if c != "return" + } + swapped[fixture]["alloc_buckets"][ + alloc_bucket_key( + "ptr-shape", + "call argument", + "rule 1 (provenance) — already served by return-shape", + ) + ] = 1 + swapped[fixture]["denial_rules"] = { + "rule 1 (provenance) — already served by return-shape": 1 + } + failures = check_alloc_bucket_floors(swapped) + assert any( + "already served by return-shape" in f and "return" in f for f in failures + ), failures # And a workload that did not run at all cannot pass by being absent. failures = check_alloc_bucket_floors({}) From 811647bf67ff96aba186b4961b401c77311ebb73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 11:44:12 +0200 Subject: [PATCH 6/9] test(opt-report): exercise the rule-to-tier invariant assertion --- crates/perry-codegen/src/opt_report/render.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/perry-codegen/src/opt_report/render.rs b/crates/perry-codegen/src/opt_report/render.rs index 611068d567..84d0d9f0f7 100644 --- a/crates/perry-codegen/src/opt_report/render.rs +++ b/crates/perry-codegen/src/opt_report/render.rs @@ -1030,6 +1030,22 @@ mod r0_bucket_tests { assert_eq!(json["summary"]["masked_by_dedup"], 7); } + /// The rule-to-tier invariant is asserted, not resolved silently — and the + /// assertion is exercised, so it is not a guard nobody has watched fire. + /// + /// `debug_assert` is compiled out in release, which is the right trade for + /// a report-only invariant: the cost of being wrong is one mislabelled + /// bucket, not a miscompile. This test runs in the debug profile where the + /// assertion is live. + #[test] + #[should_panic(expected = "carries two tiers")] + #[cfg(debug_assertions)] + fn one_rule_carrying_two_tiers_is_a_hard_error() { + let a = alloc_site("return", RULE1, Tier::CompilerLimitation, 0); + let b = alloc_site("return", RULE1, Tier::Fixable, 1); + let _ = render_json_with(&[a, b], 0); + } + /// An entry that is not an allocation site contributes to `by_rule` and to /// nothing else — the alloc table must not grow rows for ordinary locals. #[test] From ec2cfb2436cc719e499140a324e283ddc3e44de1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 11:51:06 +0200 Subject: [PATCH 7/9] chore(census): regenerate baseline with the composite allocation buckets --- benchmarks/repsel_census/baseline.json | 92 ++++++++++++++++++-------- 1 file changed, 64 insertions(+), 28 deletions(-) diff --git a/benchmarks/repsel_census/baseline.json b/benchmarks/repsel_census/baseline.json index 362121606b..f1ec5c9aba 100644 --- a/benchmarks/repsel_census/baseline.json +++ b/benchmarks/repsel_census/baseline.json @@ -29,7 +29,8 @@ "ptr_shape_set": 1, "ptr_shape_method": 1 }, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_ptr_shape_sites", @@ -61,7 +62,8 @@ "ptr_shape_update": 1, "ptr_shape_method": 1 }, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_ptr_shape_elements", @@ -92,7 +94,8 @@ "class_field_get.shape_proven_load": 1, "ptr_shape_set": 2 }, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_alloc_buckets", @@ -110,7 +113,7 @@ "spec-abi-taptr-slot": 0 }, "candidates": { - "ptr-shape": 6, + "ptr-shape": 10, "ptr-numarray": 0, "canonical-slot": 0, "int-valued-ta": 0, @@ -124,6 +127,13 @@ "return": 1, "constructor argument": 1, "object literal property value": 1 + }, + "alloc_buckets": { + "ptr-shape | return | rule 1 (provenance) \u2014 already served by return-shape": 1, + "ptr-shape | constructor argument | rule 1 (provenance)": 1, + "ptr-shape | initializer | rule 1 (provenance)": 2, + "ptr-shape | object literal property value | rule 1 (provenance)": 1, + "ptr-shape | returned expression operand | rule 1 (provenance)": 2 } }, { @@ -150,7 +160,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_canonical_slots", @@ -176,7 +187,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_int_valued_ta", @@ -202,7 +214,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_spec_abi_taptr", @@ -228,7 +241,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_loop_bounded_i32", @@ -254,7 +268,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_module_init_canonical", @@ -280,7 +295,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "batch", @@ -314,6 +330,9 @@ }, "alloc_contexts": { "return": 1 + }, + "alloc_buckets": { + "ptr-shape | return | rule 1 (provenance)": 1 } }, { @@ -340,7 +359,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_02_loop_overhead", @@ -366,7 +386,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_03_array_write", @@ -392,7 +413,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_04_array_read", @@ -418,7 +440,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_05_fibonacci", @@ -444,7 +467,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_06_math_intensive", @@ -470,7 +494,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_07_object_create", @@ -498,7 +523,8 @@ "scalar_replaced": 1 }, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_08_string_concat", @@ -524,7 +550,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_09_method_calls", @@ -556,7 +583,8 @@ "ptr_shape_get_number": 1, "ptr_shape_set": 1 }, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_10_nested_loops", @@ -582,7 +610,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_11_prime_sieve", @@ -608,7 +637,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_12_binary_trees", @@ -636,7 +666,8 @@ "scalar_replaced": 1 }, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_13_factorial", @@ -662,7 +693,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_14_closure", @@ -688,7 +720,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_15_mandelbrot", @@ -714,7 +747,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_16_matrix_multiply", @@ -740,7 +774,8 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_17_loop_data_dependent", @@ -766,8 +801,9 @@ }, "unconsumed_mechanisms": {}, "consumption_sites": {}, - "alloc_contexts": {} + "alloc_contexts": {}, + "alloc_buckets": {} } ], - "generated_at": "2026-08-01T08:24:43.033789Z" + "generated_at": "2026-08-01T09:48:17.115667Z" } From 556f171e55e932758ddc7bc2a53cc88314429740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 11:58:06 +0200 Subject: [PATCH 8/9] chore(census): regenerate baseline on the pinned base 5a06970b6 --- benchmarks/repsel_census/baseline.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/repsel_census/baseline.json b/benchmarks/repsel_census/baseline.json index f1ec5c9aba..bc9d321501 100644 --- a/benchmarks/repsel_census/baseline.json +++ b/benchmarks/repsel_census/baseline.json @@ -805,5 +805,5 @@ "alloc_buckets": {} } ], - "generated_at": "2026-08-01T09:48:17.115667Z" + "generated_at": "2026-08-01T09:57:46.100718Z" } From 2d5c88ca63987a3eedd16dc117838beecd7bc8a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 11:59:13 +0200 Subject: [PATCH 9/9] docs: update the changelog fragment for the review fixes --- changelog.d/7176-repsel-alloc-site-buckets.md | 39 ++++++++++++------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/changelog.d/7176-repsel-alloc-site-buckets.md b/changelog.d/7176-repsel-alloc-site-buckets.md index 2e7a39b214..c1801d961a 100644 --- a/changelog.d/7176-repsel-alloc-site-buckets.md +++ b/changelog.d/7176-repsel-alloc-site-buckets.md @@ -1,9 +1,10 @@ **`--opt-report`: the `Ptr` allocation-site buckets now mean what they -are read as (#7170 R0).** Report-only — emitted LLVM IR is identical in both -arms over 30 dependency modules, each with a same-compiler control run, with -both arms run *under* `--opt-report` (the only mode in which any of this code -executes). Three defects, each of which made a scheduler-facing number say -something other than what it looked like: +are read as (#7170 R0).** Report-only — emitted LLVM IR is identical across the +**29 dependency modules that emit IR** out of 30 sampled (the 30th fails to +compile identically in both arms), each with a same-compiler control run, both +arms run *under* `--opt-report` — the only mode in which any of this code +executes — and both built from one pinned commit. Four defects, each of which +made a scheduler-facing number say something other than what it looked like: - **De-duplication was masking rows, by a factor of 2.2.** Every anonymous object literal renders as `object literal { ... }` and carries @@ -27,6 +28,14 @@ something other than what it looked like: `return new C(...)` as a producer, but `deny_alloc_site` fires before any seeding. Those sites now carry their own rule string and a new `Tier::Served` so they leave the rule-1 bucket rather than inflating it. +- **`return` was a *label propagated down a subtree*, not a position.** It was + set once at `Stmt::Return` and inherited by everything nested inside the + returned expression, so `return cond ? {a} : {b}`, `return flag && {a}`, + `return await {a}` and `return {a}.x` all counted as return positions. + `scan_return` is now the only producer of a return position, servedness reads + `NewSite::is_return_position` rather than comparing a label (so renaming a + bucket cannot silently disable it), and nested allocations get their own + bucket, `returned expression operand`. The buckets are computed by the compiler (`summary.by_rule`, `summary.by_alloc_context`) instead of being reconstructed downstream, which is @@ -41,16 +50,18 @@ are in a `function` region at all; 1711 (92.9 %) are in closures, which cannot carry a return-shape fact. The instrument's real distortion was the dedup, and it ran the other way: the wall is larger than reported, not smaller. -Census: `alloc_contexts` per workload (context, never gated), 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 — -it lives in `codegen/function.rs`, and every compiler unit test sets the report -scope by hand. No existing census floor moved 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. +Census: `alloc_buckets` per workload keyed by the full `(analysis, allocation +position, denial rule)` tuple — the same identity the renderer uses, so the two +cannot describe different things — recorded as context and floored in code via +`ALLOC_BUCKET_FLOORS`, with `fixture_alloc_buckets.ts` written to land one +allocation in each bucket. The served row is the only layer that can catch the +wiring dying: it lives in `codegen/function.rs`, and every compiler unit test +sets the report scope by hand. No existing census floor moved 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. -Still open, measured but deliberately not fixed here: `statement` is now the +Still open, measured but deliberately not fixed here: `statement` is the largest bucket (455 of 1842, 24.7 %) and is a **catch-all** — the report walk's default context, holding property stores, reassignments, conditional branches and default parameters indistinguishably.