From dac2bff5086b6cc52a64d03149b9d7f1affa8bf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 00:55:31 +0200 Subject: [PATCH 1/2] fix(gc): a non-empty array literal silently voided its #7469 all-pointer declaration (#8102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `js_array_declare_all_pointer_elements` refused every array with `length != 0`, on the argument that the all-pointer claim covers `0..length` and is only vacuously true on an empty array. `emit_all_pointer_array_declaration` is emitted from the `Stmt::Let` tail — i.e. *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 silent no-op, the side mask survived, and every later `a.push(...)` failed the elided-push header test in `expr/array_push.rs` and 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 module header says the empty literal "passes vacuously", and `literal_of_object_elements_is_admitted` is its test — so the proof was being issued at compile time and discarded at run time. Discharge the claim instead of 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 mask builder and `GC_LAYOUT_UNKNOWN`'s per-slot re-validation use. The declaration therefore never has to trust the caller's static proof, and a payload it has not checked can never be declared. A refusal leaves the header untouched (in particular the raw-f64 bits are cleared only once the declaration is known to stick), which is exactly today's behaviour. `length == 0` holds vacuously, so the empty-literal path is bit-identical. Measured, `--release` compiler and `--release` runtime archives, identical compiler binary in both arms and only the `.a` pair swapped, `/usr/bin/time -l` instructions retired, medians of 3 interleaved reps, identical program output: 4,000,000 pushes into `const a: C[] = [x, y]` before 20,755,859,948 after 15,309,077,609 -26.2% control: same pushes into `const a: C[] = []` before 15,505,084,925 after 15,491,525,692 -0.1% control: `[]` + 2 pushes + the same loop before 15,613,539,814 after 15,629,659,106 +0.1% Tests: `a_non_empty_all_pointer_literal_is_declared_and_admits_the_elided_store` is the positive case, and `a_non_pointer_element_in_the_literal_still_refuses_the_declaration` is its permanent sabotage arm — it runs the identical sequence with one numeric element and asserts both that the declaration is refused and that the header is byte-unchanged, so a green positive test means the check discriminates rather than that nothing was tried. `an_empty_array_is_still_declared_vacuously` pins the unchanged path. The pre-existing `declaring_a_non_empty_array_is_refused` is renamed to `declaring_an_array_holding_a_non_pointer_element_is_refused`: its fixture pushes a number, so the push — not the length — was always what made it a refusal. Closes #8102 --- crates/perry-runtime/src/array/header.rs | 38 ++++++- crates/perry-runtime/src/gc/layout.rs | 31 ++++++ .../copying/all_pointer_elements_7469.rs | 104 +++++++++++++++++- 3 files changed, 163 insertions(+), 10 deletions(-) diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index 1b03de7a09..9e7f7ef4bf 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/crates/perry-runtime/src/array/header.rs @@ -1619,11 +1619,29 @@ 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); @@ -1631,7 +1649,15 @@ pub extern "C" fn js_array_declare_all_pointer_elements(arr: *mut ArrayHeader) { 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); diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index 1de9345c0c..00c9823ee4 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/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: diff --git a/crates/perry-runtime/src/gc/tests/copying/all_pointer_elements_7469.rs b/crates/perry-runtime/src/gc/tests/copying/all_pointer_elements_7469.rs index 45008a7855..c97e278c73 100644 --- a/crates/perry-runtime/src/gc/tests/copying/all_pointer_elements_7469.rs +++ b/crates/perry-runtime/src/gc/tests/copying/all_pointer_elements_7469.rs @@ -158,8 +158,14 @@ 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); @@ -167,9 +173,8 @@ fn declaring_a_non_empty_array_is_refused() { 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)); + } +} From fb9568283427d4fa282d99badb730174e6f3e5d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 00:56:45 +0200 Subject: [PATCH 2/2] docs(changelog): #8114 fragment for the non-empty array literal all-pointer fix --- ...8114-nonempty-array-literal-all-pointer.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 changelog.d/8114-nonempty-array-literal-all-pointer.md diff --git a/changelog.d/8114-nonempty-array-literal-all-pointer.md b/changelog.d/8114-nonempty-array-literal-all-pointer.md new file mode 100644 index 0000000000..36046aee9d --- /dev/null +++ b/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.