perf(gc): borrow the shape record in the slot visitor, stop moving the iterator (#10362) - #10371
Closed
proggeramlug wants to merge 2 commits into
Closed
proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
added 2 commits
September 16, 2026 13:11
…e iterator (PerryTS#10362) Base: 33690c5 (main, includes the PerryTS#10348 fix from PerryTS#10352). gc_child_slots built a 152-byte HeapChildSlotIterator for every traced object, 40 bytes of it a lifted ShapeDescriptor (PerryTS#8122). Two by-value moves of that iterator compiled to out-of-line 152-byte memcpy calls inside visit_gc_layout_slot_descriptors: * the Array and Closure arms' `Option::map(..).unwrap_or_else(..)` temporary, copied into the caller's slot (62% of the calls); * `for .. in child_slots` in the Masked arm, which moved the iterator into the loop (35%). On the PerryTS#10362 retained-graph fixture that was 6,181,945 memcpy calls of exactly 152 bytes (counted with an LD_PRELOAD shim). After this change: 6. * object/shapes.rs: ShapeRecordRef, a Copy handle to one live slab record (live bound, keys word, keys slot). shape_record_by_id and object_shape_record make the same slab probe as the descriptor lookups, without lifting a copy. note_old_generation_carrier and note_full_trace_carrier take the handle. ShapeDescriptor::record_ref() adapts the one caller that holds a descriptor. keys_slot() is now used only by tests, so it is cfg(test). * object/gc_slots.rs: gc_shape_keys_edge_slot and gc_field_slot_range take the handle. * gc/layout.rs: the iterator carries Option<ShapeRecordRef> (8 bytes; the iterator is now 120). The `_from` mask helpers take the handle. gc_child_slots' Array and Closure arms build the iterator directly in the return slot with let-else, as the ObjectFields arm already did. * gc/layout_slot_visit.rs: the Masked arm iterates `&mut child_slots`. Still one shape-table probe per receiver (PerryTS#8122). The handle is read at the same points the lifted copy was: the carrier notes and the keys edge, before any visit. It relies on the same record-address validity the carrier notes already write through (PerryTS#9706). The PerryTS#8112 old_carrier/ephemeron gate and the PerryTS#9726 full-trace note are unchanged. instructions:u, min of 5, same host, base (main 33690c5) vs this: gc3 12,572,092,967 -> 12,275,653,370 -2.36% w1000 1,090,107,336 -> 1,081,645,123 -0.78% w5000 2,049,860,743 -> 2,018,013,960 -1.55% w20000 5,094,775,666 -> 4,994,573,033 -1.97% oldyoung 1,525,484,973 -> 1,503,383,712 -1.45% alloc-only 320,266,130 -> 320,266,017 0.00% All outputs match node byte for byte. PERRY_GC_FROMSPACE_SCAN_ABORT=1 is clean on all six fixtures, and it aborts on the same gc3 built at fcd108b. PERRY_GC_VERIFY_EVACUATION=1 on gc3 exits 0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe GC slot visitor now uses ChangesShape record GC flow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant GC
participant gc_child_slots
participant ShapeTable
participant HeapChildSlotIterator
GC->>gc_child_slots: enumerate object child slots
gc_child_slots->>ShapeTable: resolve object_shape_record once
ShapeTable-->>gc_child_slots: return ShapeRecordRef
gc_child_slots->>HeapChildSlotIterator: construct iterator directly
HeapChildSlotIterator-->>GC: yield child slots without iterator copy
Merge Risk: ⚪ Minimal · up to No confirmed correctness or safety issue remains; the optimization is ready to merge. 🚥 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 |
This was referenced Sep 16, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 16, 2026
Contributor
Author
|
Landed via merge train #10393 (v0.5.1582). All source commits preserve authorship; merged main matches the validated train exactly. |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 17, 2026
Base: 6c9e2a6 (PR #10371's head), which this stacks on. `layout_transfer` runs for every evacuated object on every copying minor, both old-generation evacuations and `js_array_grow`. All four callers copy the source header's `_reserved` into the destination first — the minor through `reserved_with_copied_survival_age`, which rewrites only the age bits — so every layout fact a header carries has already arrived: the layout state, `GC_LAYOUT_ALL_POINTERS`, the raw-f64 / holes flags, `GC_ARRAY_ELEMENT_SHAPE` and `GC_OBJ_TYPED_LAYOUT_INTACT`. The funnel re-derived that half anyway, per object: two header classifications, a rewrite of bits that were already equal, an out-of-line call per array and per object, the #7510 flag-and-filter gate evaluated twice, and — for every intact object — a ShapeId-keyed `SHAPE_LAYOUTS` probe whose answer a relocation cannot change. Measured with gdb `stepi` on the #10362 retained-graph fixture: 160 instructions per moved array, 245 per moved object, 518M in total (4.2% of the run), none of which reached a side-table record. Both per-object maps held one key (`PERRY_LAYOUT_DIAG`). The contract the callers always satisfied is now the funnel's stated contract, asserted in test and debug builds, and the funnel moves only what a header cannot carry: the element-shape record (#7480), the residual static-prototype owner registry (#9304), and the per-object `TYPED_LAYOUTS` / `LAYOUT_SLOT_MASKS` entries (#7510). Each is gated inline by the bit or latch that governs it, and the record moves themselves live in a `#[cold]` slow path. * `gc/layout/transfer.rs` (new): the funnel, its gates and the contract assertion. `gc/layout.rs` drops to 1919 lines, off the 2000-line cap. * `gc/layout_tables.rs`: `per_object_layouts_may_hold_either` answers the #7510 gate for both maps and both addresses in one hot-slot resolution. * `object/prototype_chain.rs`: the registry latch is readable without the call. * `array/header.rs`, `array/element_shape.rs`: `transfer_array_numeric_layout` and `clear_element_shape_ptr` were only ever called by the header half of the funnel and are deleted. That also drops a spurious `invalidate_representation_change` a hole-tolerant array took on every move (the transfer cleared the flag it was about to set again); the counter feeds `PERRY_TYPED_FEEDBACK_TRACE` output only. Behaviour: one change, the lazy intact downgrade. The bit is a fact of the object and of two tables a move does not touch, so re-asking at move time could only downgrade objects that happen to move. The state it cleared — intact while no descriptor is reachable — is legal and handled: `shape_install_shared` poisons a shape's shared entry and leaves "any still-INTACT siblings" to fall back, #8115 clears the bit at the first contradicting store, the trace falls back to `GC_LAYOUT_UNKNOWN` and scans every slot, and the query helpers answer "no descriptor". An unmoved sibling keeps its bit today, so nothing could have depended on the move clearing it. `test_poisoned_shape_intact_and_per_object_record_survive_a_copying_minor` builds that exact state and drives it through a real copying minor: the moved receiver keeps the bit, answers every query as it did before the move, and its child survives and is rewritten, while the sibling that poisoned the shape keeps its per-object record across the move. It fails on the parent commit (the old funnel clears the bit) and fails again under a sabotaged funnel that does not move the per-object records. `test_layout_transfer_requires_the_relocation_header_copy` pins the contract. instructions:u, min of 5, base 6c9e2a6 vs this: gc3 12,275,577,365 -> 11,880,548,143 -3.22% w1000 1,081,647,372 -> 1,054,559,901 -2.50% w5000 2,018,014,139 -> 1,919,532,121 -4.88% w20000 4,994,632,943 -> 4,797,474,484 -3.95% oldyoung 1,503,368,405 -> 1,471,395,073 -2.13% alloc-only 320,266,214 -> 320,265,756 0.00% That is 154 instructions per relocation on gc3 (2,560,042 relocations on both arms, counted with uprobes), and 71% of the ceiling a full knockout of the funnel measured. The cold path is entered 1,213 times, 0.05% of relocations.
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.
Part of #10362 (piece 1 of the per-collection cost).
Problem
visit_gc_layout_slot_descriptorsbuilds aHeapChildSlotIterator(152 bytes) for every traced object viagc_child_slots. 40 bytes of it are aShapeDescriptorlifted out of the shape table (#8122). Twoby-value moves of that iterator compile to out-of-line
memcpy(…, 152)calls:gc_child_slotsreturn throughOption::map(..).unwrap_or_else(..),and the temporary is copied into the caller (62% of the memmove samples);
for child_slot in child_slotsin the Masked arm moves the iterator into the loop (35%).On gc3 (the perf(gc): on a retained object graph perry is 8.5× node — 13 minors at ~0.8G instructions each (measured on #10352) #10362 retained-graph fixture) that is 6,181,945 memcpy calls of exactly 152 bytes (LD_PRELOAD counter).
Fix
object/shapes.rs: newShapeRecordRef, aCopyhandle to a live slab record (live inline-slotbound,
keysword,keysslot).shape_record_by_idandobject_shape_recordmake the same slabprobe as the descriptor lookups, without lifting a copy.
note_old_generation_carrierandnote_full_trace_carriertake the handle.ShapeDescriptor::keys_slotis now used only by tests, so it iscfg(test).object/gc_slots.rs:gc_shape_keys_edge_slotandgc_field_slot_rangetake the handle.gc/layout.rs: the iterator carriesOption<ShapeRecordRef>(8 bytes; iterator 152 → 120 bytes), and so do the_fromshared-mask helpers. The Array/Closure arms build the iterator in place withlet-else.gc/layout_slot_visit.rs: the Masked arm iterates&mut child_slots.Why it is sound
visit callback runs. Nothing between resolution and use writes the record's
keysword.note_old_generation_carrieralready relied on throughShapeDescriptor::record: slabrecords never move (Shape descriptors cost 32 MB for 85,288 entries; V8 does the same job in 3.58 MB for 39,703 maps — ~9x per-descriptor overhead #9706), and a chunk is released only by
shrink_shape_tablesat the end of a major collection.old_carrier/ ephemeron gate and the Shape descriptors no live object carries are never pruned (35k of 43.7k on claude-code): retire from a full-trace carrier note #9726 full-trace carrier note are unchanged in placement and condition.Numbers (instructions:u, min of 5, same host; base = main 33690c5)
instructions:uprofile showed memmove at ~14%. That event skids on this CPU. Undercycles:ppGates
PERRY_GC_FROMSPACE_SCAN_ABORT=1exits 0 on all six, with scan lines on every cycle. Positive control: gc3 and w1000built at fcd108b (pre-fix(hir): a null-typed field is not a proof the GC may skip the slot (#10348) #10352) abort with "1 dangling reference(s)", so the gate can fail.
cargo test --release -p perry --test gc_record_null_typed_field_10348passes.cargo test --release -p perry-runtime -- --test-threads=1: same result on base and fix (3952 pass, 1 fail, 4 ignored).The failure is the same pre-existing test on both arms (
heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds).cargo fmt --checkclean. The clippy warning set is identical to base (879). File-size check OK (gc/layout.rs = 2000).PERRY_GC_VERIFY_EVACUATION=1exits 0 on this PR, but also on the known-bad pre-fix(hir): a null-typed field is not a proof the GC may skip the slot (#10348) #10352 gc3.Changelog
changelog.d/10371-gc-slot-iterator-memcpy.md(### Performance).Summary by CodeRabbit