perf(gc): skip the element-shape table when neither address of a move advertises a proof - #9808
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesElement shape transfer optimization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Array relocation now avoids unnecessary element-shape side-table work when neither address advertises a proof, while retaining cleanup when the destination does. The covered boundary behavior leaves no identified merge-readiness risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
added
the
run-extended-tests
650361b to
7d9c08d
Compare
… advertises a proof `transfer_element_shape` runs for every relocated array and already decides `had_bit` from header words it has read anyway — then took the side table's `RefCell` and hashed both addresses regardless, for two removes that remove nothing whenever the source proved nothing and the destination advertises nothing. It now returns before the table in that case. The gate cannot be `!had_bit` alone: a destination still advertising a proof describes storage the move has just replaced, so that case keeps the full fail-closed path. Both halves are pinned by a new test, which fails on its named assertion if the gate is widened. Leaving a record behind at an address whose bit is clear is not a new state: the bit is the sole authority for a read, `establish` draws identities from `ELEMENT_SHAPE_PROOF_SEQ` rather than from the record at the address, and `prune_dead_element_shape_owners` drops it on the next collection — the same guarantees a fail-closed transfer already depended on. Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
7d9c08d to
ffa9e91
Compare
|
Landed on |
1 participant
Closes part of #9792. Independent of #9807 (different table, different file);
they were measured in one binary and can land in either order.
The change
transfer_element_shaperuns fromlayout_transferfor every relocatedarray — growth forwarding, a copying minor, an old-gen defrag. It already
computes
had_bitfor free from two header words it has read anyway, then tookELEMENT_SHAPES'RefCelland hashed both addresses regardless, for tworemoves that on the common path remove nothing.It now returns before the table when neither address advertises a proof.
Why the gate has exactly one safe shape
Gating on
!had_bitalone would be wrong. A destination that still advertisesa proof is describing storage the move has just replaced, so that case must
keep the full fail-closed path — the existing test
a_fail_closed_transfer_leaves_no_record_for_the_next_array_to_inheritfailsunder exactly that sabotage, and so does the new test, on its named assertion:
(Verified by making that edit and running the suite: 2 failures, then restored.)
The safety case for what skipping leaves behind
Skipping leaves a record in
ELEMENT_SHAPESat an address whose bit is clear.That state is not new — it is one the design already names and defends against,
in three independent places:
element_shape_proofreturnsNoneon a clear bit before it touches the table;note_element_store_with_bitis gated on the same bit before its
record_for. Those are the only twocallers of
record_for, and both check first. So a record behind a clear bitis unreachable, not merely stale.
establishdraws every identity fromELEMENT_SHAPE_PROOF_SEQ, neverfrom the record at the address — its doc comment says this exists precisely
so "a survivor record — left by a fail-closed transfer, a dead array whose
prune has not run yet — can never donate its identity to the array
established here next".
prune_dead_element_shape_ownersdrops it on the next collection, whichis the same footprint-only guarantee a fail-closed transfer already relied on
("a stale record can never be read, because a recycled allocation's fresh
_reservedis zero").The new test asserts the first two directly rather than arguing them: it builds
the survivor state (record present, bit cleared), runs the transfer, asserts no
proof reads back, then establishes at that very address and asserts the epoch
is not the survivor's.
Numbers
Rig: the offline mock-API harness,
cc_relink/cc_base_new(main1d63fa91f,this PR's base) as the before arm, node measured in the same session. The
candidate is a full compile of the claude-code bundle carrying both #9807 and
#9808 — they land independently but were measured in one binary. Runtime-only
diff, so both binaries come from the same codegen.
400-character streamed reply — quiet box (1-min load 4.2–5.9), 4 paired runs,
arm order alternated each pair:
3300-character streamed reply — quiet subset (load 6.4–10.0), 3 paired runs:
timed_turn(37 keystrokes then two short turns), taken at load 35–50 and soreported for completeness only: startup 4.80 s before vs 2.60 / 1.80 s after;
typing CPU r2 1.56 vs 1.49 / 0.87 s; echo p90 66 vs 48 / 28 ms; turn r2 CPU
0.92 vs 1.03 / 1.06 s. The before arm lost one of its two runs, so one side is
n=1.
Reading, stated plainly
The CPU win is modest and inside the run-to-run spread: −2 % on the
400-char median, −4 % on the 3300-char median, and the before arm wins two of
the four paired 400-char comparisons. The one consistent directional result is
settled footprint, lower in 4 of 4 paired 400-char runs (median 470 → 442 MB,
−6 %) — which is what removing 50 MB of per-turn
Vecallocation should looklike. Peak RSS is flat.
The row that reads worse after is
CPU in the next 12 s(median 4.87 →5.67 s), and four pairs do not resolve it: the after arm's own spread there is
4.27–7.24 s against the before arm's 4.71–5.76 s.
This is the size of result that was predicted before the run — the prune costs
one walk of the live keys per collection, and removing two of three walks is a
small share of a turn in which the collector is doing much more elsewhere.
Neither metric regresses, which is the bar.
A first reading was wrong and is withdrawn
Measured at 1-min load 17–58, the candidate looked 40–58 % slower on the
3300-char arm. Repeating the same pairs on a quiet box inverted it (−4.1 %,
−0.4 %, −35 %), and the before arm's own samples for one unchanged binary
ranged 52–79 s across those loads. No CPU number taken above ~12 load on this
box is usable, and the footprint column is bimodal exactly as the campaign's
invariant 0 says (676–706 vs 1113–1134 MB in the same pair set, decided by
whether a full collection fell in the window).
Binary provenance, and the one thing the measured binary does not carry
The measured candidate is a full compile (no object-cache reuse) of the bundle
carrying this diff and #9807 on top of
1d63fa91f, which is exactlycc_base_new's commit,so the two arms differ only by the runtime change — the diff touches no
perry-codegen/perry-hirfile, so both binaries carry the same emitted JS.The branch has since been rebased onto current main. The only thing the rebase
adds is the changelog fragment's rename to
9808-…, which is what thelintjob requires to count it as a fragment at all. Nothing in
element_shape.rschanged, so the measured binary matches this diff.
What this leaves behind, and what would falsify the safety case
Skipping leaves a record in
ELEMENT_SHAPESat an address whose bit is clearwhere the old unconditional
removewould have swept it. That is a realchange in table occupancy, not only in correctness, and it deserves naming: the
records that used to be cleared opportunistically by any later move touching
that address are now cleared only by
prune_dead_element_shape_owners. Thatprune runs once per collection —
PERRY_GC_DIAGandPERRY_LAYOUT_DIAGonthe same 400-character reply show 40 copying minors plus 6 full cycles against
46 prunes — so the extra occupancy is bounded by one collection's array churn,
not unbounded.
The measurement that would falsify this is peak RSS and settled footprint:
an accumulating table shows up there first. Both are flat-or-better in the
table above (peak RSS 649 → 649 MB median at 400 chars, 1300 → 1310 MB at
3300; settled footprint down 6 %). The census does not currently break out
ELEMENT_SHAPES, so this is an indirect answer rather than a direct count —adding that row is the cheap way to make it direct, and is worth doing whether
or not this lands.
Tests
New:
a_transfer_skips_the_table_only_when_neither_address_advertises_a_proof,covering both arms of the gate's boundary. Plus a
#[cfg(test)]test_clear_element_shape_bit_onlyhelper to construct the survivor state.cargo test --release -p perry-runtime --lib -- --test-threads=1: 3,143passed, 0 failed (54 under
array::element_shape).cargo clippy -p perry-runtimeclean.The
run-extended-testslabel is on this PR so the GC gates actually runrather than showing
skipping. Reading notes: #9782 is open (full mark-sweepgc-stressarms), andgc-root-dominancered withviolations: 0is thecorpus floor ("checked 5,625 function(s), need at least 6,000"), which a
runtime-only diff cannot move.
https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
Summary by CodeRabbit
Performance
Bug Fixes
Tests