perf(codegen): interp 1.095 -> 0.844 s — gate the class-field barrier on the parent's generation, and let a hot recursive function inline its bump allocator - #7871
Conversation
…eration, and let a hot recursive function inline its bump allocator
…ain, not by proximity
…nd cold elsewhere
📝 WalkthroughWalkthroughThe PR adds allocation-hot function analysis for inline bump allocation and adds parent GC-state gating for class-field write barriers. It also adds LLVM IR regression tests and documents benchmark and validation results. ChangesAllocation-hot function classification
Class-field write-barrier gating
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant HIRModule
participant AllocHotCollector
participant CrossModuleCtx
participant compile_function
participant new_site_is_in_loop
HIRModule->>AllocHotCollector: scan loops and direct recursion
AllocHotCollector->>CrossModuleCtx: record alloc_hot_functions
CrossModuleCtx->>compile_function: provide allocation-hot function IDs
compile_function->>new_site_is_in_loop: classify allocation site
new_site_is_in_loop->>new_site_is_in_loop: select inline bump allocation
sequenceDiagram
participant ClassFieldStore
participant ParentGCState
participant BarrierBlock
participant js_write_barrier_slot
ClassFieldStore->>ParentGCState: evaluate tenure and incremental marking
ParentGCState-->>ClassFieldStore: return live barrier condition
ClassFieldStore->>BarrierBlock: branch when barrier is required
BarrierBlock->>js_write_barrier_slot: emit slot barrier
🚥 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
🧹 Nitpick comments (1)
crates/perry-codegen/src/lower_call/alloc_hot_tests.rs (1)
219-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the uncapped in-loop rule.
This test covers direct self-recursion only. It never exceeds
inline_hot_small_max_call_sites. Add a fixture with a direct in-loop call and more than four module call sites. Assert that itsnewsite uses the inline bump allocator.🤖 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/alloc_hot_tests.rs` around lines 219 - 233, Add a fixture and test alongside a_self_recursive_function_inlines_its_bump_allocator with a direct in-loop call and more than four module call sites, exceeding inline_hot_small_max_call_sites. Assert the generated IR contains INLINE_SLOW_CALL and INLINE_FAST_BLOCK for that new site and does not contain OUTLINED_CALL.
🤖 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/expr/class_field_barrier_tests.rs`:
- Around line 383-390: Update the assertion for count_reg in the incremental
barrier test to require its definition contains “load atomic i32”,
INCREMENTAL_GLOBAL, and “seq_cst”, preserving the existing failure context and
ensuring the incremental-count read is verified as atomic.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/alloc_hot_tests.rs`:
- Around line 219-233: Add a fixture and test alongside
a_self_recursive_function_inlines_its_bump_allocator with a direct in-loop call
and more than four module call sites, exceeding inline_hot_small_max_call_sites.
Assert the generated IR contains INLINE_SLOW_CALL and INLINE_FAST_BLOCK for that
new site and does not contain OUTLINED_CALL.
🪄 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: 91e4ada8-ec56-41ae-91f0-09f692fee6e9
📒 Files selected for processing (13)
changelog.d/7871-interp-round5-alloc-and-barrier.mdcrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/collectors/hot_callees.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/expr/class_field_barrier_tests.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/function.rscrates/perry-codegen/src/lower_call/alloc_hot_tests.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-codegen/src/lower_call/new_alloc.rs
| let count_reg = operand(incremental_cmp, 0).expect("icmp lhs"); | ||
| assert!( | ||
| def_of(&body, &count_reg) | ||
| .unwrap_or_default() | ||
| .contains(INCREMENTAL_GLOBAL), | ||
| "the incremental clause does not read {INCREMENTAL_GLOBAL}; skipping \ | ||
| the barrier also skips SATB shading:\n{body}" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-codegen/src/expr/write_barrier.rs \
--items all --type function --match 'emit_parent_may_need_remembering_check'
rg -n -C 6 '\bload_atomic_seq_cst\s*\(' \
crates/perry-codegen/src --glob '*.rs'
rg -n -P \
'load atomic i32.*`@PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT`.*seq_cst' \
. -g '*.ll' -g '*.rs'Repository: PerryTS/perry
Length of output: 11586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- class-field barrier test ---'
sed -n '330,405p' crates/perry-codegen/src/expr/class_field_barrier_tests.rs
printf '%s\n' '--- remembering check implementation ---'
sed -n '120,155p' crates/perry-codegen/src/expr/write_barrier.rs
printf '%s\n' '--- LLVM load emission ---'
sed -n '575,615p' crates/perry-codegen/src/block.rs
printf '%s\n' '--- test helpers and related assertions ---'
rg -n -C 8 'fn def_of|incremental_cmp|INCREMENTAL_GLOBAL|load atomic i32' \
crates/perry-codegen/src/expr/class_field_barrier_tests.rs \
crates/perry-codegen/tests crates/perry-codegen/src --glob '*.rs'Repository: PerryTS/perry
Length of output: 27252
Assert that the incremental-count read is atomic.
Require the definition to contain load atomic i32, INCREMENTAL_GLOBAL, and seq_cst.
🤖 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/expr/class_field_barrier_tests.rs` around lines 383
- 390, Update the assertion for count_reg in the incremental barrier test to
require its definition contains “load atomic i32”, INCREMENTAL_GLOBAL, and
“seq_cst”, preserving the existing failure context and ensuring the
incremental-count read is verified as atomic.
Per-lever attribution (dev box, best-of-7, interleaved, one compiler per column)Both arms link the same runtime archives, so
The disjointness is the evidence that each lever hits what it claims: the Base commit and mergeabilityMeasured as an A/B at |
Gap suite: 541 tests, zero regressions
All 23 non-passing tests were triaged, none is mine.
The measured binary is the shipped codeAfter the mini run I edited comments (renaming the issue reference to this PR) |
1 participant
interp1.095 → 0.844 s,iso_miss1.464 → 1.234 sTwo independent codegen gates, both of which restore a mechanism this repo
already had and had pointed at only one caller.
Quiet M1 mini, best-of-5, exit-checked, outputs byte-compared to
node --experimental-strip-types, taken under the bench lock (load 1.58 before/ 2.19 after, zero foreign processes at both ends). The re-measured baseline
reproduces round 4's published numbers to 0.3% (
interp1.0945 vs 1.097;iso_miss1.4635 vs 1.465), which is the check that the two rounds are on thesame instrument.
interpcyclesiso_misstreetree_widedeeplistpipelineshapes,asyncpipechurn,push_cls,retain,fib40— byte-identical binariesThe four byte-identical binaries set the run's noise floor at ±0.3%.
1. The class-field write barrier now tests the parent's generation
emit_jsvalue_slot_store_pointer_tested(#7511) put the class-field store'sthree bookkeeping calls behind one live test of the stored value — "does
this publish a heap pointer at all". It never asked the barrier's other
question, "is the parent old enough for anyone to care", although
emit_parent_may_need_remembering_checksits 400 lines above it in the samefile. That predicate had exactly one caller:
expr/array_push.rs.HIR rewrites every closed-shape object literal into a
newof a synthesizedanon-shape class, so
{ kind: "num", num: n }reaches a shared<class>_constructorthat writes its fields into an instance allocated a fewinstructions earlier in the nursery — the
!TENUREDcase, where the minorGC retraces the parent anyway and the remembered-set record is pure cost.
The same predicate now gates the class-field store, on the identical argument:
Old ⟹ TENURED, so!TENUREDcan only skip a subset of what the runtimealready skips; and the second disjunct is the incremental-cycle count, because
skipping the call also skips
barrier_child_prologue's SATB shading, which isnot a generational question. It stays a live header test (#7501's shape),
never a static claim: a parent promoted between its allocation and the store
reads
TENUREDhere and takes the call.2. A hot recursive function may inline its bump allocator
new_site_is_in_loopadmitted anewsite to the inline bump allocator onlyif it was lexically inside a loop, or its function was in
collect_hot_loop_callees— a set capped at 4 direct call sitesmodule-wide.
That cap is
inlinehint's anti-bloat backstop, where cost scales with callsites because LLVM duplicates the callee body at each one. The inline bump
allocator costs ~268 bytes per
newsite in the function, paid once,whatever the caller count. So the cap priced a cost that does not exist here —
and excluded exactly the functions that earn the inline form.
gc-handoff/apps/interp.ts'sevalNodeis the shape: the hottest function inthe program, one allocation per invocation, and 11 direct call sites because
ten of them are its own recursion.
New collector
collect_alloc_hot_functionsanswers the allocator's questionwith the allocator's cost model — ≥1 in-loop direct call site (uncapped),
or direct self-recursion, since a function that calls itself is a loop the
lexical test cannot see. It is deliberately a second set, not a widening of
hot_loop_callees: raising the shared cap to 32 instead buysinterp−26.5%but regresses
iso_miss+4.0%, because that cap also movesinlinehint.interp's compiled binary grows 16 KB (+0.13%).The two levers are independent
cmpover the corpus separates them cleanly. Forcing inline allocation(
PERRY_INLINE_NEW=1) changes 4 of 19 binaries — exactly the programs whosenewsites sit in non-loop functions. The barrier gate changes 9 — exactlythe programs whose class fields hold pointers. Only
interp,iso_missandpipelineare in both sets; 10 of 19 binaries are byte-identical across thefull change and needed no timing at all.
Validation
including
iso_miss'smisses 0counter andshapes'1176000(thesilent-wrong-answer canary from earlier this week).
PERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_FORCE_EVACUATE=1; five of them additionally underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_SCHEDULE_ALLOC_KB=64.cargo test --release -p perry-codegen— 26 test binaries, 0 failures,including perf(gc): write barriers cost 16% on an all-numeric store workload — elide on provably-non-pointer stores #7511's own
class_field_store_pointer_test(whose CFG-region walkstill finds
js_write_barrier_slotinside the guarded region) andlarge_object_barriers.--filter test_gap_, node 26.5.1): running; every failure so faris already in
test-parity/gap_snapshot.json. Result appended below.cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/addr_class_inventory.py,scripts/gc_runtime_root_holders.py— allclean.
★ The test is sabotage-verified, and its first draft was not
expr/class_field_barrier_tests.rsasserts the gate is REACHED (acond_brinto
class_field_set.barrier, not merely a block with that name), that thebarrier is on the TAKEN edge, and that
js_write_barrier_slotis stillreachable inside it — a guard, never an elision.
The first draft asserted that the TENURED mask and the incremental-count load
were present in the branching block. It passed a sabotage that hard-wired
the branch to
br i1 falsewhile leaving the now-dead predicate instructionssitting behind it — the exact "gate that cannot fail" shape CLAUDE.md
catalogues. The assertion now walks the def chain from the branch condition:
Both sabotages now go red with the diagnostic that names the failure:
"the gate's condition is the constant
false— the branch cannot fail" and"the barrier is on the FALSE edge — an untenured parent would take the call
and a tenured one would skip it".
Refuted
PERRY_WRITE_BARRIERS=0is not a cost ceiling for the write barrier. Itmakes
interp4.3× andiso_miss5.4× slower (1.46 → 6.32, 1.71 → 9.16):the knob is compile-time, and the GC's evacuation policy requires generated
barriers to be active, so with them off the copying minor is ineligible and the
program falls back to full mark-sweeps. It measures "no generational GC", not
"no barrier cost" — and reads backwards to anyone sizing a barrier lever with
it. Size the barrier by building the gate.
Not reached
Round 5's targets were
interp≤ 0.80 andiso_miss≤ 1.10. At 0.844 / 1.234they are missed. A
--debug-symbolsprofile of the patched binary shows why:with the outlined allocator and the barrier calls gone,
evalNode's own inlinecode is now essentially the whole program, and the remaining lever is the one
round 4 identified — hoisting the shape check out of a discriminant arm.
gc-handoff/INTERP5-NOTES.mdcarries the sizing and the two side findings(
PERRY_INLINE_CTOR=1is worthchurn−10.6% /cycles−5.8% on its own).Summary by CodeRabbit
Performance
Reliability