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/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/changelog.d/8988-read-stub-cache.md b/changelog.d/8988-read-stub-cache.md new file mode 100644 index 0000000000..d623748c74 --- /dev/null +++ b/changelog.d/8988-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-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-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.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/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/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/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-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) +} 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" + ); +} 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() + ); +} 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",