gc: pretenure-accumulator admission infrastructure (#7598, scope-reduced per audit) - #7623
gc: pretenure-accumulator admission infrastructure (#7598, scope-reduced per audit)#7623proggeramlug wants to merge 5 commits into
Conversation
…es (#7598) The dominant remaining cost on promote-heavy loops after #7594/#7596 is structural: every long-lived object is copied twice, Eden->survivor by the first copying minor and survivor->old by the next (measured on json_pipeline at 500k: 3.9 s of the 5.1 s build_out, 268 MB copied twice). Static pretenuring for the shape that causes it: an accumulator local admitted by the all-pointer terms (one binding, never rebound, only fresh-allocation pushes, no captures/boxes/globals) that is declared OUTSIDE every loop and pushed into only INSIDE one. Its cohort is live for the remainder of the loop by construction, so the pushed object is born in old-gen with GC_FLAG_TENURED via the existing born-tenured birth path (whose Old => TENURED obligation is contract-tested by every_old_gen_birth_path_sets_tenured since #7602). Two consumers of one mem::take'n flag (the #7590 take discipline: it reaches exactly the root allocation, nested literals read false): - lower_object_literal's shaped fast path (plain literals), and - lower_call/new.rs's outlined-call arm (the AnonShape form object literals actually reach codegen in) -- a pretenured site takes the outlined born-tenured call instead of the inline Eden bump; the ~140-cycle call is noise against the double copy it removes. Correctness is inherited, not asserted: constructor/field stores funnel through runtime_store_jsvalue_slot and the #7602-gated barrier, both of which read the LIVE parent header, so old->young field edges are remembered exactly as for a promoted object. The loop-position discriminator refuses the per-iteration accumulator (`for { const keep=[]; keep.push(..) }`) whose cohort dies every iteration -- pretenuring it would flood old-gen at allocation rate. A function-local depth-0 accumulator dropped at return IS still admitted; that adversarial case is measured in the PR alongside the win.
…ce regions (#7598) Two fixes that turn the measured v1 loss (2.73s -> 4.19s at json 200k) into a win (-> 1.54s, RSS -112 MB, hash identical): 1. arena_alloc_gc_old_born_tenured_bump: no per-allocation old_free_take_exact probe, and page registration DEFERRED into a thread-local buffer flushed at old_pages_begin_gc_cycle (every cycle kind constructs through it) or at a 64k-entry cap. The per-object register_old_object_pages was the profile's top cost: two RefCell borrows, two Vec allocations, and a linear dedup scan of the page's object list -- quadratic as a page fills. Every reader of that index runs at GC time, so cycle-start visibility is sufficient. 2. Pretenure admission now requires the REGION to run exactly once (module main/init -- entry.rs's two fact graphs pass true, every function/method/closure region passes false). A function body's depth-0 accumulator is re-entered per call and its cohort dies at return: measured 6.6x slower with 4x RSS when pretenured. Only a run-once region makes "declared outside every loop" a cohort-lifetime proof. The parameter is explicit at every call site so a new region kind must choose. json_pipeline 500k: 8.4 -> 5.4 s, RSS -173 MB; adversarial and push_bench emit zero pretenured calls (IR-verified) and are unchanged.
📝 WalkthroughWalkthroughThe change adds static accumulator eligibility analysis for all-pointer arrays, gates the resulting facts on run-once regions, and passes explicit region polarity from module and function-like code-generation paths. ChangesStatic accumulator pretenure
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen/src/lower_call/new.rs (1)
628-654: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConsume the pretenure flag before lowering constructor arguments.
Line 634 takes the flag after
lower_new_impl_innerlowers its arguments. Forout.push(new Outer({})), the argument object literal consumes the flag first.Outerthen allocates in young generation.Take the flag before the argument-lowering loop. Keep the saved value for the allocator selection.
Proposed fix
+ let pretenure = std::mem::take(&mut ctx.pretenure_next_object_literal); let mut lowered_args: Vec<String> = Vec::with_capacity(args.len()); let mut arg_roots: Vec<Option<String>> = Vec::with_capacity(args.len()); for a in args { // ... } - let pretenure = std::mem::take(&mut ctx.pretenure_next_object_literal); if pretenure || (!force_inline_new && !new_site_is_in_loop(ctx)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/lower_call/new.rs` around lines 628 - 654, Move consumption of ctx.pretenure_next_object_literal to before the constructor-argument lowering performed by lower_new_impl_inner, preserving the taken value through the allocation path. Use that saved value for the pretenure allocator selection so nested argument object literals cannot consume the flag intended for the outer constructor.
🤖 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/hir_facts.rs`:
- Around line 500-505: Split the oversized hir_facts module by moving a coherent
collector or test section out of the main file into a dedicated submodule,
preserving its behavior and updating module references as needed. Keep the
remaining logic in hir_facts intact, then run scripts/check_file_size.sh to
verify all source files meet the size limit.
---
Outside diff comments:
In `@crates/perry-codegen/src/lower_call/new.rs`:
- Around line 628-654: Move consumption of ctx.pretenure_next_object_literal to
before the constructor-argument lowering performed by lower_new_impl_inner,
preserving the taken value through the allocation path. Use that saved value for
the pretenure allocator selection so nested argument object literals cannot
consume the flag intended for the outer constructor.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 851b4618-34e5-4aec-8fb8-2113257f4b4c
📒 Files selected for processing (16)
changelog.d/7623-static-pretenure-run-once-accumulators.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/collectors/all_pointer_arrays.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/object_literal.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-runtime/src/arena/allocators.rscrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/object/alloc.rs
| // #7598: the loop-position subset whose pushed literals are born tenured. | ||
| array_facts.pretenure_accumulator_locals = | ||
| super::all_pointer_arrays::collect_pretenure_accumulator_locals( | ||
| stmts, | ||
| &array_facts.all_pointer_element_locals, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l crates/perry-codegen/src/collectors/hir_facts.rsRepository: PerryTS/perry
Length of output: 205
Split this Rust source file before merge.
crates/perry-codegen/src/collectors/hir_facts.rs is 2,576 lines and exceeds the 2,000-line source-file limit. Move a coherent collector or test section into a submodule, then run scripts/check_file_size.sh before pushing.
🤖 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/hir_facts.rs` around lines 500 - 505,
Split the oversized hir_facts module by moving a coherent collector or test
section out of the main file into a dedicated submodule, preserving its behavior
and updating module references as needed. Keep the remaining logic in hir_facts
intact, then run scripts/check_file_size.sh to verify all source files meet the
size limit.
Source: Coding guidelines
Audit — NOT merged. The headline does not reproduce against current main on the pinned host.Rebased onto v0.5.1355 (clean), built both arms identically (perry-dev,
Wall identical, RSS identical, and the census is the tell: Δ ≈ 1 MB of What I verified is right: the IR admission is correct — exactly one The open question the author-side needs to answer: the admitted site is Worth salvaging regardless of the pretenure verdict: the (Also noting: this is the third PR in this family whose promised gap-suite |
Follow-up: root cause found. The mechanism works; the measured win was a confound.Traced the whole chain on the rebased branch:
So why is the census delta ~1 MB? Because the ~113 MB the minor moves is the Which means the PR's measured 108 MB → 0 cannot have been pretenure's Disposition
Closing the loop on my earlier comment: the author-side re-measure I asked for |
The two PR audits established that the pretenure mechanism was correct but the target was wrong: json_pipeline's minor-moved cohort (~113 MB) is the runtime-allocated parse tree, not codegen-visible literals (~12 MB total, ~1 MB live at minor time), and the measured 108 MB -> 0 was a confound -- the base arm predated #7613's promote-on-first-copy seed, which on current main fires in both arms. Removed: the born-tenured allocator entry points and their keepalive anchors (an unused #[no_mangle] + #[used] pair is unused configuration per the kill-policy, and un-strippable bytes per the hello-size anchor class), the codegen consumers, and the deferred-page-registration fix (extracted separately on perf/old-page-registration-deferral, crediting this PR's finding). Kept: collect_pretenure_accumulator_locals with its refusal tests, and the explicit region_runs_once parameter on both fact-graph builders (module main/init true, function/method/closure false) with a graph-level test pinning both polarities -- the admission half a future dynamic-feedback pretenurer needs.
proggeramlug
changed the title
Re-audit of the stripped PR — the strip is correct, but this should not merge yetVerified the restructure does what it says. IR is normalized-identical to Why not merge: the collector now runs on every compile of every region That is the shape CLAUDE.md's kill-policy exists for, one level up from env Two ways I'd take it, either is fine:
I have no objection to (2) if you prefer to bank it — say which and I'll merge Unchanged from my previous comment: the deferred-page-registration half is |
Author decision: option 1 — land it with its consumer, so closing nowTaking the first of the two offered paths, and the audit's own root-cause finding is the reason: the recorded #7598 routes are dynamic feedback or allocation-context pretenure inside the JSON materialiser, and neither necessarily consumes this static collector — a materialiser-context pretenurer lives inside the runtime parser and never sees HIR accumulator proofs, and a dynamic-feedback pretenurer keys on runtime allocation sites, with the static admission at most a candidate filter. Merging the collector parked would put dead work in every compile and five threaded call sites on main as a bet on the least likely consumer, with the drift-against-no-consumer risk you named. The infrastructure is preserved where it costs nothing: branch Closing unmerged. The deferred-page-registration extraction continues separately as planned; the caller notes in this PR's body stand for that work. |
`register_old_object_pages` pays two RefCell borrows, two Vec allocations, a hash lookup and a linear dedup scan of the page's object list per object — and the scan grows as the page fills, so a burst of old-gen births into one 4 KiB page is quadratic in the objects it lands there. Since #7613's promote-on-first-copy that path is hot on ordinary workloads: a copying minor promotes straight into old-gen via `arena_alloc_gc_old`, so json_pipeline pushes ~113 MB of promotions per run through it. `arena_alloc_gc_old` now records `(header_addr, total_size)` in a thread-local buffer and one batched flush folds the whole burst in, holding a single borrow of each table and scanning only the entries that predate the batch — zero dedup comparisons for the fresh pages a bump-allocated promotion burst actually fills. Allocation policy is unchanged: the `old_free_take_exact` hole probe stays, so this is bookkeeping only. Soundness rests on one rule: every reader AND every remover of OLD_GEN_PAGE_OBJECTS / OLD_GEN_PAGE_META flushes first. Removers matter as much as readers — a removal that runs while an entry is still deferred is a no-op, and the later flush would then resurrect a dead object. `arena_alloc_gc_old_excluding_pages` (old-page defrag relocation) stays eager: it is rare, its per-object cost is dominated by the memcpy beside it, and keeping it eager narrows the proof obligation. Origin: extracted from #7623, whose pretenure half was a measurement confound and is not merging. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
) Extracted from #7623 per its audit. register_old_object_pages paid two RefCell borrows, two Vec allocations and a linear contains scan per object -- quadratic in the objects a burst lands on one page -- and since #7613's promote-on-first-copy that is a per-object path (json_pipeline 200k promotes 1,657,966 objects / 113 MB in a single minor). Now batched into an allocation-free flush that scans only the pre-batch prefix of each page. Every reader AND remover flushes first, enforced by a source-level gate that fails on a new unflushed toucher and on a stale exemption. Clean-host A/B: -3.9%/-3.7% wall at 200k/500k, census identical.
…7598) The parse cohort was the last structural GC cost on promote-heavy JSON workloads: ~113 MB single-hopping through the copying minor every run (#7613 made it single-hop; this makes it zero-hop). The #7623 audit established why codegen-side pretenuring could never reach it -- the cohort is runtime-allocated -- and named this route: allocation-context pretenure inside the materialiser. ParseBirthOldScope: a nesting-counted thread-local window (cached hot-TLS probe, the same cost class as the free-list probe every arena_alloc_gc already pays). While open, arena_alloc_gc births go straight to arena_alloc_gc_old_born_tenured. The two lazy-tape materialisation entries arm it (materialize, reparse_materialize), so engagement is size-gated by the tape's own laziness threshold, and the whole subtree -- records, strings, sub-objects, element backings, all of which funnel through arena_alloc_gc -- is born together in the generation it will live in. Subtree placement is what the #7623 parent-only attempt measurably lacked: it converts the would-be old->young field edges into old->old, so the remembered set stays out of it entirely. Costs already paid by prior work: per-object old-birth bookkeeping is deferred (#7624); Old => TENURED holds by construction through the born-tenured wrapper (#7602's contract test covers the path). Planted test: births inside the window are Old+TENURED, the window nests (stringify force-materialize reaches reparse from inside a window), and closing the last scope restores young births.
1 participant
Scope reduced per the two audits — admission infrastructure only. The original PR is preserved in the branch history (
6e73919c8/f0a2e760a); this is what remains after acting on the root-cause finding.What the audits established (both confirmed on my side)
What this PR now contains
Only the admission machinery, kept for a future dynamic-feedback pretenurer (the audit's disposition):
collect_pretenure_accumulator_locals— the loop-position proof (accumulatorletat loop depth 0, every push at depth ≥ 1) layered on the all-pointer terms, with refusal tests for the per-iteration accumulator, mixed-depth pushes, out-of-loop pushes, nested-expression pushes, and non-admitted locals.region_runs_onceparameter threaded explicitly through both fact-graph builders and every call site (module main/init = true; function/method/closure = false), with a graph-level test pinning both polarities. A function region's depth-0 accumulator is re-entered per call — the measured 6.6× adversarial regression — so only run-once regions may ever admit.Removed, deliberately:
#[no_mangle]entry point plus#[used]anchor is unused configuration (kill-policy) and un-strippable bytes (the hello-size anchor lesson). Infrastructure without callers is tests + collector only.perf/old-page-registration-deferral; not duplicated here. Caller knowledge relevant to that PR: the flush-before-cycle invariant is satisfied by all three cycle constructors (GcCycleState::new_full,gc/mod.rs:203,policy.rs's budgeted arm), andunregister_old_object_pagesis dead code, so the index accretes stale entries — thecontainsdedup is load-bearing for hole-reused addresses specifically.For #7598
The constraint that must drive the next design, from the audit: the moved cohort is runtime-allocated, so the live routes are dynamic feedback or allocation-context pretenure inside the JSON materialiser. No admission refinement fixes that; json_pipeline's literals are structurally not the target.
Process (acknowledged)
Future measurements in this family: both arms against current main on the pinned mini, moved-bytes census quoted alongside wall, and the gap-suite result posted on the PR before merge consideration. The gap suite for this branch is running; its result lands here as an edit to this body.
Summary by CodeRabbit
Performance Improvements
Documentation