Skip to content

perf(gc): swap-remove for keys-array descriptor families, with a spill index — removes up to 848 GB of memmove per turn - #9881

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:perf/family-swap-remove-main
Closed

perf(gc): swap-remove for keys-array descriptor families, with a spill index — removes up to 848 GB of memmove per turn#9881
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:perf/family-swap-remove-main

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

DRAFT, pending one paired row. At PERRY_GC_TENURING_SURVIVALS=1, over
four turns on the landing base, this change is ≈ 0 on every column against
main5 pinned the same way. The earlier "+4 % / worse in 5 of 5 pairs" reading
came from single-turn rows, where main's own turn-1 tenuring excursion
(S = 4 → 2 → 1) dominates — this change was not the variable. What is still
unmeasured is the adaptive (knob-unset) configuration over four turns; that
row (TN2) or the arena lane's fix to the excursion itself is what takes this
out of draft. See "On the landing base".

The runtime lib suite is green and is not what is blocking: cargo test --release -p perry-runtime --lib -- --test-threads=1, cold build, macOS dev
box, 3200 passed / 0 failed / 4 ignored / 0 filtered. The measurement
host's Linux run was 3192 total; the gap is exactly the 12 target_os = "macos" test items in this crate (3204 = 3192 + 12), so the suites agree. The
one earlier failure —
owned_key_count_versions_are_retired_behind_the_current_one, which asserted
the family's ORDER after a retirement — over-specified and is now a membership
comparison; see "Was the order really free?".

The defect

IdList::remove was Vec::remove(pos), which shifts every element past the
removed position. The removals that matter come from the dead-owner prune —
prune_dead_owner_side_tables_post_traceremove_descriptor_indexed_under
against a families list, i.e. the descriptor ids indexed under one
keys-array address.

Measured on the compiled claude-code TUI, single 3300-char replies, two hosts,
fourteen draws:

  • the removals sit at position ~0.31 of the list — essentially always the
    front, which is the worst case for a tail shift;
  • one owned-keys array per process grows its family to 404 k–514 k while
    every other list stays small;
  • so the same ~3.7 M removals memmove up to 848 GB in a single turn;
  • 100.000 % of those bytes are in families. by_facts moves zero
    its longest list is 1 in every draw, exactly as its doc claims;
  • retire_owned_shape_siblings is not involved: it never sees a family
    longer than 16.

What actually happens, per minor — measured with final counters

A second instrumented arm, 4 single-turn 3300 draws on the measurement host,
--graceful --exit-wait 45 so exited=true on all four and these are final
counters, not a truncated cumulative line
. Load 2.3–4.0, so the CPU column is
inflated and only the modes are read from it.

draw CPU mode biggest family removals from it elements moved bytes minors that removed from it (of ~125)
1 20.26 slow 0x58e9a055278 704,240 149.77 G 599 GB 2: 512,685 then 191,555
2 16.92 slow 0x3a16a093d58 404,890 81.97 G 328 GB 1: 404,890
3 15.27 fast none — no key ever reaches 100 k 0 0 0
4 20.26 slow 0x3bc7fe9fa80 707,010 150.31 G 601 GB 2: 512,693 then 194,317

Per-removal tail histogram, draw 1 — the distribution is what makes this a
structural defect rather than a slow constant:

tail moved 0 1 2–15 16–255 256–4k 4k–64k 64k–1M 1M+
removals 1,258,074 2,774,737 2,240,229 5,339 36,968 192,522 871,094 0

Three readings.

  1. The whole slow-mode cost is one or two minors per turn, each bulk
    front-removing a ~512 k family — 340–870 k removals apiece, most of them
    moving between 64 k and 1 M elements. The other ~123 minors remove nothing
    from that family at all. This is not a cost smeared across the turn; it is
    two enormous prunes.
  2. The family's only key is "0" — the crossing lines report
    names=len=1 [0] every time. It is half a million objects with a single own
    property named 0, all sharing one keys array, each minting its own
    descriptor.
  3. The fast draw is not the same family pruned cheaply — it is the family
    never growing past 100 k.
    The 10 k → 100 k crossing, and its re-crossing
    after the prune, occur only in slow draws (site=0 for the first 10 k,
    site=1 after). So the mode is decided by whether a minor lands and prunes
    the family before the allocation burst finishes, and the burst recurs twice
    per turn. That is why it is per-process, and why unrelated schedule changes
    were also observed to remove it.

