diff --git a/changelog.d/6919-repsel-p4b-store-elision.md b/changelog.d/6919-repsel-p4b-store-elision.md new file mode 100644 index 0000000000..fcef656b33 --- /dev/null +++ b/changelog.d/6919-repsel-p4b-store-elision.md @@ -0,0 +1,10 @@ +**Representation-selection Phase 4b (narrow) — class-field store note/addref elision + INT32 layout-poison fix (#6919)** + +Phase 4b of the RFC (§5.7), scoped down after recon: **full field-unboxing was assessed and rejected.** `number` fields are already bit-unboxed (NaN-boxing reserves only `0x7FF9..=0x7FFF`, so a number slot already holds raw IEEE bits; `raw_f64_mask` is a *proof bit*, not a storage change, and Phase 3b already deleted the read-side guard). Raw string handles at rest would break SSO for nothing, and raw `i1`/`i32` slots would need a third GC mask **plus** a layout probe at ~25 direct slot-read sites — `JSON.stringify`, `util.inspect`, `v8` IPC serde, descriptor reads — all hot, not the "rare, already-slow" surfaces the RFC's observation-equivalence bullet assumes. §5.7 records the decision. + +- **4b.1** — both `property_set.rs` class-field store sites emitted `js_gc_note_slot_layout` + `js_string_addref_if_heap_string` unconditionally. On a `Ptr`-proven receiver each is now retired when provably dead, gated **independently** because they die under different conditions: the note when the value is a non-pointer by construction, the addref when the value cannot carry `STRING_TAG` (strictly weaker — an object/array literal or closure retires the addref while keeping the note). The generational write barrier is untouched. The note elision holds in every layout state: `UNKNOWN` and `POINTER_FREE` short-circuit inside the note, an intact descriptor falls through the `pointer_mask` arm untouched, and under `SIDE_MASK` the note would only ever *clear* the bit — skipping that leaves a stale set bit over a non-pointer, which costs one extra visit and nothing else, because `mark_field_into_worklist` re-validates every slot word and the evacuation rewrite path routes through it. +- Both gates are keyed on the **value expression, never the declared field type**: Perry does not validate declared types at runtime, so a `boolean`-declared field legitimately receives a string through an `any`, and a wrong addref elision there silently corrupts it on the next in-place append. `Expr::New` is excluded from the "cannot be a heap string" set because a constructor return override makes the answer a runtime question. +- **Deliberately not elided:** a pointer-valued store into a pointer-masked slot — the larger win originally scoped — because the receiver is not guaranteed to have a descriptor. `lower_new_impl` has an exit (the `force_ctor_call` branch where `call_local_constructor_symbol` yields `None`) that returns a fresh instance *without* emitting `js_gc_init_typed_shape_layout`; such an object sits at `POINTER_FREE`, where the note is the only thing that sets the pointer-mask bit the collector reads. Closing that exit is the prerequisite. The guarded (non-`Ptr`) store keeps both calls for the same reason. +- **4b.2** — `layout_note_slot` evicts an object's `TypedLayoutDescriptor` permanently and one-way when a non-raw-f64 bit pattern lands in a raw-f64 slot. INT32 boxes genuinely reach object fields from FFI/native modules (sqlite rows, `v8` deserialization) and `runtime_store_jsvalue_slot` wrote them verbatim, so one FFI integer cost that object its fast path forever. It now applies the array precedent `canonicalize_array_numeric_store_bits` (INT32 → raw f64) for raw-f64-masked slots under an intact descriptor — no observable change (an INT32 box and its f64 are `===`), gated tag-first so the hot non-INT32 store never pays the thread-local probe, with `value_bits_to_number` supplying the `ClassRef` exclusion. + +Structural proof (`--trace llvm`, A/B with the predicates forced back to `true`), per changed function: notes 4/6/2 → 3/4/2, addrefs 4/6/2 → 2/2/2, barriers 3/4/2 unchanged; `compoundSnapshots` correctly elides nothing (both stores are `tag ||= `). Post-`opt -O3`, whole module: notes 41 → 37, addrefs 56 → 48. A new 13-section gap file covers each elision and non-elision case — string-typed field still addrefs under snapshot-then-grow, union-with-string keeps the demote, declared types are not enforced, INT32-into-typed-field reads back byte-exact — byte-exact vs the pinned Node 26.5.0 oracle under `PERRY_PTR_SHAPE_LOCALS` on/off, `PERRY_GC_FORCE_EVACUATE=1`, `PERRY_GC_VERIFY_EVACUATION=1`, `PERRY_GEN_GC=0` and `PERRY_WRITE_BARRIERS=0`, plus four runtime regression tests for 4b.2 including the negative controls. No benchmark numbers: the machine never went quiet, so the perf claim is deferred rather than published load-corrupted. diff --git a/crates/perry-codegen/src/expr/helpers.rs b/crates/perry-codegen/src/expr/helpers.rs index d2fbe9917d..cc900545fc 100644 --- a/crates/perry-codegen/src/expr/helpers.rs +++ b/crates/perry-codegen/src/expr/helpers.rs @@ -136,6 +136,85 @@ pub(crate) fn array_store_needs_write_barrier(ctx: &FnCtx<'_>, value: &Expr) -> !expr_produces_non_pointer_bits_by_construction(ctx, value) } +/// Object twin of [`array_store_needs_layout_note`] — Phase 4b.1. +/// +/// `layout_note_slot` is a provable no-op for a class-field store whose value is +/// a **non-pointer by construction**: in that case the note can only ever +/// *clear* mask state, never set it, so it is never the difference between a +/// slot being scanned and a live child being stranded. That holds in every +/// layout state the receiver can be in: +/// +/// - `GC_LAYOUT_UNKNOWN` — the note returns at its own state check. +/// - intact typed descriptor — a non-pointer value falls straight through the +/// `pointer_mask` arm without touching the descriptor. (The `raw_f64_mask` +/// arm is unreachable from the caller: it only uses this predicate when +/// `requires_raw_f64` is false, and that is the very same +/// `type_is_raw_f64_candidate` predicate the mask is built from.) +/// - `GC_LAYOUT_POINTER_FREE` — the note's `!pointer && POINTER_FREE` early +/// return. +/// - `GC_LAYOUT_SIDE_MASK` — the note would only *clear* this slot's bit. +/// Skipping that leaves a stale set bit over a non-pointer, which costs an +/// extra visit and nothing else: `mark_field_into_worklist` (`gc/trace.rs`) +/// re-validates every slot word — f64 bit patterns fall outside the 48-bit +/// user-address range and are rejected — and the evacuation rewrite path +/// routes through the same function. +/// +/// **Deliberately NOT elided: a pointer-valued store into a pointer-masked +/// slot.** That would be a no-op under an intact descriptor, but the receiver +/// is not guaranteed to have one — `lower_new_impl` has an exit +/// (`lower_call/new.rs`, the standalone-ctor-symbol branch where +/// `call_local_constructor_symbol` yields `None`) that returns a freshly +/// allocated instance *without* emitting `js_gc_init_typed_shape_layout`. Such +/// an object sits at `GC_LAYOUT_POINTER_FREE`, where the note is the only thing +/// that ever sets the pointer-mask bit the collector reads. Closing that exit +/// (#6921) is the prerequisite for the stronger elision. +pub(crate) fn class_field_store_needs_layout_note(ctx: &FnCtx<'_>, value: &Expr) -> bool { + !expr_produces_non_pointer_bits_by_construction(ctx, value) +} + +/// `js_string_addref_if_heap_string` demotes a uniquely-owned (refcount==1) +/// heap string to shared when it becomes aliased from a heap slot, and is a +/// no-op for every non-`STRING_TAG` value (`string/alloc.rs`). So it is dead +/// exactly when the stored value provably cannot be a heap string — a strictly +/// weaker condition than "cannot be a pointer", which is why this is gated +/// separately from the layout note above. +/// +/// **This is keyed on the value expression, never on the declared field type.** +/// Perry does not validate declared types at runtime (see CLAUDE.md, "No +/// runtime type *validation*"): a field declared `boolean` legitimately +/// receives a string that arrived through an `any`, and skipping the demote +/// there would leave a refcount==1 string aliased from the heap for a later +/// in-place `+=` to rewrite underneath the stored slot — silent corruption +/// with no crash to trace it back from. +pub(crate) fn class_field_store_needs_string_addref(ctx: &FnCtx<'_>, value: &Expr) -> bool { + !expr_cannot_produce_heap_string(ctx, value) +} + +/// The stored value provably does not carry `STRING_TAG`. +/// +/// Beyond the non-pointer set, the three *literal* constructor forms qualify: +/// each evaluates to a freshly allocated `POINTER_TAG` value with no path to a +/// primitive result. `Expr::New` is deliberately excluded — a constructor +/// return override (`js_ctor_return_override`) makes "what `new C()` evaluates +/// to" a runtime question, and this predicate must not depend on the answer. +fn expr_cannot_produce_heap_string(ctx: &FnCtx<'_>, expr: &Expr) -> bool { + match expr { + Expr::Object(_) | Expr::Array(_) | Expr::Closure { .. } => true, + Expr::Conditional { + then_expr, + else_expr, + .. + } => { + expr_cannot_produce_heap_string(ctx, then_expr) + && expr_cannot_produce_heap_string(ctx, else_expr) + } + Expr::Sequence(exprs) => exprs + .last() + .is_some_and(|last| expr_cannot_produce_heap_string(ctx, last)), + _ => expr_produces_non_pointer_bits_by_construction(ctx, expr), + } +} + /// `lower_expr` variant that hands an expected-type hint down to the /// object-literal lowerer (so it can pick raw f64 slots when the /// destination has a typed shape). All other expression kinds ignore diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 56302d5401..389127e1b4 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -72,6 +72,7 @@ pub(crate) use channel::{ }; pub(crate) use helpers::{ array_store_needs_layout_note, array_store_needs_write_barrier, buffer_alias_metadata_suffix, + class_field_store_needs_layout_note, class_field_store_needs_string_addref, expr_has_numeric_pointer_free_array_layout, expr_produces_non_pointer_bits_by_construction, is_global_this_builtin_function_name, is_global_this_builtin_name, lower_expr_with_expected_type, lower_js_args_array, proxy_build_args_array, unbox_str_handle, @@ -117,7 +118,7 @@ pub(crate) use v8_interop::{ }; pub(crate) use write_barrier::{ emit_array_numeric_write_note_on_block, emit_jsvalue_slot_store_on_block, - emit_jsvalue_slot_store_scalar_aware_on_block, + emit_jsvalue_slot_store_scalar_aware_on_block, emit_jsvalue_slot_store_with_flags_on_block, emit_jsvalue_slot_store_with_value_bits_on_block, emit_root_heap_word_store_on_block, emit_root_nanbox_store_on_block, emit_write_barrier, emit_write_barrier_slot_on_block, lower_array_super_init, lower_event_emitter_subclass_init, lower_node_stream_super_init, diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index a261413504..4a4f684ea2 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -19,10 +19,11 @@ use crate::type_analysis::{ use crate::types::{DOUBLE, I1, I32, I64, I8, PTR}; use super::{ - emit_jsvalue_slot_store_on_block, emit_typed_feedback_register_site, - expr_produces_non_pointer_bits_by_construction, lower_expr, lower_expr_native, - raw_f64_layout_fact, try_lower_pod_field_set, unbox_to_i64, FnCtx, TypedFeedbackContract, - TypedFeedbackKind, + class_field_store_needs_layout_note, class_field_store_needs_string_addref, + emit_jsvalue_slot_store_on_block, emit_jsvalue_slot_store_with_flags_on_block, + emit_typed_feedback_register_site, expr_produces_non_pointer_bits_by_construction, lower_expr, + lower_expr_native, raw_f64_layout_fact, try_lower_pod_field_set, unbox_to_i64, FnCtx, + TypedFeedbackContract, TypedFeedbackKind, }; fn canonicalize_raw_f64_numeric_store_value( @@ -596,15 +597,47 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } ctx.current_block = merge_idx; } else { + // Repsel Phase 4b.1: retire the two bookkeeping + // calls that are provably dead here. + // + // The receiver being `Ptr`-proven is + // what licenses the layout-note elision. Three + // facts close it: + // + // Both are decided from the VALUE expression, + // and gated independently because they are dead + // under different conditions: the note needs + // "not a pointer", the addref only "not a heap + // string". Neither is keyed on the declared + // field type — Perry does not enforce declared + // types at runtime, so a `boolean` field can + // legitimately receive a string through an + // `any`, and a wrong addref elision there + // silently corrupts it on the next in-place + // append. + // + // `requires_raw_f64` is false on this arm, so + // the raw-f64-mask arm of `layout_note_slot` — + // the one that *must* downgrade — is + // unreachable from here. The full per-layout- + // state argument, including why a pointer store + // into a pointer-masked slot is deliberately + // NOT elided, is on + // `class_field_store_needs_layout_note`. + let layout_note_needed = + class_field_store_needs_layout_note(ctx, value); + let string_addref_needed = + class_field_store_needs_string_addref(ctx, value); let blk = ctx.block(); let field_addr = blk.ptrtoint(&field_ptr, I64); - emit_jsvalue_slot_store_on_block( + emit_jsvalue_slot_store_with_flags_on_block( blk, &field_ptr, &val_double, &obj_handle, &field_idx_str, - true, + string_addref_needed, + layout_note_needed, &obj_bits, &field_addr, field_set_barrier_needed, diff --git a/crates/perry-codegen/src/expr/write_barrier.rs b/crates/perry-codegen/src/expr/write_barrier.rs index 4615ebade4..8221c47296 100644 --- a/crates/perry-codegen/src/expr/write_barrier.rs +++ b/crates/perry-codegen/src/expr/write_barrier.rs @@ -137,6 +137,46 @@ pub(crate) fn emit_jsvalue_slot_store_on_block( layout_parent_bits, slot_index, layout_note_needed, + layout_note_needed, + barrier_parent_bits, + slot_addr, + write_barrier_needed, + false, + None, + ) +} + +/// As [`emit_jsvalue_slot_store_on_block`], but with the string-addref demote +/// and the GC layout note gated **independently** (Phase 4b.1). +/// +/// The two calls answer different questions and are provably dead under +/// different conditions: the addref is a no-op unless the value is a heap +/// `STRING_TAG` string, while the note is a no-op when the slot's mask class is +/// already fixed. Class-field stores on a shape-proven receiver can retire one +/// without the other — e.g. `Foo`-typed field ← `LocalGet` elides the note +/// (pointer-masked slot) but must keep the addref (the local could hold a +/// uniquely-owned string that a later `+=` would rewrite in place). +#[allow(clippy::too_many_arguments)] +pub(crate) fn emit_jsvalue_slot_store_with_flags_on_block( + blk: &mut LlBlock, + slot_ptr: &str, + value_double: &str, + layout_parent_bits: &str, + slot_index: &str, + string_addref_needed: bool, + layout_note_needed: bool, + barrier_parent_bits: &str, + slot_addr: &str, + write_barrier_needed: bool, +) -> Option { + emit_jsvalue_slot_store_on_block_inner( + blk, + slot_ptr, + value_double, + layout_parent_bits, + slot_index, + string_addref_needed, + layout_note_needed, barrier_parent_bits, slot_addr, write_barrier_needed, @@ -164,6 +204,7 @@ pub(crate) fn emit_jsvalue_slot_store_with_value_bits_on_block( layout_parent_bits, slot_index, layout_note_needed, + layout_note_needed, barrier_parent_bits, slot_addr, write_barrier_needed, @@ -200,6 +241,7 @@ pub(crate) fn emit_jsvalue_slot_store_scalar_aware_on_block( layout_parent_bits, slot_index, layout_note_needed, + layout_note_needed, barrier_parent_bits, slot_addr, write_barrier_needed, @@ -215,6 +257,7 @@ fn emit_jsvalue_slot_store_on_block_inner( value_double: &str, layout_parent_bits: &str, slot_index: &str, + string_addref_needed: bool, layout_note_needed: bool, barrier_parent_bits: &str, slot_addr: &str, @@ -239,10 +282,13 @@ fn emit_jsvalue_slot_store_on_block_inner( // allocates fresh instead of mutating the stored element. This is the inline // codegen choke point the runtime store functions (`js_array_push_f64`, …) // are bypassed for on the fast paths. Tag-checked at runtime (a no-op for - // SSO / non-string), and only emitted when the value can be a heap pointer - // (`layout_note_needed`), so numeric stores pay nothing. Mirrors the + // SSO / non-string), and only emitted when the value can be a heap string + // (`string_addref_needed`), so numeric stores pay nothing. Most callers tie + // that to `layout_note_needed`; Phase 4b.1's class-field store gates it + // separately, because a pointer-masked slot can retire the note while the + // stored value can still be a uniquely-owned string. Mirrors the // object-field demote in `runtime_store_jsvalue_slot` (#5533). - if layout_note_needed { + if string_addref_needed { blk.call_void("js_string_addref_if_heap_string", &[(DOUBLE, value_double)]); } if !layout_note_needed && !write_barrier_needed { diff --git a/crates/perry-runtime/src/gc/barrier.rs b/crates/perry-runtime/src/gc/barrier.rs index c83276e943..9e10f311a1 100644 --- a/crates/perry-runtime/src/gc/barrier.rs +++ b/crates/perry-runtime/src/gc/barrier.rs @@ -1362,6 +1362,46 @@ pub(crate) fn runtime_write_barrier_slot(parent_addr: usize, slot_addr: usize, c js_write_barrier_slot(parent_addr as u64, slot_addr as u64, child_bits); } +/// Canonicalize an **INT32-boxed** numeric store into a raw-f64-masked slot of +/// an intact typed-shape descriptor (`0x7FFE…` → the plain IEEE bits of the same +/// number). The object twin of `canonicalize_array_numeric_store_bits` +/// (`array/header.rs`), and needed for the same reason. +/// +/// `layout_note_slot` treats any non-raw-f64 bit pattern landing in a raw-f64 +/// slot as a representation change and calls `layout_set_typed_unknown`, which +/// evicts the object's `TypedLayoutDescriptor` **permanently and one-way**. +/// INT32 boxes genuinely reach object fields from FFI / native modules (sqlite +/// row columns, `v8` deserialization, …), and unlike codegen's guarded class- +/// field store — which canonicalizes inline behind a plain-finite check — this +/// runtime choke point wrote the bits verbatim. One FFI integer therefore cost +/// the object its typed fast path forever. +/// +/// There is no observable behavior change: an INT32 box and its f64 are `===` +/// and print identically. `value_bits_to_number` supplies the class-ref +/// exclusion (a `ClassRef` shares INT32_TAG and must keep its tag), so a class +/// value still downgrades the descriptor rather than being stripped to a bare +/// number. +/// +/// Ordered tag-first so the (hot) non-INT32 store never pays the thread-local +/// descriptor probe. +#[inline] +fn canonicalize_typed_slot_store_bits( + parent_user: usize, + slot_index: usize, + value_bits: u64, +) -> u64 { + if value_bits & TAG_MASK != crate::value::INT32_TAG { + return value_bits; + } + if !crate::gc::layout_slot_is_raw_f64_typed(parent_user, slot_index) { + return value_bits; + } + match crate::array::value_bits_to_number(value_bits) { + Some(number) => number.to_bits(), + None => value_bits, + } +} + #[inline] pub(crate) fn runtime_store_jsvalue_slot( parent_user: usize, @@ -1369,6 +1409,7 @@ pub(crate) fn runtime_store_jsvalue_slot( slot_index: usize, value_bits: u64, ) { + let value_bits = canonicalize_typed_slot_store_bits(parent_user, slot_index, value_bits); unsafe { std::ptr::write(slot_addr as *mut u64, value_bits); } diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index bf984cccae..6489a38276 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/crates/perry-runtime/src/gc/layout.rs @@ -562,6 +562,43 @@ pub(crate) fn layout_note_slot(parent_user: usize, slot_index: usize, value_bits } } +/// True when `slot_index` of `parent_user` is a **raw-f64-masked slot of an +/// intact typed-shape descriptor** — i.e. exactly the case where +/// [`layout_note_slot`] would call `layout_set_typed_unknown` (permanently +/// evicting the descriptor) for a stored value whose bits are not raw f64. +/// +/// Mirrors `layout_note_slot`'s own prologue — forwarding resolution, the +/// `GC_LAYOUT_UNKNOWN` short-circuit, and the O(1) `GC_OBJ_TYPED_LAYOUT_INTACT` +/// gate before the thread-local probe — so the two agree on every object. +pub(crate) fn layout_slot_is_raw_f64_typed(parent_user: usize, slot_index: usize) -> bool { + if slot_index > 16_000_000 { + return false; + } + unsafe { + let Some(header) = layout_header_for_user(parent_user) else { + return false; + }; + if (*header).gc_flags & GC_FLAG_FORWARDED != 0 { + let new_user = forwarding_address(header) as usize; + if new_user != 0 && new_user != parent_user { + return layout_slot_is_raw_f64_typed(new_user, slot_index); + } + return false; + } + if (*header)._reserved & GC_LAYOUT_STATE_MASK == GC_LAYOUT_UNKNOWN { + return false; + } + if (*header)._reserved & GC_OBJ_TYPED_LAYOUT_INTACT == 0 { + return false; + } + TYPED_LAYOUTS.with(|m| { + m.borrow().get(&parent_user).is_some_and(|typed| { + slot_index < typed.slot_count && typed.raw_f64_mask.contains_slot(slot_index) + }) + }) + } +} + #[no_mangle] pub extern "C" fn js_gc_note_slot_layout(parent: u64, slot_index: u32, value_bits: u64) { let parent_user = strip_nanbox_user_ptr(parent); diff --git a/crates/perry-runtime/src/gc/tests/layout_trace.rs b/crates/perry-runtime/src/gc/tests/layout_trace.rs index 7dd4ac875f..93ea7008fa 100644 --- a/crates/perry-runtime/src/gc/tests/layout_trace.rs +++ b/crates/perry-runtime/src/gc/tests/layout_trace.rs @@ -1542,3 +1542,133 @@ fn test_trace_closure_uses_pointer_layout_mask() { clear_marks(); clear_mark_seeds(); } + +// Repsel Phase 4b.2 — an INT32-boxed numeric value reaching a raw-f64-masked +// object slot through the runtime store choke point must be canonicalized to +// raw f64 instead of permanently poisoning the object's typed layout. +// +// `layout_note_slot` treats any non-raw-f64 bit pattern landing in a raw-f64 +// slot as a representation change and calls `layout_set_typed_unknown`, which +// evicts the `TypedLayoutDescriptor` one-way, per object. INT32 boxes genuinely +// reach object fields from FFI / native modules (sqlite row columns, `v8` +// deserialization), so one FFI integer used to cost that object its typed fast +// path forever. Codegen's guarded class-field store already canonicalized +// inline behind its plain-finite check; `runtime_store_jsvalue_slot` wrote the +// bits verbatim. + +/// Install a two-slot typed descriptor: slot 0 raw-f64, slot 1 pointer. +unsafe fn typed_two_slot_object() -> (*mut crate::object::ObjectHeader, *mut u64) { + let (obj, fields) = alloc_old_test_object(2); + *fields = 0.0f64.to_bits(); + *fields.add(1) = crate::value::TAG_UNDEFINED; + let raw_mask = [0b01u64]; + let ptr_mask = [0b10u64]; + js_gc_init_typed_shape_layout( + obj as u64, + 2, + raw_mask.as_ptr(), + raw_mask.len() as u32, + ptr_mask.as_ptr(), + ptr_mask.len() as u32, + ); + assert!( + layout_has_typed_descriptor(obj as usize), + "test setup: the typed descriptor must install" + ); + (obj, fields) +} + +#[test] +fn test_int32_store_into_raw_f64_slot_keeps_typed_descriptor() { + let _guard = GcTestIsolationGuard::new(); + let (obj, fields) = unsafe { typed_two_slot_object() }; + + // An INT32-boxed 42, exactly as an FFI / native module hands one over. + let int32_bits = crate::value::INT32_TAG | 42u64; + runtime_store_jsvalue_slot(obj as usize, fields as usize, 0, int32_bits); + + assert!( + layout_has_typed_descriptor(obj as usize), + "an INT32-boxed integer stored into a raw-f64 slot must not evict the typed descriptor" + ); + let stored = unsafe { std::ptr::read(fields as *const u64) }; + assert_eq!( + stored, + 42.0f64.to_bits(), + "the slot holds canonical raw f64 bits, not the INT32 box" + ); + assert_eq!( + f64::from_bits(stored), + 42.0, + "and reads back byte-exact as the same number" + ); +} + +#[test] +fn test_int32_store_into_pointer_slot_is_left_verbatim() { + let _guard = GcTestIsolationGuard::new(); + let (obj, fields) = unsafe { typed_two_slot_object() }; + + // Slot 1 is pointer-masked, not raw-f64-masked: there is no raw-f64 + // contract to uphold and the note is already a no-op there, so the stored + // bits must survive untouched. + let int32_bits = crate::value::INT32_TAG | 7u64; + let slot1 = unsafe { fields.add(1) }; + runtime_store_jsvalue_slot(obj as usize, slot1 as usize, 1, int32_bits); + + assert!( + layout_has_typed_descriptor(obj as usize), + "a non-pointer value in a pointer-masked slot leaves the descriptor intact" + ); + assert_eq!( + unsafe { std::ptr::read(slot1 as *const u64) }, + int32_bits, + "canonicalization is scoped to raw-f64-masked slots" + ); +} + +#[test] +fn test_non_numeric_store_into_raw_f64_slot_still_evicts_descriptor() { + let _guard = GcTestIsolationGuard::new(); + let (obj, fields) = unsafe { typed_two_slot_object() }; + + // The negative control for the fix: a string IS a genuine representation + // change for a raw-f64 slot and must still downgrade the object — the scan + // skips raw-f64 slots, so a mask left claiming "number here" over a live + // string pointer would strand it. + let payload = crate::string::js_string_from_bytes(b"not-a-number".as_ptr(), 12); + let payload_bits = STRING_TAG | (payload as u64 & POINTER_MASK); + runtime_store_jsvalue_slot(obj as usize, fields as usize, 0, payload_bits); + + assert!( + !layout_has_typed_descriptor(obj as usize), + "a non-numeric store into a raw-f64 slot must still evict the typed descriptor" + ); + assert_eq!( + unsafe { std::ptr::read(fields as *const u64) }, + payload_bits, + "and the stored value itself is untouched" + ); +} + +#[test] +fn test_int32_store_without_typed_descriptor_is_left_verbatim() { + let _guard = GcTestIsolationGuard::new(); + let (obj, fields) = unsafe { alloc_old_test_object(1) }; + unsafe { + *fields = 0.0f64.to_bits(); + } + assert!( + !layout_has_typed_descriptor(obj as usize), + "test setup: no descriptor installed" + ); + + let int32_bits = crate::value::INT32_TAG | 5u64; + runtime_store_jsvalue_slot(obj as usize, fields as usize, 0, int32_bits); + + assert_eq!( + unsafe { std::ptr::read(fields as *const u64) }, + int32_bits, + "with no intact descriptor there is no raw-f64 contract to uphold — bits stay verbatim" + ); +} diff --git a/docs/representation-selection-rfc.md b/docs/representation-selection-rfc.md index e8d752fc9b..c26460353f 100644 --- a/docs/representation-selection-rfc.md +++ b/docs/representation-selection-rfc.md @@ -234,6 +234,51 @@ Unboxed storage extends to heap slots where the *container's* shape is proven an fact). Hole-vs-undefined observability (`in`, `Object.keys`, `JSON.stringify`) is preserved structurally: those surfaces reference the local as a bare value and therefore disqualify it, and bare (non-ToNumber) element reads never lower guard-free. +- **Unboxed object fields: assessed and REJECTED (Phase 4b).** The "unboxed field layout" + bullets at the top of this section were scoped down after recon, and the eligibility + machinery they describe was deliberately *not* built. Three findings drove that: + 1. **`number` fields are already bit-unboxed.** NaN-boxing reserves only `0x7FF9..=0x7FFF`, + so a number field slot already holds raw IEEE bits; `raw_f64_mask` + (`gc/layout.rs::layout_raw_f64_bits`) is a *proof bit*, not a storage change. Phase 3b + already deleted the read-side guard on proven receivers, so no unboxing win remains. + 2. **Raw string handles at rest would break SSO** — short strings live inline in the NaN + box and would have to be heap-materialized just to be stored "unboxed" — and buy nothing. + 3. **Raw `i1`/`i32` slots would need a third mask *plus* a layout probe at ~25 direct + slot-read sites** — `JSON.stringify`, `util.inspect`, `v8` IPC serde and descriptor reads + among them. Those are hot paths, not the "rare, already-slow" surfaces the + observation-equivalence bullet assumes, so the probe would cost more than the + representation saves. + + What Phase 4b ships instead is the bookkeeping the existing boxed layout was paying + needlessly: + - **4b.1** — a class-field store on a `Ptr`-proven receiver retires + `js_gc_note_slot_layout` when the value is a **non-pointer by construction**, and + `js_string_addref_if_heap_string` when the value provably **cannot be a heap string** (the + strictly weaker condition, which is why the two are gated independently — an object or + array literal retires the addref but keeps the note). The generational write barrier is + untouched. The note elision is sound in every layout state the receiver can be in: + `UNKNOWN` and `POINTER_FREE` short-circuit inside the note; an intact descriptor falls + through the `pointer_mask` arm untouched; and under `SIDE_MASK` the note would only ever + *clear* the slot's bit, so skipping it leaves a stale set bit over a non-pointer, which + costs one extra visit and nothing else — `mark_field_into_worklist` re-validates every slot + word, and the evacuation rewrite path routes through the same function. + The addref elision is keyed on the **value expression, never the declared field type**: + Perry does not validate declared types at runtime, so a `boolean`-declared field can + legitimately receive a string through an `any`, and a wrong elision there silently corrupts + an aliased string on the next in-place append. + + Two scope notes. **A pointer-valued store into a pointer-masked slot is deliberately not + elided**, even though it is a no-op under an intact descriptor: `lower_new_impl` has an exit + (the standalone-ctor-symbol branch where `call_local_constructor_symbol` yields `None`) that + returns a freshly allocated instance *without* emitting `js_gc_init_typed_shape_layout`, and + such an object sits at `POINTER_FREE` where the note is the only thing that ever sets the + pointer-mask bit the collector reads. Closing that exit (#6921) is the prerequisite for the + stronger elision. Likewise the **guarded (non-`Ptr`) class-field store keeps both calls** — its + receiver can be a runtime-constructed object that never had a descriptor installed. + - **4b.2** — `runtime_store_jsvalue_slot` canonicalizes an INT32-boxed numeric store into a + raw-f64-masked slot (the object twin of `canonicalize_array_numeric_store_bits`), instead + of letting one FFI/native-supplied integer evict that object's typed descriptor + permanently and one-way. ## 6. Phasing (one design; each phase sound on its own) diff --git a/test-files/test_gap_repsel_p4b_field_store_elision.ts b/test-files/test_gap_repsel_p4b_field_store_elision.ts new file mode 100644 index 0000000000..e2e72a3268 --- /dev/null +++ b/test-files/test_gap_repsel_p4b_field_store_elision.ts @@ -0,0 +1,330 @@ +// Representation-selection Phase 4b.1/4b.2: class-field store bookkeeping. +// (RFC docs/representation-selection-rfc.md §5.7.) +// +// 4b.1 retires `js_gc_note_slot_layout` and `js_string_addref_if_heap_string` +// on Ptr-proven class-field stores where each is provably dead. This +// file pins the OBSERVABLE behaviour of both the elided and the deliberately +// NON-elided cases — every section must stay byte-identical to Node. +// +// The addref cases are the sharp edge: `js_string_addref_if_heap_string` +// demotes a uniquely-owned (refcount==1) heap string to shared so a later +// in-place `+=` on the source allocates fresh instead of rewriting the stored +// field underneath it. Eliding it wrongly is SILENT corruption, so each such +// case builds a genuinely non-SSO (> 5 byte), genuinely unique (first append on +// a shared literal) string, stores it, then grows the source. + +// 1. Elides both: a by-construction non-pointer value (literal / comparison / +// `!`) can be neither a pointer the GC must track nor a heap string. +class Flags { + on: boolean; + hot: boolean; + seen: boolean; + constructor() { + this.on = false; + this.hot = false; + this.seen = false; + } +} +function boolStores(n: number): string { + const f = new Flags(); + let flips = 0; + for (let i = 0; i < n; i++) { + f.on = true; + f.hot = i > n / 2; + f.seen = !f.on; + if (f.hot) flips++; + } + return f.on + "," + f.hot + "," + f.seen + "," + flips; +} +console.log(boolStores(1000)); + +// 2. The addref boundary: the stored value is a unique heap string, so the +// demote must survive. Nothing about the declared type may elide it. +class Named { + tag: string; + constructor() { + this.tag = ""; + } +} +function stringFieldKeepsAddref(): string { + const o = new Named(); + let s = "prefix"; // 6 bytes -> heap (non-SSO), shared literal + s += "_init"; // append on shared -> fresh heap string, refcount==1 + o.tag = s; // MUST demote s to shared + s += "_more"; // refcount==1 in-place append must NOT rewrite o.tag + return "tag=" + o.tag + " s=" + s; +} +console.log(stringFieldKeepsAddref()); + +// 3. Snapshot-then-grow across two proven receivers: each stored snapshot must +// keep the value it had at store time. +function snapshots(): string { + const a = new Named(); + const b = new Named(); + let cur = "prefix"; + cur += "_one"; + a.tag = cur; + cur += "_two"; + b.tag = cur; + cur += "_three"; + return "a=" + a.tag + " b=" + b.tag + " cur=" + cur; +} +console.log(snapshots()); + +// 4. Union-with-string must NOT elide the demote: a string can land in the +// slot and has to be marked shared like any other. +class Mixed { + v: string | number; + constructor() { + this.v = 0; + } +} +function unionFieldKeepsAddref(): string { + const m = new Mixed(); + let s = "prefix"; + s += "_union"; + m.v = s; + s += "_grown"; + const first = "" + m.v; + m.v = 42; // non-pointer store into the same pointer-masked slot + return "first=" + first + " s=" + s + " then=" + m.v + " t=" + typeof m.v; +} +console.log(unionFieldKeepsAddref()); + +// 5. Declared types are NOT enforced at runtime: a `boolean`-declared field can +// receive a string smuggled through `any`. The demote is gated on the VALUE +// expression, never on the declared type, so this must stay correct. +class Loose { + flag: boolean; + n: number; + constructor() { + this.flag = false; + this.n = 0; + } +} +function declaredTypeIsNotEnforced(): string { + const l = new Loose(); + let s = "prefix"; + s += "_smuggled"; + l.flag = s as any; // a string in a `boolean` slot — must still demote + s += "_after"; + return "flag=" + l.flag + " s=" + s + " t=" + typeof l.flag; +} +console.log(declaredTypeIsNotEnforced()); + +// 6. Object- and array-typed fields: values stay exact and the children stay +// reachable across GC (these keep their note — see section 11). +class Node2 { + id: number; + next: Node2 | null; + constructor(id: number) { + this.id = id; + this.next = null; + } +} +class Holder { + head: Node2 | null; + nums: number[]; + label: string; + constructor() { + this.head = null; + this.nums = []; + this.label = ""; + } +} +function pointerFields(n: number): string { + const h = new Holder(); + let total = 0; + for (let i = 0; i < n; i++) { + // Fresh child each iteration: only the field write keeps it alive, so a + // wrongly-elided layout note would strand it for the collector. + h.head = new Node2(i); + h.nums = [i, i + 1, i + 2]; + h.label = "n" + i; + total += h.head.id + h.nums[2] + h.label.length; + } + return total + ":" + h.head!.id + ":" + h.nums.join("-") + ":" + h.label; +} +console.log(pointerFields(2000)); + +// 7. Null-out then re-point: the pointer-masked slot goes non-pointer and back. +function nullOutAndRepoint(): string { + const h = new Holder(); + h.head = new Node2(7); + const seen1 = h.head.id; + h.head = null; // non-pointer by construction into a pointer-masked slot + const seen2 = h.head === null; + h.head = new Node2(9); + return seen1 + "," + seen2 + "," + h.head.id; +} +console.log(nullOutAndRepoint()); + +// 8. 4b.2 — INT32-boxed numerics reaching a typed `number` field must not +// change what is read back. Integers that round-trip through JSON (the +// sqlite/v8-IPC shape that motivated the fix) stay integers, not null. +class Row { + id: number; + count: number; + ratio: number; + constructor(id: number, count: number, ratio: number) { + this.id = id; + this.count = count; + this.ratio = ratio; + } +} +function int32Fields(): string { + const r = new Row(0, 0, 0); + let acc = 0; + for (let i = 0; i < 500; i++) { + r.id = i | 0; // bitwise -> integral + r.count = (i * 3) | 0; + r.ratio = i / 4; + acc += r.id + r.count + r.ratio; + } + return ( + acc + + ":" + + JSON.stringify(r) + + ":" + + (r.id | 0) + + ":" + + Object.is(r.count, 1497) + + ":" + + r.ratio.toFixed(2) + ); +} +console.log(int32Fields()); + +// 9. Mixed sequence on one receiver: interleave every mask class so any +// descriptor bookkeeping mistake shows up as a wrong read rather than a +// crash-free-but-silent divergence. +class Everything { + num: number; + flag: boolean; + text: string; + list: number[]; + constructor() { + this.num = 0; + this.flag = false; + this.text = ""; + this.list = []; + } +} +function interleaved(n: number): string { + const e = new Everything(); + let sum = 0; + for (let i = 0; i < n; i++) { + e.num = i * 1.5; + e.flag = (i & 1) === 0; + e.text = "v" + (i % 10); + e.list = [i]; + sum += e.num + (e.flag ? 1 : 0) + e.text.length + e.list[0]; + } + return sum + "|" + e.num + "|" + e.flag + "|" + e.text + "|" + e.list[0]; +} +console.log(interleaved(1500)); + +// 10. Same shapes under allocation pressure so real minor collections run +// between the field writes and the reads. +function underGcPressure(n: number): string { + const h = new Holder(); + let live = 0; + for (let i = 0; i < n; i++) { + const garbage = new Array(32).fill(i); // allocation safepoint + h.head = new Node2(garbage[i % 32]); + h.label = "g" + (i % 7); + h.nums = [garbage.length]; + live += h.head.id % 3; + } + return live + ":" + h.head!.id + ":" + h.label + ":" + h.nums[0]; +} +console.log(underGcPressure(3000)); + +// 11. THE STORE SITE 4b.1 ACTUALLY CHANGES. A plain `o.f = v` on a local +// lowers to `PutValueSet` (the PutValue IC); it is compound and logical +// assignment (`+=`, `||=`, `??=`) that lowers to a `PropertySet` on a +// `LocalGet` receiver, which is the Ptr-proven class-field store +// path. One case per mask class, plus the addref boundary. +class Slot { + tag: string; + child: Node2 | null; + nums: number[] | null; + flag: boolean; + count: number; + constructor() { + this.tag = ""; + this.child = null; + this.nums = null; + this.flag = false; + this.count = 0; + } + size(): number { + return this.tag.length; + } +} + +function compoundStores(): string { + const o = new Slot(); + let s = "prefix"; // non-SSO + s += "_init"; // append on a shared literal -> refcount==1 + o.tag ||= s; // unproven RHS: note AND addref both stay + s += "_more"; // must NOT rewrite o.tag + o.child ??= new Node2(3); // Expr::New is excluded: both stay + o.nums ??= [1, 2]; // array literal: addref elided, note stays + o.flag ||= true; // non-pointer by construction: both elided + o.count += 5; // raw-f64 slot: separate inline path, untouched + return ( + o.tag + + "|" + + s + + "|" + + o.child.id + + "|" + + o.nums.join(",") + + "|" + + o.flag + + "|" + + o.count + + "|" + + o.size() + ); +} +console.log(compoundStores()); + +// 12. The changed store in a hot loop under allocation pressure: the elided +// layout note has to hold up across real collections while the +// pointer-masked slots keep freshly allocated children alive. +function compoundLoop(n: number): string { + const o = new Slot(); + let live = 0; + for (let i = 0; i < n; i++) { + const garbage = new Array(16).fill(i); // allocation safepoint + o.child = null; // clear so `??=` re-points every iteration + o.child ??= new Node2(garbage[i % 16]); + o.nums = null; + o.nums ??= [i]; + o.flag ||= i === n - 1; + o.count += 1; + live += (o.child.id % 5) + (o.nums[0] % 3); + } + return ( + live + ":" + o.child!.id + ":" + o.nums![0] + ":" + o.flag + ":" + o.count + ); +} +console.log(compoundLoop(3000)); + +// 13. The addref boundary inside the changed store, repeated: each snapshot +// taken with `||=` must keep the value it had when stored, while the +// source keeps growing. +function compoundSnapshots(): string { + const a = new Slot(); + const b = new Slot(); + let cur = "prefix"; + cur += "_one"; + a.tag ||= cur; + cur += "_two"; + b.tag ||= cur; + cur += "_three"; + return "a=" + a.tag + " b=" + b.tag + " cur=" + cur; +} +console.log(compoundSnapshots());