perf(gc): don't mint per-object pointer masks for single-slot payloads - #7812
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe GC now avoids per-object pointer masks for payloads smaller than a configurable threshold. Small payloads use conservative scanning, while larger payloads retain masks. Tests cover tracing, layout transitions, and threshold behavior. ChangesGC layout mask optimization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GcLayout
participant LayoutTables
participant PerObjectLayoutTable
GcLayout->>LayoutTables: Check payload slots and threshold
LayoutTables-->>GcLayout: Select conservative scanning or pointer mask
GcLayout->>PerObjectLayoutTable: Add or remove per-object mask
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
force-pushed
the
perf/7801-single-slot-pointer-mask
branch
from
August 10, 2026 23:09
5dbe513 to
386f285
Compare
proggeramlug
marked this pull request as ready for review
August 11, 2026 05:47
![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
🤖 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-runtime/src/gc/tests/layout_trace/per_object_tables.rs`:
- Around line 262-269: The child pointer is not rooted across the allocating
call, so both test sites can retain stale addresses after GC. In
crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L262-L269
and `#L308-L319`, use RuntimeHandleScope to root the NaN-boxed child before
js_array_alloc_with_length, reload the child value afterward, and derive
child_header only from the reloaded pointer.
🪄 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: 119f90ae-8090-47a1-8b10-a4c6f2da5719
📒 Files selected for processing (5)
changelog.d/7812-single-slot-pointer-mask.mdcrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/layout_tables.rscrates/perry-runtime/src/gc/tests/layout_trace/array_layout.rscrates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs
Comment on lines
+262
to
+269
| let child = crate::string::js_string_from_bytes(b"one-slot-child".as_ptr(), 14) as *mut u8; | ||
| let child_header = unsafe { header_from_user_ptr(child) }; | ||
| let arr = crate::array::js_array_alloc_with_length(1); | ||
| crate::array::js_array_set_f64( | ||
| arr, | ||
| 0, | ||
| f64::from_bits(STRING_TAG | (child as u64 & POINTER_MASK)), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Root the child value before allocating the array.
If js_array_alloc_with_length triggers GC, it can relocate child. Both tests then store the old address and later read child_header through a stale pointer.
Use RuntimeHandleScope to root the NaN-boxed child value before the array allocation. Reload the value after allocation. Derive child_header only after the reload.
crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L262-L269: Root and reload the single-slot child acrossjs_array_alloc_with_length.crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L308-L319: Root and reload the multi-slot child acrossjs_array_alloc_with_length.
Based on learnings: raw Rust pointer locals are neither GC roots nor reliable pins across an allocating operation.
📍 Affects 1 file
crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L262-L269(this comment)crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L308-L319
🤖 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-runtime/src/gc/tests/layout_trace/per_object_tables.rs` around
lines 262 - 269, The child pointer is not rooted across the allocating call, so
both test sites can retain stale addresses after GC. In
crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs#L262-L269
and `#L308-L319`, use RuntimeHandleScope to root the NaN-boxed child before
js_array_alloc_with_length, reload the child value afterward, and derive
child_header only from the reloaded pointer.
Source: Learnings
`interp.ts` spent ~19% of its runtime in `layout_forget_object`, plus ~6% in
the hashbrown probe underneath it, against a `LAYOUT_SLOT_MASKS` that had grown
past 400,000 live entries. Instrumented on the isolated FIB half, the
`PER_OBJECT_LAYOUTS_NONEMPTY` fast path fired 52 times in 15,000,000 calls.
The interpreter allocates `{ names: [p], vals: [a], parent }` per interpreted
call, so `layout_note_slot`'s "first pointer into a POINTER_FREE object" arm
minted 1.8M masks over payloads of exactly one slot. A mask over one slot can
skip nothing -- the tracer tag-checks that slot either way -- but the entry it
creates keeps the emptiness flag armed, which puts a two-map hash probe back on
every allocation in the program for as long as it lives.
Both mint sites now decline the mask below DEFAULT_MASK_MIN_SLOTS and use
GC_LAYOUT_UNKNOWN, the tag-checked scan-all-slots state that is already the
established fallback on this path. The tag check is exact here: neither site is
reachable for an object with an intact typed descriptor, so no raw-f64 slot can
be misread as a pointer.
Quiet M1 mini, best-of-5, interleaved against the same binaries with the policy
disabled: interp 1.894 -> 1.697, iso_miss 2.371 -> 2.157, new bench/mask_tax
probe 0.1218 -> 0.1049 with its numeric-element control flat at 1.000. No
regression on the rest of the 19-benchmark corpus, including tree, tree_wide,
retain*, cycles and deeplist.
proggeramlug
force-pushed
the
perf/7801-single-slot-pointer-mask
branch
from
August 11, 2026 06:14
386f285 to
4a01bcd
Compare
proggeramlug
added a commit
that referenced
this pull request
Aug 11, 2026
* fix(lint): unbreak the 2000-line file-size gate on main Two files crossed the cap in the 2026-08-11 batch and `lint` has been red on `main` ever since: crates/perry-hir/src/lower/pre_scan.rs 1985 -> 2011 (#7828) crates/perry-runtime/src/gc/layout.rs 1982 -> 2023 (#7809, #7812) Both are pure code moves, no logic change: * `pre_scan_weakref_locals` and its doc comment move to `lower/pre_scan/weakref_locals.rs` (1653 lines left behind). It is the cohesive unit — one top-level pre-scan with its own five local sets — and it is where #7828 added the lines. * `LayoutSlotMask` (the enum and its whole `impl`) moves to `gc/layout/slot_mask.rs` (1807 lines left behind). Its visibility widens from `pub(super)` to `pub(in crate::gc)` because the type is now one module deeper and `layout_tables.rs` / `hot_tls.rs` still name it; the reachable set is unchanged. `scripts/check_file_size.sh` passes. * docs: changelog fragment for #7830 * docs: condense #7830 changelog entry --------- Co-authored-by: Ralph Küpper <ralph3@skelpo.com> Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 11, 2026
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.
What
layout_note_slotandlayout_rebuild_from_slots_with_policyno longer mint aper-object pointer mask for a payload of a single slot. Those objects take
GC_LAYOUT_UNKNOWN— the tag-checked scan-all-slots state — instead.Draft: measurement and validation are complete (below), but the threshold is a
policy call I'd like a second opinion on before this goes green.
Why
gc-handoff/apps/interp.ts— the tree-walking interpreter, the corpus programthat best resembles real software — spent ~19% of its runtime in
layout_forget_object, plus another ~6% in the hashbrown probe underneath it.That is side-table bookkeeping, not user work, and by design it should have been
near zero: #7510's
PER_OBJECT_LAYOUTS_NONEMPTYexists precisely so theallocation/store/death/relocation paths can skip both maps while they are empty,
"which on a monomorphic workload they are".
They were not. Instrumented on
iso_FIB.ts:The disarmed fast path fired 52 times in 15 million calls; every other call
took two
RefCellround-trips and two hashes against a 400k-entry, cache-coldmap.
The interpreter allocates
{ names: [p], vals: [a], parent }per interpretedcall, so it minted 1.8M masks over payloads of exactly one slot. A mask over
one slot cannot skip anything — the tracer consults
layout_pointer_bearing_bitson that slot either way — so the side-table entrywas the mask's entire contribution. Those entries also outlive their arrays
(reclaimed only when the recycled address is allocated over), and one live entry
anywhere keeps the flag armed for every allocation in the program. This is
#7510's "one immortal entry nullifies
is_empty()", from the other direction.Safety
GC_LAYOUT_UNKNOWNis already the established fallback on this exact path(
heap_payload_slot_selectionsets it when aSIDE_MASKobject has no mask),and the tag check is exact here: neither mint site is reachable for an object
with an intact typed descriptor, so there are no raw-f64 slots whose bits could
be misread as a pointer. Both directions of the slot-count estimate are
correct, only differently priced — over-estimating mints a mask that was not
needed (the old behaviour), under-estimating routes the object to a scan that
visits a superset of what the mask would have selected. Neither can hide a
live child.
Two details that cost me a wrong turn each, both now in the code comments:
length, but only for a store into an already-formedarray. Every append protocol notes the slot before bumping
length, somid-construction
lengthis the pre-append value; judging on it strandedevery incrementally built array (a
pushloop, a JSON parse) in the scanstate regardless of final size. Capacity is not a substitute either —
MIN_ARRAY_CAPACITYis 16, so a one-element literal reports 16 and thedistinction disappears entirely.
GcHeader::size, notfield_count,because
sizeis maintained for every GC allocation whatever itstype-specific header holds.
Measured
Quiet M1 mini, best-of-5, interleaved against the same binaries with the
policy disabled (
PERRY_LAYOUT_MASK_MIN_SLOTS=0) so build drift cannotmasquerade as a win. Outputs byte-identical to node and exit codes checked per
cell.
interpiso_missbench/mask_tax(new probe)bench/mask_tax_nopointer(control)No regression on the rest of the 19-benchmark corpus, including the GC-heavy
tree(1.626),tree_wide(2.110),retain(0.538),retain_wide(1.094),cycles(0.193) anddeeplist(0.245). The policy-disabled arm reproduces thepristine baseline on every row, which is the control for this patch adding cost
of its own.
Validation
cargo test -p perry-runtime— 2053 passed, 0 failed (cargo's own exitcode, not a pipe's).
iso_missprintschecksum 437840 misses 0plain andunder
PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800,PERRY_GC_VERIFY_EVACUATION=1, andPERRY_GC_FORCE_EVACUATE=1.expected/and to the pristinebaseline build, exit 0.
check_file_size.sh,cargo fmt --all -- --check,addr_class_inventory.pyall clean. (
layout.rswent 19 lines over the 2000 cap, so the two newhelpers live in
layout_tables.rs— their natural home anyway.)Tests
Two new tests in
layout_trace/per_object_tables.rs, one per side of thethreshold: a single-slot pointer payload must leave the side tables empty
and still trace its child, and a payload at/above the threshold must still mint
a mask recording exactly the one pointer slot. The second exists so the first
would not pass equally well if per-object masks had been deleted outright.
Four existing fixtures changed, all preconditions rather than behaviour: three
widened from a 1-element to a 2-element array so they keep exercising the mask
grow/transfer/clear paths they are about, and two assertions in
test_array_mixed_bulk_producers_preserve_pointer_layoutnow assertNonewhere a one-element bulk result no longer carries a (vacuous) mask — the
behavioural assertions next to them, that the tracer reads the slot and marks
the child, are unchanged and still pass.
The threshold, and what is deliberately left on the table
DEFAULT_MASK_MIN_SLOTS = 2is the provable end of the range: at one slot themask demonstrably skips nothing, so no judgement about tracing cost is being
made. Raising it pays roughly twice as much —
9and above givesinterp1.619 and
iso_miss2.046, with still no regression anywhere on thecorpus — but 21 tests in this crate encode "a small mixed payload uses a mask"
as a precondition (5 at
2, 11 at3, saturating at 21 from9). That is acontract change worth making on purpose, not as a side effect of a perf patch,
so it is left for a follow-up.
PERRY_LAYOUT_MASK_MIN_SLOTSoverrides the threshold for bisection. Flagging itexplicitly against the knob kill-policy in CLAUDE.md: both regimes are exercised
by the two new tests, but if you would rather not carry a new GC env knob at
all, say so and I will drop the env read and keep the constant.
Summary by CodeRabbit
Performance
Bug Fixes
Tests