Merge train: #9881, #9894, #9896 - #9901
Merged
Merged
Conversation
added 7 commits
September 6, 2026 17:56
(cherry picked from commit 40cd9c6)
(cherry picked from commit 77ddaee)
(cherry picked from commit 2a79769)
(cherry picked from commit 49a4f99)
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
(cherry picked from commit 01313fc)
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. #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. (cherry picked from commit f9eb62c)
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change optimizes descriptor-family removal, adds receiver validation for ChangesDescriptor family list optimization
Performance method receiver validation
Constructor-lowering artifact publication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NativeModule
participant PerformancePrototype
participant PerformanceThunk
Caller->>NativeModule: read performance method
NativeModule->>PerformancePrototype: resolve prototype method
PerformancePrototype-->>Caller: return receiver-aware thunk
Caller->>PerformanceThunk: invoke method
PerformanceThunk->>PerformanceThunk: validate Performance receiver
✨ 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 |
This was referenced Sep 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge train: #9881, #9894, #9896.
Audit note on #9881
IdList::removebecomes a swap-remove, which reorders. The PR's own full-suite run failed exactly one test —owned_key_count_versions_are_retired_behind_the_current_one, which asserted the family's ORDER after a retirement — and the PR converts it to a membership assertion.That conversion is the kind of change that can launder a real regression, so the claim was checked rather than taken on trust. Two of the enumerated readers,
scan_shape_table_rekey_mutandscan_shape_keys_address, are described as "first carrier else any member", which reads as order-sensitive.It holds up, and the code is written so that it does.
scan_shape_keys_addressscans the family andbreaks on the first carrier, so the chosen descriptor is a carrier iff any member is one — an existential property, not a positional one. That choice feeds exactly one decision, strong (visit_usize_slot) versus weak (visit_metadata_usize_slot) root visiting, which therefore does not depend on which member was picked. Every subsequent write applies to all ids in the family, and the rewritten address derives from the family key rather than from the chosen member. The order really was free.Gate fixes
#9881's new
ID_LIST_OP_STATStripped two gates:u64counts in aCell, no address and no NaN-boxed value, sonot_a_gc_pointer.thread_local!. The gate offers two remedies, and recording it as cold debt would have been the shorter one, but it would also have been false:note_scan/note_removalare not diagnostic-gated and run on everyIdListscan and removal, which is the path this PR measures at ~3.7M removals per turn. Converted tocrate::perry_thread_local!instead, so the PR that removes hot-path cost does not add an unconditional_tlv_get_addrto it.Worth a maintainer's eye, not changed here: those counters are unconditional overhead on the hot path, cheap next to the
memmovebeing removed but never switched off.Validation
run_lint_gates: all 64 gates passed; 2 CI-only skippedSummary by CodeRabbit
Bug Fixes
performanceobject.Performance