fix(gc): bound and drain recycled block pool - #7889
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe runtime replaces the fixed per-thread arena block-pool cap with process-wide, heap-budget-aware accounting. Arena reclamation separates removed, pooled, and deallocated blocks. Critical pressure and emergency recovery drain pooled blocks, with expanded GC telemetry and tests. ChangesArena block-pool lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 |
Contributor
Author
|
@coderabbitai review |
|
![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/arena/reset.rs`:
- Around line 891-902: Update SurvivorArenaReclaimDeadBlocksState::step so each
successful state0.step() or state1.step() call immediately merges that child’s
counter delta, preserving removed_* and pooled_* statistics across multiple
reclamation slices. Add a regression test using multiple survivor blocks and a
one-unit budget to verify cumulative statistics.
🪄 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: 56978740-9c84-4076-b98a-ca872ed293d4
📒 Files selected for processing (20)
changelog.d/7889-block-pool-pressure.mdcrates/perry-runtime/src/arena/block.rscrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/arena/reset.rscrates/perry-runtime/src/arena/tests.rscrates/perry-runtime/src/arena/walk.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/heap_budget.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/pressure.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/tests/block_pool_pressure.rscrates/perry-runtime/src/gc/tests/incremental_sweep_reclaim.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/oldgen.rscrates/perry-runtime/src/gc/tests/triggers.rs
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-runtime/src/arena/tests.rs (1)
1258-1275: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the drain and process-wide census.
force_next_block_alloc_failureis consumed by the injectable first allocation, and emergency reclaim requests the pool drain.block_pool_bytes_for_test()checks only the thread-local pool. Capture the initial drain and process-wide counts, then assert the drain census includes the removed bytes and the process-wide reservation returns to its initial value.🤖 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/arena/tests.rs` around lines 1258 - 1275, Update allocation_failure_recovery_drains_mismatched_pooled_blocks to capture the initial drain census and process-wide reservation count before setup. After reserve_arena_block(BLOCK_SIZE + 1), assert the drain census accounts for the removed pooled block bytes and that the process-wide reservation count has returned to its initial value, while retaining the existing thread-local assertion.
🧹 Nitpick comments (1)
crates/perry-runtime/src/arena/tests.rs (1)
1207-1256: 🩺 Stability & Availability | 🔵 TrivialRun this test with serialized runtime tests.
The test checks process-wide pool reservations while other runtime tests can use the same process-global state. Parallel test execution can change the reservation before the final zero check.
Use
RUST_TEST_THREADS=1for focused and full runtime test commands, or use the suite's global test lock if one exists.As per coding guidelines, runtime tests are not parallel-safe because they share process-global side tables; run them with
RUST_TEST_THREADS=1.🤖 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/arena/tests.rs` around lines 1207 - 1256, Run the runtime test suite, including block_pool_cap_is_process_wide_across_live_threads, with serialized test execution by setting RUST_TEST_THREADS=1 in focused and full test commands, or use the suite’s existing global test lock if available. Keep the test’s process-wide reservation assertions unchanged.Source: Coding guidelines
🤖 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/tests.rs`:
- Around line 163-166: Update the test using run_with_fresh_arenas so its
pool-cap precondition is explicit: configure PERRY_GC_HEAP_LIMIT to a budget of
at least 16 MB before collecting stats, or change the assertions to validate the
pooled/deallocated partition for smaller budgets. Preserve the existing block
and byte accounting expectations.
---
Outside diff comments:
In `@crates/perry-runtime/src/arena/tests.rs`:
- Around line 1258-1275: Update
allocation_failure_recovery_drains_mismatched_pooled_blocks to capture the
initial drain census and process-wide reservation count before setup. After
reserve_arena_block(BLOCK_SIZE + 1), assert the drain census accounts for the
removed pooled block bytes and that the process-wide reservation count has
returned to its initial value, while retaining the existing thread-local
assertion.
---
Nitpick comments:
In `@crates/perry-runtime/src/arena/tests.rs`:
- Around line 1207-1256: Run the runtime test suite, including
block_pool_cap_is_process_wide_across_live_threads, with serialized test
execution by setting RUST_TEST_THREADS=1 in focused and full test commands, or
use the suite’s existing global test lock if available. Keep the test’s
process-wide reservation assertions unchanged.
🪄 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: 8771c320-102f-4871-ac8f-3dee67168331
📒 Files selected for processing (2)
crates/perry-runtime/src/arena/reset.rscrates/perry-runtime/src/arena/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/arena/reset.rs
Comment on lines
+163
to
+166
| assert_eq!(stats.pooled_blocks, 2); | ||
| assert_eq!(stats.pooled_bytes, stats.removed_bytes); | ||
| assert_eq!(stats.deallocated_blocks, 0); | ||
| assert_eq!(stats.deallocated_bytes, 0); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/perry-runtime/src/arena/tests.rs"
ast-grep outline "$file" --items all --type function
sed -n '130,175p' "$file"
rg -n -C 6 \
'run_with_fresh_arenas|block_pool_cap_bytes|PERRY_GC_HEAP_LIMIT|heap_budget' \
"$file" \
crates/perry-runtime/src/arena/block.rs \
crates/perry-runtime/src/gc/heap_budget.rsRepository: PerryTS/perry
Length of output: 42118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test helpers and constants ---'
sed -n '1,115p' crates/perry-runtime/src/arena/tests.rs
echo '--- target test ---'
sed -n '140,170p' crates/perry-runtime/src/arena/tests.rs
echo '--- reset/release implementation ---'
sed -n '180,270p' crates/perry-runtime/src/arena/block.rs
echo '--- pool accounting and budget helpers ---'
sed -n '120,155p' crates/perry-runtime/src/gc/heap_budget.rs
rg -n -C 8 \
'BLOCK_SIZE|BLOCK_POOL_BYTES|BLOCK_POOL_PROCESS_BYTES|block_pool_(put|cap|clear|reset)|RUST_TEST_THREADS|serial|PERRY_GC_HEAP_LIMIT' \
crates/perry-runtime/src/arena crates/perry-runtime/src/gcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
BLOCK_SIZE = 1024 * 1024
def budget_scaled(budget, default=64 * 1024 * 1024, num=1, den=8, floor=BLOCK_SIZE):
if budget is None:
return default
# Match the documented min(default, budget*num/den), then floor.
return max(floor, min(default, budget * num // den))
print("required_bytes_for_two_removed_blocks =", 2 * BLOCK_SIZE)
for mb in (1, 8, 15, 16, 17, 512, 1024, None):
budget = None if mb is None else mb * BLOCK_SIZE
cap = budget_scaled(budget)
print(f"budget_mb={mb!s:>4} pool_cap={cap // BLOCK_SIZE} blocks "
f"admits_two={cap >= 2 * BLOCK_SIZE}")
print("source facts:")
print("run_with_fresh_arenas_sets_budget = False")
print("BLOCK_POOL_PROCESS_BYTES_is_static_process_global = True")
print("BLOCK_POOL_is_thread_local = True")
print("thread_local_pool_drop_subtracts_process_counter = False")
PY
echo '--- exact budget scaling implementation ---'
sed -n '85,125p' crates/perry-runtime/src/gc/heap_budget.rs
echo '--- thread-local pool type and teardown declarations ---'
sed -n '105,155p' crates/perry-runtime/src/arena/block.rs
rg -n -C 4 \
'impl Drop for BlockPool|drain_block_pool_if_requested|request_block_pool_drain|BLOCK_POOL_PROCESS_BYTES' \
crates/perry-runtime/src/arena crates/perry-runtime/src/gcRepository: PerryTS/perry
Length of output: 12939
Make the pool-cap precondition explicit.
run_with_fresh_arenas does not set PERRY_GC_HEAP_LIMIT. For budgets below 16 MB, gc_block_pool_cap_bytes() admits only one BLOCK_SIZE, but the test requires two pooled blocks. Pin a budget that admits both blocks, or assert the pooled/deallocated partition.
🤖 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/arena/tests.rs` around lines 163 - 166, Update the
test using run_with_fresh_arenas so its pool-cap precondition is explicit:
configure PERRY_GC_HEAP_LIMIT to a budget of at least 16 MB before collecting
stats, or change the assertions to validate the pooled/deallocated partition for
smaller budgets. Preserve the existing block and byte accounting expectations.
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.
Closes #7875
What changed
perry/threadagent to retain 64 MiBPERRY_GC_HEAP_LIMIT/ the device budget, with the historical 64 MiB desktop cap unchangedReproduction and coverage
A regression test seeded one 1 MiB recycled block, called
js_gc_memory_pressure(2), and failed on the old implementation with 1,048,576 bytes still pooled. Coverage now includes synchronous, shadow-frame-deferred, and suppressed critical pressure; allocation-failure recovery; small-budget cap scaling; four concurrent owners sharing one atomic cap; pool/thread teardown; release accounting; incremental old-gen publication; and trace JSON.Validation
cargo test -p perry-runtime block_pool— 7 passedcargo test -p perry-runtimebefore the final rebase — 2,131 passed, 4 ignoredmain, two parallel full-suite runs each moved the only failure to a different unrelated shared-state/timing test (promise::keyed_tablethenpromote_in_place); both passed immediately in isolation and all memory(gc): critical pressure and device budgets do not drain or bound the per-thread 64 MiB block pool #7875 tests passed in both runsmainreports the unrelated stale-but-improveddyn_index.rsaddress-class baseline)M1 mini A/B
Same archived compiler for both arms; matching base/fix runtime + stdlib wrapper archives; two sessions in reversed order, 7 repeats per session (14 samples/arm):
Heap used, heap total, GC cycle counts, copied/promoted object and byte counts, and freed bytes were identical in both arms for every probe.
Summary by CodeRabbit
New Features
Bug Fixes
Diagnostics