fix(gc): restore safe old-page relocation - #7913
Merged
Merged
Conversation
📝 WalkthroughWalkthroughOld-generation defragmentation is enabled by default and evacuates complete source blocks safely. Code generation roots movable class-key globals. Runtime caches rewrite forwarded addresses. GC inventory checks now reject unresolved movable-address gaps. ChangesOld-generation defragmentation contract
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MinorGC
participant OldDefrag
participant RuntimeRoots
participant Cache
MinorGC->>OldDefrag: select and evacuate movable old-page blocks
OldDefrag->>RuntimeRoots: provide forwarded addresses
RuntimeRoots->>Cache: rewrite cached object addresses
Cache-->>MinorGC: retain valid relocated references
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
marked this pull request as ready for review
August 12, 2026 05:28
Contributor
Author
|
Audited and merging. The validation is the strongest in this campaign for the risk class: the historical #6206 workload rebuilt on main with relocation force-enabled reproduces the original TypeError, and the fix passes it 6/6 under evacuation verification with byte-identical stdout; the 25-program corpus is byte-identical with the perf ledger honest in both directions (the 8.29% regression found and engineered away via deferred selection; the residual +3.7–4.5% on four class-allocation kernels named as the price of the REQUIRED class-key mutable root). The exemption edits all tighten: One policy clock starts at this merge, per CLAUDE.md's GC knob kill-policy: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/gc/tests/oldgen.rs (1)
491-518: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTwo defrag tests call
evacuate_selected_old_pages_collectingwithout suppressing GC triggers. In production this helper runs insidegc_collect_minor_with_trigger_inner, which holdsGC_FLAG_IN_ALLOCfor the whole cycle so a recursivegc_check_triggerbails out. Both tests call the helper directly, so that protection is absent and the per-objectarena_alloc_gc_old_excluding_pagescall can trigger a collection between allocating a destination and installing the forwarding address. The sibling tests at lines 558 and 947 already hold the guard.
crates/perry-runtime/src/gc/tests/oldgen.rs#L491-L518: addlet _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers();aftercopying_nursery_isolation_lock()intest_old_page_defrag_moves_every_source_block_occupant_during_a_minor.crates/perry-runtime/src/gc/tests/oldgen.rs#L639-L665: add the same guard aftercopying_nursery_isolation_lock()intest_old_page_defrag_skips_pinned_old_objects.🤖 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/oldgen.rs` around lines 491 - 518, Both direct defrag tests must suppress automatic GC triggers before calling evacuate_selected_old_pages_collecting. In crates/perry-runtime/src/gc/tests/oldgen.rs:491-518, add a GcTriggerThresholdTestGuard::suppress_automatic_triggers() guard after copying_nursery_isolation_lock() in test_old_page_defrag_moves_every_source_block_occupant_during_a_minor; make the same change at crates/perry-runtime/src/gc/tests/oldgen.rs:639-665 in test_old_page_defrag_skips_pinned_old_objects.Source: Learnings
🧹 Nitpick comments (4)
crates/perry-runtime/src/gc/oldgen.rs (1)
1883-1902: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment and the
flags != 0gate disagree about dead occupants.The comment states that dead old objects "remain indexed until a full trace proves them dead, so conservatively copying them here preserves the same minor-GC retention contract." The gate does the opposite:
flags != 0makes any zero-flag occupant failsource_block_is_movable, so the whole block is declined and nothing is copied.Declining is the safe outcome, so this is not a correctness defect. The comment should describe it, because a later reader could relax
flags != 0on the belief that copying was already the intent.📝 Suggested comment wording
- // remain indexed until a full trace proves them dead, so conservatively - // copying them here preserves the same minor-GC retention contract. + // remain indexed until a full trace proves them dead; such an occupant has + // zero flags and fails the movability gate below, so the block is declined + // rather than partially moved. That preserves the minor-GC retention + // contract without copying objects a minor cannot classify.🤖 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/oldgen.rs` around lines 1883 - 1902, Update the comment above source block evacuation to match the existing `source_block_is_movable` behavior: zero-flag dead occupants cause the block to be declined rather than copied. Clarify that retaining the block is the conservative outcome and preserve the `flags != 0` gate unchanged.crates/perry-runtime/src/gc/tests/oldgen.rs (2)
1016-1035: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate marking loop.
Lines 1021-1025 set
GC_FLAG_MARKEDon every entry oflive_headers. Lines 1026-1035 iterate the same vector and set the same flag again at lines 1032-1034. The second write is redundant.Folding the page-membership assertion into the first loop makes the intent clear.
♻️ Proposed cleanup
- for &header in &live_headers { - unsafe { - (*header).gc_flags |= GC_FLAG_MARKED; - } - } for &header in &live_headers { let total = unsafe { (*header).size as usize }; assert!( old_object_pages_all_selected(header, total, &selection.pages), "every live fixture object must lie wholly on selected fragmented pages" ); unsafe { (*header).gc_flags |= GC_FLAG_MARKED; } }🤖 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/oldgen.rs` around lines 1016 - 1035, Remove the first standalone marking loop over live_headers and fold its GC_FLAG_MARKED assignment into the existing loop containing old_object_pages_all_selected. Keep the page-membership assertion and ensure each live header is marked exactly once after validation.
960-962: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpress the filler size in terms of
BLOCK_SIZE.This allocation requests a total of two
BLOCK_SIZEunits. Replace2 * 1024 * 1024with2 * crate::arena::BLOCK_SIZEso the fixture tracks allocator block sizing changes.🤖 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/oldgen.rs` around lines 960 - 962, Update the filler allocation in the old-generation GC test around old_pages_begin_gc_cycle to calculate its size as 2 * crate::arena::BLOCK_SIZE minus GC_HEADER_SIZE instead of using the hardcoded 2 MiB value.crates/perry-codegen/src/collectors/proven_this_routing_tests.rs (1)
816-831: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider scoping the ordering check to one function body.
ir.find(store)andir.find(line)search the whole module text.emitproduces severalpshapeclones, and SSA names restart per function, so identical instruction text can appear in more than one clone. In that case the two offsets can come from different functions and the ordering assertion no longer proves store-before-bind inside the function under test.Using line indices from a single
ir.lines().enumerate()pass, or slicing the IR to the clone'sdefineblock first, makes the check exact.♻️ Sketch: compare line indices from one pass
- let store_pos = ir - .find(store) - .expect("the hoisted class-keys store should be in the function"); - let bind_pos = ir - .lines() - .find(|line| line.contains("call void `@js_shadow_slot_bind`") && line.contains(slot)) - .and_then(|line| ir.find(line)) + let store_pos = ir + .lines() + .position(|line| line == store) + .expect("the hoisted class-keys store should be in the function"); + let bind_pos = ir + .lines() + .position(|line| { + line.contains("call void `@js_shadow_slot_bind`") && line.contains(slot) + }) .unwrap_or_else(|| { panic!( "the cached class-keys pointer is not a mutable shadow root; old-page moves would leave this copy stale:\n{ir}" ) });🤖 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/collectors/proven_this_routing_tests.rs` around lines 816 - 831, Scope the ordering validation around the relevant function body instead of searching the entire module. Update the checks using ir.find(store) and the js_shadow_slot_bind lookup to derive store and bind positions from one ir.lines().enumerate() pass or from the selected pshape clone’s define block, then compare those same-function line indices so identical SSA instructions in other clones cannot satisfy the assertion.
🤖 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/scalar_slot_root.rs`:
- Around line 118-120: Update the fallback in the scalar-slot root handling
around reserve_shadow_slot so a None result is not accepted when native roots
are inactive and Auto scan mode would omit conservative scanning. Reject this
unsupported configuration or route the cache slot through an existing
precise-root mechanism, ensuring moving collections cannot leave the entry slot
pointing to a relocated class-key object.
In `@crates/perry-runtime/src/gc/oldgen_defrag.rs`:
- Around line 109-111: Update old_page_defrag_enabled_from_value to trim
surrounding whitespace and compare the normalized value case-insensitively
against "0", "off", and "false", preserving default-on behavior for other
values. Add coverage for "OFF", "False", and " 0 ".
In `@crates/perry-runtime/src/gc/tests/runtime_roots.rs`:
- Line 11: Set RUST_TEST_THREADS=1 in the test command environments used by
scripts/run_memory_stability_tests.sh and scripts/native_abi_evidence_packet.sh,
matching the perry-runtime CI commands. Ensure every perry-runtime test
invocation in both scripts inherits this setting.
In `@crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs`:
- Around line 8-15: Prevent automatic collection across each forwarding
fixture’s entire from-allocation-to-set_forwarding_address sequence, including
destination allocation, so the raw from pointer remains valid until forwarding
is installed. Apply this in forwarded_string at
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L8-L15,
the fixture at `#L40-L46`, and the fixture at `#L61-L69`; use the existing
automatic-trigger suppression mechanism and preserve each fixture’s current
forwarding behavior.
- Around line 20-36: Restore thread-local cache state with panic-safe test-state
guards in all three fixtures: at
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs lines
20-36, ensure the guard clears both parse roots and PARSE_KEY_RING; at lines
47-58, clear PERF_ENTRY_KEYS_ARRAY after the assertion; and at lines 70-83,
clear DIAG_CHANNEL_BY_KEY after the assertion. Use guards so each cleanup runs
even when an assertion panics, and do not rely on arena-reset teardown to clear
thread-local cache scanners.
---
Outside diff comments:
In `@crates/perry-runtime/src/gc/tests/oldgen.rs`:
- Around line 491-518: Both direct defrag tests must suppress automatic GC
triggers before calling evacuate_selected_old_pages_collecting. In
crates/perry-runtime/src/gc/tests/oldgen.rs:491-518, add a
GcTriggerThresholdTestGuard::suppress_automatic_triggers() guard after
copying_nursery_isolation_lock() in
test_old_page_defrag_moves_every_source_block_occupant_during_a_minor; make the
same change at crates/perry-runtime/src/gc/tests/oldgen.rs:639-665 in
test_old_page_defrag_skips_pinned_old_objects.
---
Nitpick comments:
In `@crates/perry-codegen/src/collectors/proven_this_routing_tests.rs`:
- Around line 816-831: Scope the ordering validation around the relevant
function body instead of searching the entire module. Update the checks using
ir.find(store) and the js_shadow_slot_bind lookup to derive store and bind
positions from one ir.lines().enumerate() pass or from the selected pshape
clone’s define block, then compare those same-function line indices so identical
SSA instructions in other clones cannot satisfy the assertion.
In `@crates/perry-runtime/src/gc/oldgen.rs`:
- Around line 1883-1902: Update the comment above source block evacuation to
match the existing `source_block_is_movable` behavior: zero-flag dead occupants
cause the block to be declined rather than copied. Clarify that retaining the
block is the conservative outcome and preserve the `flags != 0` gate unchanged.
In `@crates/perry-runtime/src/gc/tests/oldgen.rs`:
- Around line 1016-1035: Remove the first standalone marking loop over
live_headers and fold its GC_FLAG_MARKED assignment into the existing loop
containing old_object_pages_all_selected. Keep the page-membership assertion and
ensure each live header is marked exactly once after validation.
- Around line 960-962: Update the filler allocation in the old-generation GC
test around old_pages_begin_gc_cycle to calculate its size as 2 *
crate::arena::BLOCK_SIZE minus GC_HEADER_SIZE instead of using the hardcoded 2
MiB value.
🪄 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: 932d6b1b-ce38-4d77-afad-1bd12bf7d3e2
📒 Files selected for processing (26)
changelog.d/7913-old-defrag-contract.mdcrates/perry-codegen/src/collectors/proven_this_routing_tests.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/scalar_slot_root.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/new_alloc.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rscrates/perry-codegen/src/lower_call/scalar_method.rscrates/perry-codegen/src/testing/temp_slots.rscrates/perry-codegen/tests/temp_root_operand_temporaries.rscrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/oldgen_defrag.rscrates/perry-runtime/src/gc/tests/copying/survival_and_malloc.rscrates/perry-runtime/src/gc/tests/oldgen.rscrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rscrates/perry-runtime/src/json/mod.rscrates/perry-runtime/src/node_submodules/diagnostics.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/perf_hooks.rsscripts/gc_root_dominance_check.pyscripts/gc_runtime_root_holders.jsonscripts/gc_runtime_root_holders.py
Comment on lines
+118
to
+120
| let Some(idx) = ctx.func.reserve_shadow_slot() else { | ||
| return slot; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the root-reservation implementation and its callers.
ast-grep outline crates/perry-codegen/src --items all --match 'reserve_shadow_slot|entry_init_load_global|entry_setup_call_void'
# Inspect the no-slot configuration and related collector-mode gates.
rg -n -C 6 '\breserve_shadow_slot\s*\(|shadow[-_ ]slot|shadow[-_ ]stack|PERRY_GC_OLD_DEFRAG|old[-_ ]defrag|relocat' crates scriptsRepository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scalar_slot_root.rs ---'
cat -n crates/perry-codegen/src/expr/scalar_slot_root.rs | sed -n '1,190p'
printf '%s\n' '--- reserve_shadow_slot implementation ---'
cat -n crates/perry-codegen/src/function.rs | sed -n '350,430p'
printf '%s\n' '--- native_stack_roots_enabled definitions and call sites ---'
rg -n -C 8 'fn native_stack_roots_enabled|native_stack_roots_enabled\(' crates/perry-codegen/src crates/perry-codegen/tests
printf '%s\n' '--- scalar helper callers ---'
rg -n -C 8 'entry_init_load_rooted_global|entry_init_load_global|scalar_slot_root|class.?keys|CLASS_KEYS' crates/perry-codegen/src/expr crates/perry-codegen/srcRepository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shadow-frame enablement and configuration gates ---'
rg -n -C 12 'enable_(post_init_)?shadow_frame|shadow_frame_requested|shadow_stack_enabled\(|precise_root_analysis_enabled\(|PERRY_SHADOW_STACK|PERRY_CONSERVATIVE_STACK_SCAN|PERRY_RS4GC' crates/perry-codegen/src crates/perry-runtime/src
printf '%s\n' '--- collector mode and old-page relocation gates ---'
rg -n -C 10 'old.?page|defrag|relocat|conservative.*scan|CONSERVATIVE_STACK|precise.root|moving.*GC|minor.*mov|evacuat' crates/perry-runtime/src crates/perry-codegen/src | head -n 1200
printf '%s\n' '--- class-key cache lowering and uses ---'
rg -n -C 12 'entry_init_load_rooted_global|class_keys_slots|get_class_keys|class_keys_globals' crates/perry-codegen/src/expr crates/perry-codegen/src/codegenRepository: PerryTS/perry
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- codegen helper mode definitions ---'
cat -n crates/perry-codegen/src/codegen/helpers.rs | sed -n '1,275p'
printf '%s\n' '--- exact class-key cache call sites ---'
rg -n -C 20 'entry_init_load_rooted_global|class_keys_slots' crates/perry-codegen/src/expr --glob '*.rs'
printf '%s\n' '--- runtime conservative-scan and defrag configuration ---'
rg -n -C 8 'PERRY_CONSERVATIVE_STACK_SCAN|CONSERVATIVE_STACK_SCAN|old_page|old-page|defrag|DEFRAG|evacuating|moving.*old|old.*moving' crates/perry-runtime/src --glob '*.rs' | head -n 1000Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all configuration references ---'
rg -n -C 10 'PERRY_CONSERVATIVE_STACK_SCAN|conservative_stack_scan|conservative.*scan|stack_scan_enabled|old_page.*evac|evacuation_eligible|defrag_enabled|old.*defrag' . --glob '!target/**' --glob '!node_modules/**' | head -n 1600
printf '%s\n' '--- GC root scan entry points ---'
rg -n -C 12 'scan.*stack|stack.*scan|native.*root|stack_map|gcmap|GC_MAP|conservative' crates/perry-runtime/src/gc crates/perry-runtime/src --glob '*.rs' | head -n 1800
printf '%s\n' '--- relocation collection entry points ---'
rg -n -C 12 'old_pages|evacuate_old|evacuate.*old|defrag|evacuation|collect.*old|minor.*collect|copying' crates/perry-runtime/src/gc crates/perry-runtime/src/arena --glob '*.rs' | head -n 1800Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- conservative scan decision implementation ---'
rg -n 'enum ConservativeStackScanDecision|fn conservative_stack_scan_decision|PERRY_CONSERVATIVE_STACK_SCAN|mark_stack_roots_for_decision|pin_currently_marked_as_conservative' crates/perry-runtime/src/gc --glob '*.rs'
rg -n -C 18 'enum ConservativeStackScanDecision|fn conservative_stack_scan_decision|PERRY_CONSERVATIVE_STACK_SCAN|fn mark_stack_roots_for_decision|fn pin_currently_marked_as_conservative' crates/perry-runtime/src/gc --glob '*.rs'
printf '%s\n' '--- native stack scan and conservative pin semantics ---'
rg -n -C 18 'ConservativePin|conservative.*pin|pin.*conservative|stack_roots|scan_stack' crates/perry-runtime/src/gc --glob '*.rs' | head -n 1800
printf '%s\n' '--- exact old-page evacuation eligibility decisions ---'
rg -n -C 18 'evacuation_eligible|consider_evacuation|old_page.*evac|evacuate.*page|select.*page' crates/perry-runtime/src/gc crates/perry-runtime/src/arena --glob '*.rs' | head -n 1800Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files defining conservative scan configuration ---'
rg -l 'ConservativeStackScanDecision|PERRY_CONSERVATIVE_STACK_SCAN|conservative_stack_scan_decision' crates/perry-runtime/src/gc
printf '%s\n' '--- files defining evacuation policy ---'
rg -l 'evacuation_eligible|conservative_pinned_bytes|consider_evacuation' crates/perry-runtime/src/gc crates/perry-runtime/src/arena
printf '%s\n' '--- concise configuration excerpts ---'
for f in $(rg -l 'ConservativeStackScanDecision|PERRY_CONSERVATIVE_STACK_SCAN|conservative_stack_scan_decision' crates/perry-runtime/src/gc); do
echo "### $f"
rg -n -C 20 'enum ConservativeStackScanDecision|conservative_stack_scan_decision|PERRY_CONSERVATIVE_STACK_SCAN|Scan|Skip' "$f" | head -n 220
done
printf '%s\n' '--- concise evacuation excerpts ---'
for f in $(rg -l 'evacuation_eligible|conservative_pinned_bytes|consider_evacuation' crates/perry-runtime/src/gc crates/perry-runtime/src/arena); do
echo "### $f"
rg -n -C 16 'evacuation_eligible|conservative_pinned_bytes|consider_evacuation|old_page_selected|pinned_bytes' "$f" | head -n 260
doneRepository: PerryTS/perry
Length of output: 19762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scan_mode.rs ---'
cat -n crates/perry-runtime/src/gc/roots/scan_mode.rs | sed -n '1,260p'
printf '%s\n' '--- copied-minor conservative fallback and old-page policy ---'
cat -n crates/perry-runtime/src/gc/copying.rs | sed -n '1080,1145p'
rg -n -C 25 'consider_evacuation|evacuation_policy|conservative_stack_scan_decision|conservative_pinned_bytes' crates/perry-runtime/src/gc/cycle.rs crates/perry-runtime/src/gc/oldgen.rs crates/perry-runtime/src/gc/policy.rs | head -n 1200
printf '%s\n' '--- configuration documentation and validation ---'
rg -n -C 12 'PERRY_SHADOW_STACK|PERRY_CONSERVATIVE_STACK_SCAN|PERRY_RS4GC|conservative stack scan' README.md docs crates scripts Cargo.toml .github 2>/dev/null | head -n 1600Repository: PerryTS/perry
Length of output: 50370
Provide a precise root when reserve_shadow_slot() returns None.
With PERRY_SHADOW_STACK=0 and native roots inactive, reserve_shadow_slot() returns None. The default Auto scan mode skips the conservative stack scan, so moving collections can relocate the class-key object while the entry slot retains its old address.
Reject this configuration or use a supported precise-root mechanism for the cache slot.
🤖 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/scalar_slot_root.rs` around lines 118 - 120,
Update the fallback in the scalar-slot root handling around reserve_shadow_slot
so a None result is not accepted when native roots are inactive and Auto scan
mode would omit conservative scanning. Reject this unsupported configuration or
route the cache slot through an existing precise-root mechanism, ensuring moving
collections cannot leave the entry slot pointing to a relocated class-key
object.
Source: Coding guidelines
Comment on lines
+109
to
+111
| fn old_page_defrag_enabled_from_value(value: Option<&str>) -> bool { | ||
| !matches!(value, Some("0") | Some("off") | Some("false")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare env-knob parsing conventions across the GC runtime.
set -euo pipefail
echo "=== PERRY_GC_* / PERRY_GEN_GC* env reads ==="
rg -nP -C4 'var(_os)?\(\s*"PERRY_' crates/perry-runtime/src --type=rust
echo
echo "=== Existing boolean-knob normalization helpers ==="
rg -nP -C4 '(to_ascii_lowercase|eq_ignore_ascii_case|\.trim\(\))' crates/perry-runtime/src --type=rust | rg -n -C4 'PERRY|env|knob|enabled'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== oldgen defrag parser and tests ==="
sed -n '80,155p' crates/perry-runtime/src/gc/oldgen_defrag.rs
echo
echo "=== GC boolean parser helpers ==="
rg -n -C5 'fn .*from_value|resolve_.*knob|to_ascii_lowercase|eq_ignore_ascii_case|trim\(\)' \
crates/perry-runtime/src/gc --type rust
echo
echo "=== PERRY_GC_OLD_DEFRAG documentation and references ==="
rg -n -C3 'PERRY_GC_OLD_DEFRAG|old.?gen.*defrag|old.?page.*defrag|`#6206`' \
--glob '!target/**' .Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("crates/perry-runtime/src/gc/oldgen_defrag.rs").read_text()
assert 'Some("0") | Some("off") | Some("false")' in source
def current(value):
return value not in {"0", "off", "false"}
def normalized(value):
return value is not None and value.strip().lower() not in {"0", "off", "false"} \
if value is not None else True
cases = [None, "0", "off", "false", "OFF", "False", " 0 ", " unexpected "]
for value in cases:
print(f"{value!r}: current={current(value) if value is not None else True}, normalized={normalized(value)}")
PY
echo
echo "=== Focused rollback documentation ==="
rg -n -C2 'PERRY_GC_OLD_DEFRAG|`#6206`|`#7876`' \
docs/src/internals/garbage-collector.md \
changelog.d/7913-old-defrag-contract.md \
crates/perry-runtime/src/gc/oldgen_defrag.rs \
CLAUDE.md || trueRepository: PerryTS/perry
Length of output: 2510
Normalize PERRY_GC_OLD_DEFRAG before parsing
PERRY_GC_OLD_DEFRAG=0 is the rollback switch. The matching default-on parse_promote_in_place parser trims whitespace and ignores case. Normalize this value before checking "0", "off", and "false", and test "OFF", "False", and " 0 ".
🤖 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/oldgen_defrag.rs` around lines 109 - 111, Update
old_page_defrag_enabled_from_value to trim surrounding whitespace and compare
the normalized value case-insensitively against "0", "off", and "false",
preserving default-on behavior for other values. Add coverage for "OFF",
"False", and " 0 ".
| mod interned_string_caches; | ||
| mod iter_result_keys; | ||
| mod json_shape_template; | ||
| mod old_defrag_contract; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n --hidden \
-g '*.yml' -g '*.yaml' -g '*.sh' -g 'Makefile' -g '*.toml' \
'RUST_TEST_THREADS=1|cargo test.*perry-runtime|test -p perry-runtime' .Repository: PerryTS/perry
Length of output: 2915
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- runtime_roots.rs ---'
cat -n crates/perry-runtime/src/gc/tests/runtime_roots.rs | sed -n '1,180p'
printf '%s\n' '--- workflow runtime test contexts ---'
sed -n '750,855p' .github/workflows/test.yml
sed -n '1095,1135p' .github/workflows/test.yml
printf '%s\n' '--- all perry-runtime test invocations in tracked automation ---'
rg -n -C 3 --hidden \
-g '*.yml' -g '*.yaml' -g '*.sh' -g 'Makefile' -g '*.toml' \
'cargo test( --[^\n]*)? -p perry-runtime|cargo test -p perry-runtime' .Repository: PerryTS/perry
Length of output: 25991
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- memory-stability script environment and helpers ---'
sed -n '1,120p' scripts/run_memory_stability_tests.sh
sed -n '1380,1500p' scripts/run_memory_stability_tests.sh
sed -n '1735,1845p' scripts/run_memory_stability_tests.sh
printf '%s\n' '--- native ABI script environment and runner ---'
sed -n '1,100p' scripts/native_abi_evidence_packet.sh
sed -n '360,420p' scripts/native_abi_evidence_packet.sh
rg -n -C 2 'RUST_TEST_THREADS|test-threads|run_canary|run_logged' \
scripts/run_memory_stability_tests.sh scripts/native_abi_evidence_packet.shRepository: PerryTS/perry
Length of output: 28104
Set RUST_TEST_THREADS=1 for all perry-runtime test commands.
The CI commands set this variable, but scripts/run_memory_stability_tests.sh and scripts/native_abi_evidence_packet.sh do not.
🤖 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/runtime_roots.rs` at line 11, Set
RUST_TEST_THREADS=1 in the test command environments used by
scripts/run_memory_stability_tests.sh and scripts/native_abi_evidence_packet.sh,
matching the perry-runtime CI commands. Ensure every perry-runtime test
invocation in both scripts inherits this setting.
Source: Coding guidelines
Comment on lines
+8
to
+15
| fn forwarded_string() -> (usize, usize, ValidPointerSet) { | ||
| let from = crate::string::js_string_from_bytes_longlived(b"id".as_ptr(), 2) as usize; | ||
| let valid_ptrs = build_valid_pointer_set(); | ||
| let to = crate::string::js_string_from_bytes_longlived(b"id".as_ptr(), 2) as usize; | ||
| unsafe { | ||
| set_forwarding_address(header_from_user_ptr(from as *const u8), to as *mut u8); | ||
| } | ||
| (from, to, valid_ptrs) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent collection while each forwarding fixture uses from.
Each fixture keeps from only in a raw local while allocating to. If that allocation collects, from can move before header_from_user_ptr(from) writes the forwarding address.
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L8-L15: Hold automatic-trigger suppression fromfromallocation throughset_forwarding_address.crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L40-L46: Hold automatic-trigger suppression fromfromallocation throughset_forwarding_address.crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L61-L69: Hold automatic-trigger suppression fromfromallocation throughset_forwarding_address.
Based on learnings, destination allocation must not collect before forwarding installation because the local source pointer then becomes stale.
📍 Affects 1 file
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L8-L15(this comment)crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L40-L46crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L61-L69
🤖 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/runtime_roots/old_defrag_contract.rs`
around lines 8 - 15, Prevent automatic collection across each forwarding
fixture’s entire from-allocation-to-set_forwarding_address sequence, including
destination allocation, so the raw from pointer remains valid until forwarding
is installed. Apply this in forwarded_string at
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L8-L15,
the fixture at `#L40-L46`, and the fixture at `#L61-L69`; use the existing
automatic-trigger suppression mechanism and preserve each fixture’s current
forwarding behavior.
Sources: Coding guidelines, Learnings
Comment on lines
+20
to
+36
| crate::json::test_clear_parse_roots(); | ||
| let (from, to, valid_ptrs) = forwarded_string(); | ||
| crate::json::test_seed_parse_roots( | ||
| f64::from_bits(crate::value::TAG_UNDEFINED), | ||
| from as *const _, | ||
| ); | ||
| crate::json::test_seed_parse_key_ring(from as *const _); | ||
|
|
||
| crate::json::scan_parse_roots_mut(&mut RuntimeRootVisitor::for_rewrite(&valid_ptrs)); | ||
|
|
||
| assert_eq!(crate::json::test_parse_roots_snapshot().1, to); | ||
| assert_eq!( | ||
| crate::json::test_parse_key_ring_snapshot(), | ||
| vec![to], | ||
| "the hot-key mirror must not retain the old address after its owning cache rewrites" | ||
| ); | ||
| crate::json::test_clear_parse_roots(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore thread-local cache state after each fixture.
These tests install arena addresses into thread-local runtime caches. Later tests can scan or use these stale addresses after an arena reset.
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L20-L36: Ensure cleanup also clearsPARSE_KEY_RING.crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L47-L58: ClearPERF_ENTRY_KEYS_ARRAYafter the assertion.crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L70-L83: ClearDIAG_CHANNEL_BY_KEYafter the assertion.
Use a test-state guard so cleanup also runs when an assertion panics. Based on learnings, do not assume arena-reset teardown clears thread-local cache scanners.
📍 Affects 1 file
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L20-L36(this comment)crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L47-L58crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs#L70-L83
🤖 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/runtime_roots/old_defrag_contract.rs`
around lines 20 - 36, Restore thread-local cache state with panic-safe
test-state guards in all three fixtures: at
crates/perry-runtime/src/gc/tests/runtime_roots/old_defrag_contract.rs lines
20-36, ensure the guard clears both parse roots and PARSE_KEY_RING; at lines
47-58, clear PERF_ENTRY_KEYS_ARRAY after the assertion; and at lines 70-83,
clear DIAG_CHANNEL_BY_KEY after the assertion. Use guards so each cleanup runs
even when an assertion panics, and do not rely on arena-reset teardown to clear
thread-local cache scanners.
Source: Learnings
This was referenced Aug 12, 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.
Summary
Restores the rewrite contract required for old-generation page defragmentation and enables it by default, with
PERRY_GC_OLD_DEFRAG=0as an explicit rollback switch. The core collector fix evacuates every indexed occupant of a selected source block atomically; a minor trace cannot treat an unmarked old-generation neighbor as dead.Changes
@perry_class_keys_*local copies precise function-lifetime mutable roots, and remove the now-invalid class-key immovability exemption from the static gateopen_gaporunverifiedmovable-address contractsRelated issue
Fixes #7876
Test plan
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-staticon the quiet M1 minicargo test -p perry-runtime -- --test-threads=1— 2,159 passed, 4 ignoredcargo test -p perry-codegen— 903 unit tests plus integration suites passedcargo fmt --all -- --check,git diff --check, andbash scripts/check_file_size.shTypeError: Cannot convert object to primitive value; rebuilt fix passed 6/6 runs with evacuation verification enabled, each stdout byte-identical to the clean main controlretain_wideregression was removed by deferring selection; final result +0.14% best / -0.09% median. Four tight shape/class-allocation kernels are +3.70% to +4.45% from the required class-key mutable root;iso_missis +1.44%,interpis -2.85%, and all other programs are within +/-1.3% best-time delta.The dependency-scale witness compiled the same 81 modules / 13.5 MB IR in both main and fix arms on the mini, but both hit the same existing missing-zod-export link failure before runtime. This branch therefore has no dependency-scale runtime result; the PR gate with a full
npm cienvironment remains authoritative.Screenshots / output
Not applicable.
Checklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
New Features
Bug Fixes
Tests