perf(runtime): recycle the for-of result object for array iterators (167,946 objects per 400-char claude-code reply) - #9816
Conversation
proggeramlug
added
the
run-extended-tests
|
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 (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughArray iterators now reuse one ChangesArray for-of result caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Fused array for-of iteration now reuses iterator result objects to reduce allocations while manual iterator use retains fresh results. The behavior and layout contracts are covered, with no current merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant js_for_of_next
participant dispatch_array_iterator_method_emit
participant emit_iter_result_cached
js_for_of_next->>dispatch_array_iterator_method_emit: dispatch array next with caching
dispatch_array_iterator_method_emit->>emit_iter_result_cached: emit value and done
emit_iter_result_cached-->>dispatch_array_iterator_method_emit: return cached result
dispatch_array_iterator_method_emit-->>js_for_of_next: return validated result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Scope boundary, measured rather than assumed. The same allocation-site
The string iterator is the same shape — a 2-field object with reserved floor 2, Everything else in the category is already excluded by construction: Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
c52d3d9 to
16f4c3f
Compare
The falsifier is met — counter first, numbers to followI said before building that
The population is the same to within 0.3 %, and iterator-result allocations fall For scale on the workload this was ranked from: the same counter on a Turn CPU on that smoke pair was 6.25 s → 5.83 s, but that is n=1 on a contended Provenance: the candidate Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
|
Suite on this branch, rebuilt from scratch after the test amendment: That covers the two behavioural risks the diff carries beyond the recycling The rig table is still queued behind two other lanes' builds; it will follow. Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
The rig table — and it does not move the needle on claude-codeQuiet box this time (load 4–15 for every run, against 11–22 for my last table), 400-character streamed reply, three repeats per arm
3300-character streamed reply, two repeats per arm
Typing + short turn (
|
| arm | startup | typing r2 | turn r2 | r3 turn CPU | idle 10 s | RSS end |
|---|---|---|---|---|---|---|
| off | 1.70 | 0.84 | 1.00 | 60.63 † | 5.11 | 1477 |
| on | 1.78 | 0.86 | 1.33 | 37.54 | 1.88 | 1212 |
| base | 1.66 | 0.81 | 1.19 | 60.68 † | 6.02 | 1205 |
| node | 1.23 | 0.30 | 0.11 | 0.23 | 0.03 | 343 |
† the r3 turn did not complete inside the 60 s window in those two arms, so
their CPU figures are floors, not completions. I am not claiming that row:
it is n=1, and the stream_scale 3300 pair — the same reply length, two repeats
per arm, uncensored — is flat at ~50 s in both arms. One of the two is noise and
the uncensored one is the more trustworthy.
Verdict, stated plainly
The change removes exactly what it claims, and on claude-code that is worth
nothing measurable. The counters on the same 400-character reply:
off: allocated=154,557 fused_array_advances=142,091 copying minors: 81
on: allocated= 31,535 fused_array_advances=145,307 copying minors: 81
−80 % of iterator-result objects at the same advance count — and the same 81
copying minors in both arms. That is the whole explanation for the flat table,
and it is worth stating rather than burying: ~123,000 removed objects × 40 bytes
is ~4.9 MB out of the ~157 MB a 400-character reply allocates, so the nursery
still fills at the same rate from everything else and the collection schedule
does not change at all. CPU and footprint differences between the arms are
inside their own spread in both directions.
Neither metric regresses, which is the directive's condition, but "does not
regress" is not a result and I am not going to dress it up as one.
What I checked before saying that
The counterweight to the removed allocations is that every array iterator now
carries six reserved fields instead of three (+24 bytes, and possibly a size
class) so field 5 can hold the cache without a per-iterator shape transition.
I did not count iterators, so I cannot price that directly — but the measured
settled footprint and peak RSS are flat-to-slightly-better in both arms of both
reply lengths, so the net is not negative on this workload.
Where it would pay, and why I still think it should land
The cc render loop is not iteration-bound; it is bound by the property-set and
GC work the other lanes are attacking. A program whose hot loop is generic
for…of over arrays pays one 40-byte object per element today, and this deletes
that permanently rather than making it cheaper — which is the campaign's own
ranking rule. It also collapses two copies of the recycling routine into one, so
the array and Map/Set families cannot drift the way the five result constructors
#7564 replaced had.
Staying draft, for a specific reason
Not because of the flat table — because the base has moved: this branch is
on 1d63fa91f and main is 22+ commits past it. I will rebase onto current
main, re-run the suite, and take it out of draft then, so what CI measures is
what the diff means. Tests on the current base are green (3,143 passed, 0
failed, warning-free).
Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
The fused `for…of` advance (`js_for_of_next`, the runtime entry the compiler's
desugar emits) already recycled ONE `{ value, done }` object per ITERATOR for
builtin Map/Set iterators: the result local is a compiler temporary the loop
body cannot name, and the driver reads `done`/`value` out of it before the next
advance, so mutating one cached object is unobservable. Array iterators fell
through to the generic arm and minted a fresh 40-byte object per element.
The allocation-site census of the compiled claude-code TUI attributes 100 % of
its iterator-result bytes — 14.4 MB of a 3300-character reply, 15.5 % of all
attributed arena bytes and the third-largest category — to exactly that:
`array::iter_object` under `js_native_call_method`, one object per element of
every generic `for…of`. A minified bundle reaches it whenever the iterated
value is not a statically proven array, which is nearly always.
So the array iterator takes the same fused arm, and the recycling routine moves
to `iter_result::emit_iter_result_cached` so the two families share ONE
implementation instead of a second copy — the drift PerryTS#7564 removed from the five
result constructors this module replaced.
Three things the change had to keep intact:
* The override probe still runs first, so a patched own `next` wins on the
fused path exactly as it does on the manual one.
* `node:sqlite`'s `{ done, value }` key order is observable, so the cache is
built with that iterator's own order.
* Field 5 now holds the cache, so `reserved_slot_floor_for_class_id` rises
from 5 to 6 for the array iterator — without that, the first user property
added to an iterator (`it.foo = 1`) would land on the cache field.
Manual `.next()`, spread, `Array.from`, `yield*` and `for await` are unchanged
and keep allocating fresh results.
Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
16f4c3f to
b67b4d7
Compare
|
Rebased onto current Invariant re-derived on the new base rather than trusted to a clean merge: main Changelog fragment renamed to Taking this out of draft. Both conditions I set are met, and I want the flat Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
|
Landed on |
1 participant
What
The compiler's
for…ofdesugar emits one runtime call per advance,js_for_of_next(__iter). That entry already recycles one{ value, done }object per ITERATOR for builtin Map/Set iterators — the result local is a
compiler temporary the loop body cannot name, and the driver reads
doneandvalueout of it before the next advance, so mutating one cached object isunobservable. Array iterators fell through to the generic arm and minted a
fresh 40-byte object per element.
This gives them the same arm, and moves the recycling routine into
iter_result::emit_iter_result_cachedso the two families share ONEimplementation instead of a second copy — the drift
#7564removed from thefive result constructors that module replaced.
Why this one
The allocation-site census of the compiled claude-code TUI attributes 100 % of
its iterator-result bytes to
array::iter_objectunderjs_native_call_method(14.4 MB of a 3300-character reply; 15.5 % of all attributed arena bytes, the
third-largest category), and the
[gc-primitive-dispatch]counter says a400-character reply builds 167,946 of them.
for_of_guard.rs's index looponly covers a statically proven array, which a minified bundle almost never
gives you — so every generic
for…ofover an array pays one object per element.The campaign's ranking rule is "prefer removing work over making work cheaper".
This deletes the allocation rather than speeding it up, so CPU and resident bytes
move together.
What had to stay intact
nextstill wins. The override probe runs first on the fusedpath exactly as on the manual one.
node:sqlite's{ done, value }key order is observable, so the cache isbuilt with that iterator's own order and the shared keys array (and therefore
the shape) is picked per order.
reserved_slot_floor_for_class_idrises from 5 to 6 for the array iterator.Without that, the first user property added to an iterator (
it.foo = 1)would land on the cache field — the exact defect for…of over a Set iterator with a patched .next() segfaults (pre-existing, not #9017) #9019 fixed for the other
families.
for awaitlowers to the raw.next()property call (
needs_awaitinstmt_loops.rs), not the fused helper, andspread /
Array.from/yield*/ manual.next()all take the manualdispatcher. They keep allocating fresh results, so a caller that retains one
still sees spec behaviour.
Tests
fused_next_walks_an_array_and_recycles_its_result— correct walk, terminates,the cache field is installed on the first advance, and the driver hands back
the same object twice (without that assertion the test would pass with no
recycling at all). It also pins the negative: two manual
.next()calls returndifferent objects and the first still reads its own value after the second.
fused_next_routes_other_iterators_through_the_generic_arm— a string iteratorstill gets fresh results, so the fused branch cannot be "always taken".
Status
Draft until its rig table is posted here. The candidate binary is building;
the A/B is same-binary (
PERRY_FOR_OF_ARRAY_CACHEswitches the array arm off),against
cc_base_newand node in the same session under the campaign'smeasurement lock.
The falsifier is stated in advance:
[gc-iter-result] allocatedmust fall byroughly the element count while
fused_array_advancesdoes not, and theiter_resultsite must leave the allocation-site census. Ifallocateddoesnot move, the calls are not coming from the
for…ofdesugar and this fix isaimed at the wrong caller — in which case I will say so and close it rather
than look for another justification.
Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
Summary by CodeRabbit
Performance
for…ofperformance when iterating over arrays by reusing iterator result objects and reducing per-element allocations.Compatibility
.next(), spread,Array.from,yield*, andfor awaitusage.