perf(runtime,codegen): overflow-slot properties prime the constant-key ICs (27 → 5 ms, 5×) - #9302
Merged
Merged
Conversation
added 2 commits
August 31, 2026 17:33
…y ICs (27 -> 5 ms, 5x)
A property whose slot index is past the object's inline region could never
be IC-cached for a constant-key access. `js_put_value_set_ic_miss` bailed
with `idx >= alloc_limit` before priming, and `js_object_get_field_ic_miss`
broke out of its keys walk at the same condition — so every access to such
a property missed both caches forever, paying the full miss ladder (shape
descriptor lookup, read-plan probe, byte-wise keys scan, a handle scope).
A miss-handler probe put 99.9998% of 2.4M declines on that one bail.
A plain `{}` has two inline slots (INLINE_SLOT_FLOOR), so this was the
cliff between index 1 and index 2:
field_0 at index 0 3 ms node 0 (200k write+read pairs)
field_0 at index 1 3 ms node 0
field_0 at index 2 27 ms -> 9 node 0
field_0 at index 4 25 ms -> 6 node 1
field_0 at index 8 27 ms -> 5 node 2
The fix reuses the dynamic-key IC's overflow model end to end. The miss
handlers prime `idx | IC_SLOT_OVERFLOW_BIT` (bounded by the shape's
logical key count, descriptor-bearing receivers still excluded), and the
emitted MRU hit paths branch on the bit: inline slots keep their exact
address arithmetic, encoded slots route through two new thin entries —
`js_put_value_set_ic_overflow_store`, which is `dyn_ic_try_store` (the
dynamic IC's audited validate-and-store: spill buffer, stable-tombstone
hole check, barriers), and `js_object_get_field_ic_overflow_load`, which
loads through `overflow_get` and falls back to the full miss handler on a
tombstoned slot. The bit never appears at a site whose property is inline,
so their hit path is unchanged and the new branch predicts perfectly.
One hazard is closed structurally rather than checked: `pic_prime_get`
cascades the previous MRU entry into the polymorphic ways, and the emitted
WAY path computes a raw inline address from the slot word with no bit
test. An encoded slot must therefore never enter the ways — the cascade
now refuses one, and a polymorphic site rotating overflow shapes re-primes
the MRU per shape, which is the pre-PerryTS#7753 behaviour for that site.
Honesty about the negative proofs, because two of them did not fire: with
the get helper's hole check deliberately removed, and separately with the
cascade guard removed, every fixture I could construct still matched node
— the leaked hole is normalized to undefined by every consumer tried
(strict equality, typeof, JSON, array storage), and the wild-load case
never materialized observably. Both guards stay: they are cheap, they
mirror the dynamic IC's audited path, and "could not construct the
failure" is not "cannot fail". The seven added tests guard the
invalidation story (delete, accessor install, freeze, shape change,
delete-then-revive, shape rotation, pointer values under forced
evacuation) — all verified against node first, all node-identical
including under PERRY_GC_HEAP_LIMIT=8 + FORCE_EVACUATE.
What this does NOT fix, measured: bench_object_property itself stays at
~34 ms vs node 13. Its cost is computed-key writes onto fresh objects —
shape transitions plus key concat/intern through the dynamic-key IC — a
different path from constant-key slot caching. The isolated defect this
fixes is the one a helper touching `obj.field_17` by name pays on every
wide object.
perry-runtime 2891/0 single-threaded, perry-codegen 1863/0, -D warnings 0,
thread-local policy OK, raw-handle debt 963 = baseline, 7/7 new
integration tests, 8 differential fixtures + 7 adversarial cases
node-identical.
Refs PerryTS#9287.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughConstant-key property get and put inline caches now prime overflow-buffer slots, encode them with ChangesOverflow Slot Inline Caches
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR adds overflow-slot caching for constant-key property access and preserves the existing fallback and safety checks. It is mergeable with explicit owner awareness that concurrent execution of the same generated module must be thread-confined or otherwise protect cache publication, since an unverified race could return an incorrect property value. Sequence Diagram(s)sequenceDiagram
participant ConstantKeyAccess
participant PropertyGetPIC
participant js_object_get_field_ic_overflow_load
participant overflow_get
ConstantKeyAccess->>PropertyGetPIC: access constant-key property
PropertyGetPIC->>js_object_get_field_ic_overflow_load: dispatch encoded overflow slot
js_object_get_field_ic_overflow_load->>overflow_get: read spill-buffer field
overflow_get-->>js_object_get_field_ic_overflow_load: value or TAG_HOLE
js_object_get_field_ic_overflow_load-->>PropertyGetPIC: value or miss fallback
PropertyGetPIC-->>ConstantKeyAccess: property value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description provides a detailed summary, implementation changes, related issue, test coverage, validation results, and known limitations. It does not reproduce the template headings or checklist, but the required information is substantially present. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changelog entry, runtime and codegen changes, overflow IC helpers, and integration tests are directly related to issue Full details: Docstring CoverageExplanation Docstring coverage is 85.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 6 files. (1 skipped: 1 unsupported.) ✨ 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 |
proggeramlug
added a commit
that referenced
this pull request
Aug 31, 2026
* codegen: a float accumulator over masked reads earns the dense range clone
17_loop_data_dependent: 475 ms -> 219 ms against node's 220 ms on an idle
Mac mini -- parity, from 2.16x. Sums bit-identical across 100M data-dependent
float recurrence steps.
sum = sum * x[i & 63] + x[(i * 7) & 63] // rejected
sum = sum * x[i & 63] // admitted
The discriminator was the accumulator's static numeric proof. `+` can be
concatenation, so the dense tier's per-statement proof demands both operands
numeric; a reassigned accumulator has no such proof, because its own writes
read the guarded array, whose element proof only exists once the guard has
run. A chicken-and-egg that `*` never faces -- multiplication needs only the
weaker inert fact. Confirmed by instrumenting the two conjuncts of the dense
LocalSet arm: the failing one is the proof, on exactly the fixtures whose
accumulator writes contain a plain-array read.
The matcher now peels the accumulator: when the proof fails on the LocalSet
target of a self-accumulating write, it retries with the target treated as
numeric BY CONTRACT, records it pending, and then verifies every pending
local with the same collector the lowering runs
(`collect_numeric_accumulators`), rejecting the whole dense match with its
own named trace reasons (`accumulator_needs_single_array`,
`accumulator_not_provable`) if the two disagree -- so the clone can never
contain a dynamic `+` under facts that forbid one.
The contract is enforced at run time twice over: the clone's entry emits a
genuine-double tag check on the accumulator, and the dense entry guard
validates the whole masked window hole-free. A string-seeded accumulator and
a string element both route to the slow copy and produce node's
concatenation, verified under PERRY_GC_FORCE_EVACUATE.
Supporting changes:
* `accumulator_rhs_is_numeric` accepts masked static-window reads of the
tracked array (`masked_reads_validated`), sound because the dense guard
validated the window union hole-free. Fixing that exposed a match-arm
reachability bug: `_ if offset_reads_inlined` was a guarded catch-all, so
ANY arm placed after it was unreachable whenever the flag was set -- the
first version of this change sat exactly there and verified as a no-op.
The two tests are now one combined catch-all.
* `emit_range_loop_accumulator_admission` admits a masked-only single array
(counter-bearing arrays keep priority; multiple arrays still decline).
* `MaskedWindowArrayFact` carries `numeric_accumulators` so `is_numeric_expr`
sees admitted accumulators while the clone lowers -- without this the add
inside the clone would stay dynamic, which is a collecting call under facts
that assume none (the #9259 cascade shape). Mirrors the string-window
fact's field (#9160).
perry-codegen lib 1378/0; packed-loop integration suite 59/0 across 11 files;
3 new regression tests (admission + node-identical result, string-seeded
accumulator, string element), each under forced evacuation.
Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
* refactor(runtime): split ic_miss.rs and array/tests.rs under the file cap
#9302 and #9307 each tipped a file that was already within ~35 lines of the
2000-line gate. Extracts the C3C PIC test module and the Array.prototype
method-discriminator tests into sibling files; no behaviour change.
* fix(codegen): drop the now-dead catch-all after the combined accumulator arm
#9303's combined `_ =>` arm made the trailing `_ => false` unreachable, which
is a `-D warnings` failure. Removing it is #9308's fix, which the combined arm
needs to be complete.
* style: rustfmt
* chore: changelog fragment for the train13 follow-up
---------
Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Fixes #9287.
The defect
A property whose slot index is past the object's inline region could never be IC-cached for a constant-key access.
js_put_value_set_ic_missbailed withidx >= alloc_limitbefore priming;js_object_get_field_ic_missbroke out of its keys walk at the same condition. Every access paid the full miss ladder — shape-descriptor lookup, read-plan probe, byte-wise keys scan, a handle scope. An env-gated miss-handler probe put 99.9998% of 2.4M declines on that one bail.A plain
{}has two inline slots (INLINE_SLOT_FLOOR), so the cliff sits between index 1 and index 2:field_0at index(200k write+read pairs, quiet host, checksums node-identical.)
The fix — reuse the dynamic-key IC's overflow model end to end
The dynamic-key IC solved this exact problem long ago:
IC_SLOT_OVERFLOW_BITin the slot word,dyn_ic_try_storeon a hit. This PR extends the same encoding to the constant-key ICs:idx | IC_SLOT_OVERFLOW_BIT(bounded by the shape's logical key count; descriptor-bearing receivers still excluded; the get side additionally requires the value be readable throughoverflow_getright now).js_put_value_set_ic_overflow_store(literallydyn_ic_try_store: spill store, stable-tombstone hole check, barriers) andjs_object_get_field_ic_overflow_load(overflow_get, full-miss fallback on a tombstoned slot). Sites whose property is inline never see the bit, so their code path is unchanged and the branch predicts perfectly.pic_prime_getcascades the previous MRU entry into the polymorphic ways, whose emitted path computes a raw inline address with no bit test. The cascade now refuses an encoded slot — a polymorphic site rotating overflow shapes re-primes the MRU per shape (pre-perf(codegen,runtime): polymorphic property-read cache +arr.lengthshort-circuit — interp.ts 3.96s → 2.39s #7753 behaviour for that site) instead of ever letting the bit reach the ways.Negative proofs, stated honestly
Two deliberate breakages did not discriminate, and I am reporting that rather than claiming coverage I don't have: with the get helper's hole check removed, and separately with the cascade guard removed, every fixture I could construct still matched node — a leaked hole is normalized to
undefinedby every consumer I tried (strict equality,typeof, JSON, array storage), and the wild-load case never materialized observably. Both guards stay: they are cheap, they mirror the dynamic IC's audited path, and "could not construct the failure" is not "cannot fail".What the 7 added tests do guard, all verified against node before being written: delete-after-prime, accessor-install-after-prime, freeze-after-prime, shape change, delete-then-revive, three-shape rotation at one site, and pointer values through the overflow write under
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_FORCE_EVACUATE=1.What this does not fix, measured
bench_object_propertyitself stays at ~34 ms vs node 13: its cost is computed-key writes onto fresh objects — shape transitions plus key concat/intern through the dynamic-key IC — a different path from constant-key slot caching. The defect fixed here is the one any helper touchingobj.field_17by name pays on every wide object; the benchmark's residual is follow-up work on #9287.Validation
-D warnings0issue_9287_overflow_slot_ic), 8 differential fixtures + 7 adversarial cases node-identical, including under forced evacuationSummary by CodeRabbit
Performance
Bug Fixes