What this means for the fix, and it is the reason no growth issue is filed.
A program that allocates half a million same-shape single-key objects in a burst
is doing nothing wrong. The defect is that removal was O(family), so a bulk
prune of that family cost O(n²). This change makes the bulk prune O(removals).
On claude-code specifically, #9857 removes the source of this particular family
— which is why cc's own number is expected to be unchanged on the landing base,
and why this fix is for any workload that builds a large family.

Reconciliation with an earlier reading of mine. I previously reported that
bytes moved was "necessary but not sufficient", from a draw that moved 335 GB
and stayed fast. That capture had exited=false, so its counters were truncated
before the turn tail. In this series — the only one with final counters — the
correlation is clean: the fast draw moved zero bytes from the big family and
every slow draw moved 328–601 GB. I am not claiming the memmove accounts for
every millisecond of the 4.5 s, but the earlier counter-example does not stand.

The order question, which is what makes a swap-remove available at all

The answer differs per index, so removal becomes two entry points:

  • by_facts is ordered. facts_push_front installs a process-global id as
    the canonical answer ahead of an equivalent local one, and the list is read
    first-wins. It keeps the order-preserving removal — which costs nothing, since
    it is length 1.
  • families is not. Its only order-touching reader is the "one descriptor
    stands for the family" choice in the two rekey walks
    (scan_shape_table_rekey_mut, scan_shape_keys_address): break on the first
    old_carrier || cache_carrier, else take any present member. The chosen
    descriptor then feeds exactly one expressionold_carrier || cache_carrier — whose value is the same for every carrier and the same for
    every non-carrier. The outcome is a function of the set, not the order, as
    the code's own comment says ("else any present member").

The caller declares the contract, because the caller is the one that knows
whether its order is load-bearing. A single remove() that guessed which index
it was on would be the bug.

The structure

A spilled list gains an id → index map, built once it passes
SPILL_INDEX_MIN (32) and empty below it, where a scan of a few entries is one
cache line and a hash probe is not. That makes position, contains and
removal O(1) on the lists that get long — which also removes the linear
membership scan in family_push_back
, recorded at this file's own
append_unchecked as 6.2 % of main-thread leaf samples, 95 % of it under that
one caller
.

Measured (perrymaster, 8 single-turn 3300 draws, load 0.93–1.37)

Built from this change's tree and relinked against the pre-fix binary's object
cache; the pre-fix arm ("A") is the same base without it.

A (pre-fix) this change
turn CPU fast ~14.5–14.7, slow ~19.1–19.2 14.02–14.34
slow draws 35 % of 34 draws 0 of 8
[gc-idlist] removals 20.7–21.9 M
elems_moved 4.42–4.46 M
bytes moved 5.6–16 GB (fast draws), 335–848 GB (slow) 17.7 MB, every draw
positions scanned 28.0–29.5 M
minors / fulls / steps 122–123 / 5 / 11 122–123 / 5 / 11

Schedule unchanged (minors 122–123, 17–18/105, fulls 5, steps 11, tiny-parse
11 — identical to A), so this is not a pacing change wearing a structure's
clothes. Fast-mode CPU is unchanged: 14.0–14.3 with these counters compiled
in, against 14.5–14.7 for the instrumented pre-fix arm's fast draws and 13.6
uninstrumented. On A's distribution the expected saving is a mean −1.6 s
(35 % × 4.5 s) with no fast-mode cost.

Caveat that travels with the table: exited=false on all 8 draws (the
script waits 10 s), so the quoted [gc-idlist] line is the last cumulative one
before SIGKILL — up to roughly one second of turn tail is unaccounted for. The
counters are cumulative and load-independent, so this bounds their completeness,
not their meaning.

Raw: /root/rig9831/swap_{1..8}.diag, swap.jsonl.

What the mode statement is, precisely

This removes the slow mode on the pre-#9857 binary: 0 slow draws in 8, where
the same base without it is slow in 35 % of 34. The fix is for any workload
that builds a large family
, and cc is simply the workload that exposed it.

