From 0fa1435737dc96eb7582e2ed79079edfd2a6f1ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 28 Aug 2026 23:36:44 +0200 Subject: [PATCH 1/5] perf(runtime): skip dead feedback observation on the property wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typed-feedback recording is off by default, and guard_observe and record_fallback_call both early-return in that mode — but the property wrappers had already built the whole Observation to hand them, hashing the key and resolving the receiver's shape first. On an isolated property-read loop js_typed_feedback_object_get_field_by_name_f64 was 10% of self time, nearly all of it that dead work. Apply #5094's gate, which the array index wrappers already carry and #8951 gave the fast store path: when recording is off, take the underlying op directly. Behaviour is unchanged in both modes — with recording off guard_observe returns contract_valid and the fallback recorder is a no-op, so the wrapper already reduced to exactly this call. Also: object_live_slot_count reads live_inline_slot_count through the shape table's record instead of lifting the whole ~48-byte descriptor to discard all but four bytes. That bound is consulted on essentially every property operation, and shape_descriptor_by_id was 10.1% of the same loop. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP --- .../8981-feedback-gate-and-shape-field.md | 38 +++++++++++++++++++ crates/perry-runtime/src/object/live_slots.rs | 8 ++-- crates/perry-runtime/src/object/shapes.rs | 38 +++++++++++++++++++ crates/perry-runtime/src/typed_feedback.rs | 33 ++++++++++++++++ 4 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 changelog.d/8981-feedback-gate-and-shape-field.md diff --git a/changelog.d/8981-feedback-gate-and-shape-field.md b/changelog.d/8981-feedback-gate-and-shape-field.md new file mode 100644 index 0000000000..66a4acaecb --- /dev/null +++ b/changelog.d/8981-feedback-gate-and-shape-field.md @@ -0,0 +1,38 @@ +Two dead-work removals on the property read path. The computed-key read loop +now **matches node** (23 ms vs 23 ms on the same host), and the pure property +read drops 21 → 17 ms. + +**1. Typed-feedback observation is skipped when recording is off.** Recording +is off by default, and `guard_observe` and `record_fallback_call` both +early-return in that mode — but the property wrappers built the whole +`Observation` first, hashing the key and resolving the receiver's shape, purely +to hand it to functions that discard it. `js_typed_feedback_object_get_field_by_name_f64` +was 10% of an isolated property-read loop, nearly all of it that. The array +index wrappers have carried #5094's gate for exactly this reason, and #8951 +gave it to the fast store path; the property get/set wrappers never got it. + +Behaviour is unchanged in both modes: with recording off `guard_observe` +returns `contract_valid` unmodified and the fallback recorder is a no-op, so +the wrapper already reduced to precisely the underlying call it now makes +directly. + +**2. The slot bound stops copying a descriptor to read four bytes.** +`shape_descriptor_by_id` returns `ShapeDescriptor` **by value**, so +`object_live_slot_count` — consulted on essentially every property read and +write — lifted the whole ~48-byte record and kept only +`live_inline_slot_count`. It now reads that field through the table's record +using the same way-cache probe and the same epoch validation. +`shape_descriptor_by_id` was 10.1% of the same loop. + +Interleaved A/B, min-of-21, node on the same host in brackets: + +| loop | base | this PR | | +|---|---|---|---| +| pure property read | 21 ms (4) | **17 ms** | −19% | +| computed-key read | 27 ms (23) | **23 ms** | −15% — now at parity with node | +| combined overwrite | 46 ms (31) | **41 ms** | −11% | +| write only | 21 ms (23) | 21 ms | unchanged; already faster than node | + +Suite 2779 passed (including the 55 typed-feedback tests). Private-member +output is byte-identical to base; computed-key differential is byte-identical +to node. diff --git a/crates/perry-runtime/src/object/live_slots.rs b/crates/perry-runtime/src/object/live_slots.rs index 01186812cd..c37d688a1e 100644 --- a/crates/perry-runtime/src/object/live_slots.rs +++ b/crates/perry-runtime/src/object/live_slots.rs @@ -50,9 +50,11 @@ pub extern "C" fn perry_object_header_abi_revision() -> u32 { /// configuration. #[inline] pub unsafe fn object_live_slot_count(obj: *const ObjectHeader) -> u32 { - shapes::object_shape_descriptor(obj) - .map(|descriptor| descriptor.live_inline_slot_count) - .unwrap_or(0) + // Reads the one field through the table's record instead of lifting the + // whole ~48-byte descriptor to discard all but four bytes of it. This is + // the bound consulted on essentially every property read and write, and + // `shape_descriptor_by_id` was 10.1% of an isolated property-read loop. + shapes::shape_live_inline_slot_count_by_id(shapes::object_shape_stamp(obj)).unwrap_or(0) } /// C-ABI accessor for [`object_live_slot_count`], for out-of-runtime consumers diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 2d85bd7ecb..087ef78f16 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -569,6 +569,44 @@ pub(crate) fn shape_id_for_keys_ensure(keys: *const ArrayHeader, key_count: u32) publish_shape_result(shape_descriptor_ensure(keys, key_count, key_count)) } +/// One FIELD of a shape's descriptor, without lifting the whole record. +/// +/// [`shape_descriptor_by_id`] returns `ShapeDescriptor` **by value**, so every +/// caller that wants a single `u32` still copies the entire ~48-byte record +/// out of the table. That is most of them: `object_live_slot_count` — the slot +/// bound consulted on essentially every property read and write — throws away +/// all of it but `live_inline_slot_count`. +/// +/// This shares the way-cache probe with `shape_descriptor_by_id` and reads the +/// field through the record pointer instead. Same lookup, same validation, +/// four bytes instead of forty-eight. +#[inline] +fn shape_descriptor_field_by_id( + shape_id: u32, + read: impl Fn(&ShapeDescriptor) -> T, +) -> Option { + if !is_shape_id(shape_id) { + return None; + } + let table = &crate::state::state().shapes; + let epoch = table.lookup_epoch.get(); + let way = &table.lookup_ways[(shape_id as usize) & (SHAPE_LOOKUP_WAYS - 1)]; + let (cached_id, record, cached_epoch) = way.get(); + if cached_id == shape_id && cached_epoch == epoch && record != 0 { + // SAFETY: identical to `shape_descriptor_by_id`'s hit arm — the way is + // only filled from a live `Box` and the epoch is + // bumped whenever a record's address can change under an id still in + // use, so a matching epoch means this address is the table's record. + return Some(read(unsafe { &*(record as *const ShapeDescriptor) })); + } + shape_descriptor_by_id(shape_id).map(|d| read(&d)) +} + +/// The live inline-slot bound for `shape_id`, without copying its descriptor. +pub(crate) fn shape_live_inline_slot_count_by_id(shape_id: u32) -> Option { + shape_descriptor_field_by_id(shape_id, |d| d.live_inline_slot_count) +} + pub(crate) fn shape_descriptor_by_id(shape_id: u32) -> Option { if !is_shape_id(shape_id) { return None; diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index 735cd3a33b..0a7c123787 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -954,6 +954,17 @@ pub extern "C" fn js_typed_feedback_object_get_field_by_name_f64( obj: *const ObjectHeader, key: *const crate::StringHeader, ) -> f64 { + // #5094's gate, which the property wrappers never got. Typed-feedback + // recording is OFF by default, and `guard_observe` / `record_fallback_call` + // both early-return in that mode — but the caller has already built the + // whole `Observation` to hand them, hashing the key and resolving the + // receiver's shape. On an isolated property-read loop that dead work was + // 10% of self time. Take the underlying op directly, exactly as the array + // index wrappers already do. + if !typed_feedback_enabled() { + return crate::object::js_object_get_field_by_name_f64(obj, key); + } + let object_addr = normalize_raw_object_addr(obj as u64); let (shape_addr, class_id, gc_type) = object_shape(object_addr); let observation = Observation { @@ -985,6 +996,17 @@ pub extern "C" fn js_typed_feedback_object_set_field_by_name( key: *const crate::StringHeader, value: f64, ) { + // #5094's gate, which the property wrappers never got. Typed-feedback + // recording is OFF by default, and `guard_observe` / `record_fallback_call` + // both early-return in that mode — but the caller has already built the + // whole `Observation` to hand them, hashing the key and resolving the + // receiver's shape. On an isolated property-read loop that dead work was + // 10% of self time. Take the underlying op directly, exactly as the array + // index wrappers already do. + if !typed_feedback_enabled() { + crate::object::js_object_set_field_by_name(obj, key, value); + return; + } let object_addr = normalize_raw_object_addr(obj as u64); let (shape_addr, class_id, gc_type) = object_shape(object_addr); let observation = Observation { @@ -2581,6 +2603,17 @@ pub extern "C" fn js_typed_feedback_object_set_index_polymorphic( idx: f64, value: f64, ) { + // #5094's gate, which the property wrappers never got. Typed-feedback + // recording is OFF by default, and `guard_observe` / `record_fallback_call` + // both early-return in that mode — but the caller has already built the + // whole `Observation` to hand them, hashing the key and resolving the + // receiver's shape. On an isolated property-read loop that dead work was + // 10% of self time. Take the underlying op directly, exactly as the array + // index wrappers already do. + if !typed_feedback_enabled() { + crate::object::js_object_set_index_polymorphic(obj_handle, idx, value); + return; + } let index = finite_nonnegative_u32_index(idx).unwrap_or(u32::MAX); observe_array(site_id, obj_handle as *const ArrayHeader, index); record_guard_fail(site_id); From f5ee7b708c16d1f230186eb96ab43a5506f1a1dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 00:08:25 +0200 Subject: [PATCH 2/5] perf(runtime): megamorphic read stub cache for dynamic string-keyed reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read twin of the dynamic-write stub, 2-way set-associative from the start (#8977 measured what direct-mapped costs: a colliding pair evicts each other every rotation, so both miss forever). A hit skips js_object_get_field_by_name's fast-lane guard chain — address class, interned-key flag, arena classification, header type/flags/class, keys-array validation — plus the read-plan probe, whose epoch the collector bumps at loop-poll cadence, so on a steady read loop it is repeatedly cold and falls through to a shape-index hash lookup. Safety mirrors the write stub: entries store CONTENT bits, never an address, so a recycled key address cannot produce a false hit, and keys that do not fit the inline form are not cached. Every hit re-validates heap-object type, not-forwarded, blocking flags, class id, and the receiver's current shape token — which pins the exact key set and order, so a match means the cached slot still names this key. The probe sits after the process.env and Proxy arms, which keep their own semantics, and the stub is only primed from inside the lane, once the receiver is proved ordinary. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP --- changelog.d/8984-read-stub-cache.md | 30 +++++ .../object/field_get_set/get_field_by_name.rs | 67 ++++++++++ crates/perry-runtime/src/object/mod.rs | 1 + crates/perry-runtime/src/object/read_stub.rs | 118 ++++++++++++++++++ 4 files changed, 216 insertions(+) create mode 100644 changelog.d/8984-read-stub-cache.md create mode 100644 crates/perry-runtime/src/object/read_stub.rs diff --git a/changelog.d/8984-read-stub-cache.md b/changelog.d/8984-read-stub-cache.md new file mode 100644 index 0000000000..d623748c74 --- /dev/null +++ b/changelog.d/8984-read-stub-cache.md @@ -0,0 +1,30 @@ +A megamorphic stub cache for dynamic string-keyed property reads — the read +twin of #8965/#8977's write stub, 2-way set-associative from the start. + +A hit skips `js_object_get_field_by_name`'s fast-lane guard chain (address +class, interned-key flag, arena classification, header type/flags/class, +keys-array validation) and the read-plan probe. That probe matters more than +its own cost suggests: the plan's epoch is bumped by the incremental collector +at loop-poll cadence, so on a steady read loop it is repeatedly cold and falls +through to a shape-index hash lookup. + +Interleaved A/B, min-of-21: pure property read 17 → 15 ms (−12% min, −17% +mean), computed-key read 23 → 22 ms, combined overwrite 41 → 39 ms, write +unchanged. + +**Safety.** Entries store CONTENT bits, never an address, so a key that dies +and has its address recycled cannot produce a false hit; keys that do not fit +the inline form are not cached. Every hit re-validates heap-object type, +not-forwarded, blocking flags, class id, and the receiver's CURRENT shape +token — which pins the exact key set *and order*, so a match means the cached +slot still names this key. The probe sits after the `process.env` and Proxy +arms, which keep their own semantics, and the stub is only primed from inside +the lane, once the receiver is proved ordinary. + +Because a wrong-slot read would be silent corruption rather than a crash, this +carries an adversarial differential: a delete that changes the shape under a +cached slot, an accessor defined over a cached data slot, prototype fallback +after the own property is deleted, `Object.freeze`, the same two keys inserted +in opposite orders, and a 300-key object whose slots live in the overflow +store. Output is byte-identical to node on all of it. Suite 2779 passed; +private-member output identical to base. diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs index 2e77c64d7d..e8b49bb5a9 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs @@ -111,6 +111,55 @@ pub extern "C" fn js_object_get_field_by_name( } } } + // Megamorphic read stub. Primed below once the lane has proved this + // receiver ordinary, so a hit only has to re-prove the properties that can + // change: heap-object type, not forwarded, no blocking flags, a real class + // id, and the receiver's CURRENT shape token. The token pins the exact key + // set and order, so a match means the cached slot still names this key; a + // stale entry misses rather than resolving to the wrong property. + // + // Sits after the process.env and Proxy arms above, which have their own + // semantics and must keep them, and before the lane's guard chain plus the + // read-plan probe — which is what a hit is here to skip. The plan's epoch + // is bumped by the collector at loop-poll cadence, so on a steady read loop + // it is repeatedly cold and falls through to a shape-index hash lookup. + unsafe { + if let Some(key_bits) = super::super::read_stub::read_stub_key_bits(key) { + let addr = obj as usize; + if let Some(gc) = crate::value::addr_class::try_read_gc_header(addr) { + const STUB_BLOCKING: u16 = + crate::gc::OBJ_FLAG_HAS_DESCRIPTORS | crate::gc::OBJ_FLAG_TYPED_ARRAY_PROTO; + if gc.obj_type == crate::gc::GC_TYPE_OBJECT + && gc.gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 + && gc._reserved & STUB_BLOCKING == 0 + { + let o = addr as *const ObjectHeader; + let class_id = (*o).class_id; + if class_id != 0 + && class_id != super::super::native_module::NATIVE_MODULE_CLASS_ID + { + if let Some(token) = super::super::read_stub::receiver_shape_token(o) { + if let Some(slot) = + super::super::read_stub::read_stub_probe(token, key_bits) + { + let live = crate::object::object_live_slot_count(o); + let limit = + std::cmp::max(live, crate::object::INLINE_SLOT_FLOOR as u32); + if slot < limit { + return super::accessors::js_object_get_field(o, slot); + } + if let Some(bits) = super::super::overflow_get(addr, slot as usize) + { + return JSValue::from_bits(bits); + } + } + } + } + } + } + } + } + // FAST LANE (store-plan-cache follow-up): resolve an OWN data field on a // provably-plain arena class instance with no rooting scope, no // exotic-registry probes, and no key hashing. Every gate proves a property @@ -179,6 +228,7 @@ pub extern "C" fn js_object_get_field_by_name( keys as usize, key as usize, ) { + prime_read_stub(o, key, idx); return if (idx as usize) < alloc_limit { super::accessors::js_object_get_field(o, idx) } else { @@ -214,6 +264,7 @@ pub extern "C" fn js_object_get_field_by_name( key, ) { let i = i as usize; + prime_read_stub(o, key, i as u32); super::super::prop_plan::read_plan_record( keys as usize, key as usize, @@ -1698,3 +1749,19 @@ mod null_key_guard_5972 { } } } + +/// Record `(shape token, key content) -> slot` for the megamorphic read stub. +/// +/// Only called from inside the fast lane, i.e. once the receiver has already +/// been proved an ordinary shaped heap object with a resolvable own slot, so +/// the stub never learns an entry for a receiver whose reads have other +/// semantics. Keys that cannot be represented as content bits are skipped by +/// `read_stub_key_bits`. +#[inline] +fn prime_read_stub(obj: *const ObjectHeader, key: *const crate::StringHeader, slot: u32) { + if let Some(key_bits) = super::super::read_stub::read_stub_key_bits(key) { + if let Some(token) = unsafe { super::super::read_stub::receiver_shape_token(obj) } { + super::super::read_stub::read_stub_insert(token, key_bits, slot); + } + } +} diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index c0228ad520..b7dd70f3fe 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -535,6 +535,7 @@ const KEYS_INDEX_THRESHOLD: u32 = 32; #[path = "keys_lookup.rs"] mod keys_lookup; +pub(crate) mod read_stub; pub(crate) use keys_lookup::*; pub(crate) mod array_tail_transition; diff --git a/crates/perry-runtime/src/object/read_stub.rs b/crates/perry-runtime/src/object/read_stub.rs new file mode 100644 index 0000000000..5d56c388c8 --- /dev/null +++ b/crates/perry-runtime/src/object/read_stub.rs @@ -0,0 +1,118 @@ +//! Megamorphic stub cache for dynamic string-keyed property READS. +//! +//! The read twin of the dynamic-write stub in `proxy::put_value`, and it exists +//! for the same reason: a site that rotates more keys than a per-site cache can +//! hold gets no benefit from one, so the cache has to be keyed on the PROGRAM's +//! live `(shape, key)` pairs instead of on a site. +//! +//! What a hit skips is the point. `js_object_get_field_by_name`'s fast lane +//! re-proves a long chain on every read — address-class checks, the interned-key +//! flag, arena classification, header type/flags/class, keys-array validation — +//! and then consults the read-plan cache, whose epoch is bumped by the +//! incremental collector at loop-poll cadence, so on a plain read loop it is +//! repeatedly cold and falls through to a shape-index hash lookup. A stub hit +//! replaces all of that with a handful of loads and compares. +//! +//! # Why entries cannot go stale dangerously +//! +//! Every hit re-validates the receiver's CURRENT state: heap-object type, not +//! forwarded, none of the blocking flags, a real class id, and — decisively — +//! the receiver's current shape token. The token identifies the exact key set +//! AND order, so a matching token means the cached slot still names this key. +//! A stale entry therefore misses; it cannot resolve to the wrong property. +//! +//! Entries hold no roots and no addresses: the key is stored as CONTENT bits +//! (an SSO immediate, or a short ASCII heap string folded to the bits its +//! content would encode as), so a key that dies and has its address recycled +//! cannot produce a false hit. Keys that do not fit the inline form are simply +//! not cached — the same rule the write stub follows, and for the same reason. +//! +//! # Two ways, not one +//! +//! Direct-mapped was measured on the write side and it is a trap: a colliding +//! pair evicts each other on every rotation through the key set, so both miss +//! FOREVER — the miss is permanent, not probabilistic. Making that table 2-way +//! at equal capacity was worth 50% on the write loop (#8977). This one starts +//! 2-way for that reason. + +use crate::object::ObjectHeader; + +const READ_STUB_BUCKETS: usize = 2048; +const READ_STUB_ASSOC: usize = 2; + +crate::perry_thread_local! { + static READ_STUB: [[std::cell::Cell<(u64, u64, u64)>; READ_STUB_ASSOC]; READ_STUB_BUCKETS] = + std::array::from_fn(|_| std::array::from_fn(|_| std::cell::Cell::new((0, 0, 0)))); +} + +/// Content bits for a key, or `None` when it must not be cached. +/// +/// Only keys representable inline are admitted, so an entry names a STRING +/// VALUE and never an address. See the module note on staleness. +#[inline(always)] +pub(crate) fn read_stub_key_bits(key: *const crate::StringHeader) -> Option { + unsafe { crate::string::short_ascii_sso_bits(key) } +} + +#[inline(always)] +fn bucket_of(token: u64, key_bits: u64) -> usize { + // Multiplicative mixing taking the TOP bits of the product. An SSO key's + // LOW bits are its first byte, so a low-bit index collapses a whole key + // family onto a few buckets — measured on the write side before #8977. + let h = (token ^ key_bits).wrapping_mul(0x9E37_79B9_7F4A_7C15); + ((h >> 40) as usize) & (READ_STUB_BUCKETS - 1) +} + +#[inline(always)] +pub(crate) fn read_stub_probe(token: u64, key_bits: u64) -> Option { + READ_STUB.with(|t| { + for way in t[bucket_of(token, key_bits)].iter() { + let (tok, kb, slot) = way.get(); + if tok == token && kb == key_bits && tok != 0 { + return Some(slot as u32); + } + } + None + }) +} + +#[inline(always)] +pub(crate) fn read_stub_insert(token: u64, key_bits: u64, slot: u32) { + if token == 0 || key_bits == 0 { + return; + } + READ_STUB.with(|t| { + let bucket = &t[bucket_of(token, key_bits)]; + let entry = (token, key_bits, slot as u64); + for way in bucket.iter() { + let (tok, kb, _) = way.get(); + if tok == token && kb == key_bits { + way.set(entry); + return; + } + } + for way in bucket.iter() { + if way.get().0 == 0 { + way.set(entry); + return; + } + } + for i in (1..READ_STUB_ASSOC).rev() { + bucket[i].set(bucket[i - 1].get()); + } + bucket[0].set(entry); + }); +} + +/// The receiver's shape token, or `None` when it has no live shape. +/// +/// Same discriminated form the write ICs use, so the two caches agree on what +/// "this shape" means. +#[inline(always)] +pub(crate) unsafe fn receiver_shape_token(obj: *const ObjectHeader) -> Option { + let stamp = crate::object::shapes::object_shape_stamp(obj); + if stamp == 0 { + return None; + } + Some(crate::object::shapes::PIC_ID_TOKEN_BIT | stamp as u64) +} From 4ee260a9ea8028d39acf5fad1bd35cdcc46497f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 00:23:36 +0200 Subject: [PATCH 3/5] fix(codegen): initialize imported private brands once (#8986) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(codegen): an imported class no longer installs its private brand twice (#8962) `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `___constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY * chore: PR-key the fragment --------- Co-authored-by: Ralph Küpper --- changelog.d/8986-imported-private-brands.md | 5 + .../src/lower_call/field_init.rs | 29 ++ crates/perry-hir/src/ir/decl.rs | 21 ++ ...issue_8962_imported_class_private_brand.rs | 284 ++++++++++++++++++ 4 files changed, 339 insertions(+) create mode 100644 changelog.d/8986-imported-private-brands.md create mode 100644 crates/perry/tests/issue_8962_imported_class_private_brand.rs diff --git a/changelog.d/8986-imported-private-brands.md b/changelog.d/8986-imported-private-brands.md new file mode 100644 index 0000000000..5025ed4354 --- /dev/null +++ b/changelog.d/8986-imported-private-brands.md @@ -0,0 +1,5 @@ +Imported private brands are installed once, by the defining module's standalone constructor. + +A metadata-only imported class stub is now identified explicitly, so importing a class that uses private elements no longer re-runs brand installation in the importing module. Re-branding produced a second brand for the same class, so a private access that had been valid through one import path failed through the other. + +Covers direct imports, accessors, local and imported subclasses, same-module branding, and genuine duplicate initialization. diff --git a/crates/perry-codegen/src/lower_call/field_init.rs b/crates/perry-codegen/src/lower_call/field_init.rs index 9090685aaa..6615765a10 100644 --- a/crates/perry-codegen/src/lower_call/field_init.rs +++ b/crates/perry-codegen/src/lower_call/field_init.rs @@ -693,10 +693,39 @@ pub(crate) fn apply_field_initializers_recursive( None => init_pairs.push((field.name.clone(), init, field.is_private)), } } + // #8962: an IMPORTED class installs nothing here. Its whole + // field-initializer phase — public field writes, private-field adds AND + // the shared private brand — is baked into the defining module's + // standalone `___constructor`, which `codegen/method.rs` + // emits for exactly that reason ("At the `new ImportedClass(...)` call + // site, `lower_new` applies initializers against the imported class + // stub — which has none"). That premise holds for FIELDS because the + // stub flattens every field to `is_private: false` with `init: None`, + // so the worst this loop could do was write `undefined` into a slot the + // real constructor overwrites moments later. + // + // It does NOT hold for the private BRAND. The stub copies private + // METHOD and accessor names verbatim (it needs them to resolve dispatch + // symbols), and `has_private_instance_brand` is defined purely over + // `#`-prefixed method/getter/setter names — so a stub answers `true` and + // this site emitted `js_private_brand_add` at the importing module's + // `new`, on top of the one the defining module's constructor emits. + // Installing a class's brand twice on one object is the observable + // error PrivateMethodOrAccessorAdd requires, so the runtime threw + // "Cannot initialize private elements twice on the same object" out of + // `new Hono()` — any imported class with a private method or accessor, + // whether constructed directly or reached as an ancestor through + // `AncestorsOnly`. + // + // Suppressing BOTH flags (not just the brand) is what restores the + // `continue` below for a stub whose only private elements are methods: + // for a stub the two predicates are the same question, since its fields + // are never private. let (class_has_private_elements, class_has_private_brand) = ctx .classes .get(&class_name_in_chain) .copied() + .filter(|class| !class.is_imported_stub()) .map(|class| { ( class.has_private_instance_elements(), diff --git a/crates/perry-hir/src/ir/decl.rs b/crates/perry-hir/src/ir/decl.rs index 4683d1b220..dbbe74e3cc 100644 --- a/crates/perry-hir/src/ir/decl.rs +++ b/crates/perry-hir/src/ir/decl.rs @@ -284,6 +284,27 @@ pub struct Class { } impl Class { + /// True for the metadata-only stub `compile_module` synthesizes for a class + /// IMPORTED from another module (`perry-codegen/src/codegen/mod.rs`, "Build + /// a stub Class with the minimum fields the codegen needs"). + /// + /// A stub is a NAME TABLE, not a class: it carries member names so the + /// importing module can resolve dispatch symbols, and carries no bodies, no + /// field initializers and no constructor. Everything a construction + /// actually *does* — field initializers, private-field adds, the private + /// brand — is baked into the defining module's standalone + /// `___constructor` instead (`codegen/method.rs`, + /// `is_constructor_method`), precisely because the stub has none of it. + /// + /// `id == 0` is the marker: the driver hands out class ids from 1 + /// (`run_pipeline.rs`: "Start at 1, 0 is reserved for \"no parent\"") and + /// every local class takes its id from `LoweringContext::fresh_class`, so + /// the stub built at `codegen/mod.rs` ("id: 0, // imported — no local + /// ClassId") is the only `Class` in a module's class table with id 0. + pub fn is_imported_stub(&self) -> bool { + self.id == 0 + } + /// Whether construction installs any instance-private element. pub fn has_private_instance_elements(&self) -> bool { self.fields.iter().any(|field| field.is_private) diff --git a/crates/perry/tests/issue_8962_imported_class_private_brand.rs b/crates/perry/tests/issue_8962_imported_class_private_brand.rs new file mode 100644 index 0000000000..58eea312a1 --- /dev/null +++ b/crates/perry/tests/issue_8962_imported_class_private_brand.rs @@ -0,0 +1,284 @@ +//! Regression for #8962: constructing a class IMPORTED from another module +//! threw `TypeError: Cannot initialize private elements twice on the same +//! object` when that class declared a private method or accessor. +//! +//! `import { Hono } from "hono"; new Hono()` was the report. The importing +//! module sees the class only as the metadata-only stub `compile_module` +//! builds — a name table with no bodies and no initializers — but the stub +//! copies private METHOD names verbatim (it needs them to resolve dispatch +//! symbols), and `Class::has_private_instance_brand` is defined purely over +//! `#`-prefixed member names. So the `new` site emitted `js_private_brand_add` +//! for a brand it does not own, on top of the one the DEFINING module's +//! standalone `___constructor` emits — and installing a class's +//! brand twice on one object is the error PrivateMethodOrAccessorAdd requires. +//! +//! Every case here calls the private member after construction, so a fix that +//! merely dropped the second install without leaving the first one standing +//! would fail these too: the brand check inside the private-member access +//! throws when no brand is present. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Compile `files` (relative path -> source) with `entry` as the entry point +/// and return the binary's stdout. Panics with the compiler's or the program's +/// output on any failure. +fn compile_and_run(files: &[(&str, &str)], entry: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + for (name, source) in files { + let path = root.join(name); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("mkdir"); + } + std::fs::write(&path, source).expect("write source"); + } + let entry_path = root.join(entry); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry_path) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + assert!( + run.status.success(), + "binary failed (status {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_string() +} + +/// The reduced `new Hono()`: a class with a private METHOD, declared in one +/// module and constructed in another. No inheritance is needed to trigger it. +#[test] +fn imported_class_with_private_method_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private ACCESSOR carries the same brand as a private method, and the stub +/// copies getter/setter names the same way. +#[test] +fn imported_class_with_private_getter_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + v = 41; + get #g(): number { return this.v + 1; } + call(): number { return this.#g; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private-method class reached as an ANCESTOR: the leaf is local, so the +/// brand came from the `AncestorsOnly` walk at the `new` site rather than from +/// the leaf's own entry. Both spellings of the subclass — with and without an +/// explicit constructor — take different paths through `lower_new`. +#[test] +fn local_subclass_of_imported_private_method_class() { + let base = r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#; + let with_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX { constructor() { super(); } } +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(with_ctor, "42"); + + let without_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX {} +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(without_ctor, "42"); +} + +/// hono's own shape: the base with the private members is in one module, the +/// subclass that `super()`s into it is an anonymous class expression in a +/// second, and the `new` is in a third. Every link in the chain is an imported +/// stub at the site that constructs it. +#[test] +fn imported_subclass_of_imported_private_method_class() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"const notFound = (x: string): string => "nf:" + x; +export class BaseX { + pub: number; + #path = "/"; + #nf = notFound; + constructor(options: any = {}) { + this.pub = 1; + } + #addRoute(m: string): string { return m + this.#path; } + route(m: string): string { return this.#addRoute(m) + this.#nf("!"); } +} +"#, + ), + ( + "mid.ts", + r#"import { BaseX } from "./base"; +export const DerivedX = class extends BaseX { + constructor(options: any = {}) { super(options); } +}; +"#, + ), + ( + "main.ts", + r#"import { DerivedX } from "./mid"; +const a = new DerivedX(); +console.log(a.route("GET") + "|" + a.pub); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "GET/nf:!|1"); +} + +/// The same class constructed INSIDE its defining module never had the bug — +/// there the `new` site owns the field-initializer phase and installs the +/// brand itself. Pin it, so a fix that suppressed the install unconditionally +/// (rather than only where another module already performs it) fails here. +#[test] +fn same_module_construction_still_installs_the_brand() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +export function make(): BaseX { return new BaseX(); } +"#, + ), + ( + "main.ts", + r#"import { make } from "./base"; +console.log(make().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// Double initialization must still be observable where the spec requires it: +/// a base constructor that returns an object the derived class has already +/// branded. This is the case `js_private_brand_add`'s duplicate check exists +/// for, and #8962's fix must not silence it. +#[test] +fn genuine_double_initialization_still_throws() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::write( + root.join("main.ts"), + r#"const recycled: any = {}; +class Base { + constructor() { return recycled; } +} +class Derived extends Base { + #m(): number { return 1; } + call(): number { return this.#m(); } +} +new Derived(); +new Derived(); +console.log("no throw"); +"#, + ) + .expect("write entry"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(root.join("main.ts")) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstderr:\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + let stderr = String::from_utf8_lossy(&run.stderr); + let stdout = String::from_utf8_lossy(&run.stdout); + assert!( + !run.status.success() && stderr.contains("private elements twice"), + "expected the second construction to throw the duplicate-brand \ + TypeError\nstatus: {:?}\nstdout:\n{stdout}\nstderr:\n{stderr}", + run.status.code() + ); +} From 6215a3883f29478c21860dc27add61c928694e02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 00:23:44 +0200 Subject: [PATCH 4/5] fix(runtime): inherit Array-subclass fill (#8987) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(runtime): inherit Array-subclass fill (#8953) * chore: PR-key the fragment; reuse the shared StringHeader payload helper --------- Co-authored-by: Ralph Küpper --- .../8987-array-subclass-enumeration.md | 5 ++ crates/perry-runtime/src/array/subclass.rs | 12 +-- .../src/node_stream_constructors/builders.rs | 30 ++----- .../src/object/field_get_set/accessors.rs | 37 ++++++-- .../field_get_set/get_field_by_name_tail.rs | 15 ++-- .../src/object/native_call_method.rs | 45 +++++++--- .../issue_8953_array_subclass_enumeration.rs | 84 +++++++++++++++++++ 7 files changed, 171 insertions(+), 57 deletions(-) create mode 100644 changelog.d/8987-array-subclass-enumeration.md create mode 100644 crates/perry/tests/issue_8953_array_subclass_enumeration.rs diff --git a/changelog.d/8987-array-subclass-enumeration.md b/changelog.d/8987-array-subclass-enumeration.md new file mode 100644 index 0000000000..c8cb62a190 --- /dev/null +++ b/changelog.d/8987-array-subclass-enumeration.md @@ -0,0 +1,5 @@ +### Fixed + +- Array-subclass enumeration now matches Node: `Object.keys` and `for...in` + report only enumerable indices, while `Object.getOwnPropertyNames` also + reports `length`; inherited `Array.prototype.fill` no longer leaks as an own key. diff --git a/crates/perry-runtime/src/array/subclass.rs b/crates/perry-runtime/src/array/subclass.rs index 094f174442..3bbd35361a 100644 --- a/crates/perry-runtime/src/array/subclass.rs +++ b/crates/perry-runtime/src/array/subclass.rs @@ -543,11 +543,11 @@ pub(crate) unsafe fn array_subclass_named_prefix_token_for_slot( } } - // `js_array_subclass_init` installs two canonical own properties that are - // absent from most class allocation shapes: `length` and the generic - // `fill` method. If a class declared either name, init overwrites its - // existing slot; otherwise the exact missing names must follow the - // declared prefix in that order. Anything else is instance-specific. + // The legacy shape-carried representation installs `length` and its + // compatibility `fill` closure after the declared prefix. The default + // elements-backed representation inherits `fill` from `Array.prototype` + // and has no runtime names in its shape. Anything else is + // instance-specific. let declared_count = declared_count as u32; let mut expected_runtime_names: [&[u8]; 2] = [&[]; 2]; let mut expected_runtime_count = 0usize; @@ -568,7 +568,7 @@ pub(crate) unsafe fn array_subclass_named_prefix_token_for_slot( // elements store (the store owns `length`); keep it off this token. return 0; } - if !declared_fill { + if !elements_backed && !declared_fill { expected_runtime_names[expected_runtime_count] = b"fill"; expected_runtime_count += 1; } diff --git a/crates/perry-runtime/src/node_stream_constructors/builders.rs b/crates/perry-runtime/src/node_stream_constructors/builders.rs index c1b8956409..abfbef223e 100644 --- a/crates/perry-runtime/src/node_stream_constructors/builders.rs +++ b/crates/perry-runtime/src/node_stream_constructors/builders.rs @@ -162,15 +162,11 @@ pub extern "C" fn js_event_emitter_async_resource_subclass_init(this: f64, optio /// `super(n)` for a source-compiled `class X extends Array` (e.g. lru-cache's /// `ZeroArray`: `class ZeroArray extends Array { constructor(n){ super(n); /// this.fill(0) } }`). Perry models the subclass instance as a plain object, -/// not a real exotic Array, so `super(n)` otherwise left it length-less with no -/// Array methods. Size it (`length = ToLength(n)`, a visible own property the -/// generic array-like helpers read) and install the Array surface the instance -/// relies on — currently `fill`, which delegates to `js_array_fill_generic` -/// (it operates on the receiver's own `length` + indexed properties, exactly -/// what an array-like object exposes). Indexed get/set already work as ordinary -/// object properties. Mirrors `js_event_emitter_subclass_init` (#5494); the -/// codegen `super()` lowering for an `Array` parent calls this. Additional -/// Array methods can be added to `array_subclass_methods` as bundles need them. +/// not a real exotic Array, so `super(n)` initializes its elements store. In +/// the default representation, inherited methods resolve through +/// `Array.prototype` and are not stamped as enumerable own properties. The +/// legacy shape-carried kill switch retains its old compatibility closure. +/// The codegen `super()` lowering calls this entry point. #[no_mangle] pub extern "C" fn js_array_subclass_init(this: f64, n: f64) -> f64 { let raw = raw_ptr_from_value(this); @@ -202,18 +198,6 @@ pub extern "C" fn js_array_subclass_init(this: f64, n: f64) -> f64 { unsafe { crate::array::subclass_elements::install_elements(obj, len.min(u32::MAX as f64) as u32) }; - // The Array surface the instance relies on, installed exactly as in - // the shape-carried form. It must NOT be hidden behind a property - // descriptor: that sets `OBJ_FLAG_HAS_DESCRIPTORS` on every instance, - // which the codegen class-field inline guard rejects — every field - // read then takes the IC miss (measured: 6x on the wolf-ecs twins). - // `fill` showing up in `getOwnPropertyNames` is the pre-existing - // divergence tracked in #8953, unchanged by the elements store. - let this = this_root.get_nanbox_f64(); - let obj = raw_ptr_from_value(this) as *mut ObjectHeader; - crate::closure::js_register_closure_arity(ns_array_fill as *const u8, 3); - let methods: [(&str, StubFn); 1] = [("fill", super::cast3(ns_array_fill))]; - install_methods_on_existing_object(obj, this, &methods, &[]); return this_root.get_nanbox_f64(); } let length_key = crate::string::js_string_from_bytes(b"length".as_ptr(), 6); @@ -258,9 +242,7 @@ pub unsafe extern "C" fn js_array_subclass_init_args( this.get_nanbox_f64() } -/// `Array.prototype.fill`-equivalent installed on an Array-subclass instance: -/// fills the receiver's own indexed slots `0..length` with `value`. Delegates -/// to the generic array-like fill (which reads `length` off the receiver). +/// Legacy shape-carried compatibility closure for `Array.prototype.fill`. pub(super) extern "C" fn ns_array_fill( closure: *const ClosureHeader, value: f64, diff --git a/crates/perry-runtime/src/object/field_get_set/accessors.rs b/crates/perry-runtime/src/object/field_get_set/accessors.rs index 46b190cc6d..738cfed07b 100644 --- a/crates/perry-runtime/src/object/field_get_set/accessors.rs +++ b/crates/perry-runtime/src/object/field_get_set/accessors.rs @@ -599,6 +599,29 @@ pub(crate) unsafe fn string_index_value( } } +/// Resolve an inherited `Array.prototype` property for an Array-subclass +/// instance after its own fields and class-declared methods have missed. +/// An explicit per-instance prototype replaces the ordinary class chain and +/// therefore suppresses this implicit fallback. +pub(crate) unsafe fn array_subclass_prototype_field( + obj: *const ObjectHeader, + key: *const crate::StringHeader, +) -> Option { + if obj.is_null() + || key.is_null() + || super::super::prototype_chain::object_static_prototype(obj as usize).is_some() + || !crate::array::is_array_subclass_class_id((*obj).class_id) + { + return None; + } + let key_ptr = crate::object::string_header_payload(key); + let key_len = (*key).byte_len as usize; + let name = std::str::from_utf8(std::slice::from_raw_parts(key_ptr, key_len)).ok()?; + // `array_prototype_property_value` copies `name` before its first + // allocation and roots the receiver across the prototype lookup. + array_prototype_property_value(name, obj as usize) +} + pub(crate) unsafe fn array_prototype_property_value( name: &str, receiver_addr: usize, @@ -625,6 +648,8 @@ pub(crate) unsafe fn array_prototype_property_value( let name_copy = super::HeapKeyBytes::copy_of(name.as_bytes()); let name: &str = std::str::from_utf8_unchecked(name_copy.as_bytes()); + let scope = crate::gc::RuntimeHandleScope::new(); + let receiver_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(receiver_addr as i64)); let ctor = super::super::js_get_global_this_builtin_value(b"Array".as_ptr(), 5); let ctor_value = JSValue::from_bits(ctor.to_bits()); if !ctor_value.is_pointer() { @@ -636,15 +661,11 @@ pub(crate) unsafe fn array_prototype_property_value( if !proto_value.is_pointer() { return None; } - // #7498: `js_string_from_bytes` ALLOCATES, so `Array.prototype` and the - // receiver cannot be carried across it as bare `usize`s — and the key it - // produces is itself a fresh heap string this function then hands to two - // more calls that can collect (`js_object_get_field_by_name` runs getters; - // `default_object_prototype_property_value` interns another key). Root all - // three and read each back at its point of use. - let scope = crate::gc::RuntimeHandleScope::new(); + // #7498: the receiver is rooted before the allocating global lookup above; + // `Array.prototype` and the fresh key are rooted before the calls below, + // which can collect (`js_object_get_field_by_name` runs getters and + // `default_object_prototype_property_value` interns another key). let proto_h = scope.root_nanbox_f64(proto); - let receiver_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(receiver_addr as i64)); let key_h = scope.root_nanbox_f64(crate::value::nanbox_string_key( crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32), )); diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs index f56ff584e1..4b2605db38 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs @@ -1,12 +1,9 @@ -//! Object-deref tail of `js_object_get_field_by_name`: pointer-strip, -//! handle dispatch, and the full ObjectHeader property walk. Extracted -//! verbatim from field_get_set.rs (issue #1103 split) so neither half -//! exceeds the file-size budget. Pure relocation — no logic change. +//! Object-deref tail of `js_object_get_field_by_name`: pointer stripping, +//! handle dispatch, and the full ObjectHeader property walk (#1103 split). use super::*; -/// Tail of `js_object_get_field_by_name` (everything after the leading -/// primitive/handle/Date receiver guards). Body moved verbatim. +/// Object-deref tail of `js_object_get_field_by_name`. pub(crate) fn get_field_by_name_object_tail( obj: *const ObjectHeader, key: *const crate::StringHeader, @@ -1470,6 +1467,9 @@ pub(crate) fn get_field_by_name_object_tail( { return v; } + if let Some(v) = super::accessors::array_subclass_prototype_field(obj, key) { + return v; + } if let Some(v) = ordinary_object_prototype_property_value(obj, key) { return v; } @@ -1873,6 +1873,9 @@ pub(crate) fn get_field_by_name_object_tail( { return v; } + if let Some(v) = super::accessors::array_subclass_prototype_field(obj, key) { + return v; + } if let Some(v) = ordinary_object_prototype_property_value(obj, key) { return v; } diff --git a/crates/perry-runtime/src/object/native_call_method.rs b/crates/perry-runtime/src/object/native_call_method.rs index 06a5393925..eb038fb691 100644 --- a/crates/perry-runtime/src/object/native_call_method.rs +++ b/crates/perry-runtime/src/object/native_call_method.rs @@ -2102,19 +2102,38 @@ pub unsafe extern "C-unwind" fn js_native_call_method( let method_key = crate::string::js_string_from_bytes(method_name.as_ptr(), method_name.len() as u32); if !method_key.is_null() { - let inherited = super::prototype_chain::resolve_inherited_field( - obj as usize, - method_key, - ) - .or_else(|| unsafe { - // A plain object's implicit Object.prototype is not stored in - // the recorded-prototype table. Property reads already use - // this guarded fallback, so direct `obj.method()` dispatch - // must consult it too (including user-added methods such as a - // borrowed Array.prototype.join). The helper rejects arrays, - // exotic/null-prototype objects, and explicit overrides. - super::field_get_set::ordinary_object_prototype_property_value(obj, method_key) - }); + let inherited = + super::prototype_chain::resolve_inherited_field(obj as usize, method_key) + .or_else(|| unsafe { + // A plain object's implicit Object.prototype is not stored in + // the recorded-prototype table. Property reads already use + // this guarded fallback, so direct `obj.method()` dispatch + // must consult it too (including user-added methods such as a + // borrowed Array.prototype.join). The helper rejects arrays, + // exotic/null-prototype objects, and explicit overrides. + super::field_get_set::ordinary_object_prototype_property_value( + obj, method_key, + ) + }) + .or_else(|| unsafe { + // Elements-backed Array-subclass instances inherit `fill` + // instead of carrying a bound enumerable own closure (#8953). + // A class method or explicit per-instance prototype wins; only + // the ordinary class chain reaches Array.prototype here. + let class_id = (*obj).class_id; + if method_name != "fill" + || super::prototype_chain::object_static_prototype(obj as usize) + .is_some() + || !crate::array::is_array_subclass_class_id(class_id) + || lookup_class_method_in_chain(class_id, method_name).is_some() + { + return None; + } + super::field_get_set::array_prototype_property_value( + method_name, + obj as usize, + ) + }); if let Some(field_val) = inherited { if !field_val.is_undefined() && !field_val.is_null() { let bound = crate::closure::clone_closure_rebind_this( diff --git a/crates/perry/tests/issue_8953_array_subclass_enumeration.rs b/crates/perry/tests/issue_8953_array_subclass_enumeration.rs new file mode 100644 index 0000000000..5ce97c7fcc --- /dev/null +++ b/crates/perry/tests/issue_8953_array_subclass_enumeration.rs @@ -0,0 +1,84 @@ +//! #8953: Array-subclass instances use an elements store for indices and +//! `length`, while inherited Array methods stay off the instance shape. +use std::path::PathBuf; +use std::process::{Command, Output}; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(source: &str) -> Output { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.js"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-auto-optimize") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + Command::new(&output).output().expect("run compiled binary") +} + +#[test] +fn array_subclass_enumeration_matches_node_and_fill_is_inherited() { + let run = compile_and_run( + r#" +class A extends Array {} + +const empty = new A(); +console.log("empty keys:", Object.keys(empty).join(",")); +let emptyForIn = []; +for (const key in empty) emptyForIn.push(key); +console.log("empty for-in:", emptyForIn.join(",")); +console.log("empty names:", Object.getOwnPropertyNames(empty).join(",")); +console.log("fill:", Object.hasOwn(empty, "fill"), typeof empty.fill); + +const a = new A(); +a.push(1, 2, 3); +console.log("keys:", Object.keys(a).join(",")); +let keys = []; +for (const key in a) keys.push(key); +console.log("for-in:", keys.join(",")); +console.log("names:", Object.getOwnPropertyNames(a).join(",")); +a.fill(7, 1); +console.log("filled:", a.join(",")); +const inheritedFill = a.fill; +inheritedFill.call(a, 9, 2); +console.log("extracted fill:", a.join(",")); + +class B extends Array { fill(value) { return "override:" + value; } } +const b = new B(); +console.log("override:", Object.hasOwn(b, "fill"), b.fill(5)); +"#, + ); + assert!( + run.status.success(), + "the #8953 fixture must not crash\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&run.stdout), + "empty keys: \n\ +empty for-in: \n\ +empty names: length\n\ +fill: false function\n\ +keys: 0,1,2\n\ +for-in: 0,1,2\n\ +names: 0,1,2,length\n\ +filled: 1,7,7\n\ +extracted fill: 1,7,9\n\ +override: false override:5\n" + ); +} From 906ba294b76fdd2f78909f07ced884a488fb9b8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 00:48:54 +0200 Subject: [PATCH 5/5] chore: PR-key the fragment, drop a duplicate, classify READ_STUB READ_STUB is a new identity-ratcheted thread-local holder; recorded the same not_a_gc_pointer verdict WRITE_STUB carries, since read_stub_key_bits returns short_ascii_sso_bits (content packed inline) and never a heap address. --- .../8981-feedback-gate-and-shape-field.md | 38 ------------------- ...-stub-cache.md => 8988-read-stub-cache.md} | 0 scripts/gc_runtime_root_holders.json | 6 +++ 3 files changed, 6 insertions(+), 38 deletions(-) delete mode 100644 changelog.d/8981-feedback-gate-and-shape-field.md rename changelog.d/{8984-read-stub-cache.md => 8988-read-stub-cache.md} (100%) diff --git a/changelog.d/8981-feedback-gate-and-shape-field.md b/changelog.d/8981-feedback-gate-and-shape-field.md deleted file mode 100644 index 66a4acaecb..0000000000 --- a/changelog.d/8981-feedback-gate-and-shape-field.md +++ /dev/null @@ -1,38 +0,0 @@ -Two dead-work removals on the property read path. The computed-key read loop -now **matches node** (23 ms vs 23 ms on the same host), and the pure property -read drops 21 → 17 ms. - -**1. Typed-feedback observation is skipped when recording is off.** Recording -is off by default, and `guard_observe` and `record_fallback_call` both -early-return in that mode — but the property wrappers built the whole -`Observation` first, hashing the key and resolving the receiver's shape, purely -to hand it to functions that discard it. `js_typed_feedback_object_get_field_by_name_f64` -was 10% of an isolated property-read loop, nearly all of it that. The array -index wrappers have carried #5094's gate for exactly this reason, and #8951 -gave it to the fast store path; the property get/set wrappers never got it. - -Behaviour is unchanged in both modes: with recording off `guard_observe` -returns `contract_valid` unmodified and the fallback recorder is a no-op, so -the wrapper already reduced to precisely the underlying call it now makes -directly. - -**2. The slot bound stops copying a descriptor to read four bytes.** -`shape_descriptor_by_id` returns `ShapeDescriptor` **by value**, so -`object_live_slot_count` — consulted on essentially every property read and -write — lifted the whole ~48-byte record and kept only -`live_inline_slot_count`. It now reads that field through the table's record -using the same way-cache probe and the same epoch validation. -`shape_descriptor_by_id` was 10.1% of the same loop. - -Interleaved A/B, min-of-21, node on the same host in brackets: - -| loop | base | this PR | | -|---|---|---|---| -| pure property read | 21 ms (4) | **17 ms** | −19% | -| computed-key read | 27 ms (23) | **23 ms** | −15% — now at parity with node | -| combined overwrite | 46 ms (31) | **41 ms** | −11% | -| write only | 21 ms (23) | 21 ms | unchanged; already faster than node | - -Suite 2779 passed (including the 55 typed-feedback tests). Private-member -output is byte-identical to base; computed-key differential is byte-identical -to node. diff --git a/changelog.d/8984-read-stub-cache.md b/changelog.d/8988-read-stub-cache.md similarity index 100% rename from changelog.d/8984-read-stub-cache.md rename to changelog.d/8988-read-stub-cache.md diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index 009cf679a1..aeee7e9ce3 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -322,6 +322,12 @@ "verdict": "not_a_gc_pointer", "why": "Megamorphic dynamic-write stub cache: 4096 ways of (shape_token, key_bits, slot) plain u64s. The token is a shape id and the slot an index; key_bits are content-derived by construction, because stub_key_bits admits only SSO immediates (and short ASCII heap strings folded to the SSO bits of their content) and rejects every key that would otherwise be stored under a pointer. No way holds a heap address, so nothing here keeps an object alive, and a stale entry is rejected by dyn_ic_try_store's per-hit shape-token/flags/slot-bound revalidation." }, + { + "file": "crates/perry-runtime/src/object/read_stub.rs", + "name": "READ_STUB", + "verdict": "not_a_gc_pointer", + "why": "Megamorphic property-read stub cache, the read twin of WRITE_STUB: 2-way ways of (shape_token, key_bits, slot) plain u64s. The token is a shape id and the slot an index; key_bits are content-derived by construction, because read_stub_key_bits returns short_ascii_sso_bits(key) \u2014 the key's characters packed inline \u2014 and yields None for any key that would otherwise be stored under a pointer. No way holds a heap address, so nothing here keeps an object alive, and a stale entry cannot hit: receiver_shape_token returns None for a receiver with no live shape, and the token identifies the exact key set and order, so a shape change yields a different token." + }, { "file": "crates/perry-runtime/src/pty/mod.rs", "name": "EXIT_SINK",