Skip to content

perf(class-fields): a conforming pointer store skips the layout note (#5094) - #7698

Open
proggeramlug wants to merge 2 commits into
mainfrom
perf/7691-p3-conforming-note
Open

perf(class-fields): a conforming pointer store skips the layout note (#5094)#7698
proggeramlug wants to merge 2 commits into
mainfrom
perf/7691-p3-conforming-note

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

The P3 investigation resolved the ticket's contradiction as "already fixed"

P3 (gc-handoff/PROMPT-P3-class-canonical-layout.md) opens with a contradiction
to adjudicate before writing any fix: profiles showing 25–29% of class-heavy
benches in per-object layout side tables, against a shape_install_shared path
that is supposed to store one descriptor per shape with no per-object insert.
Two hypotheses were named. Both are false, and the ticket's own acceptance
criteria 1 and 2 are already satisfied on main — its table was measured at
a853135aa, before P1 (#7686).

Counters from an instrumented runtime, tree.ts:

INIT_CALLS        20447192     NOTE_CALLS          20447156
INIT_KEYS_ZERO           0  ←  NOTE_CONFORMS       20447154
INIT_MEMO_HIT     20447192     NOTE_DOWNGRADE             0  ←
INIT_SHARED_OK           1     FORGET_CALLS        64000001
INIT_SHARED_FAIL         0     FORGET_SLOW                1  ←
residency: slot_masks=0  typed_layouts=0  per_object_flag_armed=false
  • H1 — the keys_array is attached after the layout declaration. False.
    INIT_KEYS_ZERO = 0; every instance reaches shape_install_shared keyed.
  • H2 — a.peer = b demotes the descriptor. False. NOTE_DOWNGRADE = 0.
  • Criterion 2 (residency ~0) already holds. Both per-object maps are empty,
    the emptiness flag never arms, and layout_forget_object took its slow path
    once in 64 million calls.

What was actually left, and is fixed here

20,447,154 of those 20,447,156 js_gc_note_slot_layout calls return Conforms
— a cross-crate call, header decode, TLS touch and SHAPE_LAYOUTS hashmap
lookup per pointer store, to re-derive what codegen knew when it emitted the
mask. class_field_store_needs_layout_note's doc comment named this case, why
it was left alone, and the condition that would unblock it:

Deliberately NOT elided: a pointer-valued store into a pointer-masked slot.
[…] Closing that exit (#6921) is the prerequisite for the stronger elision.

#6921 is closed. This takes the elision as a live header test with the real
note kept on the cold arm
— see the changelog fragment for the two facts that
make the taken arm a proof (the #5093 precheck has already pinned the receiver's
keys_array to this class's keys global; the slot is in the pointer mask and
checked absent from the raw-f64 mask, so neither downgrade arm can fire).

Measured — quiet M1 mini, best-of-3, interleaved, stdout byte-identical

bench before after
cycles 0.33 0.29
tree_wide 7.90 7.77
tree 5.17 5.13

Unchanged: churn 1.21, push_cls 0.89, churn_alloc 0.89, retain 2.41,
retain1 0.04, retain_wide 3.38, retain_wide1 0.06, deeplist 0.03,
cls_mistyped 0.02. Peak RSS unchanged.

The profile states it more plainly than the clock: on cycles_big,
layout_note_slot is the second-heaviest leaf frame in the base arm (155
samples)
and is absent from the fix arm entirely. Wall-clock share is
smaller because these workloads are GC-dominated under the current default
pacing.

Safety

The intact bit is not made sticky and no downgrade path is touched.

  • cls_mistyped.ts (a number-declared field constructed with a heap string
    per instance — it must demote or the collector never traces those strings)
    still prints 20000 string payload-19999.
  • PERRY_GC_VERIFY_MARK=1 PERRY_GC_VERIFY_EVACUATION=1 clean over the bench set.
  • PERRY_GC_TRACE cycle counts and copied bytes identical between arms
    (churn 13, tree 20).
  • cargo test -p perry-codegen --lib 781 passed; -p perry-runtime --lib 1935
    passed. cargo fmt --all -- --check clean.

Three new tests, unit-level so cargo-test sees them (#5960), each verified able
to fail: the positive asserts the elision is reached (a predicate silently
answering false everywhere would otherwise be invisible), and the negative
asserts a slot in neither mask (flag: boolean) keeps its unconditional note —
sabotaging the predicate to return true makes that test fail.

Note for reviewers

Measuring P3 at all required #7690. On main before it merged, tree ran
204 s against an expected 4.4 s, churn 9.1 s with 1304 GC cycles against
105, with "copying_nursery":{"eligible":false} on every trace line — #7687 had
landed the immobility guard without the pacing half. cycles was nearly
unaffected (0.36 s), which is why that state was easy to miss. All numbers here
are on main with #7690 merged.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fbf3f94c-98a2-49bc-81eb-1bd78494758b

📥 Commits

Reviewing files that changed from the base of the PR and between d1aa968 and 023f0b9.

📒 Files selected for processing (7)
  • changelog.d/7698-conforming-class-field-layout-note.md
  • crates/perry-codegen/src/expr/conforming_layout_note_tests.rs
  • crates/perry-codegen/src/expr/helpers.rs
  • crates/perry-codegen/src/expr/mod.rs
  • crates/perry-codegen/src/expr/property_set.rs
  • crates/perry-codegen/src/expr/write_barrier.rs
  • crates/perry-codegen/src/typed_shape.rs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant