fix(gc): a non-empty array literal silently voided its #7469 all-pointer declaration (#8102)

changelog.d/8114-nonempty-array-literal-all-pointer.md

@@ -0,0 +1,42 @@
+### Fixed
+
+- **A non-empty array literal silently voided its `#7469` all-pointer element
+  declaration, costing 26% on a push loop (#8102).**
+  `js_array_declare_all_pointer_elements` refused every array with
+  `length != 0`. But `emit_all_pointer_array_declaration` is emitted from the
+  `Stmt::Let` tail — *after* an array literal's element stores have run and
+  installed a per-slot side mask — so for `const a: C[] = [x, y]` the
+  declaration was a no-op. The side mask survived, every later `a.push(…)`
+  failed the elided-push header test in `expr/array_push.rs`, and each push paid
+  the per-store `js_gc_note_slot_layout` that #7469 exists to delete.
+  `collectors/all_pointer_arrays.rs` already admits such a literal (its
+  `literal_of_object_elements_is_admitted` test), so the proof was issued at
+  compile time and discarded at run time — CLAUDE.md failure mode 4, inside the
+  optimization rather than inside a gate.
+
+  The declaration now *discharges* the claim rather than assuming it:
+  `layout_all_pointer_slots_would_hold` walks the initialized prefix and
+  requires every slot to be pointer-bearing by `layout_pointer_bearing_bits`,
+  the same predicate the layout-mask builder and `GC_LAYOUT_UNKNOWN`'s per-slot
+  re-validation use, so it never has to trust the caller's static proof. A
+  refusal leaves the header byte-unchanged (the raw-f64 bits are cleared only
+  once the declaration is known to stick) and `length == 0` holds vacuously, so
+  both pre-existing paths are bit-identical. Runtime-only: no ABI change, no
+  codegen change.
+
+  Measured with one compiler binary and only the `libperry_{runtime,stdlib}.a`
+  pair swapped, `--release` both sides, instructions retired, medians of 3
+  interleaved reps: 4,000,000 pushes into `const a: C[] = [x, y]` go
+  20,755,859,948 → **15,309,077,609 (−26.2%)**, while the two controls — the
+  same pushes into `const a: C[] = []`, and `[]` plus two pushes then the same
+  loop — move −0.1% and +0.1%. Validated by `cargo test -p perry-runtime --lib`
+  (2334 passed) and an output/exit-code A/B over 61 programs
+  (`benchmarks/suite`, `benchmarks/app-patterns/kernels`, the beat-scriptc sweep
+  corpus): 61/61 structurally identical, the only differences being printed
+  elapsed-millisecond lines.
+
+  The new coverage carries its own sabotage arm:
+  `a_non_pointer_element_in_the_literal_still_refuses_the_declaration` runs the
+  identical construction sequence with one numeric element and asserts both the
+  refusal and that `_reserved` is unchanged, so a green positive test means the
+  predicate discriminates rather than that nothing was tried.

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

@@ -1619,19 +1619,45 @@ pub extern "C" fn js_array_note_numeric_write(arr: *mut ArrayHeader, value_bits:
 ///    read (`mark_field_into_worklist` re-validates every word), never a
 ///    stranded live child.
 ///
-/// Refused on a non-empty array: the claim covers `0..length`, and only on an
-/// empty array is it vacuously true of what is already stored. A refusal is
-/// silent and safe — the header keeps whatever layout it had, and the codegen
-/// header test then declines the elided store and routes the push through
-/// `js_array_push_f64`, which notes every slot as it always did.
+/// On a **non-empty** array the claim is not vacuous, so it is discharged
+/// rather than assumed: every slot in `0..length` must be pointer-bearing by
+/// `gc::layout_pointer_bearing_bits`, the same predicate the layout mask
+/// builder and `GC_LAYOUT_UNKNOWN`'s per-slot re-validation use. The walk is
+/// O(literal size), runs once at the binding, and does not have to trust the
+/// caller's static proof.
+///
+/// #8102 is why that path exists. `emit_all_pointer_array_declaration` is
+/// emitted from the `Stmt::Let` tail, i.e. *after* an array literal's element
+/// stores have already installed a per-slot side mask. Refusing every
+/// non-empty array therefore made the declaration a **silent no-op** for
+/// `const a: C[] = [x, y]`, so every later `a.push(…)` failed the codegen
+/// header test and paid the per-store layout note #7469 exists to delete —
+/// measured at +33.9% instructions on a 4M-push loop, against the
+/// byte-for-byte equivalent array built empty and pushed into.
+/// `collectors/all_pointer_arrays.rs` already admits such a literal (see
+/// `literal_of_object_elements_is_admitted`), so the proof was being issued and
+/// then discarded.
+///
+/// A refusal is still silent and safe — the header keeps whatever layout it
+/// had, and the codegen header test then declines the elided store and routes
+/// the push through `js_array_push_f64`, which notes every slot as it always
+/// did.
 #[no_mangle]
 pub extern "C" fn js_array_declare_all_pointer_elements(arr: *mut ArrayHeader) {
     let arr = clean_arr_ptr_mut(arr);
     if arr.is_null() {
         return;
     }
     unsafe {
-        if (*arr).length != 0 {
+        let length = (*arr).length as usize;
+        let slots = if length == 0 {
+            std::ptr::null()
+        } else {
+            array_elements_ptr(arr) as *const u64
+        };
+        // Clear the raw-f64 claim FIRST only when the declaration will stick:
+        // a refused declaration must leave the header exactly as it found it.
+        if !crate::gc::layout_all_pointer_slots_would_hold(slots, length) {
             return;
         }
         clear_array_numeric_layout(arr);

crates/perry-runtime/src/gc/layout.rs

@@ -444,6 +444,37 @@ pub(crate) unsafe fn layout_init_all_pointer_slots(user_ptr: *mut u8) {
     (*header)._reserved |= GC_LAYOUT_ALL_POINTERS;
 }
 
+/// Does the all-pointer claim actually hold for a payload that **already holds
+/// initialized slots** — the non-empty array literal `const a: C[] = [x, y]`?
+///
+/// [`layout_init_all_pointer_slots`]' array caller
+/// (`js_array_declare_all_pointer_elements`) used to refuse every non-empty
+/// array outright, because the claim covers `0..length` and only an empty array
+/// makes it vacuously true. #8102 is what that cost: the declaration is emitted
+/// from the `Stmt::Let` tail, i.e. *after* a literal's element stores have
+/// installed a per-slot side mask, so for a non-empty literal it was a silent
+/// no-op and every later `push` lost #7469's elided store.
+///
+/// This predicate discharges the claim instead of assuming it. Every slot must
+/// be pointer-bearing by [`layout_pointer_bearing_bits`] — the same test the
+/// mask builder and `GC_LAYOUT_UNKNOWN`'s per-slot re-validation apply — so the
+/// declaration never has to trust a caller's static proof. `slot_count == 0` is
+/// the empty case and holds vacuously, which keeps the pre-#8102 path
+/// bit-identical.
+#[inline]
+pub(crate) unsafe fn layout_all_pointer_slots_would_hold(
+    slots: *const u64,
+    slot_count: usize,
+) -> bool {
+    if slot_count == 0 {
+        return true;
+    }
+    if slots.is_null() {
+        return false;
+    }
+    (0..slot_count).all(|i| layout_pointer_bearing_bits(*slots.add(i)))
+}
+
 /// #7630: settle a materialiser-built object's layout state ONCE, after its
 /// construction loop elided the per-slot notes
 /// (`runtime_store_jsvalue_slot_layout_deferred`). Two exact outcomes:

crates/perry-runtime/src/gc/tests/copying/all_pointer_elements_7469.rs

@@ -158,18 +158,23 @@ fn a_numeric_layout_probe_revokes_the_declaration_and_the_header_test_catches_it
     );
 }
 
+/// The all-pointer claim covers `0..length`. #8102 widened the declaration to
+/// non-empty arrays whose every existing slot IS a pointer, so the property
+/// under test here is the one that always mattered: an existing **non-pointer**
+/// element refuses it. (Before #8102 this was spelled "non-empty is refused",
+/// which is why the fixture pushes a number — that push is what makes it a
+/// refusal, not the length.)
 #[test]
-fn declaring_a_non_empty_array_is_refused() {
+fn declaring_an_array_holding_a_non_pointer_element_is_refused() {
     let _guard = CopyingNurseryTestGuard::new(1);
     let arr = crate::array::js_array_alloc(4);
     crate::array::js_array_push_f64(arr, 1.0);
     unsafe {
         crate::array::js_array_declare_all_pointer_elements(arr);
         assert!(
             !codegen_would_take_the_elided_store(arr),
-            "the all-pointer claim covers 0..length; on a non-empty array it \
-             is not vacuously true of what is already stored, so it must be \
-             refused rather than asserted over existing elements"
+            "the all-pointer claim is not true of what is already stored, so \
+             it must be refused rather than asserted over existing elements"
         );
     }
 }
@@ -359,3 +364,94 @@ fn a_replace_or_a_numeric_append_downgrades_to_a_conservative_scan() {
         }
     }
 }
+
+/// #8102 — the array-LITERAL shape: `const a: C[] = [x, y]; a.push(…)`.
+///
+/// The literal's element stores run first and note each slot
+/// (`expr/array_literal.rs` publishes the payload `POINTER_FREE` and notes per
+/// slot), and only then does the `Stmt::Let` tail emit the declaration. While
+/// `js_array_declare_all_pointer_elements` refused every `length != 0` array
+/// the declaration was a silent no-op for exactly this shape, so every later
+/// push failed the codegen header test and paid the per-store layout note
+/// #7469 exists to delete — +33.9% instructions on a 4M-push loop.
+#[test]
+fn a_non_empty_all_pointer_literal_is_declared_and_admits_the_elided_store() {
+    let _guard = CopyingNurseryTestGuard::new(1);
+    let arr = crate::array::js_array_alloc(8);
+    unsafe {
+        for (slot, name) in [&b"lit_zero"[..], &b"lit_one"[..]].iter().enumerate() {
+            let bits = string_bits(fresh_string(name));
+            elided_inline_push(arr, bits);
+            crate::gc::js_gc_note_slot_layout(arr as u64, slot as u32, bits);
+        }
+        assert!(
+            !codegen_would_take_the_elided_store(arr),
+            "the literal's own stores leave a precise side mask, which is NOT \
+             a state the elided push may run in — this is the pre-declaration \
+             state the fixture exists to start from"
+        );
+
+        crate::array::js_array_declare_all_pointer_elements(arr);
+
+        assert!(
+            codegen_would_take_the_elided_store(arr),
+            "a non-empty literal whose every slot is a pointer must still be \
+             declared; refusing it is #8102"
+        );
+        assert_eq!(
+            test_heap_child_slot_count(arr as *mut u8),
+            2,
+            "the two elements already stored must stay enumerable across the \
+             declaration"
+        );
+    }
+}
+
+/// Sabotage arm for the test above, made permanent: the SAME sequence with one
+/// element that is not a pointer. The declaration must refuse and must leave
+/// the header exactly as it found it — otherwise the widening would be
+/// declaring `0..length` all-pointer over a payload it had not checked, and a
+/// green positive test above would mean nothing.
+#[test]
+fn a_non_pointer_element_in_the_literal_still_refuses_the_declaration() {
+    let _guard = CopyingNurseryTestGuard::new(1);
+    let arr = crate::array::js_array_alloc(8);
+    unsafe {
+        let ptr_bits = string_bits(fresh_string(b"lit_zero"));
+        elided_inline_push(arr, ptr_bits);
+        crate::gc::js_gc_note_slot_layout(arr as u64, 0, ptr_bits);
+        let number_bits = 42.0f64.to_bits();
+        elided_inline_push(arr, number_bits);
+        crate::gc::js_gc_note_slot_layout(arr as u64, 1, number_bits);
+
+        let header = header_from_user_ptr(arr as *const u8);
+        let before = (*header)._reserved;
+
+        crate::array::js_array_declare_all_pointer_elements(arr);
+
+        assert_eq!(
+            (*header)._reserved,
+            before,
+            "a refused declaration must not touch the header — in particular \
+             it must not clear the raw-f64 bits on its way out"
+        );
+        assert!(
+            !codegen_would_take_the_elided_store(arr),
+            "codegen must keep routing this array's pushes through \
+             js_array_push_f64"
+        );
+    }
+}
+
+/// The empty-literal path — the one this function has always served — must be
+/// bit-identical after the widening.
+#[test]
+fn an_empty_array_is_still_declared_vacuously() {
+    let _guard = CopyingNurseryTestGuard::new(1);
+    let arr = crate::array::js_array_alloc(4);
+    unsafe {
+        assert!(!codegen_would_take_the_elided_store(arr));
+        crate::array::js_array_declare_all_pointer_elements(arr);
+        assert!(codegen_would_take_the_elided_store(arr));
+    }
+}