perf(gc): stop the runtime write barrier re-deriving its parent address (#7187 increment 1) - #7193
Conversation
`classify_heap_generation` is 19.03% of `benchmarks/app-patterns/kernels/batch.ts` (657M instructions, #7170) — the largest symbol in that program, called from the write barrier with zero collections running. Four page-map classifications happen per barriered element store; this removes the one that answers a question the caller had already answered, and makes the rest cheaper. 1. `runtime_write_barrier_slot` receives `parent_addr: usize` and handed it to `js_write_barrier_slot` as a bare `u64`, so `decode_heap_addr` paid a full `classify_heap_generation` to recover it immediately before `barrier_parent_needs_remembering` classified the same address again. The three runtime entry points now share `write_barrier_slot_decoded`. 2. That round-trip was also FILTERING. `malloc_gc_parent_addr` dereferences `parent - GC_HEADER_SIZE` behind a bare `< GC_HEADER_SIZE + 0x1000` floor, safe only because callers happened to filter first — including by accident: NaN-boxing a high-bit address corrupts the tag, so the decode rejected it, and `closure/dynamic_props.rs` (which parks props under non-address owner keys) depended on that. Both filters are now one explicit predicate, `barrier_parent_addr_is_dereferenceable`, applied in the entry point AND inside `malloc_gc_parent_addr` itself. 3. `PAGE_GENERATIONS` carried an identity hasher. hashbrown takes its SIMD control byte from `hash >> 57`, and the keys are `addr >> 20` — so the control byte was zero for every entry and every group probe matched every occupied slot. Measured with the old hasher reinstated: 1 distinct control byte across 64 buckets. Now `fast_hash::PtrHasher`. Refs #7187, #7170, #5094 Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
📝 WalkthroughWalkthroughThe PR adds decoded-parent write-barrier paths, shared parent-address validation, and regression tests. It also replaces the page-generation identity hasher with ChangesArena generation-map hashing
Decoded-parent write barrier
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RuntimeCaller
participant barrier_child_prologue
participant write_barrier_decoded_parent
participant RememberedSet
RuntimeCaller->>barrier_child_prologue: decode child and mark incrementally
barrier_child_prologue->>write_barrier_decoded_parent: decoded parent and child
write_barrier_decoded_parent->>write_barrier_decoded_parent: validate and classify parent
write_barrier_decoded_parent->>RememberedSet: record old-to-young edge
🚥 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 |
Verification logEverything below was run in one worktree, one target dir,
|
| FAIL count | arms | |
|---|---|---|
origin/main |
10 | evac_minor force_evac force_verify rep_i32_off rep_int_valued_off rep_ptr_numarray_off rep_ptr_shape_off rep_spec_abi_off rep_str_off rep_str_static_off |
| this branch | 10 | identical set |
Same failure signature on both (output-mismatch cycles=2 evacuated=0 scavenged=6514). This is a requires=move arm hitting evacuated=0 scavenged=…, i.e. the copying-minor path — untriaged today, and it belongs to whoever owns #7016 rather than to this PR. It is not in gc_repsel_triage.txt; I have deliberately not added an entry, since triaging a red cell to make a table green is what that file's header forbids.
Arm liveness — this is a write-barrier change, so it matters: every requires=move arm bit on 28/29 corpus files (copy-minor 28/29, moved-objects 28/29). The barrier's remembered-set path was genuinely exercised under relocation, not certified by an inert arm.
benchmarks/gc_ratchet
The pinned baseline is stale: check reports minor_cycles = 0 and heap_used_bytes +5333% on every probe, because the artifact was pinned while the evacuating minor was default-ON and #7161 flipped it default-OFF. That verdict is about #7161, not this PR, so it cannot say anything either way here. (Related: the gc-ratchet workflow's last runs on main are all cancelled — CLAUDE.md's failure mode #3.)
So I ran a direct branch-vs-base A/B instead, plus a base-vs-base run to establish that the metrics are capable of registering a difference:
| comparison | gated differences (retention + GC accounting, 8 probes x 9 metrics) |
|---|---|
base run #1 vs base run #2 (both origin/main) |
0 |
| base vs branch | 0 |
| base run #2 vs branch | 0 |
Correctness pass on all 8 probes in every run. The base-vs-base row is the point: the gated medians are deterministic run-to-run even on this (loaded) host, so "0 differences" is a measurement rather than an absence of one.
Ungated column. RSS is flat — every probe within ±0.3%, most within 0.05%:
01_nursery_churn rss 49,774,592 -> 49,643,520 (-0.3%)
03_cross_gen_writes rss 59,817,984 -> 59,834,368 (+0.0%)
04_dead_after_deep_stack rss 178,094,080 -> 177,930,240 (-0.1%)
07_array_grow_evacuate rss 108,478,464 -> 108,478,464 (+0.0%)
08_map_set_sidetables rss 151,764,992 -> 151,781,376 (+0.0%)
wall_ms came out 3–21% lower on all eight probes, and I am not claiming that: the base measurement ran at load 60.6/34.96/22.29 and the branch at 8.95/11.95/12.35. The timing column here is load-confounded and is not evidence of anything.
Unit tests
cargo test -p perry-runtime --lib (--profile perry-dev; the release test binary did not fit the disk budget alongside the matrix build):
| scan mode | result |
|---|---|
default, --test-threads=1 |
1616 passed, 0 failed |
PERRY_CONSERVATIVE_STACK_SCAN=off, --test-threads=1 |
1616 passed, 0 failed |
| default, parallel | 3–6 failures, set varies per run — gc::tests::teardown::map_set_*, global_this_webassembly, native_module_stream, object::prop_plan, root_words. origin/main produced 6 from the same family in the same worktree. Pre-existing and order-dependent; #7161 recorded the same set. |
Sabotage
| arm | result |
|---|---|
| identity hasher reinstated | control_byte_is_spread_across_generation_class_keys FAILS: got 1 distinct values from 64 consecutive buckets |
| plausibility guard removed | runtime_barrier_entry_points_reject_implausible_parents_without_dereferencing SIGSEGV, and so does the pre-existing closure::dynamic_props::tests_1802::dyn_prop_scanner_visits_values_without_holding_props_lock |
| parent dropped on the decoded route | 4 tests FAIL, incl. "old→young store through the decoded entry point must dirty the slot page" |
Repo gates
| gate | result |
|---|---|
cargo fmt --all -- --check |
clean |
scripts/check_file_size.sh |
red at base for 9 files; zero new offenders — verified by diffing offender lists against an origin/main worktree. My additions did push gc/tests/barrier.rs to 2099 lines, which is why they now live in gc/tests/barrier_decoded_parent.rs. |
scripts/addr_class_inventory.py |
byte-identical output to origin/main (red at base for child_process/value_util.rs and fs/dirent.rs, neither touched here) |
version bump / CHANGELOG.md |
none, per contributor policy |
Boundary
#7154 is closed. jdalton's live work is #7192, entirely in perry-codegen/src/expr/*, lower_call/new.rs, a script and a gap test — zero file overlap, and neither PR touches gc/verify.rs. Checked today.
Deferred
No instruction-count measurement of this change. The 19.03% / 657M figures are #7170's, from the Pi, which belongs to another session; the claims here are static (one classification removed per runtime-path barriered store) plus the hasher's control-byte degeneracy, which is measured by unit test. A perf stat A/B on the Pi is the outstanding evidence, and it would also settle how much of the 19.03% call #1 was — calls #1 and #2 share a bucket, so removing #1 promotes #2 from cache-hit to cache-miss and the net is not derivable from call counts alone.
![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/gc/tests/barrier_decoded_parent.rs`:
- Around line 98-135: Track whether any iteration of the parent/child barrier
parity loop produces a non-empty remembered-state fingerprint, using the
existing `remembered_state_fingerprint` result. After both loops complete,
assert that at least one cell was remembered; the old-parent/young-child
combination must satisfy this assertion while preserving the per-cell
fingerprint equality checks.
- Around line 211-253: Update the test cleanup at the end of the
implausible-parent isolation case, immediately after reset_remembered_set(), to
invoke clear_marks() so marks shaded by barrier_child_prologue are removed
before the test exits. Preserve the existing remembered-set reset and ensure
cleanup covers marks outside the arena/malloc walk.
🪄 Autofix (Beta)
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: cff764d8-58c3-4827-b340-4b9efc9ba365
📒 Files selected for processing (6)
changelog.d/7193-barrier-parent-classify.mdcrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/tests/barrier_decoded_parent.rscrates/perry-runtime/src/gc/tests/mod.rs
| for (child_label, child_bits) in children { | ||
| for parent_label in ["old", "nursery", "malloc"] { | ||
| let (parent_addr, slot_addr) = unsafe { | ||
| match parent_label { | ||
| "old" => { | ||
| let (obj, fields) = alloc_old_test_object(1); | ||
| (obj as usize, fields as usize) | ||
| } | ||
| "nursery" => { | ||
| let (obj, fields) = alloc_nursery_test_object(1); | ||
| (obj as usize, fields as usize) | ||
| } | ||
| _ => { | ||
| let fields = (malloc_parent as *mut u8) | ||
| .add(std::mem::size_of::<crate::object::ObjectHeader>()); | ||
| (malloc_parent as usize, fields as usize) | ||
| } | ||
| } | ||
| }; | ||
| unsafe { | ||
| std::ptr::write(slot_addr as *mut u64, child_bits); | ||
| } | ||
|
|
||
| reset_remembered_set(); | ||
| js_write_barrier_slot(ptr_bits(parent_addr), slot_addr as u64, child_bits); | ||
| let nanboxed = remembered_state_fingerprint(); | ||
|
|
||
| reset_remembered_set(); | ||
| runtime_write_barrier_slot(parent_addr, slot_addr, child_bits); | ||
| let decoded = remembered_state_fingerprint(); | ||
|
|
||
| assert_eq!( | ||
| decoded, nanboxed, | ||
| "parent={parent_label} child={child_label}: the decoded entry point must \ | ||
| leave the collector exactly the remembered set the NaN-boxed one does" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that at least one cell actually remembered an edge.
Every assertion in the loop compares two fingerprints. Both sides can be (0, 0, 0). If both entry points regressed to an unconditional early return, all 12 cells would compare (0, 0, 0) against (0, 0, 0) and this test would still pass. The test is advertised as pinning parity across every parent generation and child kind, so it must also prove the barrier ran.
Record whether any cell produced a non-empty remembered set, then assert it after the loops. The old-parent plus young-child cell guarantees one.
💚 Proposed fix to pin a non-vacuous cell
+ let mut any_edge_remembered = false;
for (child_label, child_bits) in children {
for parent_label in ["old", "nursery", "malloc"] {
@@
reset_remembered_set();
runtime_write_barrier_slot(parent_addr, slot_addr, child_bits);
let decoded = remembered_state_fingerprint();
+ any_edge_remembered |= decoded != (0, 0, 0);
assert_eq!(
decoded, nanboxed,
"parent={parent_label} child={child_label}: the decoded entry point must \
leave the collector exactly the remembered set the NaN-boxed one does"
);
}
}
+ assert!(
+ any_edge_remembered,
+ "the matrix must contain at least one remembered edge; otherwise every cell \
+ compares two empty remembered sets and the parity assertion is vacuous"
+ );As per coding guidelines: "GC gates must assert that the behavior under test actually ran, such as requiring copied_objects > 0, rather than passing merely because no exception occurred."
📝 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.
| for (child_label, child_bits) in children { | |
| for parent_label in ["old", "nursery", "malloc"] { | |
| let (parent_addr, slot_addr) = unsafe { | |
| match parent_label { | |
| "old" => { | |
| let (obj, fields) = alloc_old_test_object(1); | |
| (obj as usize, fields as usize) | |
| } | |
| "nursery" => { | |
| let (obj, fields) = alloc_nursery_test_object(1); | |
| (obj as usize, fields as usize) | |
| } | |
| _ => { | |
| let fields = (malloc_parent as *mut u8) | |
| .add(std::mem::size_of::<crate::object::ObjectHeader>()); | |
| (malloc_parent as usize, fields as usize) | |
| } | |
| } | |
| }; | |
| unsafe { | |
| std::ptr::write(slot_addr as *mut u64, child_bits); | |
| } | |
| reset_remembered_set(); | |
| js_write_barrier_slot(ptr_bits(parent_addr), slot_addr as u64, child_bits); | |
| let nanboxed = remembered_state_fingerprint(); | |
| reset_remembered_set(); | |
| runtime_write_barrier_slot(parent_addr, slot_addr, child_bits); | |
| let decoded = remembered_state_fingerprint(); | |
| assert_eq!( | |
| decoded, nanboxed, | |
| "parent={parent_label} child={child_label}: the decoded entry point must \ | |
| leave the collector exactly the remembered set the NaN-boxed one does" | |
| ); | |
| } | |
| } | |
| let mut any_edge_remembered = false; | |
| for (child_label, child_bits) in children { | |
| for parent_label in ["old", "nursery", "malloc"] { | |
| let (parent_addr, slot_addr) = unsafe { | |
| match parent_label { | |
| "old" => { | |
| let (obj, fields) = alloc_old_test_object(1); | |
| (obj as usize, fields as usize) | |
| } | |
| "nursery" => { | |
| let (obj, fields) = alloc_nursery_test_object(1); | |
| (obj as usize, fields as usize) | |
| } | |
| _ => { | |
| let fields = (malloc_parent as *mut u8) | |
| .add(std::mem::size_of::<crate::object::ObjectHeader>()); | |
| (malloc_parent as usize, fields as usize) | |
| } | |
| } | |
| }; | |
| unsafe { | |
| std::ptr::write(slot_addr as *mut u64, child_bits); | |
| } | |
| reset_remembered_set(); | |
| js_write_barrier_slot(ptr_bits(parent_addr), slot_addr as u64, child_bits); | |
| let nanboxed = remembered_state_fingerprint(); | |
| reset_remembered_set(); | |
| runtime_write_barrier_slot(parent_addr, slot_addr, child_bits); | |
| let decoded = remembered_state_fingerprint(); | |
| any_edge_remembered |= decoded != (0, 0, 0); | |
| assert_eq!( | |
| decoded, nanboxed, | |
| "parent={parent_label} child={child_label}: the decoded entry point must \ | |
| leave the collector exactly the remembered set the NaN-boxed one does" | |
| ); | |
| } | |
| } | |
| assert!( | |
| any_edge_remembered, | |
| "the matrix must contain at least one remembered edge; otherwise every cell \ | |
| compares two empty remembered sets and the parity assertion is vacuous" | |
| ); |
🤖 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/barrier_decoded_parent.rs` around lines 98
- 135, Track whether any iteration of the parent/child barrier parity loop
produces a non-empty remembered-state fingerprint, using the existing
`remembered_state_fingerprint` result. After both loops complete, assert that at
least one cell was remembered; the old-parent/young-child combination must
satisfy this assertion while preserving the per-cell fingerprint equality
checks.
Source: Coding guidelines
| let implausible: [(&str, usize); 6] = [ | ||
| ("closure prop owner key", 0xC10C_AB1E_0000_1803), | ||
| ("common handle band", 0x1234), | ||
| ("fetch handle band", 0x4_0010), | ||
| ("proxy id band", 0xF_0008), | ||
| ("above platform heap range", 0x9000_0000_0000), | ||
| ("unaligned heap-shaped word", 0x0000_7f31_0000_1003), | ||
| ]; | ||
|
|
||
| for (label, parent) in implausible { | ||
| runtime_write_barrier_slot(parent, slot_addr, child_bits); | ||
| runtime_write_barrier_external_slot(parent, slot_addr, child_bits); | ||
| runtime_write_barrier_gc_slot(parent, slot_addr, child_bits); | ||
| assert_eq!( | ||
| remembered_state_fingerprint(), | ||
| (0, 0, 0), | ||
| "{label}: an implausible parent must record no edge" | ||
| ); | ||
| } | ||
|
|
||
| let counters = take_write_barrier_trace_counters(); | ||
| if tracing { | ||
| assert_eq!(counters.calls, 18); | ||
| assert_eq!( | ||
| counters.non_pointer_parent_skips, 18, | ||
| "every implausible parent must be rejected before any header read" | ||
| ); | ||
| assert_eq!(counters.old_to_young_slow_hits, 0); | ||
| } | ||
|
|
||
| // The predicate itself, pinned: `malloc_gc_parent_addr` must answer false | ||
| // without dereferencing. If the guard is removed this line segfaults | ||
| // rather than failing, which is the point of asserting it here as well as | ||
| // through the entry points above. | ||
| for (label, parent) in implausible { | ||
| assert!( | ||
| !malloc_gc_parent_addr(parent), | ||
| "{label}: must not be classified as a malloc-GC parent" | ||
| ); | ||
| } | ||
|
|
||
| reset_remembered_set(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the plausibility predicate bounds and the GC test isolation guard's Drop.
set -euo pipefail
fd -t f 'addr_class' --exec ast-grep outline {} --items all \;
rg -n -C 20 'fn is_plausible_heap_addr' --type=rust
rg -n -C 25 'impl Drop for GcTestIsolationGuard' --type=rust
rg -n -C 5 'fn clear_marks' --type=rustRepository: PerryTS/perry
Length of output: 3177
🏁 Script executed:
#!/bin/bash
# Inspect the relevant predicate implementation and test definitions.
set -euo pipefail
sed -n '1,250p' crates/perry-runtime/src/value/addr_class.rs
printf '\n--- test file relevant section ---\n'
sed -n '1,120p' crates/perry-runtime/src/gc/tests/barrier_decoded_parent.rs
printf '\n--- all clear_marks/GcTestIsolationGuard references ---\n'
rg -n -C 8 'fn clear_marks|struct GcTestIsolationGuard|impl Drop for GcTestIsolationGuard|clear_marks\(' crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
# Inspect only the relevant predicate and guard definitions with targeted searches.
set -euo pipefail
printf '%s\n' '--- addr_class relevant lines ---'
sed -n '160,210p' crates/perry-runtime/src/value/addr_class.rs
printf '%s\n' '--- exact guard identifiers ---'
rg -n -C 12 'GcTestIsolationGuard|struct GcTestIsolationGuard|impl Drop for GcTestIsolationGuard|fn clear_marks\(' crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs' --max-count 80
printf '%s\n' '--- barrier_child_prologue and prologue implementation ---'
rg -n -C 10 'fn barrier_child_prologue|fn incremental_mark_barrier_value' crates/perry-runtime/src -g '*.rs'
printf '%s\n' '--- current test section ---'
sed -n '191,260p' crates/perry-runtime/src/gc/tests/barrier_decoded_parent.rsRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
# Use Python to extract only the targeted definitions and predicate behavior.
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("crates/perry-runtime/src/value/addr_class.rs")
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if line.startswith("pub(crate) fn is_plausible_heap_addr("):
start = i
break
else:
raise SystemExit("is_plausible_heap_addr not found")
brace_count = 0
end = None
for j, line in enumerate(text.splitlines()[start-1:], start):
brace_count += line.count("{") - line.count("}")
if brace_count == 0:
end = j
break
print("--- is_plausible_heap_addr ---")
for j in range(max(1, start-5), end+5+1):
print(f"{j}: {text.splitlines()[j-1]}")
body = "\n".join(text.splitlines()[start-1:end])
print("--- constants used in body ---")
for name in {"U64_HEAP_LIMIT", "U64_HEAP_MASK", "HEAP_UPPER", "HEAP_FLOOR", "is_bit_set", "is_unified_heap_addr", "gc_malloc_implausible"}:
m = re.search(rf"\b{re.escape(name)}\b", body)
print(f"{name}: {'FOUND' if m else 'NOT FOUND'}")
print("--- guard definition file references ---")
for ref in re.finditer(r"\b(GcTestIsolationGuard(?:::new|::\{)|struct GcTestIsolationGuard|impl Drop for GcTestIsolationGuard)\b", text):
line = text[:ref.start()].count("\n") + 1
print(f"{line}: {ref.group(0)}")
for file in Path("crates/perry-runtime/src/gc").rglob("*.rs"):
s = file.read_text(errors="ignore")
if "GcTestIsolationGuard" in s or "fn clear_marks" in s or "impl Drop for GcTestIsolationGuard" in s:
print(f"\n--- {file} ---")
for idx, line in enumerate(s.splitlines(), 1):
if "GcTestIsolationGuard" in line or "impl Drop for GcTestIsolationGuard" in line or re.search(r"\bfn\s+clear_marks\s*\(", line):
# show up to 12 lines before and 20 after
lines = s.splitlines()
lo = max(1, idx-12); hi = min(len(lines), idx+20)
print(f"[{idx}-{lo}-{hi}]")
for num in range(lo, hi+1):
print(f"{num}: {lines[num-1]}")
# Probe current function using read-only text extraction; do not run repo code.
def parse_function_body(text, func_name):
lines = text.splitlines()
for idx, line in enumerate(lines, 1):
if f"fn {func_name}(" in line:
start = idx
brace = 0
for j in range(start, len(lines)+1):
brace += lines[j-1].count("{") - lines[j-1].count("}")
if brace == 0:
return "\n".join(lines[start-1:j])
raise SystemExit(f"function not found")
body = parse_function_body(text, "is_plausible_heap_addr")
print("\n--- extracted function body ---")
print(body)
print("--- address threshold references in whole file ---")
for pat in [r"\b[U+]?0x9000_0000_0000\b", r"\b9000_0000_0000\b", r"\btwo_tb\b", r"\b0x2_0000_0000_0000\b"]:
for m in re.finditer(pat, text):
print(f"0x9000_0000_0000/two_tb reference: line {text[:m.start()].count(chr(10))+1}")
PYRepository: PerryTS/perry
Length of output: 50370
Add the closing mark-clear to this isolation guard.
barrier_child_prologue shades young on each rejected parent path, but GcTestIsolationGuard::Drop unsets marks per clear_marks()’ arena/malloc walk, not globally. Add a final clear_marks() after reset_remembered_set() or otherwise ensure no GC marks leak between tests.
🤖 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/barrier_decoded_parent.rs` around lines 211
- 253, Update the test cleanup at the end of the implausible-parent isolation
case, immediately after reset_remembered_set(), to invoke clear_marks() so marks
shaded by barrier_child_prologue are removed before the test exits. Preserve the
existing remembered-set reset and ensure cleanup covers marks outside the
arena/malloc walk.
1 participant
Summary
classify_heap_generationis 19.03% ofbenchmarks/app-patterns/kernels/batch.ts— 657M instructions, the largest single symbol in that program, 2.3x the next one, called from the write barrier with zero collections running (#7170's ranked profile; full account and the design for the rest on #7187).Reading the path, four page-map classifications happen per barriered element store:
decode_heap_addr(parent)barrier_parent_needs_rememberingremembered_child_needs_trackingremember_old_to_young_slotThe single-entry
PAGE_GENERATION_CACHEtherefore thrashes by construction — parent/slot and child alternate between two buckets, so two of four calls take the full hashmap path on every store, forever.This PR takes #1, which answers a question the caller had already answered, and makes the remaining lookups cheaper. It is the first of the increments designed on #7187; lazy barrier arming (which removes #2–#4 entirely until the first collection) stays a design pending its own dispatch.
1. Stop re-deriving the parent address
runtime_write_barrier_slotreceivesparent_addr: usize— a decoded GC user pointer — and handed it tojs_write_barrier_slotas a bareu64, sodecode_heap_addrfell into its "possible raw pointer" arm and paid a fullclassify_heap_generationto recover it, immediately beforebarrier_parent_needs_rememberingclassified the same address again.The three runtime entry points now share
write_barrier_slot_decoded, which keeps the parent ausizethroughout. The never-skippable half — child decode plusincremental_mark_barrier_value— is factored intobarrier_child_prologueso the #6011 primitive-store fast path (child_addr == 0) stays the cheapest exit and is provably shared by both routes.Outcome-preserving, including the one case that changes counter: a malloc-GC parent used to exit at
NonPointerParentSkips(classification returnedUnknown, so the decode returned 0) and now exits atParentNotOldSkips. Different reason, same remembered-set effect — and the new reason is the one that is actually true.2. A latent dereference the round-trip was hiding
This is the part worth reviewing.
malloc_gc_parent_addrdereferencesparent_addr - GC_HEADER_SIZEbehind a bare< GC_HEADER_SIZE + 0x1000floor, which admits every handle-band id and every out-of-range garbage word. It was safe only because its callers happened to filter first — including by accident:runtime_write_barrier_external_slotNaN-boxed its parent, and an address with high bits set ORs into something that is no longerPOINTER_TAG, so the decode rejected it before the deref.closure/dynamic_props.rsparks props under exactly such non-address owner keys and depends on that accident — its own unit test uses0xC10C_AB1E_0000_1803. Removing the round-trip without noticing SIGSEGVs there, which is how this was found.Both filters are now one explicit predicate,
barrier_parent_addr_is_dereferenceable— the canonicaladdr_class::is_plausible_heap_addr(the repo's answer to this whole hazard class) plus the 8-alignmentaddr_classdoes not cover — applied inwrite_barrier_slot_decodedand insidemalloc_gc_parent_addritself, so the function that dereferences carries its own guard instead of trusting callers. That is a strict improvement on the pre-existing state: the guard now also covers the NaN-boxed callers, where a plausible-but-unaligned parent previously reached the header read.3. The page-generation map's hasher
PAGE_GENERATIONScarried a bespoke identity hasher whosewrite_usizestored the key verbatim.HashMapis hashbrown: the bucket index comes from the hash's low bits, but the SIMD control byte ishash >> 57. Keys areaddr >> GENERATION_CLASS_SHIFT— around 2^26 for a real heap address — so the control byte was zero for every entry in the table. Every group probe then matches every occupied slot, and each match costs a real key comparison (a scattered load into a bucket) before the right one is found — on a lookup the write barrier performs several times per heap store.Not an argument — measured by the new regression test with the old hasher reinstated:
The map now uses
fast_hash::PtrHasher, which is the project's existing answer to exactly this (see its module doc, and themix(h) = h ^ (h >> 32)step whose comment records a 455 ms -> 830 ms regression from omitting it).PAGE_GENERATIONSis only ever point-queried —get/get_mut/insert/remove; thefirst_key..=last_keyloops walk key ranges, not the map — so iteration order is not observable and there is no determinism exposure.Tests, and what each one is red for
gc/tests/barrier_decoded_parent.rs(new sibling module — the additions pushedbarrier.rspast the 2000-line cap) andarena/page_meta.rs's test module. Every sabotage below was run:control_byte_is_spread_across_generation_class_keysFAILS: got 1 distinct valueswrite_barrier_slot_decoded+malloc_gc_parent_addrruntime_barrier_entry_points_reject_implausible_parents_without_dereferencingSIGSEGV, and so does the pre-existingclosure::dynamic_props::tests_1802::dyn_prop_scanner_visits_values_without_holding_props_lockwrite_barrier_slot_decoded(0, …))bucket_index_is_spread_across_generation_class_keyspasses under the identity hasher too — deliberately. It is the guard against over-correcting into a hasher that fixes the control byte and wrecks the bucket index, not a duplicate of the test above it.runtime_write_barrier_slot_matches_nanboxed_entry_pointwalks every parent generation (old / nursery / malloc) x every child kind (young / old / malloc / primitive) and asserts the decoded route leaves the collector byte-identical remembered-set state to the NaN-boxed one.Verification
cargo test -p perry-runtime --lib, default scan modePERRY_CONSERVATIVE_STACK_SCAN=offcargo test)origin/mainproduces 6 from the same family in the same worktree —gc::tests::teardown::map_set_*,global_this_webassembly,native_module_stream,root_words. Pre-existing, and #7161 recorded the same set.scripts/gc_repsel_matrix.sh --arms all --pressure 8benchmarks/gc_ratchetgated + ungatedcargo fmt --all -- --checkscripts/check_file_size.shorigin/mainworktree)scripts/addr_class_inventory.pyorigin/main.node-version)Built once, one target dir,
--profile release,-p perry -p perry-runtime-static -p perry-stdlib-static.Boundary with concurrent GC work
#7154 is closed; jdalton's live work is #7192 (
fix/7154-root-store-dominance), which touchesperry-codegen/src/expr/*,lower_call/new.rs, a script and a gap test — zero file overlap with this PR, and neither touchesgc/verify.rs. Checked same-day.Not measured
Refs #7187, #7170, #5094.
Summary by CodeRabbit
Performance
Reliability
Tests