fix(runtime): prove an unpatched iterator prototype without allocating a "next" key string - #9848
Conversation
`call_overridden_iterator_next` minted a fresh 4-byte "next" key string on
every built-in iterator step, purely to run a by-name prototype lookup that
concluded nothing was patched. The `ITERATOR_PROTOTYPE_PTR == 0` early-out
that was supposed to prevent this is dead after the first iterator any
program allocates: every iterator allocator calls `attach_iterator_prototype`
-> `ensure_iterator_prototypes`, which materializes the tower.
Adds `prototype_next_is_canonical`: the prototype's own `next` slot holds a
closure whose native entry is the canonical thunk, and no accessor descriptor
is recorded for "next". Both reads are non-allocating. Any other state falls
through to the by-name path, unchanged.
This is the third-ranked site by count in the 2026-09-06 claude-code
allocation census (~122,880 x 32 B per 400-character reply), which had
attributed it to `Intl.Segmenter` substring copying. Caller walk in the
shipped binary `cc_relink/cc_int_0905`:
js_for_of_next+0xd0
-> dispatch_array_iterator_method_inner+0x218 (bl call_overridden_iterator_next)
-> call_overridden_iterator_next+0x67c (bl js_string_from_bytes_with_capacity)
-> string_storage_alloc
Measured on a relinked claude-code binary carrying this fix plus a
measurement-only hit/miss counter. Before the fix every probe allocated, so
`hits + byname` is the pre-fix count and `byname` is what survives:
400-char reply, run A 144,189 probes byname 0
400-char reply, run B 144,303 probes byname 0
3300-char reply 887,076 probes byname 0
`byname = 0` on every one of the 173 per-minor reports across the three runs:
the proof answers 100 % of probes on a real program, which is what rules out
the one silent failure mode (the accessor half is a per-key Bloom bit, so a
colliding accessor on the prototype would disable the fast path with no test
failing).
`cargo test -p perry-runtime --release --lib -- --test-threads=1`: 3,171
passed, 0 failed. Four sabotage arms, each failing only its named assertion:
removing the fast path entirely reads exactly 32,000 bytes over 1,000 probes;
dropping only the accessor half fails only the accessor test; dropping only
the native-entry comparison fails only the replaced-`next` test.
An integration arm for the allocation-free proof: compiles
`test-files/test_gap_iterator_prototype_next_patch.ts` and byte-compares
stdout against node v26.5.1, captured 2026-09-06 on this box.
Three of the lines are the ones that can only pass if the proof is exactly
right:
F-bound-copy 100,200 a `bind` of the original has the SAME native entry as
the builtin thunk but a different `this`; a proof that
compared native entries without first reading the
prototype's own slot would print `1,2`.
G-accessor 1,2 true `defineProperty(proto,"next",{get})` leaves the old
closure in the data slot, so the own read alone still
sees the canonical closure — only the per-key accessor
Bloom bit makes the proof decline.
H true a deleted `next` must throw a TypeError, never fall
through to the builtin advance.
The allocation-free proof reads the prototype's own `next` slot as a RAW value before deciding anything, so a number, a string, `undefined`, `null` and a plain object each have to defeat it and throw a TypeError rather than be mistaken for the builtin closure. Node v26.5.1 throws for all five; pinned in the integration arm.
The fragment was written before the issue existed and carried 9840, which is an unrelated open GC issue. PerryTS#9846 is the filed report for this defect.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe runtime replaces the dead iterator prototype guard with an allocation-free canonical-thunk check. Tests cover allocation behavior and patched, restored, accessor, deleted, bound, and non-callable ChangesIterator
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change removes per-step iterator probe allocation while preserving observable behavior when iterator prototype methods are modified. The covered override and error cases leave no current merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant IteratorOperation
participant call_overridden_iterator_next
participant prototype_next_is_canonical
participant IteratorPrototype
participant ByNameLookup
IteratorOperation->>call_overridden_iterator_next: request iterator next step
call_overridden_iterator_next->>prototype_next_is_canonical: inspect prototype next
prototype_next_is_canonical->>IteratorPrototype: read own slot and accessor state
prototype_next_is_canonical-->>call_overridden_iterator_next: canonical or overridden
call_overridden_iterator_next->>ByNameLookup: resolve next for overridden cases
ByNameLookup-->>IteratorOperation: invoke resolved next
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 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
the
run-extended-tests
|
CI note, recorded before anyone reads the red: this PR carries Because this PR touches for-of iteration too, that shared-red history is a reason to CHECK rather than a reason to dismiss: the failure signature should be compared against #9816's before it is attributed to either change. The rest of the |
|
Landed on |
1 participant
Closes #9846.
What was wrong
call_overridden_iterator_next— the per-step probe that lets a userreplacement of
%ArrayIteratorPrototype%.next(and the Map / Set / Stringfamily prototypes) drive
for…of, spread,Array.fromand manual.next()—minted a fresh 4-byte
"next"key string on every iteration step ofevery built-in iterator, purely to run a by-name prototype lookup that
concluded nothing was patched.
The early-out meant to prevent that cannot fire.
ITERATOR_PROTOTYPE_PTR == 0means "the tower was never materialized, so no override can exist" — but every
iterator allocator calls
attach_iterator_prototype→ensure_iterator_prototypes,which materializes the tower. The guard is true exactly once per program and
false forever after.
The fix
An allocation-free proof of "not overridden" on the path every real program
takes: the prototype's OWN
nextslot still holds a closure whose native entryis the canonical thunk (#9480's certified non-allocating own-field read), AND
no accessor descriptor is recorded for
"next"on it (#6759 C2's per-keyaccessor Bloom bit — needed because
defineProperty(proto,"next",{get})leavesthe old closure in the data slot and records the accessor in the side table).
Anything else — replaced, deleted, an accessor, a bound copy of the original —
takes the by-name path, unchanged.
The slow path deliberately keeps
js_string_from_bytesrather than interning"next": interning would make the counter below pass whether or not the fastpath fires.
Counter — one binary gives both arms
Before the fix every probe allocated, so on a binary carrying this fix plus a
measurement-only hit/miss counter,
hits + bynameis the pre-fix count andbynameis what survives. Relinked claude-code bundle,stream_scalerig:"next"stringsbyname= post-fixbyname = 0on every one of the 173 reports: the proof answers 100 % ofprobes on a real program. At 32 B a string that is 4.6 MB / 28.4 MB of
allocation removed per process. The two 400-char runs differ by 0.08 %.
That counter is not optional and is the reason it was taken on the real bundle:
the accessor half is a per-key Bloom bit, so any accessor whose key collides
in that word would disable the fast path forever with no test failing.
byname = 0rules that out;hits = 0would have meant the change was inertwhile every unit test still passed.
No timing claim is made from that binary (it carries the counter, and the box
was under heavy concurrent load). For the record only:
turn_cpu_s7.28 / 6.82at 400 chars and 24.99 at 3300, RSS 551 / 555 / 809 MB.
Where it was found
Third-ranked site by count in a claude-code allocation census (2026-09-06:
~122,880 × 32 B per 400-character reply, 17.1 % of the top-30 count), which had
misattributed it to
Intl.Segmentersubstring copying. Resolved by an explicitcaller walk in the shipped binary, because the census's
js:frame labels arenearest-symbol and were wrong:
Tests
test-files/test_gap_iterator_prototype_next_patch.tsdrives a replacednextthroughfor…of, spread,Array.fromand manual.next()on allfour families, plus restore-by-identity, a second replace after a restore, a
bound copy of the original (same native entry, different
this— must NOT bemistaken for the builtin), an accessor
next, a deletednext, and fivenon-callable
nextvalues.crates/perry/tests/issue_9846_…byte-comparesits output against node v26.5.1 — re-captured independently on this box
before this PR, all 28 lines identical.
materialized must move
arena_in_use_bytesby zero, with the minor-cyclecount pinned across the window so a collection cannot manufacture the zero.
cargo test -p perry-runtime --release --lib -- --test-threads=1: 3,171passed, 0 failed, 4 ignored.
Sabotage — four arms, each failing only its named assertion
next, accessornext)probe_declines_when_an_accessor_next_is_defined_on_the_prototypefailsprobe_honours_a_replaced_prototype_next_and_a_restored_onefailsEach half of the proof therefore has a test that fails when, and only when,
that half is removed.
Summary by CodeRabbit
Performance
Bug Fixes
nextimplementations now produce the expected iteration results or errors.Tests
for…of, spread syntax,Array.from, and manual iterator usage.Conformance context, and what the suite cannot see
This PR does not touch
Intl.Segmenter, but it was found while working on it,so for the record:
scripts/test262_subset.py --dir intl402/Segmenteron thisbranch is 72 pass / 0 diff / 0 runtime-fail / 2 compile-fail (74 judged;
--all-features: 74 / 0 / 1 / 2 of 77). All three failures are constructorlocale handling — the two
locales-invalid.jscases are the only ones thatincludes: [testIntl.js]and fail compiling that harness.Two limits of that suite, so a reviewer knows what it cannot see: it buckets by
agreement with node, so a case both engines get wrong scores
pass; andArray.isArray(segments),segments.lengthandsegments[0]— perry answerstrue/ a number / a record where V8 answersfalse/undefined/undefined— are covered by none of the 79 cases.