fix(runtime): birth-stamp the class allocators #8009 left lazy, and gate the split population - #8010
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughClass-instance allocators now birth-stamp objects with runtime ShapeIds. Shape and PIC tests verify stable IDs and interchangeable cache tokens across sibling instances. Changelog and bisect notes document the regression mechanism, measurements, allocator coverage, and validation. ChangesClass-instance shape stamp consistency
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClassAllocator
participant ShapeCache
participant ObjectHeader
participant PICMissHandler
participant SiblingInstance
ClassAllocator->>ShapeCache: retrieve or mint runtime ShapeId
ShapeCache-->>ClassAllocator: return ShapeId
ClassAllocator->>ObjectHeader: birth-stamp class instance
PICMissHandler->>SiblingInstance: resolve property miss
SiblingInstance-->>PICMissHandler: return ShapeId-based PIC token
PICMissHandler->>SiblingInstance: prime cache entry
SiblingInstance-->>PICMissHandler: compute matching token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
proggeramlug
changed the title
proggeramlug
pushed a commit
that referenced
this pull request
Aug 13, 2026
proggeramlug
marked this pull request as ready for review
August 13, 2026 04:05
proggeramlug
marked this pull request as draft
August 13, 2026 04:05
Contributor
Author
|
HOLD — do not merge. #8009 ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@changelog.d/8010-class-instance-shape-stamp-population.md`:
- Around line 1-46: Rewrite the changelog entry to describe only the final
shipped behavior and user impact: shape stamps remain limited to plain objects,
while class-instance inline allocations retain an unstamped shape word until
codegen support is added. Remove benchmark tables, bisect history, intermediate
implementation details, and test-development narrative; preserve those details
in gc-handoff/BISECT-NOTES.md instead.
In `@gc-handoff/BISECT-NOTES.md`:
- Line 160: Run the required validation command RUST_TEST_THREADS=1 cargo test
--release -p perry-runtime, then update the test result recorded in
BISECT-NOTES.md to reflect that command and its actual outcome; do not retain
the narrower --lib, non-release command as the validation record.
🪄 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: 6b33db49-70a9-4c80-ae9f-1f88cc931228
📒 Files selected for processing (5)
changelog.d/8010-class-instance-shape-stamp-population.mdcrates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/field_get_set/ic_miss.rscrates/perry-runtime/src/object/shapes.rsgc-handoff/BISECT-NOTES.md
Comment on lines
+1
to
+46
| ### Hold the C3 shape stamp at plain objects — class instances split the PIC population (#7983) | ||
|
|
||
| `#6759 C3 rung 1` (#7983) dropped the `class_id == 0` gate on the header shape | ||
| word so class instances would carry a ShapeId too. Codegen INLINE-allocates | ||
| `new C(…)`, though: the emitted bump-pointer allocation stores the header words | ||
| as literals — including a literal `0` into the shape word — and never calls | ||
| `js_object_alloc_class_with_keys`. The runtime's stamp therefore reached only | ||
| the instances that happened to take a by-name resolve, and one shape's | ||
| population split in two. | ||
|
|
||
| The emitted read PIC derives its whole cache token from that word, so a stamped | ||
| receiver and a newborn one of the same shape compute two different tokens and a | ||
| site that sees both can never hold a hit: instance #1 misses, is stamped, primes | ||
| the id token; instance #2 is newborn, computes the keys pointer, misses; the | ||
| handler re-primes the same id; instance #3 misses. Forever. | ||
|
|
||
| Bisected to `4784d5da7` on instructions retired, isolated against its own | ||
| parent, and recovered in full by this change: | ||
|
|
||
| | bench | `main` | fixed | recovered | | ||
| |---|--:|--:|--:| | ||
| | cycles | 2,758,649,326 | 1,788,432,369 | −35.2% | | ||
| | deeplist | 1,303,183,101 | 888,880,890 | −31.8% | | ||
| | interp | 14,893,088,080 | 11,615,285,338 | −22.0% | | ||
| | pipeline | 3,193,275,738 | 2,582,793,215 | −19.1% | | ||
| | iso_miss | 17,459,670,264 | 14,205,774,080 | −18.7% | | ||
|
|
||
| The object-literal benchmarks never moved in either direction (`churn` +1.2%, | ||
| `retain` +0.2%, `fib40` +0.04% at the regressing commit) — literals are | ||
| birth-stamped by every allocation path (#6804), so their population was always | ||
| uniform. Isolated on one program and one build: a read pass over 3,000,000 | ||
| newborn class instances costs 43.6 instructions per read, a second pass over the | ||
| same now-stamped instances 15.5. | ||
|
|
||
| `shapes::shape_word_is_stampable` restores the discriminant in one named place, | ||
| on both the read and the write side. Rung 1's structural content stays — the | ||
| helper trio, the `RegExpHeader`-alias hole it closed, and the delete/re-mint | ||
| machinery. Lifting the gate is rung 2 and belongs in codegen: the inline | ||
| allocation must store the class's ShapeId where it currently stores `0`; when | ||
| that lands, `shape_word_is_stampable` becomes `shape_word_is_writable`. | ||
|
|
||
| The gate asserts the discriminating property and passes in BOTH uniform states, | ||
| failing only on the mixture: the token the miss handler primes must equal the | ||
| token a freshly-allocated sibling computes. The five tests that pinned rung 1's | ||
| half-state are rewritten rather than deleted, so the machinery rung 2 depends on | ||
| stays covered while the auto-stamp is held. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reduce this fragment to the shipped behavior.
This changeset includes failed implementation history and detailed benchmark investigation. Keep that material in gc-handoff/BISECT-NOTES.md. State the final behavior and user impact in this release-note entry.
Based on learnings, “For changelog fragments under changelog.d/, describe the final shipped behavior as one coherent release-note entry. Do not include separate development-slice narratives that may contradict one another when the release notes are assembled.”
🤖 Prompt for 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.
In `@changelog.d/8010-class-instance-shape-stamp-population.md` around lines 1 -
46, Rewrite the changelog entry to describe only the final shipped behavior and
user impact: shape stamps remain limited to plain objects, while class-instance
inline allocations retain an unstamped shape word until codegen support is
added. Remove benchmark tables, bisect history, intermediate implementation
details, and test-development narrative; preserve those details in
gc-handoff/BISECT-NOTES.md instead.
Source: Learnings
|
|
||
| ## Validation | ||
|
|
||
| * `cargo test -p perry-runtime --lib` (`RUST_TEST_THREADS=1`): **2278 pass, 0 fail**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run the required release-package test.
The recorded command omits --release and restricts execution to --lib. Run RUST_TEST_THREADS=1 cargo test --release -p perry-runtime, then update this validation record with its result.
As per coding guidelines, “crates/perry-runtime/**/*.rs: RUST_TEST_THREADS=1 cargo test --release -p perry-runtime” and “perry-runtime's tests are not parallel-safe — run them RUST_TEST_THREADS=1.”
🤖 Prompt for 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.
In `@gc-handoff/BISECT-NOTES.md` at line 160, Run the required validation command
RUST_TEST_THREADS=1 cargo test --release -p perry-runtime, then update the test
result recorded in BISECT-NOTES.md to reflect that command and its actual
outcome; do not retain the narrower --lib, non-release command as the validation
record.
Source: Coding guidelines
added 3 commits
August 13, 2026 06:25
…ate the split #8009 (C3 rung 2) stamps a class instance's ShapeId at birth on the COMPILED path — `js_object_alloc_class_inline_keys_stamped`, called from the inline `new C(…)` lowering. Three other class-instance allocators were left on rung 1's lazy self-heal, which its own doc comment states: js_object_alloc_class_with_keys js_object_alloc_class_dynamic_parent js_object_alloc_class_inline_keys (the compatibility entry point) For any class that lands on one of those, the shape's population is still SPLIT — and a split population is not a slow start, it is a permanent 0% PIC hit rate. The emitted read PIC derives its entire cache token from the header shape word: is_stamp = (parent_class_id - 0x8000_0000) u< 0x4000_0000 token = is_stamp ? (parent_class_id | 1<<62) : keys_array so instance #1 misses, is stamped, primes the id token; instance #2 is newborn, computes the keys pointer, misses; the handler re-primes the same id; instance #3 misses. Forever. That is exactly #7983's defect, which this bisected: on instructions retired, isolated against its own parent, `cycles` +54.3%, `deeplist` +45.2%, `interp` +28.3%, `pipeline` +23.9%, `iso_miss` +22.9%, while the object-literal benchmarks (`churn` +1.2%, `retain` +0.2%) and `fib40` (+0.04%) did not move — literals have been birth-stamped since #6804. All three now stamp at birth. The two shape-cached allocators read the id out of the `ShapeCacheEntry::runtime_shape_id` their existing probe already returns (`shape_cache_get` -> `shape_cache_get_with_id`): one extra u32 from a loaded cache line, a compare and a store. The compatibility entry point mints from its canonical keys array — one shape-table probe, and it is not the compiled hot path. THE GATE. `a_fresh_class_instance_computes_the_token_the_miss_handler_primed` asserts the token the miss handler PRIMES equals the token a freshly-allocated sibling COMPUTES, using `emitted_pic_token` — the emitted IR's formula transcribed into the test. It FAILS on `main` as of #8009 and passes here, which is the whole point: #8009's own test asserts that a newborn CARRIES a stamp, and that is a presence check. Both-stamped and both-unstamped each satisfy it; only the MIXTURE is the bug, and only a test that compares the two sides can see it. This one also passes in either uniform state, so it does not need rewriting if the policy ever flips back. Two premise assertions in `delete_rest::shape_transition_tests_6759` said a fresh instance is unstamped. They are updated, not deleted, and the parent-chain test is strictly stronger for it: the word is now clobbered from birth, so there is no window in which it was ever valid inheritance data.
proggeramlug
force-pushed
the
fix/7983-class-instance-birth-stamp
branch
from
August 13, 2026 04:38
31c87d4 to
a36cddd
Compare
proggeramlug
changed the title
proggeramlug
marked this pull request as ready for review
August 13, 2026 04:38
Contributor
Author
|
Hold lifted — rewritten and re-validated on top of #8009. Summary of the change of course: #8009 landed mid-validation and IS the fix for the corpus-visible regression (current main is within 0.1-0.6% of the good endpoint), so the original approach here was dropped. What remains is the slice #8009 did not cover — the three class-instance allocators still on rung 1's lazy self-heal — plus the gate, which fails on main as of #8009 and passes here. Ready for review. |
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.
Bisect of the broad regression between
0a21611feand843ef621f, and the partof it that is still live on
mainafter #8009.The bisect
The commit is
4784d5da7(#7983, "#6759 C3 rung 1 — make the shape worduniform"), alone. Neither leading hypothesis survived: #7997 (the aarch64 SVE
prologue decoder) and #7994 (per-thread prototype addresses) both landed after
the regression was already fully present.
Wall clock on the dev box cannot resolve this (load 30–200 all session), so this
is instructions retired (
/usr/bin/time -l, best-of-3, exit-checked, outputcmp-ed againstm0810/expected/). Every hop rebuilt-p perry -p perry-runtime-static -p perry-stdlib-staticinto one target dirwith the
.amtimes verified.0a21611fegood23a8aad31parent4784d5da7#7983843ef621ftipf58b73f4fmain (with #8009)The whole regression appears at #7983 and nothing after it adds any. #8009
recovers it — current main is within 0.1–0.6% of the good endpoint.
fib40isflat to four decimals throughout; the +2.7% originally flagged was noise.
Mechanism
The split is by receiver kind, not program size:
cycles(class Cell),deeplist(class LNode),interp,pipeline,iso_missregressed;churnand
retain(type … = { … }object literals) did not.The emitted read PIC (
expr/property_get/generic_dispatch.rs) derives itsentire cache token from the header shape word:
Rung 1 stamped class instances lazily, at the first by-name resolve, so one
shape's population split in two and the two halves compute two different tokens:
instance #1 misses, is stamped, primes the id token; instance #2 is newborn,
computes the keys pointer, misses; the handler re-primes the same id; instance
#3 misses. Hit rate 0%, forever — not a slow start.
Single-build proof
Three programs, one compiler, differing only in read passes over the same
3,000,000-instance array:
Pass 1 sees each instance NEWBORN; pass 2 sees the SAME instances already
stamped. 2.8×, and the only difference is the stamp.
What this PR fixes
#8009 stamps only the compiled entry point,
js_object_alloc_class_inline_keys_stamped. Three class-instance allocators arestill on rung 1's lazy self-heal, as its own doc comment states:
js_object_alloc_class_with_keysjs_object_alloc_class_dynamic_parentjs_object_alloc_class_inline_keys(the compatibility entry point)For any class reaching one of those, the population is still split and its
sites still never hit. All three now birth-stamp: the two shape-cached ones read
the id out of the
ShapeCacheEntry::runtime_shape_idtheir existing probealready returns (
shape_cache_get→shape_cache_get_with_id) — one extra u32from a loaded cache line, a compare and a store; the compatibility entry point
mints from its canonical keys array, which is off the compiled hot path.
This is neutral on the perf corpus (all nine programs' classes take the
compiled path #8009 already fixed) — the value is the classes that do not, and
the gate.
The gate
a_fresh_class_instance_computes_the_token_the_miss_handler_primedasserts thetoken the miss handler primes equals the token a freshly-allocated sibling
computes, against the emitted IR's formula transcribed into a test-only
emitted_pic_token.It fails on
mainas of #8009 —left: 2199047503880(a keys pointer),right: 4611686020574871741(bit 62 | id) — and passes here.#8009's own test asserts that a newborn carries a stamp. That is a presence
check: both-stamped and both-unstamped each satisfy it, and it is the mixture
that is the bug, so it cannot see this. Comparing the two sides is what can. The
gate also passes in either uniform state, so it survives a future policy flip
rather than needing a rewrite.
Sabotage-verified with the fix committed first: deleting the birth stamp from
js_object_alloc_class_with_keysfails exactly this test and leaves the otherfour
c3c_pic_testsgreen. Restored and REBUILT (Compiling perry-runtimepresent on the rebuild) before re-confirming.
Two premise assertions in
delete_rest::shape_transition_tests_6759said a freshinstance is unstamped; they are updated, not deleted, and the parent-chain
test is strictly stronger for it — the word is now clobbered from birth, so there
is no window in which it was ever valid inheritance data.
Question 2: churn / retain / asyncpipe
Reported at +13% / +13% / +27% wall on the quiet mini but flat in
instructions. Peak RSS and GC collection counts are both load-independent, so
this is answerable on a busy box (
PERRY_GC_DIAG=1andPERRY_GC_TRACE=1—DIAG alone prints nothing; positive control: 445 lines and 22
collection_kind":"minor"forcycles).0a21611fefour, zero full collections anywhere.
★ Note the control:
cyclesregressed 54% with RSS and collection countsalso flat. Flat RSS/counts can only REFUTE the scheduling and footprint
explanations; they never confirm "nothing changed". The positive statement for
churn/retain is the instruction count.
With no work added, no collection rescheduled and no footprint change, there is
no mechanism left for a 13% wall move: churn and retain are mini-side
variance, and both should regain their node wins now that #8009 has landed.
asyncpipeis the one row with a real attributable cost: +2.1%, not +27%,and #8009 returns it to 1,209,636,954 against the good endpoint's 1,205,121,699.
The remaining ~25 points are either mini variance or idle/parked time, which
neither instructions retired nor cycles elapsed can observe for an async program
— only the quiet mini can settle that.
Validation
cargo test -p perry-runtime --lib(RUST_TEST_THREADS=1): 2279 pass, 0 fail.iso_misscanary printschecksum 437840 misses 0, including underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_VERIFY_EVACUATION=1;cycles/deeplist/interpbyte-identicalunder the same knobs.
m0810/expected/at every arm.test-parity/gap_snapshot.json: 68 pass, 1 expected-fail, 0 unexpected.The 70th (
test_gap_3527_http_ctor_prototype) is fix(link): refuse a link whose ext wrappers bundle a different tokio (#7629) #7999's own tokio link guardrefusing an ad-hoc build set without
-p perry-ext-http— an artefact ofcompiling outside the harness.
★ The full 554-test gap suite could not be run on this host: the harness stalls
at 0% CPU after 10–25 tests, on a full run and a 1/8 shard alike, while the same
compile finishes in seconds standalone. The dev box was at 8.6 GiB free (the
protocol wants ≥ 25 GiB for parity runs) under heavy contention. Host
constraint, not a property of this change — CI runs the full suite here.
Summary by CodeRabbit