diff --git a/changelog.d/9214-decl-prototype-reverse-index.md b/changelog.d/9214-decl-prototype-reverse-index.md new file mode 100644 index 0000000000..b2cb20391a --- /dev/null +++ b/changelog.d/9214-decl-prototype-reverse-index.md @@ -0,0 +1,33 @@ +**"Which class's `.prototype` is this object?" is now O(1)** (#9180). Every +`Object.defineProperty`, `Object.getOwnPropertyDescriptor` and `delete` asked +`class_id_for_decl_prototype_object` about its receiver, and the answer was a +linear scan of every materialized declared-class prototype. The comment above +it explained that the table was small and the path was a cold reflection path; +a bundled application falsifies both — esbuild's `__export(exports, { … })` +runs `defineProperty` thousands of times during module init, the receivers are +never prototypes, and a miss walked the whole table. It was 3.10% of +`cc --help`. + +The registry now carries its own inverse. `CLASS_DECL_PROTOTYPE_OBJECTS` holds +a `DeclPrototypeTable` whose two maps are private to one file, so the six +existing mutation sites — the store, both GC root scanners, the per-slot GC +step, the test reset and the test seeds — go through methods that update both +directions together, and a seventh cannot be written without editing that file. +That matters more than it sounds: an earlier pointer-keyed cache with +hand-placed invalidation went stale at the sites it missed, and the symptom was +not a crash but `getOwnPropertyDescriptor(C.prototype, "g")` quietly returning +`undefined` where node returns an accessor descriptor. + +Two things keep it honest beyond privacy. The reverse map is an exact inverse +only while one address belongs to one class id and is never re-pointed; +`insert` is where either could break, checks both while inserting with one +extra reverse lookup, and on anything unusual abandons the index for good and +answers from the same authoritative forward-table scan as before. In a +`debug_assertions` build every reverse lookup is compared against that scan, so +the whole runtime test suite is checking the index rather than trusting it. + +Measured on Linux with 400 declared-class prototypes materialized, per +operation on non-prototype receivers: `getOwnPropertyDescriptor` 420 → 200 ns +(2.10×), `defineProperty` 1585 → 1055 ns (1.50×), `delete` 2685 → 2445 ns. The +scan's signature was the slope — `getOwnPropertyDescriptor` cost 180/210/305/420 +ns at 0/50/200/400 prototypes before, and is flat at ~200 ns after. diff --git a/crates/perry-runtime/src/object/class_gc_roots.rs b/crates/perry-runtime/src/object/class_gc_roots.rs index d968c67225..b0a221ac95 100644 --- a/crates/perry-runtime/src/object/class_gc_roots.rs +++ b/crates/perry-runtime/src/object/class_gc_roots.rs @@ -54,9 +54,9 @@ pub fn scan_class_inheritance_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Ok(mut guard) = table.write() { if let Some(map) = guard.as_mut() { - for ptr in map.values_mut() { + map.visit_root_slots(|ptr| { visitor.visit_usize_slot(ptr); - } + }); } } }); @@ -85,9 +85,10 @@ pub(crate) fn test_seed_class_inheritance_roots(proto_cid: u32, proto_ptr: usize #[cfg(test)] pub(crate) fn test_seed_decl_class_prototype_root(class_id: u32, proto_ptr: usize) { CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { - let mut guard = table.write().unwrap(); - guard - .get_or_insert_with(std::collections::HashMap::new) + table + .write() + .unwrap() + .get_or_insert_with(Default::default) .insert(class_id, proto_ptr); }); } @@ -122,7 +123,7 @@ pub(crate) fn test_decl_class_prototype_root(class_id: u32) -> usize { .read() .unwrap() .as_ref() - .and_then(|m| m.get(&class_id).copied()) + .and_then(|m| m.get(class_id)) .unwrap_or(0) }) } @@ -148,7 +149,7 @@ pub(crate) fn test_clear_class_inheritance_roots(proto_cid: u32, closure_cid: u3 }); CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Some(m) = table.write().unwrap().as_mut() { - m.remove(&proto_cid); + m.remove(proto_cid); } }); CLASS_PARENT_CLOSURES.with(|table| { diff --git a/crates/perry-runtime/src/object/class_registry.rs b/crates/perry-runtime/src/object/class_registry.rs index 27e330209c..a50575f88e 100644 --- a/crates/perry-runtime/src/object/class_registry.rs +++ b/crates/perry-runtime/src/object/class_registry.rs @@ -46,6 +46,7 @@ mod builtin_alias_construct; mod class_meta; mod construct; pub(crate) use construct::scan_current_new_target_root_mut; +pub mod decl_prototype_table; mod dispatch; mod function_prototype; mod gc_roots; diff --git a/crates/perry-runtime/src/object/class_registry/decl_prototype_table.rs b/crates/perry-runtime/src/object/class_registry/decl_prototype_table.rs new file mode 100644 index 0000000000..0844667271 --- /dev/null +++ b/crates/perry-runtime/src/object/class_registry/decl_prototype_table.rs @@ -0,0 +1,358 @@ +//! The declared-class `.prototype` registry, with its reverse index built in. +//! +//! # Why this is a type and not a `HashMap` +//! +//! `class_id_for_decl_prototype_object` answers "which declared class's +//! `.prototype` is this heap object?". Its callers +//! (`descriptor_state::disable_inline_guards_for_descriptor_target`, +//! `Object.getOwnPropertyDescriptor`, `delete`, the proxy and +//! `Reflect.metadata` paths) ask it about ARBITRARY objects, so the common +//! answer is "none". Until #9180 that answer cost a linear scan of every +//! materialized declared-class prototype, and on a large application — where +//! esbuild-style `__export(exports, { … })` runs `Object.defineProperty` +//! thousands of times during module init — the scan was 3.10% of `cc --help`. +//! +//! The obvious repair is a pointer-keyed reverse cache invalidated at the +//! writers. That was tried and it was wrong: the table has six mutation sites +//! (the store, the two GC root scanners, the per-slot GC step, the test reset +//! and the test seeds), a missed one leaves the cache stale, and a stale +//! reverse lookup does not crash — it silently reports "not a prototype", so +//! `getOwnPropertyDescriptor(C.prototype, "g")` starts returning `undefined` +//! where node returns an accessor descriptor. +//! +//! So the invalidation here is not maintained by diligence. Three properties +//! carry it, in decreasing order of how much they are asked to do. +//! +//! ## 1. Privacy — a seventh writer cannot forget +//! +//! `forward` and `reverse` are private to THIS FILE. No other module can name +//! them, so no other module can insert, remove, or rewrite an address; the +//! only ways in are the handful of methods below, and each updates both +//! directions in one statement sequence. Adding a mutation site now means +//! editing this file. +//! +//! ## 2. The fallback is the code that was already correct +//! +//! A targeted `reverse` update is an exact inverse only while `forward` is +//! *injective* — one prototype address per class id, never re-pointed. That +//! is how the registry is really built (every entry is a fresh +//! `js_object_alloc`, stored once, and afterwards only ever relocated by the +//! collector), but it is a precondition, not a theorem, and a precondition +//! that quietly stopped holding is exactly the failure mode above: with two +//! class ids on one address, retiring one entry drops the other's reverse +//! key and its lookups start missing. +//! +//! `insert` is the only place either half of that precondition can break, so it +//! checks both while inserting (with one extra reverse lookup), and when a +//! check trips it gives the index up for good. In that mode `class_id_for` +//! falls back to `scan_class_id_for`, the same authoritative forward-table +//! scan used before #9180. +//! +//! ## 3. The index is checked against its own ground truth +//! +//! In a `debug_assertions` build — i.e. throughout the runtime test suite — +//! every `class_id_for` compares the index against that same linear scan, and +//! every mutation re-checks the whole invariant. A mutation added here that +//! forgets `reverse` fails a test rather than shipping a wrong descriptor. +//! +//! # Why the GC cannot invalidate it either +//! +//! Evacuation rewrites the stored addresses through `visit_usize_slot`. Both +//! visit helpers below capture the slot's value, hand the slot to the +//! visitor, and re-key `reverse` from what the visitor left behind — so a +//! move updates both maps or neither. + +use crate::fast_hash::PtrHashMap; + +/// `class_id → *mut ObjectHeader` for materialized declared-class +/// prototypes, plus the pointer-keyed inverse. +#[derive(Default)] +pub struct DeclPrototypeTable { + /// The authoritative direction. Values are raw addresses (`usize` so the + /// table stays `Send + Sync`); they are GC roots, visited and rewritten + /// by the scanners below. + forward: PtrHashMap, + /// Exact inverse of `forward` while `reverse_index_abandoned` is false. + /// Never consulted for anything `forward` does not already say. + reverse: PtrHashMap, + /// Sticky: set when [`Self::insert`] sees something that would make + /// `reverse` less than an exact inverse (a second class id claiming an + /// address, or a class id re-pointed at a different object). From then on + /// the reverse lookup falls back to the linear scan, which is always + /// right. Never observed in practice — see the module docs. + reverse_index_abandoned: bool, +} + +impl DeclPrototypeTable { + /// Register `class_id`'s prototype object. + pub(crate) fn insert(&mut self, class_id: u32, ptr: usize) { + // Both halves of the injectivity precondition, checked in O(1) + // BEFORE the forward map changes. + let address_already_owned = + matches!(self.reverse.get(&ptr), Some(&owner) if owner != class_id); + let previous = self.forward.insert(class_id, ptr); + let class_was_re_pointed = previous.is_some_and(|previous| previous != ptr); + + if address_already_owned || class_was_re_pointed { + self.abandon_reverse_index(); + return; + } + if !self.reverse_index_abandoned { + self.reverse.insert(ptr, class_id); + } + self.debug_assert_consistent(); + } + + /// Forward lookup: the prototype object registered for `class_id`. + #[inline] + pub(crate) fn get(&self, class_id: u32) -> Option { + self.forward.get(&class_id).copied() + } + + /// Reverse lookup: the declared class whose `.prototype` is `ptr`. + /// O(1) — this is the whole point of the type. + #[inline] + pub(crate) fn class_id_for(&self, ptr: usize) -> Option { + if self.reverse_index_abandoned { + return self.scan_class_id_for(ptr); + } + let answer = self.reverse.get(&ptr).copied(); + debug_assert_eq!( + answer, + self.scan_class_id_for(ptr), + "decl-prototype reverse index disagrees with the forward table" + ); + answer + } + + /// Every registered class id. Read-only by construction (`u32` copies), + /// so a caller cannot reach the addresses through it. + pub(crate) fn class_ids(&self) -> impl Iterator + '_ { + self.forward.keys().copied() + } + + /// Drop `class_id`'s registration. Test-support only today; kept next to + /// `insert` so the pair is read together. + #[cfg(test)] + pub(crate) fn remove(&mut self, class_id: u32) { + if let Some(previous) = self.forward.remove(&class_id) { + if self.reverse.get(&previous) == Some(&class_id) { + self.reverse.remove(&previous); + } + } + self.debug_assert_consistent(); + } + + /// Hand every root slot to a GC visitor, then re-key `reverse` for any + /// address the visitor rewrote (evacuation). + pub(crate) fn visit_root_slots(&mut self, mut visit: impl FnMut(&mut usize)) { + let mut moved = false; + for slot in self.forward.values_mut() { + let before = *slot; + visit(slot); + moved |= *slot != before; + } + if moved && !self.reverse_index_abandoned { + self.rebuild_reverse(); + } + self.debug_assert_consistent(); + } + + /// Single-slot twin of [`Self::visit_root_slots`], for the step-wise + /// (cycle-based) root machine that visits one recorded class id at a time. + pub(crate) fn visit_root_slot_for(&mut self, class_id: u32, mut visit: impl FnMut(&mut usize)) { + let Some(slot) = self.forward.get_mut(&class_id) else { + return; + }; + let before = *slot; + visit(slot); + let after = *slot; + if after != before && !self.reverse_index_abandoned { + // Injective `forward` (see the module docs) means `before` was + // this class id's address and nobody else's, so the retarget is + // exact; if it were not, `reverse_index_abandoned` would already + // be set and this arm unreachable. + if self.reverse.get(&before) == Some(&class_id) { + self.reverse.remove(&before); + } + self.reverse.insert(after, class_id); + } + self.debug_assert_consistent(); + } + + /// Give up the O(1) index for the life of the table and answer every + /// future reverse lookup with the linear scan instead. + fn abandon_reverse_index(&mut self) { + self.reverse_index_abandoned = true; + self.reverse.clear(); + self.reverse.shrink_to_fit(); + } + + fn rebuild_reverse(&mut self) { + self.reverse.clear(); + for (&class_id, &ptr) in self.forward.iter() { + self.reverse.insert(ptr, class_id); + } + } + + /// Ground truth: what the pre-#9180 linear scan answered, and what this + /// table still answers once the index is abandoned. Also the oracle the + /// `debug_assert_eq!` in [`Self::class_id_for`] checks the index against. + /// The assertion is compiled out of release builds, so it costs nothing + /// there. + fn scan_class_id_for(&self, ptr: usize) -> Option { + self.forward + .iter() + .find(|(_, &candidate)| candidate == ptr) + .map(|(&class_id, _)| class_id) + } + + #[cfg(debug_assertions)] + fn debug_assert_consistent(&self) { + if self.reverse_index_abandoned { + debug_assert!( + self.reverse.is_empty(), + "abandoned decl-prototype index must not be consulted" + ); + return; + } + debug_assert_eq!( + self.forward.len(), + self.reverse.len(), + "decl-prototype reverse index lost or gained an entry" + ); + for (&class_id, &ptr) in self.forward.iter() { + debug_assert_eq!( + self.reverse.get(&ptr).copied(), + Some(class_id), + "decl-prototype reverse index is missing {ptr:#x} → {class_id}" + ); + } + } + + #[cfg(not(debug_assertions))] + #[inline(always)] + fn debug_assert_consistent(&self) {} + + #[cfg(test)] + fn is_abandoned(&self) -> bool { + self.reverse_index_abandoned + } +} + +#[cfg(test)] +mod tests { + use super::DeclPrototypeTable; + + #[test] + fn reverse_answers_what_the_linear_scan_answered() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(9, 0x2000); + assert!(!table.is_abandoned()); + assert_eq!(table.class_id_for(0x1000), Some(7)); + assert_eq!(table.class_id_for(0x2000), Some(9)); + assert_eq!(table.class_id_for(0x3000), None); + assert_eq!(table.class_id_for(0), None); + assert_eq!(table.get(7), Some(0x1000)); + assert_eq!(table.get(11), None); + } + + #[test] + fn re_registering_the_same_address_is_not_a_retarget() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(7, 0x1000); + assert!(!table.is_abandoned()); + assert_eq!(table.class_id_for(0x1000), Some(7)); + } + + #[test] + fn removing_a_class_retires_both_directions() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(9, 0x2000); + table.remove(7); + assert_eq!(table.class_id_for(0x1000), None); + assert_eq!(table.get(7), None); + assert_eq!(table.class_id_for(0x2000), Some(9)); + } + + /// The evacuation case the naive pointer-keyed cache got wrong. + #[test] + fn evacuation_rekeys_the_reverse_index() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(9, 0x2000); + table.visit_root_slots(|slot| *slot += 0x10_0000); + assert_eq!(table.class_id_for(0x1000), None); + assert_eq!(table.class_id_for(0x2000), None); + assert_eq!(table.class_id_for(0x10_1000), Some(7)); + assert_eq!(table.class_id_for(0x10_2000), Some(9)); + assert_eq!(table.get(7), Some(0x10_1000)); + } + + #[test] + fn stepwise_evacuation_rekeys_only_the_visited_class() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(9, 0x2000); + table.visit_root_slot_for(7, |slot| *slot = 0x9000); + assert_eq!(table.class_id_for(0x1000), None); + assert_eq!(table.class_id_for(0x9000), Some(7)); + assert_eq!(table.class_id_for(0x2000), Some(9)); + table.visit_root_slot_for(1234, |_| unreachable!("unregistered class visited")); + } + + #[test] + fn a_visit_that_moves_nothing_leaves_the_index_alone() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.visit_root_slots(|_| {}); + table.visit_root_slot_for(7, |_| {}); + assert_eq!(table.class_id_for(0x1000), Some(7)); + let mut ids: Vec<_> = table.class_ids().collect(); + ids.sort_unstable(); + assert_eq!(ids, vec![7]); + } + + /// Two class ids on one address would make a targeted reverse update + /// drop the other one's key. The table gives the index up instead and + /// answers exactly as the pre-#9180 linear scan did — including through + /// a later evacuation, where the abandoned index must stay out of the way. + #[test] + fn a_shared_prototype_address_falls_back_to_the_scan() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(9, 0x1000); + assert!(table.is_abandoned()); + assert!(matches!(table.class_id_for(0x1000), Some(7) | Some(9))); + assert_eq!(table.class_id_for(0x2000), None); + assert_eq!(table.get(7), Some(0x1000)); + assert_eq!(table.get(9), Some(0x1000)); + + table.insert(11, 0x3000); + assert_eq!(table.class_id_for(0x3000), Some(11)); + table.visit_root_slots(|slot| *slot += 0x10_0000); + assert_eq!(table.class_id_for(0x3000), None); + assert_eq!(table.class_id_for(0x10_3000), Some(11)); + assert!(matches!(table.class_id_for(0x10_1000), Some(7) | Some(9))); + table.visit_root_slot_for(11, |slot| *slot = 0x4000); + assert_eq!(table.class_id_for(0x4000), Some(11)); + assert_eq!(table.class_id_for(0x10_3000), None); + } + + /// Re-pointing one class at a different object is the other half of the + /// precondition; it degrades the same way rather than silently stranding + /// the old address in `reverse`. + #[test] + fn re_pointing_a_class_falls_back_to_the_scan() { + let mut table = DeclPrototypeTable::default(); + table.insert(7, 0x1000); + table.insert(7, 0x4000); + assert!(table.is_abandoned()); + assert_eq!(table.class_id_for(0x1000), None); + assert_eq!(table.class_id_for(0x4000), Some(7)); + assert_eq!(table.get(7), Some(0x4000)); + } +} diff --git a/crates/perry-runtime/src/object/class_registry/gc_roots.rs b/crates/perry-runtime/src/object/class_registry/gc_roots.rs index 4a4bea8362..b60b7412dd 100644 --- a/crates/perry-runtime/src/object/class_registry/gc_roots.rs +++ b/crates/perry-runtime/src/object/class_registry/gc_roots.rs @@ -119,9 +119,9 @@ pub fn scan_class_side_table_roots_mut(visitor: &mut crate::gc::RuntimeRootVisit CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Ok(mut guard) = table.write() { if let Some(map) = guard.as_mut() { - for proto_addr in map.values_mut() { + map.visit_root_slots(|proto_addr| { visitor.visit_usize_slot(proto_addr); - } + }); } } }); @@ -264,7 +264,7 @@ fn class_side_table_root_snapshot() -> Vec { CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Ok(guard) = table.read() { if let Some(map) = guard.as_ref() { - for &class_id in map.keys() { + for class_id in map.class_ids() { slots.push(ClassSideTableRootSlot::DeclPrototypeObject { class_id }); } } @@ -397,8 +397,10 @@ fn scan_class_side_table_root_slot( ClassSideTableRootSlot::DeclPrototypeObject { class_id } => { CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Ok(mut guard) = table.write() { - if let Some(proto_addr) = guard.as_mut().and_then(|map| map.get_mut(class_id)) { - visitor.visit_usize_slot(proto_addr); + if let Some(map) = guard.as_mut() { + map.visit_root_slot_for(*class_id, |proto_addr| { + visitor.visit_usize_slot(proto_addr); + }); } } }); @@ -749,7 +751,7 @@ pub(crate) fn test_class_decl_prototype_object_root_addr(class_id: u32) -> usize table .read() .ok() - .and_then(|guard| guard.as_ref().and_then(|map| map.get(&class_id).copied())) + .and_then(|guard| guard.as_ref().and_then(|map| map.get(class_id))) .unwrap_or(0) }) } diff --git a/crates/perry-runtime/src/object/class_registry/state.rs b/crates/perry-runtime/src/object/class_registry/state.rs index 7638aa0bcd..01cfd58616 100644 --- a/crates/perry-runtime/src/object/class_registry/state.rs +++ b/crates/perry-runtime/src/object/class_registry/state.rs @@ -1,3 +1,4 @@ +use super::decl_prototype_table::DeclPrototypeTable; use super::*; use crate::object::class_image::{ImageTable, StaticAccessorTable, StaticMethodTable}; use std::collections::HashMap; @@ -326,7 +327,7 @@ crate::perry_thread_local! { /// inheritance shortcuts. Declared class prototypes need stable heap identity /// for `typeof C.prototype`, `Object.getPrototypeOf(new C())`, and /// `C.prototype.isPrototypeOf(instance)` without perturbing those paths. - pub static CLASS_DECL_PROTOTYPE_OBJECTS: RwLock>> = RwLock::new(None); + pub static CLASS_DECL_PROTOTYPE_OBJECTS: RwLock> = RwLock::new(None); } crate::perry_thread_local! { @@ -475,10 +476,9 @@ pub(crate) fn class_decl_prototype_object_root_store(class_id: u32, proto_ptr: * } CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { let mut guard = table.write().unwrap(); - if guard.is_none() { - *guard = Some(HashMap::new()); - } - guard.as_mut().unwrap().insert(class_id, proto_ptr as usize); + guard + .get_or_insert_with(DeclPrototypeTable::default) + .insert(class_id, proto_ptr as usize); }); crate::gc::runtime_write_barrier_root_raw_ptr(proto_ptr); } @@ -531,22 +531,23 @@ pub(crate) fn parent_closure_in_chain(class_id: u32) -> Option { /// Reverse lookup: which declared class's `.prototype` is this heap object? /// Used by `Object.getOwnPropertyDescriptor(C.prototype, name)` to surface -/// vtable accessors as own properties of the prototype object. Linear scan — -/// the table is small (one entry per materialized declared-class prototype) -/// and this only runs on the reflection slow path. +/// vtable accessors as own properties of the prototype object, and by +/// `descriptor_state::disable_inline_guards_for_descriptor_target` on every +/// `Object.defineProperty`. +/// +/// #9180: this was a linear scan over every materialized declared-class +/// prototype, on the strength of a "the table is small and this is a cold +/// reflection path" comment that a bundled application falsifies twice over +/// — it was 3.10% of `cc --help`. Callers ask about arbitrary objects, so the +/// common case is a MISS, and a miss walked the whole table. +/// [`DeclPrototypeTable`] carries the inverse of the map next to it and keeps +/// the two in step by construction; see that module for why the invalidation +/// is structural rather than enumerated. pub(crate) fn class_id_for_decl_prototype_object(ptr: usize) -> Option { if ptr == 0 { return None; } - CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { - table - .read() - .ok()? - .as_ref()? - .iter() - .find(|(_, &p)| p == ptr) - .map(|(k, _)| *k) - }) + CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| table.read().ok()?.as_ref()?.class_id_for(ptr)) } /// #7757: a monomorphized specialization (`Gen$num`) must present the GENERIC's @@ -572,7 +573,7 @@ pub(crate) fn class_decl_prototype_object(class_id: u32) -> *mut ObjectHeader { CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { if let Ok(read) = table.read() { if let Some(map) = read.as_ref() { - return map.get(&class_id).copied().unwrap_or(0) as *mut ObjectHeader; + return map.get(class_id).unwrap_or(0) as *mut ObjectHeader; } } std::ptr::null_mut() diff --git a/crates/perry-runtime/src/proxy/metadata.rs b/crates/perry-runtime/src/proxy/metadata.rs index 4e1d58b900..3f2182416d 100644 --- a/crates/perry-runtime/src/proxy/metadata.rs +++ b/crates/perry-runtime/src/proxy/metadata.rs @@ -319,9 +319,10 @@ mod tests { register_test_class(cid); let fake_proto_ptr: usize = 0x1_0000; // arbitrary; only used as a map key crate::object::CLASS_DECL_PROTOTYPE_OBJECTS.with(|table| { - let mut guard = table.write().unwrap(); - guard - .get_or_insert_with(std::collections::HashMap::new) + table + .write() + .unwrap() + .get_or_insert_with(Default::default) .insert(cid, fake_proto_ptr); }); let target = f64::from_bits(POINTER_TAG | (fake_proto_ptr as u64 & POINTER_MASK)); diff --git a/crates/perry/tests/issue_9180_decl_prototype_reverse_lookup.rs b/crates/perry/tests/issue_9180_decl_prototype_reverse_lookup.rs new file mode 100644 index 0000000000..cb35ad1cad --- /dev/null +++ b/crates/perry/tests/issue_9180_decl_prototype_reverse_lookup.rs @@ -0,0 +1,242 @@ +//! Regression test for #9180: the reverse "which declared class's `.prototype` +//! is this heap object?" lookup. +//! +//! The lookup used to be a linear scan of `CLASS_DECL_PROTOTYPE_OBJECTS`; it is +//! now answered from an index maintained alongside that table. The failure mode +//! of getting an index like this wrong is SILENT — it does not crash, it reports +//! "not a prototype", and the class's vtable accessors stop being surfaced as +//! own properties of `C.prototype`. A first attempt at a pointer-keyed cache +//! shipped exactly that: `Object.getOwnPropertyDescriptor(C.prototype, "g")` +//! returned `undefined` where node returns a getter/setter descriptor. +//! +//! So this pins the observable surface rather than the data structure: +//! descriptor shapes for accessor and data properties, `hasOwnProperty`, +//! prototype identity, and `delete` — each of which routes through the reverse +//! lookup — with enough sibling classes materialized that a size-one table +//! cannot make a broken index look right, and repeated across allocation churn +//! because the collector rewrites the very addresses the index is keyed by. + +use std::path::PathBuf; +use std::process::Command; +use std::sync::Once; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn workspace_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../..") + .canonicalize() + .expect("canonicalize workspace root") +} + +fn runtime_dir() -> PathBuf { + static BUILD_RUNTIME: Once = Once::new(); + BUILD_RUNTIME.call_once(|| { + let cargo = std::env::var_os("CARGO").unwrap_or_else(|| "cargo".into()); + let mut command = Command::new(cargo); + command.current_dir(workspace_root()).arg("build"); + if !cfg!(debug_assertions) { + command.arg("--release"); + } + let build = command + .args(["-p", "perry-runtime-static"]) + .output() + .expect("build static runtime archive"); + assert!( + build.status.success(), + "static runtime build failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&build.stdout), + String::from_utf8_lossy(&build.stderr) + ); + }); + + perry_bin() + .parent() + .expect("Perry binary directory") + .to_path_buf() +} + +fn compile_and_run(dir: &std::path::Path, source: &str) -> String { + let entry = dir.join("main.ts"); + let output = dir.join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-auto-optimize") + .arg("--no-cache") + .env("PERRY_RUNTIME_DIR", runtime_dir()) + .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) + .current_dir(dir) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +/// Sibling classes whose prototypes are materialized before the assertions, so +/// the table under test holds more than the one entry being asked about. +const FILLERS: &str = r#" +class F0 { f0() { return 0; } get g0() { return 0; } } +class F1 { f1() { return 1; } get g1() { return 1; } } +class F2 { f2() { return 2; } get g2() { return 2; } } +class F3 { f3() { return 3; } get g3() { return 3; } } +class F4 { f4() { return 4; } get g4() { return 4; } } +class F5 { f5() { return 5; } get g5() { return 5; } } +const FILL: any[] = [ + F0.prototype, F1.prototype, F2.prototype, + F3.prototype, F4.prototype, F5.prototype, +]; +function churn(n: number): number { + let sink = 0; + for (let i = 0; i < n; i++) { + const a: any[] = []; + for (let j = 0; j < 40; j++) a.push({ i, j, s: "x" + j }); + sink += a.length; + } + return sink; +} +"#; + +#[test] +fn class_prototype_accessors_reflect_as_own_descriptors() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + &format!( + r#"{FILLERS} +class Base {{ bm() {{ return "bm"; }} }} +class C extends Base {{ + m() {{ return "m"; }} + get g() {{ return "getter-g"; }} + set g(v: string) {{ (this as any)._g = v; }} +}} + +function report(tag: string) {{ + const acc: any = Object.getOwnPropertyDescriptor(C.prototype, "g"); + console.log(tag + ".accessor.present=" + (acc !== undefined)); + console.log(tag + ".accessor.hasGet=" + (acc !== undefined && typeof acc.get === "function")); + console.log(tag + ".accessor.hasSet=" + (acc !== undefined && typeof acc.set === "function")); + console.log(tag + ".accessor.get()=" + (acc !== undefined && acc.get ? acc.get.call({{}}) : "MISSING")); + console.log(tag + ".accessor.hasValue=" + (acc !== undefined && "value" in acc)); + const data: any = Object.getOwnPropertyDescriptor(C.prototype, "m"); + console.log(tag + ".data.present=" + (data !== undefined)); + console.log(tag + ".data.value()=" + (data !== undefined && data.value ? data.value.call({{}}) : "MISSING")); + console.log(tag + ".hasOwn.g=" + Object.prototype.hasOwnProperty.call(C.prototype, "g")); + console.log(tag + ".identity=" + (Object.getPrototypeOf(new C()) === C.prototype)); + console.log(tag + ".isProtoOf=" + C.prototype.isPrototypeOf(new C())); + // A miss must stay a miss: a plain object is not any class's prototype. + console.log(tag + ".plainMiss=" + (Object.getOwnPropertyDescriptor({{ a: 1 }}, "g") === undefined)); +}} + +console.log("fillers=" + FILL.length); +report("fresh"); +churn(2000); +report("afterChurn"); + +// A class whose prototype is first demanded AFTER the churn — the table grows +// again once addresses have already moved. +class Late {{ lm() {{ return "lm"; }} get lg() {{ return "lg"; }} }} +const late: any = Object.getOwnPropertyDescriptor(Late.prototype, "lg"); +console.log("late.hasGet=" + (late !== undefined && typeof late.get === "function")); +churn(1000); +const late2: any = Object.getOwnPropertyDescriptor(Late.prototype, "lg"); +console.log("late.afterChurn.hasGet=" + (late2 !== undefined && typeof late2.get === "function")); +"# + ), + ); + + assert!(stdout.contains("fillers=6"), "stdout:\n{stdout}"); + for tag in ["fresh", "afterChurn"] { + for (line, why) in [ + ( + format!("{tag}.accessor.present=true"), + "accessor descriptor", + ), + (format!("{tag}.accessor.hasGet=true"), "getter"), + (format!("{tag}.accessor.hasSet=true"), "setter"), + (format!("{tag}.accessor.get()=getter-g"), "getter call"), + ( + format!("{tag}.accessor.hasValue=false"), + "accessor is not a data descriptor", + ), + (format!("{tag}.data.present=true"), "method descriptor"), + (format!("{tag}.data.value()=m"), "method call"), + (format!("{tag}.hasOwn.g=true"), "hasOwnProperty"), + (format!("{tag}.identity=true"), "prototype identity"), + (format!("{tag}.isProtoOf=true"), "isPrototypeOf"), + (format!("{tag}.plainMiss=true"), "non-prototype receiver"), + ] { + assert!( + stdout.contains(&line), + "missing {why} ({line})\nstdout:\n{stdout}" + ); + } + } + assert!(stdout.contains("late.hasGet=true"), "stdout:\n{stdout}"); + assert!( + stdout.contains("late.afterChurn.hasGet=true"), + "stdout:\n{stdout}" + ); +} + +#[test] +fn deleting_a_prototype_method_removes_it_from_instances() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + &format!( + r#"{FILLERS} +class E {{ em() {{ return "em"; }} get eg() {{ return "eg"; }} }} +const proto: any = E.prototype; +console.log("fillers=" + FILL.length); +console.log("before=" + (typeof new E().em)); +console.log("delete.em=" + (delete proto.em)); +console.log("descAfter=" + (Object.getOwnPropertyDescriptor(proto, "em") === undefined)); +try {{ + (new E() as any).em(); + console.log("call=NO_THROW"); +}} catch (e: any) {{ + console.log("call=throw:" + (e && e.constructor ? e.constructor.name : "?")); +}} +console.log("delete.eg=" + (delete proto.eg)); +console.log("descEgAfter=" + (Object.getOwnPropertyDescriptor(proto, "eg") === undefined)); +churn(1500); +console.log("descAfterChurn=" + (Object.getOwnPropertyDescriptor(proto, "em") === undefined)); +"# + ), + ); + + for line in [ + "before=function", + "delete.em=true", + "descAfter=true", + "call=throw:TypeError", + "delete.eg=true", + "descEgAfter=true", + "descAfterChurn=true", + ] { + assert!(stdout.contains(line), "missing {line}\nstdout:\n{stdout}"); + } +}