fix(gc): make per-object layout gate authoritative - #7894
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe runtime replaces the split layout-latch state with one atomic armed-thread count. Thread-local teardown now disarms the count. Code generation reads the 32-bit count with an aligned monotonic atomic load. Tests cover races, thread exit, and address reuse. ChangesPer-object layout latch
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant PerObjectLayoutHint
participant typed_shape_init
participant layout_forget_object
Worker->>PerObjectLayoutHint: create armed thread-local layout state
PerObjectLayoutHint->>PerObjectLayoutHint: increment atomic armed-thread count
typed_shape_init->>typed_shape_init: atomically load global count
typed_shape_init->>layout_forget_object: call cleanup when count is nonzero
Worker->>PerObjectLayoutHint: exit with live layout records
PerObjectLayoutHint->>PerObjectLayoutHint: Drop decrements the global count
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 11, 2026 23:05
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: 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 `@changelog.d/7894-layout-latch.md`:
- Around line 1-3: Add a concise “Validation:” line to the changelog fragment
after the behavior summary, recording the passed runtime tests with
RUST_TEST_THREADS=1, codegen checks, concurrent-layout checks, production
checks, formatting checks, and file-size checks.
In `@crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs`:
- Around line 164-170: Update the setup around previous_tenant and child so
child is allocated before retaining previous_tenant, or root previous_tenant and
reload its potentially updated pointer after child allocation. Ensure
layout_note_slot receives the current valid previous_tenant address while
preserving the existing slot and pointer-tag values.
🪄 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: 9858038d-b12d-479b-b66a-c7067a567e68
📒 Files selected for processing (9)
changelog.d/7894-layout-latch.mdcrates/perry-codegen/src/block.rscrates/perry-codegen/src/dialect/mod.rscrates/perry-codegen/src/inst.rscrates/perry-codegen/src/lower_call/typed_shape_bake_tests.rscrates/perry-codegen/src/lower_call/typed_shape_init.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-runtime/src/gc/layout_tables.rscrates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs
Comment on lines
+1
to
+3
| Fixed the process-wide per-object GC layout gate so concurrent thread arm/disarm | ||
| transitions cannot publish a false zero, and workers that exit with live layout | ||
| records no longer leave the allocation fast path permanently armed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add validation notes to the changelog fragment.
The fragment describes the behavior change but does not record validation. Add a concise Validation: line with the runtime, codegen, concurrent-layout, production, formatting, and file-size checks that passed. Record RUST_TEST_THREADS=1 for runtime tests.
Proposed change
Fixed the process-wide per-object GC layout gate so concurrent thread arm/disarm
transitions cannot publish a false zero, and workers that exit with live layout
records no longer leave the allocation fast path permanently armed.
+Validation: Runtime tests with RUST_TEST_THREADS=1, codegen tests, concurrent-layout
+tests, production checks, and formatting/file-size checks.As per coding guidelines, changelog fragments must include detailed history and validation notes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Fixed the process-wide per-object GC layout gate so concurrent thread arm/disarm | |
| transitions cannot publish a false zero, and workers that exit with live layout | |
| records no longer leave the allocation fast path permanently armed. | |
| Fixed the process-wide per-object GC layout gate so concurrent thread arm/disarm | |
| transitions cannot publish a false zero, and workers that exit with live layout | |
| records no longer leave the allocation fast path permanently armed. | |
| Validation: Runtime tests with RUST_TEST_THREADS=1, codegen tests, concurrent-layout | |
| tests, production checks, and formatting/file-size checks. |
🤖 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 `@changelog.d/7894-layout-latch.md` around lines 1 - 3, Add a concise
“Validation:” line to the changelog fragment after the behavior summary,
recording the passed runtime tests with RUST_TEST_THREADS=1, codegen checks,
concurrent-layout checks, production checks, formatting checks, and file-size
checks.
Source: Coding guidelines
Comment on lines
+164
to
+170
| let previous_tenant = crate::object::js_object_alloc(0, 2); | ||
| let child = crate::object::js_object_alloc(0, 0); | ||
| crate::gc::layout_note_slot( | ||
| previous_tenant as usize, | ||
| 1, | ||
| POINTER_TAG | (child as u64 & POINTER_MASK), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the layout_trace test module disables moving collection, and how sibling tests root allocations.
set -euo pipefail
fd -t f 'per_object_tables.rs' | xargs -r rg -n -C 5 'ManualGcScanGuard|ScopedRootScannerRegistryGuard|RuntimeHandleScope|force_full_scan|clear_marks'
# How other layout_trace tests hold an allocation across a later allocation.
fd -t d 'layout_trace' | xargs -r rg -n -C 4 'js_object_alloc'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/perry-runtime/src/gc/tests/layout_trace/per_object_tables.rs"
printf '%s\n' '--- target test and module setup ---'
sed -n '1,220p' "$file"
printf '%s\n' '--- allocation and GC-trigger definitions ---'
rg -n -C 8 'pub\s+unsafe\s+extern.*js_object_alloc|fn\s+js_object_alloc|js_object_alloc\(|CopyingNurseryTestGuard|GcTriggerThresholdTestGuard|ManualGcScanGuard|ScopedRoot' crates/perry-runtime/src crates/perry-runtime/tests 2>/dev/null | head -n 500
printf '%s\n' '--- explicit collection and relocation helpers in the target module ---'
rg -n -C 8 'collect|relocat|move|forget|layout_transfer|force|trigger|assert_flag_sound' "$file"Repository: PerryTS/perry
Length of output: 49245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant runtime files ---'
fd -t f . crates/perry-runtime | rg '/(object|gc|tests|mod)\.rs$|gc/tests'
printf '%s\n' '--- exact allocation definition and callers ---'
rg -n -C 12 'pub extern "C" fn js_object_alloc|pub fn js_object_alloc|unsafe fn js_object_alloc|fn js_object_alloc' crates/perry-runtime/src
printf '%s\n' '--- test guards and collection helpers ---'
rg -n -C 10 'struct (CopyingNurseryTestGuard|GcTriggerThresholdTestGuard)|impl (CopyingNurseryTestGuard|GcTriggerThresholdTestGuard)|ManualGcScanGuard|ScopedRootScannerRegistryGuard|RuntimeHandleScope|fn clear_marks|fn clear_mark_seeds|collect_garbage|gc_collect|force_full_scan' crates/perry-runtime/src/gc
printf '%s\n' '--- layout_trace module declaration ---'
rg -n -C 10 'mod layout_trace|per_object_tables' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate object files ---'
rg --files crates/perry-runtime/src | rg '(^|/)(object|allocation|arena|gc)([^/]*)\.rs$'
printf '%s\n' '--- exact js_object_alloc definitions ---'
rg -n -C 15 'fn js_object_alloc\b|js_object_alloc\s*=' crates/perry-runtime/src --glob '*.rs' | head -n 250
printf '%s\n' '--- guard definitions only ---'
rg -n -C 12 'struct CopyingNurseryTestGuard|struct GcTriggerThresholdTestGuard|impl CopyingNurseryTestGuard|impl GcTriggerThresholdTestGuard' crates/perry-runtime/src/gc --glob '*.rs' | head -n 300
printf '%s\n' '--- target test module ancestry ---'
rg -n -C 15 'per_object_tables' crates/perry-runtime/src/gc/tests --glob '*.rs' | head -n 200Repository: PerryTS/perry
Length of output: 30434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- object allocation path ---'
sed -n '1,180p' crates/perry-runtime/src/object/alloc.rs
rg -n -C 20 'js_object_alloc_with_parent|alloc_object|gc_alloc|arena_alloc|maybe.*gc|trigger' crates/perry-runtime/src/object/alloc.rs crates/perry-runtime/src/gc --glob '*.rs' | head -n 500
printf '%s\n' '--- test support guard behavior ---'
sed -n '349,470p' crates/perry-runtime/src/gc/tests/support.rs
rg -n -C 8 'fn clear_marks|fn clear_mark_seeds|pub.*gc_collect_minor|fn gc_collect_minor|automatic.*trigger' crates/perry-runtime/src/gc/tests/support.rs crates/perry-runtime/src/gc --glob '*.rs' | head -n 300
printf '%s\n' '--- guards used by layout_trace tests ---'
rg -n 'CopyingNurseryTestGuard|GcTriggerThresholdTestGuard|js_shadow_slot' crates/perry-runtime/src/gc/tests/layout_trace --glob '*.rs'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- arena_alloc_gc definition ---'
rg -n -C 25 'fn arena_alloc_gc\b|pub.*arena_alloc_gc\b|arena_alloc_gc\s*\(' crates/perry-runtime/src --glob '*.rs' | head -n 350
printf '%s\n' '--- automatic trigger conditions ---'
rg -n -C 15 'GC_NEXT_TRIGGER_BYTES|GC_NEXT_MALLOC_TRIGGER|maybe_trigger|trigger.*gc|gc_collect_minor|gc_collect_full' crates/perry-runtime/src/arena crates/perry-runtime/src/gc crates/perry-runtime/src --glob '*.rs' | head -n 500
printf '%s\n' '--- all setup calls in layout_trace files ---'
for f in crates/perry-runtime/src/gc/tests/layout_trace/*.rs; do
printf '%s\n' "--- $f ---"
rg -n 'CopyingNurseryTestGuard|GcTriggerThresholdTestGuard|suppress_automatic_triggers|js_shadow_frame_push|gc_collect' "$f" || true
doneRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- arena_alloc_gc slow/fast path ---'
sed -n '250,430p' crates/perry-runtime/src/arena/allocators.rs
printf '%s\n' '--- layout_trace test setup calls ---'
for f in crates/perry-runtime/src/gc/tests/layout_trace/*.rs; do
matches=$(rg -n 'CopyingNurseryTestGuard|GcTriggerThresholdTestGuard|suppress_automatic_triggers|js_shadow_frame_push|gc_collect' "$f" || true)
if [ -n "$matches" ]; then
printf '%s\n' "--- $f ---"
printf '%s\n' "$matches"
fi
done
printf '%s\n' '--- trigger check call sites in allocator ---'
rg -n -C 12 'gc_check_trigger|GC_NEXT_TRIGGER_BYTES|GC_NEXT_MALLOC_TRIGGER' crates/perry-runtime/src/arena/allocators.rs crates/perry-runtime/src/arena/block.rs crates/perry-runtime/src/gc --glob '*.rs' | head -n 400Repository: PerryTS/perry
Length of output: 44364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- arena allocation trigger boundary ---'
sed -n '748,825p' crates/perry-runtime/src/arena/block.rs
printf '%s\n' '--- root-scan policy for raw stack locals ---'
rg -n -C 18 'conservative_stack_scan_decision|mark_stack_roots|stack.*scan|C-stack|native_stack_fallback|ScanDisabled|SkipDisabled' crates/perry-runtime/src/gc --glob '*.rs' | head -n 500
printf '%s\n' '--- shadow-root API ---'
rg -n -C 12 'js_shadow_slot_set|RuntimeHandleScope|root_raw_mut_ptr|root_nanbox|shadow.*slot' crates/perry-runtime/src/gc crates/perry-runtime/src --glob '*.rs' | head -n 500
printf '%s\n' '--- free-list reuse and object death behavior ---'
rg -n -C 15 'free_list|layout_forget_object|layout_clear_for_ptr|arena_reset_empty_blocks|reclaim.*dead' crates/perry-runtime/src/arena crates/perry-runtime/src/gc --glob '*.rs' | head -n 500Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- conservative scan decision implementation ---'
rg -n -C 25 'pub\(super\)? fn conservative_stack_scan_decision|fn conservative_stack_scan_decision|ConservativeStackScanDecision::SkipDisabled|ManualGcScanGuard' crates/perry-runtime/src/gc/roots.rs crates/perry-runtime/src/gc --glob '*.rs' | head -n 450
printf '%s\n' '--- gc_check_trigger moving-collection path ---'
rg -n -C 30 'pub.*gc_check_trigger|fn gc_check_trigger|gc_collect_minor_copying|gc_scavenge_enabled|moving_loop_polls' crates/perry-runtime/src/gc --glob '*.rs' | head -n 500
printf '%s\n' '--- clear_marks and test initialization ---'
rg -n -C 15 'fn clear_marks|fn clear_mark_seeds|ensure_gc_initialized|gc_init\(\)' crates/perry-runtime/src/gc/tests crates/perry-runtime/src/gc --glob '*.rs' | head -n 350Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scan mode implementation ---'
sed -n '1,280p' crates/perry-runtime/src/gc/roots/scan_mode.rs 2>/dev/null || true
rg --files crates/perry-runtime/src/gc | rg 'scan_mode'
file=$(rg --files crates/perry-runtime/src/gc | rg 'scan_mode' | head -n 1)
if [ -n "${file:-}" ]; then sed -n '1,300p' "$file"; fi
printf '%s\n' '--- gc_check_trigger definition ---'
rg -n -C 20 'fn gc_check_trigger\b|pub extern "C" fn js_gc_check_trigger|gc_check_trigger\(' crates/perry-runtime/src/gc --glob '*.rs' | head -n 250
printf '%s\n' '--- direct trigger and scan guard call sites ---'
rg -n -C 25 'force_full_scan|ManualGcScanGuard::new|ManualGcScanGuard|GcTriggerKind::Direct|GcTriggerKind::Automatic' crates/perry-runtime/src/gc --glob '*.rs' | head -n 500Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files defining gc_check_trigger ---'
rg -l 'gc_check_trigger' crates/perry-runtime/src/gc --glob '*.rs' | sort
printf '%s\n' '--- gc_check_trigger body candidates ---'
for f in $(rg -l 'gc_check_trigger' crates/perry-runtime/src/gc --glob '*.rs' | sort); do
line=$(rg -n '(^|[[:space:]])(pub\(crate\) )?(unsafe )?fn gc_check_trigger|gc_check_trigger\s*=' "$f" | cut -d: -f1 | head -n1 || true)
if [ -n "$line" ]; then
start=$((line-20)); [ "$start" -lt 1 ] && start=1
end=$((line+180))
printf '%s\n' "--- $f:$line ---"
sed -n "${start},${end}p" "$f"
fi
doneRepository: PerryTS/perry
Length of output: 13578
Avoid the allocation while holding previous_tenant.
js_object_alloc(0, 0) can enter the allocation-point GC path when the arena block is full. Allocate child first, or root previous_tenant and reload its updated pointer before layout_note_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-runtime/src/gc/tests/layout_trace/per_object_tables.rs` around
lines 164 - 170, Update the setup around previous_tenant and child so child is
allocated before retaining previous_tenant, or root previous_tenant and reload
its potentially updated pointer after child allocation. Ensure layout_note_slot
receives the current valid previous_tenant address while preserving the existing
slot and pointer-tag values.
Source: Coding guidelines
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
AtomicU32armed-thread countPerObjectLayoutHintTLS destructor when a worker exits armedReproduction
The synchronization-hook regression forces the reported ordering: A decrements the last arm, B re-arms in the former publication window, then A resumes. Before the fix it ended with
count=1, gate=0and failed. The isolated worker test also failed before the fix with a leaked count of1afterjoin().Performance
Quiet M1 mini, same base commit and runtime/compiler pairs, 5 warmups plus 25 alternating measured pairs of the 50,000,000-allocation pointer-free typed-shape probe:
A seq-cst generated load was measured and rejected before this final version: 286.584 ms to 299.293 ms (+4.43%). The final monotonic load remains atomic and authoritative but avoids that ARM acquire-load tax; the counter transitions remain seq-cst.
Validation
cargo test -p perry-runtime: 2,145 passed, 0 failed, 4 ignored; doc tests cleancargo test -p perry-codegen --lib: 897 passedcargo check -p perry-runtime -p perry-codegenFixes #7873
Summary by CodeRabbit
Bug Fixes
Tests