What used to follow this sentence was a prediction, and its status has moved
twice.
It read: "on the landing base #9857 stops cc from creating that family,
so cc's own number is expected to be unchanged"
. Single-turn rows appeared to
falsify it (worse in 5 of 5 pairs) and it was retracted here; four-turn rows at
S=1 then showed this change ≈ 0 on every column, and the single-turn deltas
to be main's own turn-1 tenuring excursion. Both the claim and the retraction
rested on data that did not isolate the variable. See "On the landing base"
below — the adaptive configuration still has no paired four-turn row.

The mechanism section above says why the mode exists at all (one or two minors bulk-pruning a 512 k
family, versus a draw where it never grows that far), which is also why it is
per-process rather than a property of the binary.

Measured on the mini (pre-#9857 binary), tenuring pinned

A second quiet host (macOS, the shipped platform), 8 runs, exited=true on
all of them
, load 1.70, zero interference from other work. Four 3300-char
turns in one process then a 120 s idle window, with the tenuring survival
threshold pinned at =1 and =2. Two different controls appear below and
each comparison names which one it uses
— a same-sandbox control built from
the same tree without this change (n=2, =2 only), and an earlier five-draw
series on the same binary family.

Idle CPU at =2: the 23 s mode is gone

arm idle CPU
this change, =2 3.94 / 4.18 / 3.79 — a 0.39 s band
this change, =1 1.86 / 1.93 / 1.85
control, five-draw series, =2 4.41 / 23.52 / 23.33 / 23.34 / 4.38
control, same sandbox, =2 4.40 / 4.41

The 23–24 s mode occurs in no draw. Stated with its power: the prior
high-mode rate was 3 of 5, so 0 of 3 is p ≈ 0.064 under "unchanged" —
suggestive on this data alone and convincing beside the mechanism and the
per-minor counters above, but n = 3 cannot exclude a rarer high mode. The
same-sandbox control's two draws both landed in the low mode
, so it does not
independently re-demonstrate the bimodality; that evidence is the five-draw
series.

What is not claimed: idle CPU does not fall to the =1 range. It lands at
3.79–4.18 s, about =1's 1.85–1.93. This change removed the ~19 s high
mode; a residual ~2 s of =2-specific idle CPU remains and is not the prune
memmove.

Four-turn CPU: the spread collapses

arm n 4-turn totals min spread
this change, =1 3 73.9 / 74.4 / 74.3 73.9 0.5 s
control, five-draw series, =1 5 131.6 / 99.6 / 97.6 / 95.1 / 91.5 91.5 40.1 s
this change, =2 3 80.9 / 83.2 / 83.4 80.9 2.5 s
control, same sandbox, =2 2 108.5 / 129.2 108.5 20.7 s
control, five-draw series, =2 5 175.2 / 127.0 / 109.5 / 86.0 / 104.3 86.0 89.2 s
  • =1: −19 % on minima against the five-draw series, spread 40.1 → 0.5 s.
  • =2: −25 % on minima against the same-sandbox control (80.9 vs 108.5);
    spread 89.2 → 2.5 s against the five-draw series — 36×.

Two named prior behaviours are absent from every draw: the turn-1 split at
=2
(this change 15.0/16.7/16.9 against the control's 34.6/37.0) and the
long tail (to 46 s previously, 48.4 s in the same-sandbox control).

That one change removes the idle burn, the turn-1 split and the CPU tail
together is the "one mechanism, three sides" prediction, and it is met.

Observation, not a claim: peak RSS at pinned =2 moved UP

arm peak RSS
this change, =2 2521 / 2355 / 2343
control, same sandbox, =2 2229 / 2239
control, five-draw series, =2 2231 / 2233 / 2236 / 2235 / 2242
this change, =1 1389 / 1439 / 1432 — unchanged

Every =2 draw of this change exceeds every prior =2 draw from either source
+114 MB at the minimum, ranges disjoint — and the same-sandbox control
rules out sandbox and protocol. It is visible in the arena: at =2 capacity
reaches 541–566 MB where =1 stays at 278–289 MB, doubling inside
turn 2.

Reported as a surprise, and deliberately not defended. =1 peak is
unchanged, and pinned =2 is not a landing configuration — it is a
tenuring experiment's arm. The tenuring arms on the landing base will be run
with this change included, which is where that interaction belongs.

And one hypothesis is refuted directly by the same data: the natural reading
— that the arena regrows at the start of turn 2 — is wrong. Capacity is
253–254 MB at the end of turn 1 and 255–256 MB at the start of turn 2, i.e.
continuous across the boundary. The doubling happens within turn 2, not at its
edge.

Raw on the mini under ~/ccperf/logs/; MINI_9881.tsv, p9881_an.py and
RESULT_9881_mini.md in the campaign directory.

On the landing base — and this falsifies a prediction made above

Everything above was measured on the pre-#9857 base. On the landing base
(main5 33e2856c5, bundle vs the same bundle relinked with this change),
perrymaster, 5×3300 + 1×400, load 0.9–5.6:

3300 turn CPU, per round main5 + this change (knob unset) + this change, PERRY_GC_TENURING_SURVIVALS=1
1 2.81 2.83 2.42
2 4.12 4.38 4.01
3 3.43 4.43 3.25
4 3.83 4.47 3.07
5 2.71 3.09 2.55
min 2.71 2.83 (+4 %) 2.42 (−11 %)
mean 3.38 3.84 3.06
pairs worse in 5/5 better in 5/5
other metrics main5 unset S=1
400 turn CPU 0.99 1.01 0.71 (−28 %)
peak RSS +3…+23 MB +9…+22 MB
settled RSS after 120 s idle, 3300 473 492 (+19 MB)
settled RSS, 400 455 466
idle CPU equal equal

The prediction this falsifies is mine. The "mode statement" above says cc's
own number is expected to be unchanged on the landing base, because #9857
removes the family that made the prune expensive. Measured, it is not
unchanged: with the tenuring knob unset this change costs 4 % on minima and
is worse in 5 of 5 pairs, with +19 MB settled. That expectation was
wrong and is retracted rather than reworded.

The sign depends on the tenuring policy's state, not on this diff alone. The
same binary, with S pinned to 1, beats main5 by 11 % at 3300 and 28 % at
400
, better in 5 of 5 pairs. perry-b4's reading, which the tenuring arms will
test: on this tree the adaptive lock no longer settles at S=1, because the
swap-remove removed the churn its schedule was reading — so "unset" runs in the
S ≥ 2 regime the mini priced, which is exactly where the mini also saw
+114 MB peak. Two hosts, two configurations, one consistent story: this
change is a clear win in the S=1 regime and a cost in the S ≥ 2 regime, and
which regime a run lands in is decided by a policy this diff does not touch but
does perturb.

So this PR is back to DRAFT and no code changes until the tenuring arms
read.
They run four env arms on main5 + this change + the lock — s1, unset,
s2, and main5 itself at S=1 as the control that was missing — with an
S-histogram, lock transitions and promotion per arm.

The pre-#9857 results above (perrymaster's 0/8 slow draws, the mini's spread
collapse and idle-mode removal) stand as measured; they are results about a
binary whose family this change makes cheap to prune. What is now open is
whether the landing base wants it before the tenuring policy is settled.

Raw: /root/rig9831/combTN0.jsonl, idleTN0.jsonl.

What TN0 actually measured — and it was not this change

TN, four arms on the landing base (main5 + this change + the lock), 2 rounds ×
four-turn 3300 + 400
, load ≤ 1. The arm TN0 was missing is the first column:
main5 itself pinned to S=1.

pinned S=1, 4-turn m5s1 (main5) s1 (main5 + this change)
turn-1 CPU 2.93 / 3.05 3.02 / 2.95
Σ turns 2–4 6.33 / 6.36 6.35 / 6.41
promoted 39.7 / 38.0 MB 39.0 / 39.0 MB
peak (vmhwm) 657 / 651 MB 655 / 656 MB
idle CPU 0.74 / 0.80 0.78 / 1.49 (one outlier)
S-histogram S1:27 S1:27

Equal on every column. So at S=1, over four turns, this change is ≈ 0
which is what the body originally claimed for the landing base.

TN0's ±4–14 % was main's own adaptive policy, not this diff. With the knob
unset, main takes a turn-1 excursion — S = 4 → 2 → 1 — worth +0.35–0.45 s at
3300 and +50 % of turn 1 at 400
. TN0's rows were single-turn, i.e. 100 %
turn 1, so that excursion dominated every one of them and this change was never
the variable being measured. That is this campaign's own named trap — a
single-turn measurement can invert a verdict
— and it caught both of my
statements in turn: the original "unchanged on the landing base" and the
retraction I published when TN0 appeared to falsify it. Neither was
supported by data that isolated the variable.
The four-turn S=1 rows are the
first that do.

Two rows are still missing, and this stays draft until one of them lands:

  1. TN2 — main5-unset vs main5 + this change-unset, over four turns, so the
    adaptive configuration gets its own paired row. Queued as a 3-round relink
    stage.
  2. The excursion fix itself — the arena lane is repairing the turn-1
    S = 4 → 2 → 1 startup claim, after which the question is moot.

Tests, with the sabotage named at each

test what it pins sabotage that fails it
unordered_removal_moves_o1_elements_and_scans_o1_entries 2,000 front removals move ≤ 4N elements and scan ≤ 4N entries point remove_unordered at remove_ordered — the ordered path moves N(N−1)/2 = 1,999,000, 250× the bound; raising SPILL_INDEX_MIN above N fails the scan half
the_spill_index_agrees_with_the_vector_after_every_operation index vs a plain Vec oracle after front, middle and back removals, plus replace drop the pos.insert(ids[pos], pos) fixup for the element the swap relocated
a_short_spilled_list_builds_no_index a list below the threshold allocates no map lower SPILL_INDEX_MIN

The bound is asserted as a multiple of N, not as an absolute, so the
assertion is about the complexity class rather than about one N. The oracle test
exists because a drifting index is a wrong answer — a descriptor that cannot
be found — not a slow one.

[gc-idlist] under PERRY_GC_DIAG=1 reports removals, elements moved and
positions scanned.

Base and scope

Rebased onto main 35c36f425. Main moved 60 commits under these files since the
measured base (644b9d362) — including #9756's SlotIndex — but IdList
itself is byte-identical on both
, and the only conflict was an additive one in
copying.rs where main and this change each append a diag call at the same
point (both kept). Verified after the rebase that the only two IdList
removal call sites
are family_remove → unordered and facts_remove
ordered, so no writer main added is left unrouted.

Was the order really free? — the one test that said otherwise

The first full-suite run failed exactly one test, and it was the right one to
fail: owned_key_count_versions_are_retired_behind_the_current_one asserted
test_shape_ids_for_keys(keys) == vec![cached, current] after a retirement, and
swap-removing the two stale versions moved current in front of cached.

So the claim above was re-checked against that test's subject, by enumerating
every production reader of families on this base:

reader what it takes order?
retire_owned_shape_siblings filter, all but keep set
prune_dead_shape_keys_young snapshot, remove all set
scan_shape_table_rekey_mut first carrier else any member set
move_shape_family wholesale remove + re-insert n/a
relevant_shape_keys the KEYS, then sort_unstable + dedup normalised
scan_shape_keys_address first carrier else any member set
census (heap_bytes, len, max) aggregates n/a
retire_owned_shape_siblings' slot-list twins (×2) filter, all but one set
IdList::replace caller replaces in place position-preserving

The two rekey walks are the only order-touching readers, and what they take
is a single choice fed to exactly one expression — old_carrier || cache_carrier — whose value is the same for every carrier and the same for
every non-carrier. Nothing reads a position.

And the helper the failing assertion used is #[cfg(test)]:
test_shape_ids_for_keys renders families.as_slice().to_vec() for tests only,
and its .first() sibling is likewise #[cfg(test)] and is only ever called on
families the caller seeded with one descriptor. #9706's contract is about
which ids survive a same-address retirement, not the order they survive in
;
the assertion compared against a Vec because the helper returns one.

The assertion is now a sorted comparison with that reasoning written at it. The
pre-retirement assertion is deliberately left alone — no removal has happened
there, so it legitimately documents that adds append — with a comment saying
that is a property of the add path and not a contract.

Corroborating, and it is the part that would have caught a real order
dependency: the suite ran 3192 with exactly one failure, so no other test in
the tree asserts a family's order across a removal.

Not addressed: why one family reaches half a million descriptors. That is a
separate defect, still being measured, and will be a separate change.

Summary by CodeRabbit

  • Performance

    • Improved performance when managing large object shape families, with faster lookups and removals.
    • Reduced unnecessary movement and scanning during garbage collection and runtime updates.
    • Improved memory accounting for larger shape collections.
  • Reliability

    • Preserved canonical ordering for fact-based lookups while allowing efficient unordered updates where ordering is not required.
  • Diagnostics

    • Added runtime reporting to help monitor shape-list operations and garbage-collection behavior.

IdList::remove was Vec::remove(pos), which shifts every element past the
removed position. The removals that matter come from the dead-owner prune
(prune_dead_owner_side_tables_post_trace -> remove_descriptor_indexed_under)
against a `families` list -- the descriptor ids indexed under one keys-array
address.

Measured on the compiled claude-code TUI, single 3300-char replies, two
hosts, fourteen draws:

  * the removals sit at position ~0.31 of the list -- essentially always
    the FRONT, which is the worst case for a tail shift;
  * one owned-keys array per process grows its family to 404k-514k while
    every other list stays small;
  * so the same ~3.7M removals memmove up to 848 GB in a single turn;
  * 100.000% of those bytes are in `families`. `by_facts` moves ZERO: its
    longest list is 1 in every draw, exactly as its doc claims.
  * retire_owned_shape_siblings is NOT involved -- it never sees a family
    longer than 16.

THE ORDER QUESTION, because a swap-remove is only available if order is not
load-bearing, and the answer differs per index:

  * `by_facts` IS ordered -- facts_push_front installs a process-global id
    as the canonical answer ahead of an equivalent local one, read
    first-wins. It keeps remove_ordered, and that costs nothing: length 1.
  * `families` is NOT. Its only order-touching reader is the "one descriptor
    stands for the family" choice in the two rekey walks, which breaks on
    the first carrier and otherwise takes any present member -- and the
    chosen descriptor feeds exactly one expression, old_carrier ||
    cache_carrier, whose value is the same for every carrier and the same
    for every non-carrier. The outcome is a function of the SET.

So removal comes in two flavours and THE CALLER DECLARES THE CONTRACT,
because the caller is the one that knows whether its order matters; a single
remove() that guessed would be the bug.

A spilled list gains an id -> index map, built once it passes
SPILL_INDEX_MIN (32) and empty below it, where a scan of a few entries is
one cache line and a hash probe is not. The index makes
position/contains/remove O(1) on the lists that get long, which also removes
the linear membership scan in family_push_back -- recorded at this file's
own append_unchecked as 6.2% of main-thread leaf samples, 95% of it under
that one caller.

Three tests. The guard asserts its bound as a MULTIPLE of N, so it is about
the complexity class rather than about one N: 2,000 front removals may move
at most 4N elements, where the ordered path moves N(N-1)/2 = 1,999,000.
Sabotage: point remove_unordered at remove_ordered (250x the bound); raising
SPILL_INDEX_MIN above N fails the scan half of the same test. The second
checks the index against a plain Vec oracle after front, middle and back
removals, because an index that drifts is a WRONG ANSWER -- a descriptor
that cannot be found -- not a slow one; its sabotage is dropping the fixup
for the element the swap relocated. The third pins that a short spilled list
builds no index at all.

[gc-idlist] under PERRY_GC_DIAG=1 reports removals, elements moved and
positions scanned.

NOT CLAIMED: that the memmove explains the bimodal turn CPU. On the
pre-fix binary one draw moved 335 GB and was as fast as one that moved
16 GB, so bytes moved is necessary but not sufficient for the slow mode.
What this removes is unambiguously wasted work.

NOT ADDRESSED: why one family reaches half a million descriptors. That is a
separate defect, still being measured, and will be a separate change.

Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp
@proggeramlug proggeramlug added the run-extended-tests Opt PR into compile-smoke/parity/doc-tests/drizzle-mysql-smoke label Sep 6, 2026
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: bdac300b-bcd4-4487-9525-209ae00f2870

📥 Commits

Reviewing files that changed from the base of the PR and between a681446 and f9eb62c.

📒 Files selected for processing (5)
  • changelog.d/9879-gc-family-list-swap-remove.md
  • crates/perry-runtime/src/gc/copying.rs
  • crates/perry-runtime/src/object/shapes.rs
  • crates/perry-runtime/src/object/shapes_store.rs
  • crates/perry-runtime/src/object/shapes_tests.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The change replaces linear, order-preserving family-list removal with indexed swap removal. Facts lists retain ordered removal. Tests document membership semantics, and the copying collector reports ID-list operation statistics.

Changes

Shape ID-list optimization

Layer / File(s) Summary
SpillList storage and removal operations
crates/perry-runtime/src/object/shapes_store.rs, changelog.d/9879-gc-family-list-swap-remove.md
Spilled lists now use an index above the size threshold. Ordered and unordered removal paths update the index and operation counters. Tests cover movement bounds, index consistency, and short-list behavior.
Family and facts removal integration
crates/perry-runtime/src/object/shapes.rs, crates/perry-runtime/src/object/shapes_tests.rs
Family removal uses swap removal. Facts removal preserves order. Tests compare family membership instead of element order.
GC diagnostic reporting
crates/perry-runtime/src/gc/copying.rs
The copying collector reports shape ID-list operation statistics after minor collection.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to f9eb6

The optimized removal behavior preserves the documented contracts, with no actionable merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main performance change: swap-removal for keys-array descriptor families with a spill index. The quantified impact is relevant, although the title is somewhat long.
Description check ✅ Passed The description thoroughly explains the defect, implementation, measurements, tests, scope, and current draft status. It does not follow the repository template headings or explicitly provide a relate…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

perrymaster's run of the PR tree: 3192 tests, 1 failed --
owned_key_count_versions_are_retired_behind_the_current_one, at the
post-retirement assert_eq!(test_shape_ids_for_keys(keys), vec![cached,
current]); swap-removing stale_a/stale_b moved `current` in front of
`cached`.

THE TEST OVER-SPECIFIES, and the evidence is an enumeration of every
production reader of `families` on this base, done with this test's subject
in mind:

  shapes.rs:1408  retire_owned_shape_siblings  filter all-but-keep    SET
  shapes.rs:1906  prune_dead_shape_keys_young  snapshot, remove all   SET
  shapes.rs:1946  scan_shape_table_rekey_mut   first-carrier-else-any SET
  shapes.rs:2067  move_shape_family            wholesale remove/insert  n/a
  shapes.rs:2092  relevant_shape_keys          KEYS, sort+dedup       normalised
  shapes.rs:2182  scan_shape_keys_address      as 1946                SET
  shapes.rs:2337  census heap_bytes sum                               aggregate
  shapes.rs:2390  census len/max                                      aggregate
  slot_list:402   retire all but `current`     filter                 SET
  slot_list:597   ids.replace(old, new)        position-PRESERVING    n/a
  slot_list:686   retire all but `id`          filter                 SET

The two rekey walks are the only order-TOUCHING readers, and what they take
from the list is a single choice fed to exactly one expression, old_carrier
|| cache_carrier, whose value is the same for every carrier and the same for
every non-carrier -- so the outcome is a function of the set. Nothing else
reads a position.

And the helper the assertion uses, test_shape_ids_for_keys, is #[cfg(test)]:
it renders families.as_slice().to_vec() for tests only. Its .first() sibling,
test_shape_id_for_keys, is also #[cfg(test)] and is only ever called on
families the caller has seeded with one descriptor. PerryTS#9706's contract is about
WHICH ids survive a same-address retirement, not the order they survive in;
the assertion compared against a Vec because the helper returns one.

So the post-retirement assertion becomes a sorted comparison, with the
contract and the reader enumeration written at it. The pre-retirement
assertion is left as-is -- no removal has happened there and it legitimately
documents that adds append -- with a comment saying that is a property of the
add path and not a contract.

Corroborating: the suite ran 3192 with exactly ONE failure, so no other test
in the tree asserts a family's order across a removal.
@proggeramlug
proggeramlug marked this pull request as ready for review September 6, 2026 14:15
@proggeramlug
proggeramlug marked this pull request as draft September 6, 2026 16:24
proggeramlug pushed a commit that referenced this pull request Sep 6, 2026
ID_LIST_OP_STATS is three plain u64 counts in a Cell — no address, no
NaN-boxed value — so it is not_a_gc_pointer for the holder inventory.

It was also a raw thread_local!, and its increments are NOT diagnostic-
gated: note_scan/note_removal run on every IdList scan and removal, which
is the path #9881 measures at ~3.7M removals per turn. That is the hot
case perry_thread_local! exists for, so it is converted rather than
recorded as cold debt.
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main via merge train #9901. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,944 tests, 0 failures). Thanks!

proggeramlug pushed a commit to proggeramlug/perry that referenced this pull request Sep 6, 2026
…d is measured

The symmetric half of PerryTS#9851. That commit stopped the occupancy rule concluding
"promote on first copy" -- a claim about LIFETIME derived from a measurement of
SPACE. The same formula makes the same category error at the other end:

    compute_target_survivals = 1 + desired / influx      (capped at the ceiling)

returns the ceiling for a tiny influx AND for a zero one. On the first minors of
a process -- heap nearly empty, no cohort ever followed -- occupancy therefore
claims the MAXIMUM, before a single object has been given the chance to die.
It is the expensive direction of the error, because every survivor is then
copied up to three times before it may be promoted.

Measured on the landing base (main5 + PerryTS#9881, one binary, four env arms, two
rounds of 4 turns at 3300 and 400, quiet host), this startup excursion is the
WHOLE difference between the adaptive loop and a pinned threshold:

  * unset vs pinned S=1: turn-1 CPU 3.41 s vs 3.02 s at 3300 (+0.35..0.45 s
    both rounds) and 1.05 s vs 0.72 s at 400 (+50 %), while the sum over turns
    2-4 is within noise (6.18-6.23 vs 6.35-6.41);
  * the adaptive arm's transitions are `4 -> 2 (occupancy) -> 1 (lock)` and ALL
    of them land inside turn 1; turns 2-4 run at S=1 with nothing copied.

So the adaptive policy's only cost on this workload was a startup claim it had
no evidence for, and its steady state was already the pinned one.

The rule is now symmetric: **until one survivor round has actually been rated,
the occupancy rule holds at `OCCUPANCY_MIN_SURVIVALS`.** That value is not a
tuning choice; it is the lowest threshold that PRODUCES the measurement the
rule needs in order to say anything -- at 1 nothing enters the survivor space,
at 2 exactly one cohort does. The power-on threshold becomes the same value for
the same reason: starting at the ceiling is a lifetime claim made before the
process has run.

`SURVIVOR_ROUND_MEASURED` is set the moment a cohort the previous cycle copied
becomes rateable, so the gate lifts after about two minors and the ladder is
unchanged from then on -- it delays the claim until evidence exists, it does not
remove the ladder. The two paths that MEASURE mortality are untouched: the
survival-rate lock and the sweep seed may still reach 1 whenever they have the
evidence for it.

`compute_target_survivals` is again left alone, and its test is again the proof:
the arithmetic still returns the ceiling for a zero and a tiny influx. Only what
the loop may do with that changes.

Tests. A new two-phase test: eight startup-shaped minors (tiny influx, nothing
copied) must leave the loop at the floor and out of the lock; then, once a
cohort has gone through the survivor space and been followed, the debounced rise
must still reach the ceiling. Sabotage: delete the gate, or restore the power-on
value to the ceiling, and phase 1 fails.

Two existing tests move with the power-on value and keep their properties:
`drops_immediately_and_rises_debounced` is about the ladder's ASYMMETRY, so it
now seeds a fully-dying cohort first (which rates a round without involving the
lock) and then tests the same immediate-drop / debounced-rise behaviour;
`sweep_seed_refuses_a_small_fully_live_eden` asserts the threshold is unchanged
from power-on, which is the floor now. `survival_rate_lock_breaks_a_saturated_
pipeline` needs no change -- the lock firing implies a rated round, so its
ladder recovery is unaffected.

NOT COMPILED: the box is at 7 GB free, under this campaign's 12 GB build floor,
so neither the build nor the suite has been run against this commit. The braces
balance and the reasoning above is stated per test, but that is a review and not
a check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extended-tests Opt PR into compile-smoke/parity/doc-tests/drizzle-mysql-smoke

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant