perf(shapes): append a freshly allocated descriptor id without the family membership scan - #9768
Closed
proggeramlug wants to merge 1 commit into
Closed
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 (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe runtime adds unchecked ChangesShape descriptor interning
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change removes redundant membership scans when publishing newly allocated shape descriptors while preserving checked handling for existing IDs. The covered ordering and reuse behavior indicates no remaining merge-blocking 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 |
…mily membership scan `shape_descriptor_ensure_with_holes` ends with `facts_push_back` + `family_push_back`, and both answer "is this id already in the list?" with a linear scan. A family holds every descriptor ever created for one keys array, so interning the n-th descriptor for a keys array cost O(n) and a render loop that keeps bumping a shape's semantic generation paid quadratic time in that history. On the compiled claude-code TUI `IdList::contains` was 6.2 % of main-thread leaf samples during a streamed reply and 5.9 % in the window after it, 95 % of it under `ShapeTableInner::family_push_back`. The scan is dead work at those sites: `alloc_shape_id` handed the id out two statements earlier and never reuses a value (it parks at `SHAPE_ID_END` rather than wrapping), so an id allocated after a list was built cannot be in it. * `IdList::append_unchecked`, with the invariant that licenses it. * `ShapeTableInner::family_append_fresh` / `facts_append_fresh` use it; the two sites that append a just-allocated id call those. The metadata rekey, which re-files EXISTING ids under a moved keys address, keeps `push_back`. Test: interning_appends_each_new_descriptor_to_the_family_exactly_once. Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
proggeramlug
force-pushed
the
perf/keystroke-shape-family-append
branch
from
September 5, 2026 08:56
fd03784 to
c8b9449
Compare
Contributor
Author
Contributor
Author
|
Landed on |
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Sep 5, 2026
Rebasing onto main brings in PerryTS#9768's `family_append_fresh`, the append that skips `IdList`'s membership scan for a freshly allocated id. It is the append `shape_descriptor_intern` uses, and it did not exist when this branch added rule-1 arming to `family_push_back` / `family_push_front`, so the rebase merges clean and silently drops the note for every freshly interned descriptor. `keys` is the canonical keys array's ADDRESS and the minor-scoped rekey scanner visits only logged keys, so an unlogged family is invisible to a copying minor: the keys array moves, the family stays filed under the old address, and the descriptor is lost. Both intents kept — the membership scan stays gone, the note comes back. Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
proggeramlug
pushed a commit
that referenced
this pull request
Sep 5, 2026
Rebasing onto main brings in #9768's `family_append_fresh`, the append that skips `IdList`'s membership scan for a freshly allocated id. It is the append `shape_descriptor_intern` uses, and it did not exist when this branch added rule-1 arming to `family_push_back` / `family_push_front`, so the rebase merges clean and silently drops the note for every freshly interned descriptor. `keys` is the canonical keys array's ADDRESS and the minor-scoped rekey scanner visits only logged keys, so an unlogged family is invisible to a copying minor: the keys array moves, the family stays filed under the old address, and the descriptor is lost. Both intents kept — the membership scan stays gone, the note comes back. Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
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.
What
shape_descriptor_ensure_with_holes— the interning funnel every object shapegoes through — ends with
and both helpers answer "is this id already in the list?" with a linear scan
(
IdList::contains). The family under a keys array holds every descriptor evercreated for that keys array (that is what it is for: the shapes a keys array has
had). So interning the n-th descriptor for a keys array cost O(n), and a
program that keeps bumping a shape's semantic generation — a TUI re-rendering the
same component tree — pays quadratic time in the history of each keys array.
The scan is dead work at both sites:
idwas handed out byalloc_shape_idtwostatements earlier, and that allocator is a strictly increasing counter that
never reuses a value (on exhaustion it parks at
SHAPE_ID_ENDrather thanwrapping, precisely so an id can never alias). An id allocated after a list was
built cannot be in it — in that family or any other.
The change
IdList::append_unchecked— append without the membership scan, documentedwith the invariant that licenses it.
ShapeTableInner::family_append_fresh/facts_append_freshuse it, and thetwo sites that append a just-allocated id (
shape_descriptor_ensure_with_holesand the id move in
shapes_slot_list) call those.push_back: the metadata rekey that re-filesexisting ids under a moved keys address can legitimately meet an id the
destination list already holds.
What is claimed
This is a complexity fix, not a CPU win. Interning a shape descriptor is now
O(1) in the number of descriptors the keys array has ever had, instead of O(n) —
a cost that grows with process lifetime and therefore does not show its true size
in any short measurement. That is the whole claim.
Proof that the scan is gone (macOS
sample, main thread, leaf samples, 400-charstreamed reply on the claude-code TUI,
cc_ks2= the same branch without thiscommit):
cc_ks2)cc_ks4)IdList::containsfamily_push_back200x fewer samples.
shape_descriptor_ensureitself stays at 0.36 %, i.e.interning still happens, it just no longer walks the family. On
mainthe samesymbol reads 95 samples (0.79 %) in that window — it grows as the shape table
does, which is exactly the point.
End to end it is flat, and I am explicitly not claiming otherwise. Same
session, same
measure_lock.sh, 400-character reply(
stream_scale.py --mem --idle 12):cc_ks2(without this PR)cc_ks4(with this PR + #9769)0.11 s on a 7 s turn is inside the run-to-run spread. It is not a CPU win and
must not be counted as one. (A 3300-character arm of the same candidate came in
at 37.95 s against 51.8 s previously recorded for
cc_ks2— the direction thispredicts, longer run ⇒ longer families ⇒ more scan removed — but the two numbers
are from different sessions and I am not treating that gap as measured either.)
Correctness
New test
interning_appends_each_new_descriptor_to_the_family_exactly_oncepinsthe observable consequence — six distinct descriptors for one keys array appear
in its family exactly once each, in birth order, and re-interning the same facts
reuses the existing id and appends nothing — so a later change that routes a
recycled id through the fresh path fails here instead of silently duplicating a
family entry. Verified running, not merely present.