perf(gc): answer arena valid-pointer membership from the census runs (#7592)

[proggeramlug]

Re-deriving the next json_pipeline lever after #7624 and #7633 (#7592) turned up a whole-heap data structure that shadows one the collector was already building.

The bug-shaped part

ValidPointerSet kept two structures over the same addresses:

• arena_runs: Vec<Vec<usize>> — the census walk's arena object starts, handed over by ArenaObjectCursorBuilder::new(ArenaWalkOrder::Address) in ascending address order, documented as existing for enclosing_object's floor lookups.
• lookup_set: BTreeSet<usize> — exact membership, fed by the same push_arena call, plus malloc starts.

The runs are sorted by construction, so a floor lookup that lands on the query already is the membership answer. The B-tree was a shadow: one insert per live arena object, for a question the runs could answer.

On json_pipeline 500k that shadow is 245.5 ms of a 748.3 ms full collection — the GC's own phase_us.build_valid_pointer_set — i.e. 12.6% of the build_out phase, and it was the second-largest single leaf in the symbolicated profile (BTreeSet::insert, 139 of 1504 build_out samples).

The fix

• push_arena stops inserting; membership for arena starts is find_arena_floor(ptr) == Some(ptr).
• lookup_set → malloc_lookup, holding only malloc-tracked starts, which have no address order to exploit. Skipped entirely when empty.
• New arena_run_firsts: Vec<usize> mirrors arena_runs[i][0] into one contiguous vector, so the run-level binary search reads 8-byte fences instead of chasing a Vec header per probe. This is load-bearing, not tidiness — without it the lookup path is measurably worse than the B-tree (see below).
• lookup_count() keeps snapshot_for_tests meaningful (arena starts + malloc starts), so cycle_state.rs's "grows by exactly one per stepped object" assertions still hold.

No policy, pacing, trigger, promotion or root-set behaviour is touched. The membership set is identical; only how it is stored and probed changes.

Measured — pinned quiet mini, interleaved A/B, 7 rounds, PERRY_NO_AUTO_OPTIMIZE=1, pinned PERRY_RUNTIME_DIR

	200k	500k
build_out phase	731 → 620 ms (−15.2%)	1,956 → 1,654 ms (−15.4%)
total wall	1,155 → 1,041 ms (−9.9%)	3,008 → 2,703 ms (−10.1%)
the full collection's pause	294.4 → 182.8 ms (−37.9%)	750.3 → 458.3 ms (−38.9%)
↳ phase_us.build_valid_pointer_set	94.3 → 9.1 ms	247.0 → 23.4 ms
↳ phase_us.trace_worklist	152.9 → 129.2 ms	388.8 → 326.4 ms
peak RSS (3 runs)	461.7 → 446.2 MB (−3.3%)	1,024.7 → 1,026.2 MB (flat, ±1%)
output SHA-256	identical	identical

The lookup path got faster, not slower — but only because of the fence array. An earlier probe kept arena_runs as Vec<Vec<usize>> and searched it with run.first() per probe: that arm moved trace_worklist the wrong way, 388.8 → 493.1 ms (+99 ms over ~9.6M lookups), and netted only −6.3% on build_out. Replacing the per-probe pointer chase with the contiguous fence array turned that +99 ms into −62 ms and took the whole change from −6.3% to −15.4%. Recorded because the intermediate result reads like "the B-tree was buying something" and it was not — it was buying an indirection.

Semantics — the collector's own counters

PERRY_GC_TRACE=1, both arms, both sizes, every non-timing key of every gc_cycle event compared field by field: 0 differences. Same cycle count, same kinds, same triggers, same promoted_bytes / promoted_objects / freed_bytes / copied_*, same remembered_set, old_pages, layout_scans, root_sources, sweep.

gc-ratchet — both arms, back to back, two independent sessions

Run on the dev Mac rather than the pinned mini: the shipped perry dyn-links homebrew libz3.4.15 and the mini has 4.16, so the compiler will not launch there. That is the right host for this comparison anyway — the gating retention and GC-accounting families are load-independent by the artifact's own cross-host evidence, and the wall/RSS story for this change is the json_pipeline A/B above, which was done on the pinned mini.

12 probes × 9 gated semantic metrics, --repeats 5:

• 106 of 108 cells byte-identical. Every minor_cycles, step_cycles, copied_objects, copied_bytes, promoted_objects, promoted_bytes, freed_bytes and heap_total_bytes cell matches exactly, on all 12 probes. correctness=pass on all 12 in both arms.
• Two heap_used_bytes cells move, both downward, and both reproduce exactly across two independent sessions (0% within-session spread over 5 repeats in every arm):

probe	base (s1 / s2)	fix (s1 / s2)	Δ
08_map_set_sidetables	1,548,960 / 1,548,960	1,512,456 / 1,512,456	−36,504 B (−2.36%)
12_large_live_set	59,942,816 / 59,942,816	59,942,456 / 59,942,456	−360 B (−0.001%)

12_large_live_set.heap_used_bytes is the cell tolerances.json already de-gates by probe override, with a documented spread of 9,072 B over 36 runs; −360 B is a twenty-fifth of that.

08_map_set_sidetables is gated, so it needs an explanation rather than a shrug. heap_used_bytes is read after the probe's own explicit gc() — the one site that forces the conservative native-stack scan ([gc-scan-fallback] site=manual_collect) — and js_arena_stats reports Σ block.offset − old_free_bytes(). What a conservative scan retains depends on stack and callee-saved-register residue at the moment of the scan, and this change alters the runtime's own code path immediately before it (a Vec push loop instead of a B-tree insert loop leaves different values in the callee-saved registers setjmp hands the scan). Fewer accidental false roots ⇒ more swept ⇒ larger old_free_bytes ⇒ lower heap_used. Both moved cells are in that direction; the band on this family is one-sided (growth is the regression), so neither is a check failure.

gc_ratchet.py classify — the tool tolerances.json prescribes for exactly this — settles it quantitatively. It re-reads each probe with PERRY_CONSERVATIVE_STACK_SCAN=off and splits retention into precise (what the collector's own roots account for) and excess (false-root residue):

probe	precise, base	precise, fix	excess, base	excess, fix
08_map_set_sidetables	1,512,456	1,512,456	36,504 (2.36%)	0 (0.00%)
12_large_live_set	51,668,568	51,668,568	8,274,248	8,273,888
the other ten	—	identical	—	identical

Precise retention is byte-identical on all twelve probes. The 08_map_set_sidetables movement is exactly its false-root residue — 36,504 B in the base arm, 0 B here — i.e. the fix's conservative reading equals the base's precise reading to the byte. Nothing about real retention changed; a different code path immediately before the scan left different register/stack residue for it to trip over, and this arm trips over none.

Gates run

• cargo fmt --all -- --check; scripts/check_file_size.sh
• gc_store_site_inventory.py (+--self-test), addr_class_inventory.py (+--self-test), class_id_collisions.py, raw_handle_debt.py (+--self-test), gc_gate_wiring_check.py (+--self-test), gc_matrix_liveness_check.py (--self-test, --check-registry), check_test_registration.py (+--self-test) — all pass
• RUST_TEST_THREADS=1 cargo test --profile perry-dev --lib -p perry-runtime --no-fail-fast — 1902 passed, 0 failed, 3 ignored
• No codegen crate is touched, so emitted IR cannot change and the root-dominance gate has no new surface.

Not in this PR

The larger item on the same profile is the copying minor's eligibility preflight — a second full traversal of the young graph, 21.8% of build_out and 14.5% of total wall — filed separately. It is a guard on the moving collector and needs a pin-site completeness gate, a sabotage test and a deliberate ratchet counter shift before it can move.

[coderabbitai]

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c4db854-51d6-48a7-95e9-6230679ac62e

📥 Commits

Reviewing files that changed from the base of the PR and between d3a4b2e and 9444b98.

📒 Files selected for processing (2)

• changelog.d/7646-gc-census-runs-membership.md
• crates/perry-runtime/src/gc/trace.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Audit — merging as v0.5.1367, with one addition

The change is right and the argument for it is exact: arena_runs is handed over in ascending address order by ArenaWalkOrder::Address, so a floor lookup that lands on the query already is the membership answer, and the BTreeSet was buying one insert per live arena object for nothing.

The arena_run_firsts note is the part I'd have been most likely to "simplify" away later, and the fragment records the measurement that forbids it: the direct Vec<Vec<usize>> search moved trace_worklist +99 ms and netted only −6.3%, which reads exactly like "the B-tree was buying something". It wasn't; it was buying an indirection. Recording the failed arm alongside the successful one is what makes that non-obvious fact survive.

What I added: the ordering requirement this creates

Membership from sorted runs has a precondition the BTreeSet did not: the B-tree was complete after every push_arena; the runs are only complete once finalize() has sealed current_arena_run. Up to VALID_POINTER_ARENA_RUN_CAPACITY (1024) censused starts are invisible before that seal.

A query on an unsealed set is a false negative, and a false negative here is not a missed optimisation — the conservative scan drops the root, the object is swept live, and it surfaces cycles later as TypeError: value is not a function. The invariant holds today (the phase machine puts Finalize before Done, and the set escapes only through finish()), but nothing asserted it, so a phase added after Finalize would corrupt the heap silently.

Added a debug_assert on the query path — free in release, and verified to fire: skipping the seal trips it with "4 censused starts are invisible to this lookup".

And a gap in the existing coverage

build_valid_pointer_set_sliced_build_preserves_contains_and_enclosing_object allocates 1100 arena strings — enough to cross the 1024 boundary — but asserts membership for only .take(16). All sixteen live in the first sealed run. That test passes unchanged if every run after the first is lost, which is precisely the failure this PR's storage change makes possible.

Added valid_pointer_membership_spans_every_census_run_including_the_partial_one_7646: ≥3 runs, every censused start checked rather than a prefix, the fence mirror asserted index-aligned with the runs, and the negative direction — an interior pointer floors to its object and must still be rejected as a start, or the conservative scan would begin tracing mid-object. Sabotage-verified: dropping the seal in finalize() and misaligning the fence mirror each redden it.

Verification

build_out −15.2%/−15.4%, total −9.9%/−10.1%, full-collection pause −37.9%/−38.9%, output SHA identical at both sizes, and every non-timing gc_cycle field compared field by field with 0 differences — that last one is what makes "the membership set is unchanged, only its storage" a measurement rather than a claim.

On the one ratchet cell that moved (08_map_set_sidetables.heap_used_bytes, −2.36%): the conservative-scan false-root explanation checks out and is consistent with #7558. Worth flagging that this is the second time a ratchet cell has moved for reasons that are an artifact of gc()'s forced conservative scan rather than of the change under test — #7558 is the right place for that to get fixed, and until it is, every ratchet delta on a retained-heap cell needs this same classify cross-check before it means anything.

Gates: 20/20 from the lint job enumerated out of test.yml, cargo fmt clean, cargo check --all-targets clean, cargo test -p perry-runtime --lib --no-fail-fast 1909 passed / 0 failed.

#7645 was the right call to file rather than take — it is a guard on the moving collector, and a 21.8% prize is exactly the kind that gets taken without the pin-site completeness gate it needs.