perf(repsel): make the Ptr<Shape> alloc-site buckets mean what schedulers read them as (#7170 R0) - #7176
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds allocation-context and ordinal tracking to optimization reports, classifies served return allocations, separates allocation buckets, exposes aggregate report data, and adds census fixtures, validation, tests, and baseline entries. ChangesAllocation reporting and census validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CensusFixture
participant FunctionCodegen
participant PtrShapeReport
participant OptReport
participant RepselCensus
CensusFixture->>FunctionCodegen: lower allocation fixture
FunctionCodegen->>OptReport: enter function region with producer status
PtrShapeReport->>OptReport: record allocation context and ordinal
OptReport-->>RepselCensus: return rule and allocation buckets
RepselCensus-->>RepselCensus: validate bucket and served-rule floors
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
crates/perry-codegen/src/opt_report/mod.rs (1)
838-889: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one
Entryconstructor betweendenyanddeny_alloc.
deny_allocrepeats the whole body ofdeny. Onlyalloc_contextandalloc_ordinaldiffer. This PR already had to edit everyEntryliteral in this file to add two fields. A shared builder removes that churn for the next field.♻️ Proposed refactor
pub(crate) fn deny(d: Denial<'_>) { + deny_inner(d, None, None); +} + +pub(crate) fn deny_alloc(d: Denial<'_>, alloc_context: &str, alloc_ordinal: u32) { + deny_inner(d, Some(alloc_context.to_string()), Some(alloc_ordinal)); +} + +fn deny_inner(d: Denial<'_>, alloc_context: Option<String>, alloc_ordinal: Option<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("<unknown>"), String::from("<unknown>"), RegionKind::Function, None, ), }); push(Entry { // ... unchanged fields ... - alloc_context: None, - alloc_ordinal: None, + alloc_context, + alloc_ordinal, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/opt_report/mod.rs` around lines 838 - 889, Refactor the Entry construction used by deny and deny_alloc into a shared constructor or builder that accepts the common Denial and scope data, with optional alloc_context and alloc_ordinal values. Update both functions to use it, preserving deny’s non-allocation fields and deny_alloc’s allocation metadata without duplicating the full Entry literal.crates/perry-codegen/src/opt_report/render.rs (1)
415-423: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
alloc_bucketsfilters onalloc_context, not onPosition::AllocSite.The section heading is "Unbound allocation sites by position", and
Entry::alloc_contextis documented as "For a [Position::AllocSite] entry". This function admits any denied entry that carries analloc_context, whatever its position.The two coincide today, because only
deny_allocpopulatesalloc_contextand onlydeny_alloc_sitecalls it, always withPosition::AllocSite. The test at lines 988-999 shows the gap: to keep aPosition::Localrow out of the table, it must null outalloc_contextby hand rather than rely on the position.Add the position to the predicate, and let the test assert the position alone.
♻️ Proposed change
fn alloc_buckets(entries: &[Entry]) -> Vec<JsonAllocBucket<'_>> { let mut out: BTreeMap<(&str, &str, &str), usize> = BTreeMap::new(); - for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) { + for e in entries + .iter() + .filter(|e| e.outcome == Outcome::Denied && e.position == Position::AllocSite) + { let Some(context) = e.alloc_context.as_deref() else { continue; };And in the test, drop the manual field reset so the position carries the assertion:
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();Note that
Positionmust derivePartialEqfor this comparison; thededup_keytuple already contains it, so verify the derive before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/opt_report/render.rs` around lines 415 - 423, Update alloc_buckets to filter entries by Position::AllocSite in addition to Outcome::Denied, so alloc_context alone does not admit other positions. Modify the related test to leave alloc_context populated and verify exclusion based solely on the entry position; ensure Position derives PartialEq if required for the predicate comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs`:
- Around line 509-513: The test documentation for
only_the_return_position_is_marked_served does not match its current scenario.
Either extend the test to exercise return f(new C()) and verify the nested call
argument is not marked served, or remove the call-argument sentence from the doc
comment so it describes only the existing return new C(new C()) case.
In `@crates/perry-codegen/src/collectors/ptr_shape_report.rs`:
- Around line 410-419: Update deny_alloc_site and the return-expression scanning
flow so served-return classification applies only to an allocation that is the
direct expression of Stmt::Return, not allocations nested in conditional,
logical, await, unary, binary, or other child expressions. Track this
direct-return status explicitly on NewSite or through an equivalent flag set by
the Stmt::Return arm, while preserving the existing argument handling for direct
Expr::New calls.
In `@crates/perry-codegen/src/opt_report/mod.rs`:
- Around line 1143-1159: Make render output independent of the process-global
MASKED_BY_DEDUP state. Update the r0_bucket_tests in render.rs to create and
retain a test_support::Session before calling render_text or render_json, or
otherwise pass the masked count explicitly through those rendering functions
instead of having render_text read masked_by_dedup(). Ensure direct render tests
cannot observe values stored by other tests.
In `@crates/perry-codegen/src/opt_report/render.rs`:
- Around line 396-413: Update rule_buckets to validate that every entry sharing
an (analysis, rule) key has the same tier instead of silently retaining the
first tier. When a later entry’s tier differs from the bucket’s stored tier,
assert or otherwise fail immediately; preserve the existing denied-count
aggregation and serialized tier for valid consistent buckets.
In `@scripts/compiler_output_harness/repsel_census.py`:
- Around line 466-470: The census must keep allocation context and denial rule
together in a single bucket key. In
scripts/compiler_output_harness/repsel_census.py:466-470 add a map keyed by
analysis, allocation context, and rule; at 523-525 increment it for each denied
entry and at 560 return it in the census result. At 824-876 require the served
floor for the ptr-shape/return/rule 1 provenance tuple, and at 1364-1430 add a
sabotage case swapping that rule onto a non-return context and assert validation
fails.
---
Nitpick comments:
In `@crates/perry-codegen/src/opt_report/mod.rs`:
- Around line 838-889: Refactor the Entry construction used by deny and
deny_alloc into a shared constructor or builder that accepts the common Denial
and scope data, with optional alloc_context and alloc_ordinal values. Update
both functions to use it, preserving deny’s non-allocation fields and
deny_alloc’s allocation metadata without duplicating the full Entry literal.
In `@crates/perry-codegen/src/opt_report/render.rs`:
- Around line 415-423: Update alloc_buckets to filter entries by
Position::AllocSite in addition to Outcome::Denied, so alloc_context alone does
not admit other positions. Modify the related test to leave alloc_context
populated and verify exclusion based solely on the entry position; ensure
Position derives PartialEq if required for the predicate comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1682f992-509a-4a61-8771-c6be8de7ff14
📒 Files selected for processing (9)
benchmarks/repsel_census/baseline.jsonbenchmarks/repsel_census/fixtures/fixture_alloc_buckets.tscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/collectors/cjs_scaffolding.rscrates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rscrates/perry-codegen/src/collectors/ptr_shape_report.rscrates/perry-codegen/src/opt_report/mod.rscrates/perry-codegen/src/opt_report/render.rsscripts/compiler_output_harness/repsel_census.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7176-repsel-alloc-site-buckets.md`:
- Around line 1-4: Update the changelog statement in the `--opt-report`
comparison summary to specify 29 compilable dependency modules with identical
emitted LLVM IR, plus one additional module that failed identically in both
arms; remove the wording “over 30 dependency modules” and any implication that
all modules emitted IR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bc9156d-988f-4ff5-98ad-e12f4b66f927
📒 Files selected for processing (1)
changelog.d/7176-repsel-alloc-site-buckets.md
added 9 commits
August 1, 2026 11:54
…lers read them as (#7170 R0) Report-only. Three defects in the `--opt-report` instrumentation, each of which made a scheduler-facing number say something other than what it looked like, and two of which have now mis-scheduled work twice (#7152, #7170). 1. Per-function alloc-site dedup masked rows. Every anonymous literal renders as `object literal { ... }` and carries `byte_offset: 0`, so every unbound literal in one function collapsed onto a single `Entry::dedup_key`. An `alloc_ordinal` (the site's index in the region's deterministic walk) discriminates them; a function lowered twice still walks the same body and still collapses, and what the drain DID collapse is now reported as `summary.masked_by_dedup` rather than left implicit. 2. `constructor argument` conflated two mechanisms. A closed-shape literal lowers to `new __AnonShape_N(v0, ...)` whose constructor arguments ARE its property values, so `{a: {b: 1}}` filed its inner literal as a constructor argument. Split into `constructor argument` and `object literal property value` — measured 5 and 418 on the dependency corpus, so the old label was 98.8 % not-constructor-arguments. 3. The `return` bucket counted syntactic sites, not opportunities. `ptr_shape_returns.rs` (#7107) already admits a bare `return new C(...)` as a producer, but `deny_alloc_site` fires before any seeding. Such sites now carry their own rule and a new `Tier::Served`, so they leave the rule-1 bucket instead of inflating it. The buckets are also computed by the compiler now (`summary.by_rule`, `summary.by_alloc_context`) instead of being reconstructed downstream with `jq` — the reduction that could not see defects 2 and 3 by construction. Census: `alloc_contexts` per workload, plus `ALLOC_BUCKET_FLOORS` and `ALLOC_RULE_FLOORS` held in code, and `fixture_alloc_buckets.ts` written to land one allocation in each bucket. The rule floor is the only layer that can catch the served-return wiring dying, because that wiring lives in `codegen/function.rs` and every compiler unit test sets the report scope by hand. No census floor moved on the existing corpus and no existing workload's candidate count moved: the distortion is invisible on hand-written benchmarks and doubles the row count on real dependency JS, which is why it survived.
…ession 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.
proggeramlug
force-pushed
the
perf/7170-r0-report-buckets
branch
from
August 1, 2026 09:59
a194e26 to
2d5c88c
Compare
Contributor
Author
Review round 2 is complete — one external blockerAll six inline threads are answered, both 🟠 Majors fixed reproducer-first, and Re-run on the final base
Blocker, and it is not this PR:
That makes Deliberately not included here — silently carrying another PR's lint debt into a |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
changelog.d/7176-repsel-alloc-site-buckets.md (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReflow the paragraph to avoid a
#-led line (MD018).Line 47 starts with
#7170'sbecause of the hard line wrap. markdownlint's MD018 rule flags any line beginning with#immediately followed by non-space text as a missing-space ATX heading, even when the#is part of an issue reference inside a paragraph, not an intended heading.Rewrap the paragraph so no line starts with
#.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelog.d/7176-repsel-alloc-site-buckets.md` around lines 45 - 51, Reflow the paragraph in the changelog entry so the issue reference “#7170” is not the first characters of any line, while preserving the wording and Markdown formatting of the paragraph.Source: Linters/SAST tools
crates/perry-codegen/src/collectors/ptr_shape_report.rs (1)
721-766: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "push site + scan args" sequence in
scan_returnandscan_expr.
scan_return'sExpr::Newarm (Lines 733-744) andscan_expr'sExpr::Newarm (Lines 771-784) both callpush_new_site, then computearg_context(class_name), then loop overargscallingscan_expr(a, depth, arg_ctx, out). The only difference is thecontextandis_return_positionvalues passed topush_new_site.Extract a shared helper that takes
contextandis_return_positionas parameters and performs the push-plus-arg-scan sequence once. This class of duplicated context-propagation logic is exactly what produced the return-position bug this PR fixes; consolidating it removes the risk of the two call sites drifting apart again.♻️ Proposed refactor
+fn push_new_site_and_scan_args( + out: &mut Vec<NewSite>, + class_name: &str, + args: &[Expr], + context: &'static str, + depth: u32, + byte_offset: u32, + is_return_position: bool, +) { + push_new_site(out, class_name, context, depth, byte_offset, is_return_position); + let arg_ctx = arg_context(class_name); + for a in args { + scan_expr(a, depth, arg_ctx, out); + } +} + fn scan_return(e: &Expr, depth: u32, out: &mut Vec<NewSite>) { 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); - } + push_new_site_and_scan_args(out, class_name, args, RETURN, depth, *byte_offset, true); } _ => scan_expr(e, depth, RETURNED_OPERAND, out), } }Expr::New { class_name, args, 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); - } + // Never a return position: `scan_return` handles the one expression + // that is, and it does not route through here. + push_new_site_and_scan_args(out, class_name, args, ctx, depth, *byte_offset, false); }Also applies to: 777-782
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/collectors/ptr_shape_report.rs` around lines 721 - 766, Extract the duplicated allocation handling from scan_return and scan_expr into a shared helper that accepts class_name, args, byte_offset, depth, context, and is_return_position, performs push_new_site, derives arg_context, and scans every argument with scan_expr. Update both Expr::New arms to call this helper while preserving RETURN/true for scan_return and the existing scan_expr context/false classification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@changelog.d/7176-repsel-alloc-site-buckets.md`:
- Around line 45-51: Reflow the paragraph in the changelog entry so the issue
reference “#7170” is not the first characters of any line, while preserving the
wording and Markdown formatting of the paragraph.
In `@crates/perry-codegen/src/collectors/ptr_shape_report.rs`:
- Around line 721-766: Extract the duplicated allocation handling from
scan_return and scan_expr into a shared helper that accepts class_name, args,
byte_offset, depth, context, and is_return_position, performs push_new_site,
derives arg_context, and scans every argument with scan_expr. Update both
Expr::New arms to call this helper while preserving RETURN/true for scan_return
and the existing scan_expr context/false classification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a64a76f4-cbad-47a2-ad5c-6e77ea93eba4
📒 Files selected for processing (10)
benchmarks/repsel_census/baseline.jsonbenchmarks/repsel_census/fixtures/fixture_alloc_buckets.tschangelog.d/7176-repsel-alloc-site-buckets.mdcrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/collectors/cjs_scaffolding.rscrates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rscrates/perry-codegen/src/collectors/ptr_shape_report.rscrates/perry-codegen/src/opt_report/mod.rscrates/perry-codegen/src/opt_report/render.rsscripts/compiler_output_harness/repsel_census.py
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/perry-codegen/src/codegen/function.rs
- crates/perry-codegen/src/collectors/cjs_scaffolding.rs
- crates/perry-codegen/src/opt_report/render.rs
- crates/perry-codegen/src/opt_report/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements R0 from #7170. Report-only: the compiler emits identical LLVM IR
in both arms, verified with a same-compiler control (§4).
--opt-report'sPtr<Shape>alloc-site buckets did not mean what schedulersread them as. Three defects, each measured, two of which have already
mis-scheduled work twice (#7152 scheduled #7149 off one; #7170 was itself
scheduled off another).
1. Per-function alloc-site de-duplication was masking rows — by a factor of 2.2
Entry::dedup_keywas(module, function, name, position, analysis, outcome, rule, site). Every anonymous object literal renders asobject literal { ... }and carries
byte_offset: 0(perry-hir/src/lower/expr_object.rsneverpopulates one for them), so every unbound literal in one function collapsed
onto a single row.
PR #7171 measured one direction of this: suppressing 379 scaffolding rows
revealed 62 real user rows the dedup had been hiding, and suppressing only the
record moved its row between buckets while changing the total by zero.
Measured over 196 dependency modules, on top of #7171:
ptr-shapealloc-site denial rowsEntry::alloc_ordinal— the site's index in the region's deterministic walk —discriminates them. It is not a source location and says so; a function lowered
twice (a boxed entry plus a typed clone) walks the same body, produces the same
ordinals, and still collapses.
alloc_contextis deliberately not also inthe key: the ordinal already implies it, and a second enforcement point no
sabotage can kill is how #7171's first pass ended up with four green holes.
What the drain did collapse is now reported as
summary.masked_by_deduprather than left implicit (29 rows corpus-wide in the R0 arm — real
double-lowerings).
2.
constructor argumentwas 98.8 % not constructor argumentsA closed-shape literal lowers to
new __AnonShape_N(v0, v1, …)whoseconstructor arguments are its property values, so
{a: {b: 1}}filed itsinner literal under
constructor argument— the label for a genuinenew C(arg). #7170 §5.1 measured 2.0 % vs 24.5 % at source level. On thecorrected compiler-side instrument the split is:
object literal property valueconstructor argumentThey are different opportunities: a nested literal is a field value of a parent
allocation that is itself unbound, so proving its shape licenses nothing on its
own (#7170 T1). Merged, they read as an interprocedural-argument problem.
The most visible instance is Perry's own preamble —
const __cjs_module = { exports: {} }— which is 188 of the 194 rows #7171 measured under the oldlabel, and whose test premise this PR updates to the corrected one.
3. The
returnbucket counted syntactic sites, not opportunitiesreport::deny_alloc_siteruns at the top ofcollect_shape_proven_ptr_locals,before any seeding, while
ptr_shape_returns.rs(#7107) already admits a barereturn new C(...)with no local at all as a return-shape producer. Such a sitewas reported as a rule-1 denial while simultaneously feeding a shipped
mechanism.
These rows now carry their own rule string and a new
Tier::Served("already served by another mechanism — not a missed opportunity"), so they
leave the bucket a scheduler counts rather than merely being annotated inside
it. The claim is stated precisely in the code: the fact was issued for the
enclosing function; whether a caller consumed it is a different question with a
different counter.
And this correction turns out to be worth 4 sites of 1842 (0.2 %) — not the
material inflation §5.2 hypothesised. The reason is #7170 §6, replicated here
on the corrected instrument: only 62 of 1842 unbound allocations are in a
functionregion at all. 1711 (92.9 %) are inclosureregions, which cannotcarry a return-shape fact.
3b.
returnwas a label propagated down a subtree, not a positionFound in review, and it is the same defect class as §2 one level down.
RETURNis set once, atStmt::Return(Some(e)), andscan_exprpropagates itscontext unchanged through the fallback arm — only
New,Callarguments andArrayoverride it. So everything nested inside a returned expression inheritedthe label:
Reproduced red first, one test per shape, each forcing the producer flag on so
the classifier is tested on its own terms rather than on an invariant in another
file.
a_bare_return_is_still_a_return_position_and_still_servedpassedthroughout, so "never mark anything served" cannot satisfy the set.
Stmt::Returnnow routes throughscan_return, the only producer of a returnposition, and servedness reads
NewSite::is_return_positionrather thancomparing the label — which also closes a trap nobody had noticed: renaming a
report bucket would have silently disabled the classification. The label and the
flag are set together in that one function, so a sabotage kills both.
Nested allocations get an honest bucket of their own,
returned expression operand, with a composite floor and a fixture shape (pickPoint).Measured effect on the published numbers:
returnreturned expression operandThe rule-1 total is unchanged at 1838 — the 88 never left rule 1, they left the
returnbucket — so the headline replacing 506/963 stands. R1's ceiling doesnot, and the correction is re-posted on #7170.
One correction to the finding, stated because it changes the reasoning and not
the verdict: the served half is not reachable in a production build today.
producer_return_classadmits only a bareExpr::NeworExpr::LocalGetreturn, so a function returning a ternary carries no fact at all. But that is a
distant invariant in another file, and R2 widening the producer side would turn
it into a wrong
Servedrow silently. Fixed as a live bug, not documented as alatent one.
4. Byte-neutrality
Both arms are pinned at
5a06970b6, differing only by this patch, built-p perry -p perry-runtime-static -p perry-stdlib-static,--profile perry-dev, one target dir per arm in sequence.The pinning is load-bearing, and a run without it nearly shipped a wrong
result. A re-measurement reported 13 DIFFERENT for a report-only patch:
mainhad advanced five commits between the rebase and the base build, one ofthem changing
stmt/loops.rssafepoint emission, so the base arm carried acodegen change the R0 arm did not — the A/B was measuring
main. Diagnosed byrunning base/base/R0/base/R0 on one differing module (base stable 3x, R0
differing 2x) and reading the diff:
call void @js_gc_loop_safepoint(), whichnothing in this PR can emit. Rebuilt from one pinned SHA: 29 identical, 0
DIFFERENT, 0 control-failed. The base arm must come from the branch's own
merge-base, never from a moving
origin/main; the harness now prints the pinnedSHA and both binary mtimes.
Every number in this PR was measured at three successive bases
(
4cede62f6,b3d0541dd,5a06970b6) and is identical across all three.Compared on
--trace llvmover 30 dependency modules, each with asame-compiler control run, and both arms run WITH
--opt-report=json—neutrality measured with the report off would be vacuous, since every line this
PR touches sits behind
opt_report::enabled().identical 29, DIFFERENT 0, control-failed 0, no-ir 1 (the module that fails
to compile in both arms) — i.e. every one of the 29 modules that emits IR.
Three normalisations, each measured before being applied, none of which can
hide a real difference:
module —
__perry_cap_<n>m<hash>capture symbols carry a hash of the sourcepath, and one string constant is the absolute path. With
mkdtempper run,29 of 29 modules failed their own control. Same class as compile driver leaks its perry-objs-<pid>-<nanos>/ staging dir on the --no-link path #7167/codegen: perry_method_<Class>__<name> emission order is HashMap-nondeterministic when a method name is shared across classes (#7038/#7039 family) #7172.
@.str.Nis emitted in HashMaporder. Each symbol is rewritten to
@str<its own bytes>, so pool contentsand every use site still participate and only the arbitrary index is
neutralised.
js_register_function_nameruns — the pre-existingnondeterminism PR perf(repsel): stop charging Perry's own cjs_wrap preamble to the Ptr<Shape> report (#7152) #7171 §4 named by hand.
Anti-vacuity: the same harness, same normalisations, reports
DIFFERforPERRY_PTR_SHAPE_LOCALS=0against the same base binary. It can still see a realcodegen change.
Object hashes were not used: #7175 reports the macOS drift no longer
reproduces, but that landed after this measurement, and IR is the finer
instrument anyway.
fixture_alloc_buckets.tsalso runs byte-identically against the pinnedoracle (
alloc_buckets:12, Node 26.5.1), though no behaviour is touched:the IR result above is the stronger statement.
5. Census
alloc_contextsis extracted per workload and written to the baseline ascontext (never gated), so the wall can be re-derived from a checked-in artifact
rather than from a
jqline in an issue comment — which is how the 506/963headline outlived its measurement.
The gated assertion is held in code, like
LIVENESS_FLOORS, and is keyed bythe full bucket identity —
(analysis, allocation position, denial rule):ALLOC_BUCKET_FLOORSbyposition,
ALLOC_RULE_FLOORSby rule) with one compositeALLOC_BUCKET_FLOORS.Independently, a report that filed a
returnunder the unserved rule andemitted the served rule on some other position satisfied both floors while
describing a compiler that had stopped classifying anything correctly. That
arm is now in
census-self-testand goes red.opt_report::render::alloc_bucketsuses, so therenderer and the census cannot describe different things.
codegen/function.rs, and every compiler unit test sets the report scope byhand, so all of them stay green with
falsehard-coded there.benchmarks/repsel_census/fixtures/fixture_alloc_buckets.tslands oneallocation in each bucket, and its header says which edits would silently empty
each one.
No existing 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 this long. The only baseline
movement is the new fixture's own row.
6. Sabotage
24 arms (S1-S21 in the compiler, C1-C3 in the census harness), each reverted
alone; every one goes red, and both directions of every classification are
covered. S20 was a GREEN hole on the first pass — a
debug_assertnothingexercised — and now has
one_rule_carrying_two_tiers_is_a_hard_error.arg_contextalwaysconstructor argumenta_nested_object_literal_is_not_filed_as_a_constructor_argument+the_report_walk_drops_the_scaffolding_allocations_onlyarg_contextalwaysobject literal property valuea_real_constructor_argument_keeps_its_own_bucketa_return_in_a_return_shape_producer_is_reported_as_served,only_the_return_position_is_marked_serveda_return_in_a_plain_function_is_still_a_rule_1_denial,a_closure_region_never_reports_a_served_return,unbound_allocation_sites_are_reportedonly_the_return_position_is_marked_servedenter_function_regionignores the facta_closure_region_never_reports_a_served_returnalloc_ordinaltwo_unbound_literals_in_one_function_are_two_rows,a_region_lowered_twice_still_collapses_and_says_how_muchmasked_by_deduphard-coded 0a_region_lowered_twice_still_collapses_and_says_how_muchTier::ALLdropsServedtier_all_lists_every_variant,a_served_site_renders_under_its_own_heading_not_the_roadmap_oneby_alloc_contextalways emptyjson_reports_the_alloc_buckets_separately,text_reports_the_allocation_positions_as_a_tableby_rulemerges every rulejson_by_rule_separates_served_sites_from_the_rule_1_wallTierserde rename removedserialized_tier_matches_as_str,json_by_rule_separates_…RULE1stringa_return_in_a_return_shape_producer_is_reported_as_servedcodegen/function.rspassesfalseALLOC-SITE BUCKET NO LONGER DISTINGUISHED(no unit test can see it)scan_returnfalls through (returnleaks to operands)Newclaims to be a return positiononly_the_return_position_is_marked_serveda_bare_return_is_still_a_return_position_and_still_served+ 2rule_bucketstier invariant unassertedone_rule_carrying_two_tiers_is_a_hard_errorthe_masked_row_count_is_printed_only_when_nonzerocensus-self-testcensus-self-testcensus-self-test(the hole the two-table shape could not fail)7. What this does NOT do, and what it found
selected3 andconsumed11 across 196 dependencymodules, identical in both arms.
returns are 4 of 1842. The instrument's real distortion was the dedup, and it
ran the other way: the wall is larger than reported, not smaller.
statementis now thelargest bucket at 455 of 1842 (24.7 %), and it is a catch-all — the walk's
default context, which
walk_expr_childrenpropagates through everyexpression it does not have an arm for. Property stores, reassignments,
conditional branches, default parameters and binary operands (repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170 §3's
T1/T5/T6) are all inside it, indistinguishable. It is deliberately left for a
follow-up rather than fixed here without the liveness fixture and sabotage
arms the three buckets above carry — adding four more buckets that can each
silently read zero is the failure mode this PR exists to remove.