perf(runtime): stop re-deriving the receiver on every array push (−74%)

changelog.d/10414-array-push-layout-note.md

@@ -0,0 +1,14 @@
+Drop the provably-no-op layout note from the raw-f64 array push.
+`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`, where the caller has
+already proved the value is a plain number. The mask work is then a no-op in
+every layout state the receiver can be in — the same argument already written
+out for the codegen-side elision on `array_store_needs_layout_note`'s object
+twin. The #7480 element-shape invariant is kept, through the resolved-flags
+entry so it reads the header the caller already holds.
+
+Validated on the shape that would expose a mistake: array slots filled with
+POINTERS, popped, then refilled with plain numbers, with a retained live graph.
+Three seeds under from-space protection, evacuation verification and
+PERRY_GC_FROMSPACE_SCAN_ABORT=1 each ran ~270,000 copying minors and ~33,700
+from-space scans with dangling=0 and missing_rewrites=0, byte-identical to node.

changelog.d/10414-array-push-receiver-requeries.md

@@ -0,0 +1,30 @@
+Stop re-deriving the receiver on every `Array.prototype.push`. A hot
+`for (…) { a.push(v); a.pop(); }` loop cost 1,350 instructions per push+pop
+pair, and a symbol-resolved profile showed more than half of that was not the
+append but the append re-asking questions about a receiver it had already
+resolved, once per helper in the chain. 1,350.6 -> 432.0, **-68.0%**, with
+nothing inlined and nothing cached.
+
+Four removals. `typed_feedback::numeric_array_push_guard` probed the property
+descriptor of `"length"` on every push — the heaviest frame in the loop at
+16.5% of samples — although the flag test three lines above had already
+rejected every receiver that could make it answer true (a non-writable
+`length` always marks `OBJ_FLAG_ARRAY_DESCRIPTORS`).
+`js_array_numeric_push_f64_unboxed` re-ran `clean_arr_ptr` three more times by
+asking `array_is_sealed_or_no_extend`, `array_is_frozen` and
+`guard_writable_length` in sequence, each of which goes through the
+non-resolved `array_object_flags`; that function alone was 24.0% of the loop.
+The append chain resolved twice more, and the exotic and numeric-layout checks
+once each. `js_array_pop_f64` already carried this exact fix — `push` never got
+it.
+
+Also fixes a parity bug the new 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 correct for the internal CreateDataProperty-style append
+that builds fresh result arrays and wrong for user `push`; the observable entry
+now throws, with node's wording. `Object.seal` had masked it by routing down
+the exotic path for an unrelated reason.
+
+Known divergence left: frozen `pop` still reports "Cannot mutate a frozen
+array" rather than node's "Cannot delete property 'N' of [object Array]".

crates/perry-runtime/src/array/header.rs

@@ -1340,6 +1340,18 @@ pub(crate) unsafe fn array_numeric_layout(arr: *const ArrayHeader) -> Option<Num
     if arr.is_null() {
         return None;
     }
+    unsafe { array_numeric_layout_resolved(arr) }
+}
+
+/// [`array_numeric_layout`] for an already-resolved head.
+///
+/// # Safety
+/// `arr` is a non-null [`clean_arr_ptr`] result with no intervening allocation
+/// or safepoint.
+#[inline]
+pub(crate) unsafe fn array_numeric_layout_resolved(
+    arr: *const ArrayHeader,
+) -> Option<NumericArrayLayout> {
     array_has_raw_f64_layout_flag(arr).then_some(NumericArrayLayout::RawF64)
 }
 
@@ -1375,6 +1387,17 @@ pub(crate) unsafe fn ensure_array_numeric_raw_f64(arr: *mut ArrayHeader) -> bool
     if arr.is_null() {
         return false;
     }
