perf(array): copy an ordinary dense array's spread instead of driving the iterator protocol (#7533) - #7540
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughDense-array spread now uses a validated direct-copy path. The path converts holes to ChangesDense array spread optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant array_from_spread_value
participant dense_spread_source
participant dense_spread_copy
participant ArrayHeader
array_from_spread_value->>dense_spread_source: validate source
dense_spread_source-->>array_from_spread_value: eligible dense array
array_from_spread_value->>dense_spread_copy: copy source
dense_spread_copy->>ArrayHeader: allocate and copy elements
ArrayHeader-->>array_from_spread_value: completed array
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 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 |
7534 was already taken by the engine-plan baseline fragment on main. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
|
@coderabbitai review |
|
Additional validation: string aliasing
The strings are 64 chars built by repeated |
Known residual in the guard itself, for the recordAfter the change
Both are cheap relative to the 49× they buy, and both are needed for correctness as written, so they are deliberately left alone here rather than optimised under the same change. Caching the well-known symbol pointer runtime-side would be the obvious next step and is exactly the shape CLAUDE.md warns about — "a runtime-side cache of a raw heap pointer is a GC root, and the static checker cannot see it" — so it wants its own change with a |
Gap-suite triage: one flagged test, investigated, not oursA local gap run flagged
The output is byte-identical between this branch and Worth its own issue independent of this PR: a 56–178 s gap test that the snapshot expects to pass is either relying on a much larger CI timeout than the 10 s used locally, or is a latent CI flake waiting for a slow runner. Flagging rather than filing, since I have not confirmed which. The other gap failures seen in the same run — |
Gap suite: second flagged test is a documented host-local flakeRunning the full gap suite locally (
Everything else failing is already in the snapshot: two Notably |
Gap suite: first run invalidated by a full disk at ~test 400; re-runningRecording this rather than quietly re-running, because a disk-full parity sweep produces a storm of fake The host went from 42 GB free to 0 during the run (this branch's two release target dirs plus other work on the same machine). Everything from roughly test 400/489 onward in that log is garbage and must not be read as parity data. The portion before the disk filled (~400 tests) is valid and clean. Ten failures, of which eight are already recorded in
Disk has been reclaimed (25 GB free, 24 stale |
… the iterator protocol (#7533) `[...arr]` ran the full ECMA-262 GetIterator + per-element `.next()` protocol even for a plain dense array. On the `object_deep_clone` app-pattern kernel — the worst row in the public artifact at 37.5x bun — a symbolicated profile puts 90.45% of the whole process inside `array_from_spread_value`, and the identical copy through `Array.from`'s memcpy tail is 66x cheaper (0.09 s vs 5.93 s at N=500,000). Two structural costs, neither observable for an ordinary array: `@@iterator` resolves through the by-name prototype tower and builds a named bound closure (30.89% inclusive), and every `.next()` allocates five heap objects in `build_iter_result` (the `{value, done}` object, its two key strings, its keys array) plus the iterator object once per spread — ~25 allocations for a 3-element spread. `dense_spread_source` proves ordinariness (real GC_TYPE_ARRAY via `try_read_gc_header`, which rejects the handle band and the header-less small-buffer slab without a deref; not exotic; `Array.prototype[Symbol.iterator]` unmodified; no own `[Symbol.iterator]`) and `dense_spread_copy` copies the elements, normalising TAG_HOLE to undefined. Anything unproven falls through to the unchanged protocol. The own-symbol guard is a new non-invoking `symbol::has_own_symbol_property`: `own_symbol_property` answers by reading, which calls a user getter, and the slow path reads the property again — a getter must observe exactly one call. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…t path `js_array_grow` (issue #233) leaves a `GC_FLAG_FORWARDED` header at the OLD address, whose first eight bytes — where `length`/`capacity` used to live — now hold the forwarding pointer. `dense_spread_source` took the raw NaN-boxed address and read `length` off that retired header, and the memcpy sized from it walked off the heap: `[...sparse]` after `sparse.length = 5` and `[...beyond]` after `beyond[9] = 9` both took EXC_BAD_ACCESS in `_platform_memmove`. Band/slab validation still runs first (`try_read_gc_header` must reject a handle id without dereferencing it), then `clean_arr_ptr` follows the chain, then the `GC_TYPE_ARRAY` check is re-read off the POST-forwarding header. `dense_spread_copy` cleans on both of its reads, so the post-allocation re-read cannot regress to the stale address either. `a_grown_arrays_forwarding_header_is_followed_not_copied` pins it, and asserts the probe really forced a grow-and-forward (`cleaned != src`) before testing anything — a version that failed to grow would pass vacuously. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
#6081 sibling) `js_array_map` roots its callback (#6081): a callback allocated by a frameless caller — the arrow in `xs.map(x => …)` — is reachable only through the raw parameter and the native stack, which an evacuating minor does not scan, so from the second element on the dispatch reads a moved-or-swept closure. `js_array_map_discard` was missed and kept using the bare parameter across `js_closure_call3`, which allocates. Latent until now, because a stale root only bites when a collection lands inside its window. #7533's dense-spread fast path removes ~25 allocations per iteration of `object_deep_clone` and moved every subsequent collection; one now lands squarely in this loop and `PERRY_GC_PROTECT_FROMSPACE=1` faults on a retired `obj_type=4` (GC_TYPE_CLOSURE) at `js_array_map_discard + 788`. The kernel faults under the same instrument at the PREVIOUS commit too — at a different site, inside `array_from_spread_value` — so this is a pre-existing defect exposed by new timing, not one introduced by it. Rooted NaN-boxed rather than via `root_raw_const_ptr`, so the per-callsite read-back is a `get_nanbox_f64` and the module stays out of `scripts/raw_handle_debt.py`'s ledger (unchanged at 999). Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…d tests The addr-class ratchet flags bare band literals. Same coverage, named bands. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
7534 was already taken by the engine-plan baseline fragment on main. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
f625394 to
a3bb5d6
Compare
|
@coderabbitai full review |
|
Final gap-suite note (post-merge, for the record)Both full gap runs on this host were invalidated by local infrastructure, each for a different reason, and neither produced evidence against this change. Recording so the logs are not later mistaken for parity data:
Run 2's very first Across the two runs' valid portions, nothing failed that is not already recorded in |
1 participant
Closes #7533.
Phase 1 — the profile, before touching anything
object_deep_clonewas the worst row in the public artifact by a wide margin(657 ms vs bun 17.5 ms, 37.5×; next worst 11.4×). A symbolicated profile on
the pinned quiet mini (
PERRY_DEBUG_SYMBOLS=1, 4665 leaf samples, main thread)says the entire gap is one line of the kernel:
array_from_spread_valueis 90.45% of the whole process. Decomposition byconstruct (N=500 000, best-of-3 wall, quiet M1 mini):
dc_full(the kernel)dc_spread[...o.meta.tags]dc_map.map(x => ({…}))dc_litdc_slicetags.slice()dc_arrfromArray.from(tags)dc_readThe same copy via
Array.fromis 66× cheaper than via spread, for anidentical result —
Array.fromreachesjs_array_clone's memcpy tail, spreaddoes not. Inside the spread:
array_from_spread_valuejs_object_get_symbol_property— resolving@@iteratorjs_iterator_to_array— the.next()drainjs_native_call_method→dispatch_array_iterator_methodjs_object_get_field_by_name(.value/.done)build_iter_resultRuntimeHandleScope_tlv_get_addrTwo structural costs, neither observable for an ordinary array.
@@iteratorresolves through the by-name prototype tower (
js_object_get_field_by_name_f64→
get_field_by_name_object_tail→array_prototype_property_value→ recursion→
default_object_prototype_property_value→fetch_subclass_handle_id) andthen builds a named bound closure. And every
.next()allocates five heapobjects in
build_iter_result— the{ value, done }object, its two keystrings, its keys array — plus the iterator object once per spread. A 3-element
[...tags]costs ~25 allocations where bun does one allocation and a 24-bytememcpy.
Phase 2 — the self-suspicion, on the record: the rooting work is exonerated
The issue asked whether #7495 / #7516 / #7527 were material. Two independent
answers, both no.
1. At
f06270d06the kernel does not run at all. Built at that commit withmy own build, identical
-pset:object_deep_cloneexits 1 withTypeError: next is not a function— the #7475 defect #7495 fixed. There is no "before"timing to regress from; the kernel is measurable only because of that stack.
2. On every path that runs in both arms,
mainis faster. Interleavedbest-of-5 on the quiet host:
f06270d06maindc_litdc_mapdc_readdc_slicedc_map_numdc_arrfromThe single measurable rooting cost is
dc_arrfrom: ~40 ns perjs_array_clonecall, from #7497's
RuntimeHandleScopein the memcpy tail. That path is noton this kernel at all (the spread never reaches the memcpy tail), and 0.02 s
against a 6.20 s kernel would be 0.3% if it were. The day's perf work (#7510,
#7515, #7496) more than pays for it. The 37.5× is entirely structural and
entirely pre-existing.
Phase 3 — the fix
array_from_spread_valuenow takes a dense fast path first.dense_spread_sourceproves the array is ordinary;
dense_spread_copycopies the elements. Anythingit cannot prove falls through to the unchanged protocol. Every gate rejects a way
the copy could differ from the drain:
try_read_gc_header+GC_TYPE_ARRAY— a real dense array, not aSet/Map/Buffer/TypedArray/lazy array (each has its own
obj_type), not a proxyor native handle (rejected by band, without a deref), not a header-less
small-buffer slab allocation.
class X extends Arrayis object-backed andkeeps its snapshot path.
array_iteration_is_exotic— no per-index accessor descriptors, noArray.prototype/Object.prototypeindex properties, no live indices past thedense store.
array_proto_iterator_modified— the sticky flag for a replaced or deletedArray.prototype[Symbol.iterator].has_own_symbol_property— a new non-invoking existence probe. Theexisting
own_symbol_propertyanswers by reading, which calls a user getter;the slow path this falls back to reads the property again, and a getter must
observe exactly one call.
Holes are the one place a copy and the drain disagree — the drain reads
arr[i](
undefined), the slot holdsTAG_HOLE— so they are normalised, keeping[...[1, , 3]]at[1, undefined, 3]and1 in [...[1, , 3]]attrue.Result (same host, same method, best-of-7)
object_deep_clone(N=50 000)dc_spreadalone (N=500 000)dc_full(N=500 000)15.3× on the kernel, 49× on the spread itself. Against the artifact's bun
figure the row moves from 37.5× to ~2.3×, and from 11.5× node to 0.67×
node — a win.
Two defects found and fixed along the way
js_array_map_discardnever rooted its callback — #6081's missed sibling.js_array_maproots it because a callback allocated by a frameless caller (thearrow in
xs.map(x => …)) is reachable only through the raw parameter and thenative stack, which an evacuating minor does not scan. The discard variant used
the bare parameter across
js_closure_call3, which allocates.It was latent: a stale root only bites when a collection lands in its window.
Removing ~25 allocations per iteration moved every subsequent collection and
dropped one inside that loop, so the kernel started failing
TypeError: value is not a functionandPERRY_GC_PROTECT_FROMSPACE=1named the site — a retiredobj_type=4(GC_TYPE_CLOSURE) atjs_array_map_discard + 788. The kernelfaults under the same instrument at the parent commit too, at a different site
inside
array_from_spread_value, which is how it was established as pre-existingrather than introduced by this change. Rooted NaN-boxed, so
scripts/raw_handle_debt.pystays at exactly 999.The fast path itself shipped one bug during development (fixed in its own
commit, recorded because the shape recurs): it read
lengthstraight off theaddress in the NaN-box without
clean_arr_ptr.js_array_grow(#233) leaves aGC_FLAG_FORWARDEDheader at the OLD address whose first eight bytes — wherelength/capacityused to live — now hold the forwarding pointer, so the memcpywas sized from a forwarding pointer read as a length.
[...sparse]aftersparse.length = 5and[...beyond]afterbeyond[9] = 9both tookEXC_BAD_ACCESS in
_platform_memmove.Validation
cargo test -p perry-runtime --no-fail-fast— 1772 passed, 0 failed(10 new).
scripts/auto_opt_app_patterns.sh— 12/12, each linking a freshly builtperry-auto-*archive.(auto-optimize and
PERRY_NO_AUTO_OPTIMIZE=1).[...arr]semantics matrix (holes, sparse, past-capacity indices,frozen, index accessors, subclass, own
@@iterator, patchedArray.prototype[Symbol.iterator], strings/Set/Map/generators/iterators, a1000-element array, multi-element and call spreads) — byte-identical to
pre-change Perry, and to node except two pre-existing divergences (below).
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800on aPERRY_GC_MOVING_LOOP_POLLS=1build: 50 005 retired page sets quarantined, zero faults, correct
checksum. The instrument is proven live by its
[gc-fromspace-protect] mode=… retired_set=#Nlines, not assumed.raw_handle_debt.py999/999,check_file_size.shOK,addr_class_inventory.pypassed,
cargo fmt --all -- --checkclean.The new tests pin
dense_spread_source's verdict, not only the resultingelements — the slow path is a correct fallback, so an elements-only test would
stay green if the fast path silently stopped applying (CLAUDE.md's fourth way a
gate can be unable to fail). The forwarding test asserts the probe really forced
a grow-and-forward before testing anything.
Two pre-existing
[...arr]divergences from node, filed separatelyFound while building the semantics matrix; unchanged by this PR and verified
byte-identical at
f06270d06:[...MyArr.from([1,2,3])]onclass MyArr extends Arraythrowsvalue is not iterable.Array.prototype[Symbol.iterator]is ignored by spread —[...[1,2,3]]yields[1,2,3]where node yields the patched iterator'soutput.
Residual
array_from_spread_value's@@iteratorwalk still runs the by-name prototypetower for every non-dense-array spread (Sets, Maps, generators, iterators), and
build_iter_resultstill allocates five objects per.next(). Both are now offthe common path but remain worth their own tickets.
Summary by CodeRabbit
Performance
undefined.Bug Fixes
Documentation
0.5.1308.