Merge train 213: array push receiver, copying-minor single decode (v0.5.1591) - #10546
Merged
Merged
Conversation
A hot `for (…) { a.push(v); a.pop(); }` loop costs 1,350 instructions per
push+pop pair. Profiled with symbols, more than half of that is not the append
— it is the append re-asking questions about a receiver it has already
resolved, once per helper in the chain.
Four removals, no inlining and no caching; the runtime gets smaller.
1. `typed_feedback::numeric_array_push_guard` probed the property descriptor
of `"length"` on EVERY push — a string-keyed lookup that was the single
heaviest frame in the loop at 16.5% of samples, more than the append it
guarded. It was also unreachable-true: a non-writable `length` is only
reachable through a descriptor write, every one of which marks the receiver
`OBJ_FLAG_ARRAY_DESCRIPTORS` (documented on
`array::named_props::mark_array_descriptors` as the shared "index-accessor /
non-writable-length / sparse-index" gate), and the flag test three lines
above has already returned `false` for any receiver carrying it.
`array_length_is_non_writable_with_flags` encodes the same implication the
other way round, short-circuiting on the flag before it will look anything
up.
2. `js_array_numeric_push_f64_unboxed` asked `array_is_sealed_or_no_extend`,
`array_is_frozen` and `guard_writable_length` in sequence. Each goes through
the non-resolved `array::header::array_object_flags`, which re-runs
`clean_arr_ptr` — allocator-ownership plus forwarding classification —
before every single bit test, on the pointer `clean_arr_ptr_mut` resolved on
the line above. `array_object_flags` was 24.0% of the loop, every sample of
it from this one function. One read of the resolved header now answers all
three.
3. The append chain resolved twice more: `array_numeric_raw_f64_push_inbounds`
and then `ensure_array_numeric_raw_f64` inside it.
4. `array_iteration_is_exotic` and the layout chain
(`js_array_is_numeric_f64_layout` → `array_numeric_layout`) each resolved
again. The exotic check keeps its Buffer / TypedArray registry probes — the
flag word cannot answer that, since those headers are not `GC_TYPE_ARRAY`
and read as flags `0`.
1,350.6 -> 432.0 instructions per push+pop pair, -68.0%. Measured by
differencing two probes that differ only in push count, inside each binary, so
driver dispatch and code layout cancel before the arms are compared; the bare
loop control reads 0.05 and -0.04 in the two arms.
`js_array_pop_f64` already carries this exact fix — its comment describes the
same three redundant classifications per pop. `push` never got it.
Also fixes a parity bug the integrity fixture found:
`Object.preventExtensions(a); a.push(1)` silently kept the old length where
node throws. The dense append answers `SEALED | NO_EXTEND` with a bare
`return arr`, which is right for `js_array_push_f64` — the INTERNAL
CreateDataProperty-style append that runtime code uses to build fresh result
arrays, and which must not throw — and wrong for user `push`. The observable
entry now throws. `Object.seal` masked this: sealing also marks the element
descriptors, so it routed down the exotic path and threw for another reason.
The frozen-push message now matches node's wording too, since what fails is
CreateDataProperty for the new index rather than a write to a read-only one.
Known remaining divergence, not addressed here: frozen `pop` reports "Cannot
mutate a frozen array" where node says "Cannot delete property 'N' of
[object Array]". It throws from an earlier branch than the push path, and
getting it right needs care about the empty-array ordering.
`layout_note_slot` was 15.0% of a push/pop loop — 96 of its 109 samples from the single call in `array_numeric_raw_f64_push_inbounds`. Its MASK work is a provable no-op there, because the caller has already proved the value is a plain number (`value_bits_to_number` returned `Some` two lines above). That argument is not new: it is written out in full on the object twin of `array_store_needs_layout_note`, which is how the codegen-side element store already elides the same call. It holds in every layout state the receiver can be in — `GC_LAYOUT_UNKNOWN` returns at the note's own state check; an intact typed descriptor lets a non-pointer fall through the pointer-mask arm untouched; `GC_LAYOUT_POINTER_FREE` hits the `!pointer && POINTER_FREE` early return; and under `GC_LAYOUT_SIDE_MASK` the note could only ever CLEAR this slot's bit, so skipping it leaves at worst a stale set bit over a numeric word, which costs one extra visit and nothing else because `gc::trace::mark_field_into_worklist` re-validates every slot word and rejects f64 bit patterns as out-of-range addresses. The #7480 element-shape invariant is NOT part of that argument and is kept — through `note_element_store_resolved_flags`, so it reads the header this function already holds instead of classifying the parent a second time. 346.0 instructions per push+pop pair, from 432.0. Together with the receiver work in this branch's first commit: 1,350.7 -> 346.0, -74.4%. The stale-mask case is the one that has to survive, so it gets a fixture built to produce it: fill array slots with POINTERS, pop them all, then refill the same slots with plain numbers, and interleave numbers and pointers in one array while a retained subset keeps a live graph. Under seeded GC scheduling with from-space protection, evacuation verification and PERRY_GC_FROMSPACE_SCAN_ABORT=1, three seeds each ran ~270,000 copying minors and ~33,700 from-space scans with dangling=0 and missing_rewrites=0, output byte-identical to node every time.
`array_numeric_raw_f64_push_inbounds` existed only to resolve the receiver and delegate. Its one caller now holds a resolved head and calls the _resolved core directly, so the wrapper is dead — `-D warnings` caught it as an unused import plus a never-used function, which is the right verdict. Deleted rather than re-exported or silenced: leaving a resolving wrapper in place is exactly the shape this branch is removing.
Base: e6dcb62 (main). A raw (untagged) word was classified TWICE on the copying minor's slot path: `CopyingPointerSet::decode_bits` classified it only to validate it, and `mark_addr` classified it again. Every traced shaped object visits its shape record's `keys` word, a raw address, so that was a second page-table probe and `plausible_gc_header` read per traced object. The remembering arm then re-decoded the very slot the visit had just decoded. `visit_value_bits_child` now decodes, classifies and marks once, and returns the child's address as the word reads after the visit. The validating classification is the one the mark uses (the memo is still consulted after it, in the same order as before), and the remembering arm reuses that child; only a raw word that MOVED is validated again, which is all the re-decode could still reject. Two codegen facts are load-bearing, measured, and pinned by comment: * the decode is `#[inline(always)]` — out of line, its frame and the by-memory return of its result cost as much as the classification it saves (that first cut measured flat to +1.05% on the six fixtures); * `barrier_parent_needs_remembering` is asked BEFORE the visit. It reads only the parent and the slot's own address, never the child, so the order cannot change its answer — but asked after, the optimizer duplicated the call into both decode arms and stopped inlining it, which cost a third of the win on gc3 and more on w20000. instructions:u, min of 5, same host, base vs this: gc3 11,756,388,818 -> 11,541,805,498 -1.83% w5000 1,886,467,237 -> 1,853,133,603 -1.77% w20000 4,727,775,800 -> 4,655,712,038 -1.52% oldyoung 1,454,636,978 -> 1,433,367,558 -1.46% w1000 1,045,691,197 -> 1,036,890,921 -0.84% alloc 320,204,861 -> 320,203,910 -0.00% Exact counts under callgrind agree (gc3 -1.79%) and attribute it: `classify_arena` calls fall from 6.09M to 4.20M on gc3, and on the pointer-slot control the per-slot term falls from 379.2 to 349.6 instructions at K=16. Peak RSS and max pause are flat within their own run-to-run spread on all six fixtures. Witness: `gc::tests::copy_slot_decode`, two behavioural tests each with a sabotaged twin — a raw word's child must be evacuated and the word rewritten (sabotage: drop the validated raw word, and the word goes stale), and an old parent's edge must be re-remembered from the decoded child (sabotage: forget it, and `restore_surviving_dirty_coverage`'s cross-check refuses the cycle).
#10491's new `copy_slot_decode.rs` helper open-codes the StringHeader payload offset as `.add(size_of::<StringHeader>())`, which raises the string payload-access ratchet for perry-runtime from 350 to 351. The baseline is debt, not an allowance for new code. `crate::string::string_data()` is the sanctioned accessor and is what `OwnedStringBytes::copy_from_header` uses internally, so the read is byte-for-byte identical. Follows `gc/tests/concat_site.rs:29`.
This was referenced Sep 17, 2026
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThe changes optimize numeric array push paths, correct integrity-error handling, and remove duplicate copying-GC word decoding. New tests cover array integrity, pointer-slot reuse, raw-child evacuation, and remembered-set behavior. ChangesNumeric Array Push
Copying Minor Decode
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium ✨ 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 |
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.
This train lands #10414 and #10491 as v0.5.1591, on
5030e6eed6. Seven source commits, each verified to preserve its patch-id and authorship. The two PRs touch no file in common.#10362) — the copying minor decodes each visited word once: a raw word's validating classification is the one the mark uses, and the remembering arm reuses the child the visit decoded instead of re-decoding the slot.Train repair
#10491 raised the string payload-access ratchet,
perry-runtime350 → 351. Its newgc/tests/copy_slot_decode.rshelper open-codes the payload offset as(s as *const u8).add(size_of::<StringHeader>()). The ratchet's own header says the committed baseline is debt, not an allowance for new code, so re-baselining would have been the wrong fix even though the script suggests it.Replaced with
crate::string::string_data(s), which is literally the same expression —— and is what
OwnedStringBytes::copy_from_headeruses internally, so the read is byte-for-byte identical. Follows the existing form atgc/tests/concat_site.rs:29.lintreturns to 1 of 83.One test is release-incompatible by design
gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_checkfails under--releaseand cannot do otherwise. Its observable is therestore_surviving_dirty_coveragecross-check, gated behind#[cfg(debug_assertions)]atcopying.rs:1033; in a release build that walk silently re-adds the page the sabotaged arm failed to remember, so the forgotten entry is invisible. The test's own doc says exactly this.Verified where the mechanism exists, rather than assumed:
[profile.gcaudit]is release codegen withdebug-assertions = true, and exists inCargo.tomlfor precisely this. Worth stating plainly, because[profile.release]and[profile.perry-dev]both compile these assertions out: a guard or sabotage test evidenced only by adebug_assert!is unenforced in what ships.Validation
Validated head
80f7a4b615. Five-package release build pinned and hash-verified, and re-verified after the gap run.main's long-knownheap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds, whose assertion is likewise#[cfg(debug_assertions)]-gated.push,array,weak,gc,object,shape.pushis clean — the most direct filter for perf(runtime): stop re-deriving the receiver on every array push (−74%) #10414.Reds attributed
Eleven fixtures A/B'd against
main's own artifact set; ten behave identically on both arms, with distinct build stamps asserted per arm. These were re-attributed from scratch rather than reused from earlier trains: #10414 changes array push receiver derivation and #10491 changes the copying minor, so array, gc, weak and shape fixtures are all genuinely in scope and a verdict measured on binaries that predate them proves nothing. The two that mattered most —test_issue_2656_weakref_finalization_gcandtest_guarded_raw_numeric_arrays— are both clear.The eleventh,
test_issue_1425_gc_unsafe_zones, is environmental: both arms refuse to build with byte-identical error text because the fixture needs the fastify adapter, which was removed from the in-stdlib build and requires compiling withoutPERRY_NO_AUTO_OPTIMIZE. Symmetric agreement in a known environmental mode, not a code difference.test_issue_4826_array_headersneeds no A/B — it isnode_fail, an oracle failure, and node never runs Perry's output.test_issue58_object_stringis listed verbatim inrun_parity_tests.sh'sSKIP_TESTSonmain.Note that
--filteris a substring match over all fixtures, so these runs selecttest_issue_*/test_perry_*names that CI's gap suite (--filter test_gap_) never runs. Everytest_gap_*fixture in scope passed.Before merging, the pushed head and unchanged main are checked again. After merging, the rewritten commits are checked for preserved authorship and the main tree must match the validated train exactly.
Summary by CodeRabbit
New Features
Array.prototype.pushperformance for numeric arrays.Bug Fixes
Documentation