+    unsafe { ensure_array_numeric_raw_f64_resolved(arr) }
+}
+
+/// [`ensure_array_numeric_raw_f64`] for a receiver the caller has already put
+/// through [`clean_arr_ptr_mut`].
+///
+/// # Safety
+/// `arr` is that non-null resolved head, with no intervening allocation or
+/// safepoint — the same contract [`array_object_flags_resolved`] carries.
+#[inline]
+pub(crate) unsafe fn ensure_array_numeric_raw_f64_resolved(arr: *mut ArrayHeader) -> bool {
     let length = (*arr).length as usize;
     let capacity = (*arr).capacity as usize;
     if length > capacity || length > 16_000_000 {
@@ -1437,12 +1460,23 @@ pub(crate) unsafe fn array_numeric_raw_f64_set_inbounds(
 }
 
 #[inline]
-pub(crate) unsafe fn array_numeric_raw_f64_push_inbounds(
+
+/// [`array_numeric_raw_f64_push_inbounds`] for an already-resolved receiver.
+///
+/// The append chain re-entered `clean_arr_ptr` — allocator-ownership plus
+/// forwarding classification — once per helper: the unboxed push entry
+/// resolved, then this did, then `ensure_array_numeric_raw_f64` did again, all
+/// on the one pointer the entry had already proved live. Threading the resolved
+/// head through removes the repeats instead of caching their answer.
+///
+/// # Safety
+/// `arr` is a non-null [`clean_arr_ptr_mut`] result with no intervening
+/// allocation or safepoint.
+pub(crate) unsafe fn array_numeric_raw_f64_push_inbounds_resolved(
     arr: *mut ArrayHeader,
     value: f64,
 ) -> bool {
-    let arr = clean_arr_ptr_mut(arr);
-    if arr.is_null() || !ensure_array_numeric_raw_f64(arr) {
+    if !ensure_array_numeric_raw_f64_resolved(arr) {
         return false;
     }
     let length = (*arr).length;
@@ -1458,7 +1492,27 @@ pub(crate) unsafe fn array_numeric_raw_f64_push_inbounds(
     let elements_ptr = array_elements_ptr(arr) as *mut f64;
     // GC_STORE_AUDIT(POINTER_FREE): raw-f64 push stores numeric payloads only.
     std::ptr::write(elements_ptr.add(length as usize), number);
-    crate::gc::layout_note_slot(arr as usize, length as usize, number.to_bits());
+    // `layout_note_slot`'s MASK work is a provable no-op for a value that is a
+    // plain number, and `value_bits_to_number` just proved that above. The
+    // argument is the one already written out for the codegen-side elision on
+    // `array_store_needs_layout_note`'s object twin, and 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 note's `!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 non-pointer 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.
+    //
+    // That leaves the #7480 element-shape invariant, which is NOT part of that
+    // argument and is kept — through the resolved-flags entry, so it reads the
+    // header this function already holds instead of classifying the parent a
+    // second time. Measured: `layout_note_slot` was 15.0% of a push/pop loop,
+    // 96 of its 109 samples from this one call.
+    let flags = array_object_flags_resolved(arr);
+    crate::array::note_element_store_resolved_flags(arr, length as usize, number.to_bits(), flags);
     (*arr).length = length + 1;
     true
 }
@@ -1617,8 +1671,22 @@ pub extern "C" fn js_array_is_numeric_f64_layout(arr: *const ArrayHeader) -> i32
     if arr.is_null() {
         return 0;
     }
+    unsafe { js_array_is_numeric_f64_layout_resolved(arr) }
+}
+
+/// [`js_array_is_numeric_f64_layout`] for a caller holding a resolved head.
+///
+/// The typed-feedback push guard reaches this with a pointer it has already
+/// normalized, header-checked and proved non-forwarded, so the entry's own
+/// `clean_arr_ptr` — and the second one `array_numeric_layout` used to perform
+/// inside it — were both re-deriving that proof once per push.
+///
+/// # Safety
+/// `arr` is a non-null resolved head with no intervening allocation or
+/// safepoint.
+pub(crate) unsafe fn js_array_is_numeric_f64_layout_resolved(arr: *const ArrayHeader) -> i32 {
     unsafe {
-        if array_numeric_layout(arr) == Some(NumericArrayLayout::RawF64) {
+        if array_numeric_layout_resolved(arr) == Some(NumericArrayLayout::RawF64) {
             return 1;
         }
         // #6011 follow-up: a holes-flagged array (`new Array(n)` mid-fill)

crates/perry-runtime/src/array/indexing.rs

@@ -98,6 +98,35 @@ pub(crate) fn array_iteration_is_exotic(arr: *const ArrayHeader) -> bool {
     unsafe { array_iteration_is_exotic_resolved(arr, flags) }
 }
 
+/// [`array_iteration_is_exotic`] for a caller holding a resolved head and its
+/// flag word, but which has NOT excluded Buffer / TypedArray receivers.
+///
+/// This is the shape the hot append wants: `js_array_numeric_push_f64_unboxed`
+/// resolved the receiver and read the header once, so the only thing it still
+/// needs from `array_iteration_is_exotic` is the registry probe plus the policy
+/// tests — not a second `clean_arr_ptr`, which re-runs allocator-ownership and
+/// forwarding classification on a pointer already proved live one call up.
+///
+/// The registry probes stay: a Buffer or typed array must still be routed to
+/// the spec path, and the flag word cannot answer that (their headers are not
+/// `GC_TYPE_ARRAY`, so `array_object_flags_from_tag` reads them as `0`, which
+/// on its own would let a typed array reach the raw-f64 append).
+///
+/// # Safety
+///
+/// `arr` and `flags` must satisfy [`array_object_flags_resolved`]'s contract.
+pub(crate) unsafe fn array_iteration_is_exotic_cleaned(
+    arr: *const ArrayHeader,
+    flags: u16,
+) -> bool {
+    if crate::buffer::is_registered_buffer(arr as usize)
+        || crate::typedarray::lookup_typed_array_kind(arr as usize).is_some()
+    {
+        return true;
+    }
+    unsafe { array_iteration_is_exotic_resolved(arr, flags) }
+}
+
 /// [`array_iteration_is_exotic`] for a caller that already resolved the live
 /// plain-array head, excluded Buffer/TypedArray receivers, and owns the header
 /// word: the policy tests without a second receiver resolution and registry

crates/perry-runtime/src/array/mod.rs

@@ -102,7 +102,7 @@ pub use self::concat_reverse::{
 };
 pub(crate) use self::element_shape::{
     forget_element_shape, invalidate_all_element_shapes, note_element_store,
-    prune_dead_element_shape_owners, transfer_element_shape,
+    note_element_store_resolved_flags, prune_dead_element_shape_owners, transfer_element_shape,
 };
 pub use self::element_shape::{
     js_array_element_shape_check, js_array_element_shape_class, js_array_element_shape_epoch,
@@ -142,9 +142,9 @@ pub(crate) use self::generic_object::{
     object_splice,
 };
 pub(crate) use self::header::{
-    array_has_arguments_object_flag, mark_array_as_arguments_object,
-    rebuild_array_numeric_raw_f64_allow_holes, rebuild_array_numeric_raw_f64_dense_window,
-    rebuild_array_numeric_raw_f64_dense_window_i32,
+    array_has_arguments_object_flag, js_array_is_numeric_f64_layout_resolved,
+    mark_array_as_arguments_object, rebuild_array_numeric_raw_f64_allow_holes,
+    rebuild_array_numeric_raw_f64_dense_window, rebuild_array_numeric_raw_f64_dense_window_i32,
 };
 pub use self::header::{
     js_array_clear_numeric_layout, js_array_declare_all_pointer_elements,
@@ -160,8 +160,8 @@ pub use self::immutable::{
 };
 pub(crate) use self::indexing::{
     array_custom_prototype, array_has_own_index, array_iteration_is_exotic,
-    array_iteration_is_exotic_resolved, array_prototype_has_index_flag, array_spec_get,
-    array_spec_has_index, array_spec_set,
+    array_iteration_is_exotic_cleaned, array_iteration_is_exotic_resolved,
+    array_prototype_has_index_flag, array_spec_get, array_spec_has_index, array_spec_set,
 };
 pub use self::indexing::{
     js_array_get_element, js_array_get_element_f64, js_array_get_f64, js_array_get_f64_unchecked,
@@ -221,6 +221,7 @@ pub(crate) use self::prototype_addr::{
     test_memoized_prototype_addr, test_prototype_addr_cache_wiring, test_prototype_addr_cell_count,
     test_rewrite_prototype_addr_slot,
 };
+pub(crate) use self::push_pop::throw_non_extensible_array_push;
 pub(crate) use self::sort::object_prototype_has_index_prop;
 pub(crate) use self::sort::object_prototype_index_get as sort_object_prototype_index_get;
 pub(crate) use self::sort::object_prototype_index_get_with_receiver as sort_object_prototype_index_get_with_receiver;
@@ -281,18 +282,18 @@ pub(crate) use self::alloc::{js_array_from_arraylike, js_array_from_string_codep
 pub(crate) use self::flat_clone::{dense_spread_copy, dense_spread_source, flattenable_array_ptr};
 pub(crate) use self::header::{
     array_byte_size, array_is_frozen, array_is_sealed_or_no_extend, array_numeric_raw_f64_get,
-    array_numeric_raw_f64_push_inbounds, array_numeric_raw_f64_set_inbounds, array_object_flags,
-    array_object_flags_from_tag, array_object_flags_resolved, array_ptr_as_proxy,
-    array_receiver_addr, array_receiver_gc_tag, buffer_receiver_as_uint8_typed_array,
-    canonicalize_array_numeric_store_value_from_flags, clean_arr_ptr, clean_arr_ptr_mut,
-    clear_array_numeric_layout, clear_array_numeric_layout_ptr, finish_array_dense_move_layout,
-    gc_element_slot_range, mark_array_layout_unknown, mark_array_raw_f64_holes_fresh,
-    normalize_array_receiver, note_array_slot, note_array_slot_layout_only,
-    note_array_slot_resolved_flags, rebuild_array_layout, rebuild_array_layout_exact,
-    reclassify_array_numeric_layout_from_slots, refresh_array_numeric_layout,
-    replay_array_growth_write_barriers, set_array_numeric_layout, store_array_slot,
-    store_array_slot_resolved, typed_array_receiver, value_bits_to_number, NumericArrayLayout,
-    MIN_ARRAY_CAPACITY,
+    array_numeric_raw_f64_push_inbounds_resolved, array_numeric_raw_f64_set_inbounds,
+    array_object_flags, array_object_flags_from_tag, array_object_flags_resolved,
+    array_ptr_as_proxy, array_receiver_addr, array_receiver_gc_tag,
+    buffer_receiver_as_uint8_typed_array, canonicalize_array_numeric_store_value_from_flags,
+    clean_arr_ptr, clean_arr_ptr_mut, clear_array_numeric_layout, clear_array_numeric_layout_ptr,
+    finish_array_dense_move_layout, gc_element_slot_range, mark_array_layout_unknown,
+    mark_array_raw_f64_holes_fresh, normalize_array_receiver, note_array_slot,
+    note_array_slot_layout_only, note_array_slot_resolved_flags, rebuild_array_layout,
+    rebuild_array_layout_exact, reclassify_array_numeric_layout_from_slots,
+    refresh_array_numeric_layout, replay_array_growth_write_barriers, set_array_numeric_layout,
+    store_array_slot, store_array_slot_resolved, typed_array_receiver, value_bits_to_number,
+    NumericArrayLayout, MIN_ARRAY_CAPACITY,
 };
 pub(crate) use self::named_props::{
     array_has_named_properties_resolved, array_has_sparse_index_properties_resolved,

crates/perry-runtime/src/array/push_pop.rs

@@ -33,6 +33,17 @@ fn array_length_is_non_writable_with_flags(arr: *const ArrayHeader, flags: u16)
             .unwrap_or(false)
 }
 
+/// §23.1.3.21 push performs `Set(O, len, value, true)`; on a non-extensible
+/// receiver `CreateDataProperty` for the new index fails, and `Throw=true`
+/// makes that a TypeError. Node words it exactly this way for
+/// `preventExtensions`, `seal` AND `freeze`.
+#[cold]
+pub(crate) fn throw_non_extensible_array_push(index: u32) -> ! {
+    crate::collection_iter::throw_type_error(&format!(
+        "Cannot add property {index}, object is not extensible"
+    ));
+}
+
 #[cold]
 fn throw_non_writable_length() -> ! {
     crate::collection_iter::throw_type_error(
@@ -892,6 +903,25 @@ static KEEP_JS_ARRAY_PUSH_U31_WITH_LENGTH: extern "C" fn(
 #[no_mangle]
 pub extern "C" fn js_array_push_f64_spec(arr: *mut ArrayHeader, value: f64) -> *mut ArrayHeader {
     if let Some(plain) = direct_plain_push_receiver(arr) {
+        // A non-extensible receiver must THROW here, not decline silently.
+        // `js_array_push_f64_resolved` answers `SEALED | NO_EXTEND` with a bare
+        // `return arr`, which is right for its other caller — `js_array_push_f64`
+        // is the INTERNAL CreateDataProperty-style append that runtime code uses
+        // to build fresh result arrays, and those must not throw. It is wrong for
+        // user `push`: `Object.preventExtensions(a); a.push(1)` silently kept the
+        // old length where Node raises TypeError. `Object.seal` happened to throw
+        // only because sealing also marks the receiver's element descriptors,
+        // which sends it down the exotic route instead of this one.
+        //
+        // FROZEN is left to the resolved append below, which throws its own
+        // frozen message; only the extensibility bits are answered here.
+        // SAFETY: `direct_plain_push_receiver` just proved the resolved head.
+        let flags = unsafe { array_object_flags_resolved(plain) };
+        if flags & crate::gc::OBJ_FLAG_FROZEN == 0
+            && flags & (crate::gc::OBJ_FLAG_SEALED | crate::gc::OBJ_FLAG_NO_EXTEND) != 0
+        {
+            throw_non_extensible_array_push(unsafe { (*plain).length });
+        }
         crate::string::js_string_addref_if_heap_string(value);
         return unsafe { js_array_push_f64_resolved(plain, value) };
     }
@@ -982,15 +1012,34 @@ pub extern "C" fn js_array_numeric_push_f64_unboxed(
     if arr.is_null() {
         return js_array_alloc(0);
     }
-    if array_is_sealed_or_no_extend(arr) || array_is_frozen(arr) {
+    // ONE read of the already-resolved header answers all three integrity
+    // questions. Each of `array_is_sealed_or_no_extend`, `array_is_frozen` and
+    // `guard_writable_length` went through the non-resolved
+    // `array::header::array_object_flags`, which re-runs `clean_arr_ptr` — the
+    // allocator-ownership and forwarding classification — before every single
+    // bit test. On a pointer `clean_arr_ptr_mut` resolved on the line above,
+    // that is three further resolutions to re-derive a fact already proved.
+    //
+    // `array_object_flags_from_tag` keeps the exact semantics of the helpers it
+    // replaces: a receiver whose header is not `GC_TYPE_ARRAY` (a Buffer, a
+    // typed array) reads as flags `0`, just as `array_object_flags` returned 0
+    // for it, so the exotic check below still owns those receivers.
+    //
+    // Measured on `for (…) { a.push(v); a.pop(); }`: `array_object_flags` was
+    // 24.0% of all samples in the loop, every one of them from this function.
+    let flags = crate::array::array_object_flags_from_tag(crate::array::array_receiver_gc_tag(arr));
+    if flags
+        & (crate::gc::OBJ_FLAG_SEALED | crate::gc::OBJ_FLAG_NO_EXTEND | crate::gc::OBJ_FLAG_FROZEN)
+        != 0
+    {
         return arr;
     }
-    guard_writable_length(arr);
+    guard_writable_length_with_flags(arr, flags);
     unsafe {
-        if crate::array::array_iteration_is_exotic(arr) {
+        if crate::array::array_iteration_is_exotic_cleaned(arr, flags) {
             return js_array_push_f64_spec(arr, value);
         }
-        if array_numeric_raw_f64_push_inbounds(arr, value) {
+        if crate::array::array_numeric_raw_f64_push_inbounds_resolved(arr, value) {
             return arr;
         }
     }

crates/perry-runtime/src/object/native_call_method/common_methods.rs

@@ -897,7 +897,10 @@ pub(super) unsafe fn dispatch_common(
                 jsval.as_pointer::<crate::array::ArrayHeader>() as *mut crate::array::ArrayHeader;
             // Spec §23.1.3.21: length is Set even with 0 args, so guards fire regardless
             if crate::array::array_is_frozen(arr_ptr) {
-                crate::collection_iter::throw_type_error("Cannot mutate a frozen array");
+                // What fails is CreateDataProperty for the NEW index, not a
+                // write to an existing read-only one, so node words this the
+                // same for freeze / seal / preventExtensions.
+                crate::array::throw_non_extensible_array_push(unsafe { (*arr_ptr).length });
             }
             crate::array::guard_writable_length(arr_ptr);
             let mut arr = arr_ptr;