perf(intl): one shared shape for Intl.Segmenter records — removes the program's largest allocation category - #9769
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 (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesIntl.Segmenter shared record shapes
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Intl.Segmenter records now reuse per-thread shared key arrays to reduce allocation overhead, with GC handling for cached arrays covered by runtime-root tests. No merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant IntlSegmenter
participant SegmentRecordCache
participant GarbageCollector
IntlSegmenter->>SegmentRecordCache: build or reuse shared keys array
SegmentRecordCache-->>IntlSegmenter: create indexed segment record
GarbageCollector->>SegmentRecordCache: scan cached keys arrays
SegmentRecordCache-->>GarbageCollector: mark or rewrite array references
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Marking draft: the rationale in the description is falsified by our own measurement. This was proposed to remove inline-cache misses by giving every What remains true is the allocation: 175,797 records per 400-char reply each minting throwaway key strings and cloning the keys array. That is worth having, but it should be reviewed and titled as an allocation fix, not as an IC fix. Leaving it draft until it is either re-scoped that way or closed. |
proggeramlug
changed the title
…ne per record `make_segment_record` built its result property-by-property with `set_field`, which allocates a fresh `StringHeader` for the key name on every call and routes through `js_object_set_field_by_name`, which clones the object's key list before writing. Every record therefore got 3-4 throwaway key strings, a keys array cloned and regrown once per property, and — because `shape_id_for_keys_ensure` keys the shape table on the keys array's ADDRESS — its own ShapeId. A fresh ShapeId per record makes every read of `.segment` / `.index` / `.input` a guaranteed inline-cache miss (the PIC token is the ShapeId) and adds one descriptor to the shape table per record. This is the defect PerryTS#7564 fixed for `{ value, done }` iterator results; `Intl.Segmenter` was not covered, and grapheme-aware text measurement (`string-width`, and so every terminal UI built on ink) segments every string it renders. One 400-character reply in the compiled claude-code TUI produces 175,797 segment records; `PERRY_IC_DIAG` attributes 175,797 of that turn's 2,589,696 IC misses to the `.segment` read site alone. * `SEGMENT_RECORD_KEYS`: a per-thread `GC_FLAG_SHAPE_SHARED` keys array for each of the two record shapes (`isWordLike` is attached only for word granularity, ECMA-402 18.5.1), built at most twice per thread with interned names so they are pointer-identical to the ones the read side hashes. * `make_segment_record` installs the shared array with `js_object_set_keys` and writes fields by index: one allocation instead of five to eight, and one ShapeId for every segment record in the program. * `scan_segment_record_keys_roots_mut`, registered beside the iterator-result scanner: nothing else references these arrays, and an evacuating collection moves them like any other array. Rooting follows `build_iter_result_ordered`: the caller's two heap values are rooted first, the keys cache is filled before the record is allocated, and every pointer used after an allocation is re-read from storage the collector rewrites. Tests: the five root-scanner tests `iter_result_keys.rs` holds, mirrored for this cache (mark, rewrite, empty-cache no-op, registration, build-once). Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
131fa9e to
7eb183c
Compare
|
Rebased onto current Re-scoping summary for reviewers: the IC rationale this PR was opened with was tested and did not hold — I have left the falsification in the body rather than quietly deleting it, and the real cause of those misses turned out to be unrelated to shapes (#9802: cc's module is full-outlined, so there was no inline property-read cache to hit at all). What stands is the allocation half, which the gc-churn lane's allocation-site histogram independently makes the largest single allocation category in the program: 43.5 % of the streamed turn, sourced to exactly these per-segment records. |
|
Landed on |
|
Attribution evidence for this PR's allocation half, from the gc-churn lane, with I ran the compiled claude-code TUI (
The correction: the alloc-site sampler labels those same allocations Why this raises the PR's ceiling. My lane established a threshold the hard Artefacts are in this session's scratchpad ( Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
1 participant
What
Intl.Segmenterbuilds its segment records property-by-property:set_fieldallocates a freshStringHeaderfor the key name on every call(
js_string_from_bytes(key)) and then goes throughjs_object_set_field_by_name,which clones the object's key list before writing — the copy-on-write
contract shared keys arrays depend on. So every record costs 3-4 fresh key
strings that carry no information (the same bytes every time), a keys array
cloned and regrown once per property, and one more descriptor in the shape
table.
Grapheme-aware text measurement —
string-width, and therefore every terminalUI built on ink — segments every string it renders. On the compiled claude-code
TUI, one 400-character assistant reply produces 175,797 segment records.
This is the defect #7564 fixed for
{ value, done }iterator results. It wasnever applied to
Intl.Segmenter.Why it matters: it is the largest allocation category in the program
Independently measured by the gc-churn lane's allocation-site histogram, on its
own candidate (current
mainplus its two PRs, no pacing changes), attributingevery allocation in one streamed turn:
That lane names
Intl.Segmenter's per-segment record as the source of the topcategory — i.e. exactly the allocations this PR removes. Two lanes reached the
same site from opposite directions (an allocation histogram and an IC-miss
table), which is the main reason to trust it.
Campaign directive A3 is "the program allocates ~650 MB to produce 400
characters of output"; this is the largest single named contributor to it.
The change
The same construction
iter_result.rsuses, for the two shapes a segment recordcan have (
isWordLikeis attached only for word granularity, ECMA-402 18.5.1):SEGMENT_RECORD_KEYS— a per-thread,GC_FLAG_SHAPE_SHAREDkeys array pershape, built at most twice per thread, with interned names so they are
pointer-identical to the ones the read side hashes.
make_segment_recordinstalls the shared array withjs_object_set_keysandwrites the fields by index — one allocation (the record) instead of five to
eight, and one ShapeId for every segment record in the program.
scan_segment_record_keys_roots_mut, registered next to the iterator-resultscanner in
gc/mod.rs: nothing else in the heap references these arrays, andan evacuating collection moves them like any other array.
Rooting follows the same rule as
build_iter_result_ordered: the caller's twoheap values are rooted first, the keys cache is filled before the record is
allocated, and every pointer used after an allocation is re-read from storage
the collector rewrites (the handle, or the scanned thread-local).
Measured
The allocation half ran, and is real. Main-thread leaf samples, 400-character
streamed reply, candidate
cc_ks4against the same branch without this commit:cc_ks2)cc_ks4)js_string_from_bytes(the per-record key names)The bundle really does construct
new Intl.Segmenter(void 0,{granularity:"grapheme"})and…{granularity:"word"},so the code is on the path, and the throwaway key strings and keys-array clones
are gone.
End-to-end CPU on that candidate was flat (6.98 s vs 7.09 s turn CPU, 658 vs
659 MB peak RSS, node 0.29 s / 375 MB) — within the run-to-run spread. The claim
here is allocation volume, corroborated by the histogram above, not turn CPU on
one measurement.
Correction: the inline-cache rationale this PR was opened with was WRONG
The original description claimed a fresh keys array per record means a fresh
ShapeId, hence a guaranteed inline-cache miss on
.segment, hence a slice ofthe 2.5 M IC misses per turn. I tested that and it does not hold:
cc_ks2cc_ks4.segment.valueUnchanged. The falsifier was already in my own data —
.value/.doneareread off iterator results whose keys array #7564 already shares, and they miss
~178k times per turn anyway.
The real cause has since been found and is unrelated to shapes: cc's module is
past the full-outline threshold (#5391 path 3), so every generic property read
lowers to one
js_object_get_field_iccall, and that helper had no fast pathat all — it called the miss handler unconditionally on every read.
nm -uonthe compiled object shows
js_object_get_field_ic_missis not referenced by ccat all: there was never an inline cache to hit. The prime split confirms it —
95.2 % of all primes re-write the token the site already held.
So: none of the 2.5 M were shape-instability misses, and this PR was never going
to move them. It is judged here purely as an allocation fix.
Tests
The five root-scanner tests pass (mark, rewrite, empty-cache no-op,
registration, build-once-per-shape). They are what makes the shared keys array
safe under an evacuating collection and should be kept whatever else changes.
Summary by CodeRabbit
Performance
Intl.Segmenterperformance by enabling segment records to share common internal metadata, reducing repeated allocation and lookup overhead.Bug Fixes
Tests