arena: recycled-block pool — block round-trips through the allocator were the tree.ts RSS term (#7438) - #7449
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 (8)
📝 WalkthroughWalkthroughThe runtime adds a capped thread-local pool for recycled arena blocks, reuses exact-size pooled blocks, and updates reclamation paths and diagnostics. Tests cover reuse, accounting, and thread cleanup. The change also reformats and reorders an unrelated codegen test module. ChangesArena block pool
Code generation test cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ArenaReclamation
participant BlockPool
participant try_alloc_block
ArenaReclamation->>BlockPool: block_pool_put(block.data, block.size)
BlockPool-->>ArenaReclamation: pooled or rejected
try_alloc_block->>BlockPool: take(exact_size)
BlockPool-->>try_alloc_block: pooled block or no match
try_alloc_block-->>ArenaReclamation: ArenaBlock with reset offset
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/arena/block.rs`:
- Around line 81-84: Update the BLOCK_POOL storage to wrap each (data, size)
pair in an owning Drop type that deallocates the raw block using its original
layout during thread-local teardown. Keep BLOCK_POOL_BYTES as metadata and
ensure pooled entries are no longer merely cleared without deallocation. Add a
regression test that verifies retained pooled blocks are released when the
thread exits.
- Around line 104-109: Update the block recycling path around the Unix madvise
call to check the return value of MADV_FREE before pushing the block into
BLOCK_POOL. If MADV_FREE fails, do not recycle that block; otherwise preserve
the existing BLOCK_POOL and BLOCK_POOL_BYTES updates. Ensure the behavior
remains correct for non-page-aligned blocks created by try_alloc_block and the
matching deallocation path.
🪄 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: 6cbd0a25-0ef4-47bd-a62a-68ae7deca620
📒 Files selected for processing (8)
changelog.d/7449-recycled-block-pool.mdcrates/perry-codegen/src/codegen/clone_suffix_tests.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-runtime/src/arena/block.rscrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/reset.rscrates/perry-runtime/src/arena/tests.rscrates/perry-runtime/src/gc/oldgen.rs
…lived' The count is n_blocks - general_n — survivors + longlived + old — but the label said longlived, which reads as an 84 MB longlived-arena leak on a workload whose longlived arena holds 1 MB (measured while diagnosing #7438: the trace's per-region arena_bytes tells the truth, the DIAG line did not).
…ipped (#7438) Block dealloc/realloc round-trips through the process allocator were the dominant term of tree.ts's scavenge-on peak RSS: each promoted-then-dropped cohort released its old-gen blocks and the next cohort's promotions landed in fresh allocator segments, so ever-dirtied pages grew with cumulative promotion volume — peak commit 257.5 MiB vs 140.5 MiB scavenge-off for a ~35 MB live set, while a cap matrix showed the young-cap dial barely moves RSS (64/32/16 MB caps -> 235/221/226 MB peak RSS). Reclaimed blocks now enter a capped 64 MB thread-local pool (MADV_FREE'd so the OS can take the pages under pressure) and the block reservation funnel reuses them before minting fresh mappings. No collection decision changes; thread teardown still frees for real.
tree.ts peak RSS: no pool 225 MB, 64 MB pool 190 MB, 128 MB pool 210 MB - pooled pages are MADV_FREE'd but stay resident until the OS wants them, so an oversized pool holds free pages past the optimum.
The thread-local held a bare Vec<(*mut u8, usize)>, so a thread exiting with a non-empty pool ran the Vec's destructor — freeing the Vec's own buffer and stranding every block it pointed at, up to BLOCK_POOL_CAP_BYTES per thread. perry/thread's spawn/parallelMap give each agent its own arena and GC, so repeated spawns leaked without bound in the one change whose purpose is lowering RSS. Ownership lives on the pool value rather than in a drain called from Arena::drop: both are TLS destructors, their relative order is unspecified, and LocalKey::with panics once its own destructor has run, so a drain could be skipped exactly when it is needed.
5733045 to
028a266
Compare
|
Audited and merged, with one fix pushed onto the branch. The pool leaked its blocks at thread exit. Fixed by giving the pool a Added What I verified of your work:
I also rebased the branch onto today's Two notes on the writeup, both in your favour: ruling out the young-cap dial with an actual 4-point matrix, and catching that the |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Progress on #7438 (does not fully close it — see the floor analysis at the end). All numbers from the dedicated quiet bench host (Mac mini M1/8 GB, Spotlight off), same-session A/B, best-of-3.
What the investigation ruled out
longlived=counted every non-general block; relabeled here).What was actually burning the memory
Block dealloc/realloc round-trips through the process allocator. Every promoted-then-dropped cohort released its old-gen blocks; the next cohort's promotions landed in fresh mimalloc segments, so the union of ever-dirtied pages grew with cumulative promotion volume, not the concurrent high-water. mimalloc's own accounting shows it directly on tree: peak commit 257.5 MiB scavenge-on vs 140.5 MiB scavenge-off for a ~35 MB live set.
Fix — recycled-block pool (
arena/block.rs)Released arena blocks enter a capped thread-local pool instead of round-tripping; the single block-reservation funnel (
try_alloc_block) reuses them before minting fresh mappings. Pooled pages areMADV_FREE'd (the OS can take them under pressure; contents are undefined on reuse, which every consumer tolerates — blocks are bump-filled from offset 0 and re-registered by the adopting arena). No collection decision changes; thread teardown still frees for real; the forced-allocation-failure test hook keeps priority over the pool.The 64 MB cap is measured, not guessed: no pool → 225 MB, 64 MB → 190 MB, 128 MB → 210 MB (an oversized pool holds resident
MADV_FREE'd pages past the optimum).Results (same session, same host)
peak commit on tree ON: 257.5 → 225.8 MiB. Counters are byte-identical everywhere (the pool makes no collection decisions), so the freshly pinned #7446 baseline needs no re-pin.
Why #7438 stays open
The remaining ON−OFF gap (~88 MB) is now down to structure the pool cannot touch: the young cap's committed high-water (64 vs 16 MB — the OFF arm's scale never grows), the one-time early promotion spike before the survival lock settles, and the promoting design's double-residency of the live set during transitions. Closing the last stretch means reducing early promotion volume (ramping the tenuring lock in) — a separate, riskier change tracked in the issue.
Also included: the DIAG sweep line relabel (
longlived=→non_general=) that misled this investigation.Summary by CodeRabbit
Performance
Diagnostics
Documentation