perf(gc): stop re-deriving compile-time mask facts on every construction (#7578) - #7586
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTyped shape layout initialization now passes raw mask pointers and word counts to shape-install memoization. The memo stores pointer-mask emptiness in packed dimensions. Memo hits restore the correct header layout state without rebuilding a descriptor. ChangesTyped shape layout installation
| Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FFIEntry
participant LayoutInitializer
participant ShapeInstallMemo
participant LayoutHeader
FFIEntry->>LayoutInitializer: pass raw masks and word counts
LayoutInitializer->>ShapeInstallMemo: probe raw masks and dimensions
ShapeInstallMemo-->>LayoutInitializer: pointer-mask emptiness or miss
LayoutInitializer->>LayoutHeader: restore layout state on hit
LayoutInitializer->>ShapeInstallMemo: record masks and emptiness on slow path
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
0df86c7 to
5e9266d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/gc/layout.rs`:
- Around line 978-983: Update the caller that processes raw mask pairs before
invoking mask_words: when either non-zero mask count has a null pointer, call
layout_set_typed_unknown and return. Keep mask_words for valid pairs, including
zero-count masks, and preserve existing handling for valid pointer masks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7a17cb5-c612-48ee-b2af-c4c083ec872c
📒 Files selected for processing (2)
crates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/shape_install.rs
5e9266d to
6039ec0
Compare
…ion (#7578) `gc::layout::typed_shape_layout_entry` re-measured at 22.7% of `push_cls` self time on the pinned quiet host, plus 4.1% in `layout_forget_object` — 26.7% for a steady-state path whose content is two header bit writes. #7578's hypothesis (the FFI call frame, per #7566) was tested and is false: outlining the install cut the prologue from a 336-byte frame with twelve callee-saved spills to 80 bytes with none, and made `push_cls` 0.72 -> 0.75 s and `churn_alloc` 0.72 -> 0.79 s. The function is bound by instruction count. Roughly 30 of the ~70 instructions on the hit path re-derived compile-time constants of the class, per call, because the FFI boundary makes them opaque: twelve normalising two (pointer, length) pairs into slices only compared as integers, ~11 of `words_intersect` setup over two immutable globals, and ~6 computing `gc_type_layout_slot_kind` a second time. Carry the raw pairs and build a slice only where one is indexed; move the disjointness check below the memo probe (an intersecting shape is downgraded before it can reach `record`, so no intersecting tuple can be in the table to hit); store the pointer-mask-empty bit in `dims` bit 62; compute the slot kind once. push_cls 1.091x, churn_alloc 1.075x, churn 1.042x, deeplist 0.993x, rest flat. +0 bytes: no codegen crate is touched, so emitted IR is unchanged. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
6039ec0 to
b29ab0f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Audit before merge — verified, merged as v0.5.1332I proposed the remedy this PR refutes, so the refutation is the most valuable Verified the soundness argument directly, since it is the load-bearing The detail that makes this worth writing down rather than just fixing: the (One citation slip: the guard is Sabotage-verified myself: dropping the pointer-mask-empty bit from Gates re-run here: The +0-bytes and root-dominance claims hold structurally, not just On the negative result — outlining cutting the frame 336 → 80 bytes and
|
1 participant
Closes the measurement half of #7578 and lands the fix.
Re-measured first, and it had not collapsed
Three tickets in this campaign were worked from a headline that was already
stale, so this started with a fresh leaf profile on the pinned quiet host
(
perry-macos.local, load < 2), not with code.gc::layout::typed_shape_layout_entryreads 22.7% ofpush_clsself time —still the largest single symbol in object construction — with
layout_forget_objecta further 4.1%. 26.7% between them, for asteady-state path whose entire content is two header bit writes.
The hypothesis in the ticket is false, and I tested it rather than assuming
#7578 proposed the cost is the FFI call and its thread-local resolutions, and
the remedy is to emit the hit path inline at the
newsite. The prologue lookedlike it agreed —
sub sp, sp, #0x150and sixstppairs, a 336-byte frame andtwelve callee-saved registers, sized by LLVM for a descriptor-build path that
runs once per shape.
I built that fix. Outlining the install behind
#[cold] #[inline(never)]cutthe frame to 80 bytes and the twelve spills to zero, and made the benchmark
slower:
push_cls0.72 → 0.75 s,churn_alloc0.72 → 0.79 s, reproducedacross two runs against a
mainthat reproduced to the millisecond three times.On this core those spills are cheap dual-issued stores off the critical path;
removing them bought nothing, while keeping six arguments live to forward to the
outlined call cost a dozen register moves that were not there before. This
function is bound by instruction count, not frame size, which is why #7566's
result does not transfer.
Where the 22.7% actually goes
Counted off the disassembly: roughly 30 of the ~70 instructions on the hit path
re-derive compile-time constants of the class, per call, because the FFI
boundary makes them opaque parameters — 12 normalising two
(pointer, length)pairs into slices the hit path only compares as integers, ~11 of
words_intersectsetup over two immutable globals, ~6 computinggc_type_layout_slot_kinda second time.So: carry the raw pairs and materialise a slice only where one is indexed; move
the disjointness check below the memo probe (a hit proves an install already ran
it over the same globals — an intersecting shape is downgraded before it can
reach
record, so no intersecting tuple can be in the table to hit); store thepointer-mask-empty bit in the memo; and compute the slot kind once.
Why replaying two mask predicates is not what the memo's soundness bar
forbids. That bar is about the object —
field_count == slot_countand theper-slot validation stay per-instance, because an object's contents change under
the mutator. These two read only the mask globals, which are codegen-emitted
private unnamed_addr constants: read-only image, never written, never freed.An entry matches on their addresses and lengths, so a matching address is a
matching byte string for the life of the process. The residual failure mode is
still a miss, never a wrong hit.
Results
push_clschurn_allocchurncyclesretaintreedeeplistBest-of-7 wall clock, two independent runs per arm,
mainre-measured betweenthem, and the set re-confirmed after rebasing onto
main's newer head.deeplistpays 0.7–1.3%, reproducibly across three runs — its nodes havepointer fields so it takes the validating entry point, whose per-slot loop now
builds its slices inside its own branch rather than finding them hoisted.
The leaf profile moves the way the mechanism predicts: 26.7% of 710 ms
becomes 20.4% of 650 ms — 190 ms → 132 ms against a 60 ms wall-clock
improvement.
Binary size: +0 bytes, structurally. The diff is two files, both in
perry-runtime; no codegen crate is touched, so emitted IR is unchanged byconstruction. Measured both ways anyway: all seven benchmark binaries are
byte-for-byte the same size as
main's (12,222,200 each), and thesymbol-carrying build is 320 bytes smaller.
The codegen remedy the ticket proposed is unsound — recorded so it is not revisited
It looks free:
declare-path classes must have an empty pointer mask, so theirdeclared state is byte-identical to what the allocator writes, and since #7566 a
newin a loop writes itsGcHeaderas one i64 constant — OR-ingGC_OBJ_TYPED_LAYOUT_INTACTin would cost +0 instructions and +0 bytes.It is also a use-after-free factory. Setting the bit without an installed
descriptor breaks "intact ⟹ some descriptor is reachable", and
layout_note_slothas a hole only that invariant closes: on a contradicting store to an
intact-but-descriptor-less object it resolves a
Noneverdict, falls through toordinary pointer-mask bookkeeping, moves the object to
SIDE_MASK— and neverclears the intact bit, because
layout_set_typed_unknownis reached only fromthe
Some(verdict)arm. The object is then simultaneouslySIDE_MASK(collector:slot K holds a live pointer) and intact (the codegen-inlined guard in
expr/class_field_inline_guard.rs, which consults no map by design: slot K israw-f64). The guard passes,
property_set.rs's raw-store fast path writes adouble over the pointer with no write barrier and no layout note, and the
next collection walks slot K as a heap pointer.
layout_transferre-derives thebit correctly, but only for objects actually evacuated, so the window is the
object's lifetime.
Validation
cargo test -p perry-runtime(1820) and-p perry-codegen --lib(672) green;check_file_size.sh,addr_class_inventory.py,raw_handle_debt.py(998,unchanged),
cargo fmt --all -- --checkclean;gc_root_dominance_check.py--self-testplus both gated modes over a freshly emitted corpus with--seeded-violations 40.Two new tests, sabotage-verified rather than merely run:
the_pointer_mask_empty_bit_round_trips_per_entry— dropping the bit frompack_dimsturns it red; makinghitreturn a constantSome(true)turns itred and takes down
memo_installed_objects_survive_a_copying_minor_with_their_children(the GCwitness) and
a_memo_hit_produces_the_same_header_state_as_the_install.packed_dims_fields_do_not_overlap_the_empty_bit— the word-count fieldsnarrowed 20 → 19 bits for bit 62; widening one back turns it red. An overlap
would make a wide-mask shape read back as
POINTER_FREEand the collectorwould skip live pointer slots.
The existing
a_contradicting_field_is_refused_even_with_the_memo_warmearnedits keep: an earlier draft probed the memo before the per-slot validation and
that test caught it on the counter assertion its message names. The probe now
sits after validation, exactly where it was.
CI has a deep backlog and may not report on this branch; everything above is
local validation on the pinned host.
https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
Summary by CodeRabbit
Performance
Bug Fixes
Documentation
Chores