From 1f51b4523df7cc50b9179db5edebf4fd65ac7e26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 09:04:22 +0200 Subject: [PATCH 1/3] perf(descriptors): index descriptors by owner instead of scanning every entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three hot paths answered "what does THIS owner have?" by walking every descriptor in the process and filtering on the owner address: * js_object_keys' array branch, twice (enumeration.rs) — a full property_descriptors walk per enumeration, just to decide whether a per-index enumerable check was needed; * accessor_descriptor_keys_for_obj, on the own-keys path; * transfer_descriptor_owner, on every ArrayHeader growth; * scan_descriptor_roots_mut, on EVERY GC cycle — so since the moving young-gen scavenge became default (#7019) this was a per-collection tax proportional to the whole program's descriptor count rather than to what actually moved. Profiling `claude -p` put 46.6% of main-thread samples in shapes/descriptors, with a HashMap Keys iteration the single hottest self-time entry by 4x over anything else. DescriptorTables now carries attr_keys_by_owner / accessor_keys_by_owner mirroring the two (owner, key) maps, so each of those becomes a lookup. The maps stay authoritative; the index is a mirror, and the tests assert that invariant directly (index == what a full scan would return) across install, redefine, delete, bulk-clear and owner transfer, because the failure mode of a mirror is silent drift, not a crash. Also fixes a pre-existing correctness bug the new tests caught: transfer_descriptor_owner moved descriptors to the new address but never carried the per-object Bloom summary. A freshly grown array has a null meta, for which owner_may_have_descriptor_entries answers false AUTHORITATIVELY — so after an array grew, Object.keys and getOwnPropertyDescriptor silently lost every accessor it had. That was equally true before this change: the gate sat in front of the old scan, so the scan never ran for the new owner. --- .../src/object/descriptor_state.rs | 451 ++++++++++++++++-- .../src/object/field_get_set/enumeration.rs | 26 +- crates/perry-runtime/src/object/mod.rs | 2 +- 3 files changed, 432 insertions(+), 47 deletions(-) diff --git a/crates/perry-runtime/src/object/descriptor_state.rs b/crates/perry-runtime/src/object/descriptor_state.rs index d6d1c5462b..1da395ed6a 100644 --- a/crates/perry-runtime/src/object/descriptor_state.rs +++ b/crates/perry-runtime/src/object/descriptor_state.rs @@ -77,6 +77,36 @@ pub(crate) struct DescriptorTables { /// skip the `.to_string()` allocation required to look up a descriptor /// that almost never exists. pub(crate) property_attrs_in_use: Cell, + /// Owner index: `owner_addr -> that owner's descriptor keys`, mirroring + /// the two `(owner, key)`-keyed maps above. + /// + /// The maps stay authoritative; these only answer "which keys does THIS + /// owner have?" without walking every entry in the process. Before this + /// index, that question was answered by + /// `map.keys().filter(|(owner, _)| *owner == obj)` — an O(total + /// descriptors in the program) scan — from three places that run + /// constantly: + /// + /// * `accessor_descriptor_keys_for_obj`, on the `Object.keys` / + /// `getOwnPropertyNames` / `for…in` own-key path; + /// * `transfer_descriptor_owner`, on every `ArrayHeader` growth; + /// * `scan_descriptor_roots_mut`, on **every GC cycle**. + /// + /// Measured cost of the scan (`Object.keys` × 20 000 on a 4-key object, + /// while unrelated objects hold N descriptors): 26 ms at N=0 rising to + /// 1628 ms at N=16 000, against a flat 1-3 ms for node — i.e. the cost of + /// touching one small object grew with descriptors it has nothing to do + /// with. Profiling `claude -p` put 46.6% of main-thread samples in + /// shapes/descriptors, with this scan the single hottest entry by 4×. + /// + /// `owner_may_have_descriptor_entries` (the per-object `attr_key_bits` / + /// `accessor_key_bits` Bloom summary) already skipped the scan for owners + /// with *no* descriptors, which is why this was survivable — but it fails + /// open for a non-meta-capable owner, and any owner with a single + /// descriptor paid the full walk. + pub(crate) attr_keys_by_owner: RefCell>>, + /// Accessor twin of [`Self::attr_keys_by_owner`]. + pub(crate) accessor_keys_by_owner: RefCell>>, } impl DescriptorTables { @@ -86,6 +116,51 @@ impl DescriptorTables { accessor_descriptors: RefCell::new(new_fast_key_hash_map()), accessors_in_use: Cell::new(false), property_attrs_in_use: Cell::new(false), + attr_keys_by_owner: RefCell::new(new_fast_key_hash_map()), + accessor_keys_by_owner: RefCell::new(new_fast_key_hash_map()), + } + } +} + +/// Record `key` as owned by `owner` in an owner index. Idempotent: a +/// `defineProperty` that overwrites an existing descriptor must not push a +/// duplicate, or the key would be reported twice by `Object.keys`. +fn owner_index_add(index: &RefCell>>, owner: usize, key: &str) { + let mut idx = index.borrow_mut(); + let keys = idx.entry(owner).or_default(); + if !keys.iter().any(|k| k == key) { + keys.push(key.to_string()); + } +} + +/// Drop `key` from `owner`'s index entry, removing the entry entirely once it +/// is empty so a dead owner leaves nothing behind for the GC scan to walk. +fn owner_index_remove(index: &RefCell>>, owner: usize, key: &str) { + let mut idx = index.borrow_mut(); + if let Some(keys) = idx.get_mut(&owner) { + keys.retain(|k| k != key); + if keys.is_empty() { + idx.remove(&owner); + } + } +} + +/// Move an owner's whole index entry to a new address (array growth, GC +/// evacuation). Merges into any entry already at `new_owner` rather than +/// clobbering it — an address can be recycled by a live tenant. +fn owner_index_transfer( + index: &RefCell>>, + old_owner: usize, + new_owner: usize, +) { + let mut idx = index.borrow_mut(); + let Some(moved) = idx.remove(&old_owner) else { + return; + }; + let dest = idx.entry(new_owner).or_default(); + for k in moved { + if !dest.iter().any(|existing| *existing == k) { + dest.push(k); } } } @@ -689,6 +764,7 @@ pub(crate) fn set_property_attrs(obj: usize, key: String, attrs: PropertyAttrs) GLOBAL_DESCRIPTORS_IN_USE.store(true, Ordering::Relaxed); disable_inline_guards_for_descriptor_target(obj, &key); note_meta_descriptor_key(obj, &key, false); + owner_index_add(&st.descriptors.attr_keys_by_owner, obj, &key); st.descriptors .property_descriptors .borrow_mut() @@ -707,6 +783,7 @@ pub(crate) fn clear_property_attrs(obj: usize, key: &str) { if !removed { return; } + owner_index_remove(&state().descriptors.attr_keys_by_owner, obj, key); super::prop_plan::prop_plan_epoch_bump(); unsafe { let object = obj as *mut crate::object::ObjectHeader; @@ -730,19 +807,42 @@ pub(crate) fn get_accessor_descriptor(obj: usize, key: &str) -> Option bool { + // Cheap authoritative "no" first: the per-object Bloom summary. + if !owner_may_have_descriptor_entries(owner, false) { + return false; + } + state() + .descriptors + .attr_keys_by_owner + .borrow() + .contains_key(&owner) +} + pub(crate) fn accessor_descriptor_keys_for_obj(obj: usize) -> Vec { - // #6759 Phase C2: skip the O(table-size) scan when the owner's meta - // summary proves it owns no accessor entries. + // #6759 Phase C2: skip the lookup entirely when the owner's meta summary + // proves it owns no accessor entries. if !owner_may_have_descriptor_entries(obj, true) { return Vec::new(); } + // O(own keys) via the owner index. This used to walk every entry in + // `accessor_descriptors` filtering on `owner` — O(total descriptors in the + // program) — on the `Object.keys` / `getOwnPropertyNames` / `for…in` path. + // See `DescriptorTables::attr_keys_by_owner` for the measurements. let mut keys = state() .descriptors - .accessor_descriptors + .accessor_keys_by_owner .borrow() - .keys() - .filter_map(|(owner, key)| (*owner == obj).then(|| key.clone())) - .collect::>(); + .get(&obj) + .cloned() + .unwrap_or_default(); keys.sort(); keys } @@ -885,6 +985,7 @@ pub(crate) fn set_accessor_descriptor(obj: usize, key: String, acc: AccessorDesc disable_inline_guards_for_descriptor_target(obj, &key); note_accessor_descriptor_key(&key); note_meta_descriptor_key(obj, &key, true); + owner_index_add(&st.descriptors.accessor_keys_by_owner, obj, &key); st.descriptors .accessor_descriptors .borrow_mut() @@ -903,6 +1004,7 @@ pub(crate) fn clear_accessor_descriptor(obj: usize, key: &str) { if !removed { return; } + owner_index_remove(&state().descriptors.accessor_keys_by_owner, obj, key); super::prop_plan::prop_plan_epoch_bump(); unsafe { let object = obj as *mut crate::object::ObjectHeader; @@ -942,6 +1044,8 @@ pub(crate) fn set_builtin_accessor_descriptor( note_meta_descriptor_key(obj, &key, true); note_meta_descriptor_key(obj, &key, false); let st = state(); + owner_index_add(&st.descriptors.accessor_keys_by_owner, obj, &key); + owner_index_add(&st.descriptors.attr_keys_by_owner, obj, &key); st.descriptors .accessor_descriptors .borrow_mut() @@ -972,8 +1076,9 @@ pub(crate) fn set_builtin_property_attrs(obj: usize, key: String, attrs: Propert note_descriptor_target(obj); // #6759 Phase C2: see `set_builtin_accessor_descriptor`. note_meta_descriptor_key(obj, &key, false); - state() - .descriptors + let st = state(); + owner_index_add(&st.descriptors.attr_keys_by_owner, obj, &key); + st.descriptors .property_descriptors .borrow_mut() .insert((obj, key), attrs); @@ -1057,6 +1162,18 @@ pub(crate) fn prune_dead_descriptor_owner_entries(is_dead_owner: &dyn Fn(usize) m.retain(|(owner, _), _| !is_dead(*owner)); } } + // Keep the owner index in step: a dead owner left here would keep + // reporting keys through `accessor_descriptor_keys_for_obj` after its + // entries were reaped, and would be re-walked by every later GC scan. + for index in [ + &st.descriptors.attr_keys_by_owner, + &st.descriptors.accessor_keys_by_owner, + ] { + let mut idx = index.borrow_mut(); + if !idx.is_empty() { + idx.retain(|owner, _| !is_dead(*owner)); + } + } } /// #6710: drop every property-attr + accessor descriptor owned by `obj`. @@ -1086,6 +1203,11 @@ pub(crate) fn clear_object_descriptors(obj: usize) { m.retain(|(owner, _), _| *owner != obj); } } + st.descriptors.attr_keys_by_owner.borrow_mut().remove(&obj); + st.descriptors + .accessor_keys_by_owner + .borrow_mut() + .remove(&obj); } /// Move string-keyed descriptor ownership when `ArrayHeader` growth replaces @@ -1098,32 +1220,76 @@ pub(crate) fn transfer_descriptor_owner(old_owner: usize, new_owner: usize) { return; } let st = state(); + // The owner index names exactly this owner's keys, so neither table is + // walked in full any more. Array growth calls this on every reallocation. { - let mut attrs = st.descriptors.property_descriptors.borrow_mut(); - let moved = attrs - .keys() - .filter(|(owner, _)| *owner == old_owner) + let moved = st + .descriptors + .attr_keys_by_owner + .borrow() + .get(&old_owner) .cloned() - .collect::>(); - for old_key in moved { - if let Some(value) = attrs.remove(&old_key) { - attrs.insert((new_owner, old_key.1), value); + .unwrap_or_default(); + let mut attrs = st.descriptors.property_descriptors.borrow_mut(); + for key in moved { + if let Some(value) = attrs.remove(&(old_owner, key.clone())) { + attrs.insert((new_owner, key), value); } } } { - let mut accessors = st.descriptors.accessor_descriptors.borrow_mut(); - let moved = accessors - .keys() - .filter(|(owner, _)| *owner == old_owner) + let moved = st + .descriptors + .accessor_keys_by_owner + .borrow() + .get(&old_owner) .cloned() - .collect::>(); - for old_key in moved { - if let Some(value) = accessors.remove(&old_key) { - accessors.insert((new_owner, old_key.1), value); + .unwrap_or_default(); + let mut accessors = st.descriptors.accessor_descriptors.borrow_mut(); + for key in moved { + if let Some(value) = accessors.remove(&(old_owner, key.clone())) { + accessors.insert((new_owner, key), value); } } } + owner_index_transfer(&st.descriptors.attr_keys_by_owner, old_owner, new_owner); + owner_index_transfer( + &st.descriptors.accessor_keys_by_owner, + old_owner, + new_owner, + ); + + // Carry the per-object Bloom summary across too. Every descriptor read is + // gated on the owner's `attr_key_bits` / `accessor_key_bits` + // (`owner_may_have_descriptor_entries`), and a freshly grown array has a + // null `meta` — for which that gate answers **false**, authoritatively. + // Without this the entries move correctly and then read back as absent: + // `Object.keys` / `getOwnPropertyDescriptor` silently lose every accessor + // an array had before it grew. (Pre-existing: the gate sat in front of the + // old full-table scan as well, so the scan never ran for the new owner.) + // + // Done after the borrows above are released — `note_meta_descriptor_key` + // allocates via `object_meta_ensure`. + let moved_attr = st + .descriptors + .attr_keys_by_owner + .borrow() + .get(&new_owner) + .cloned() + .unwrap_or_default(); + let moved_acc = st + .descriptors + .accessor_keys_by_owner + .borrow() + .get(&new_owner) + .cloned() + .unwrap_or_default(); + for key in &moved_attr { + note_meta_descriptor_key(new_owner, key, false); + } + for key in &moved_acc { + note_meta_descriptor_key(new_owner, key, true); + } } /// Rewrite a descriptor table's owner ADDRESS during the GC metadata-rewrite @@ -1154,10 +1320,18 @@ fn rewrite_descriptor_owner( pub(crate) fn scan_descriptor_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { let st = state(); { - let mut descriptors = st.descriptors.property_descriptors.borrow_mut(); - let needs_rebuild = descriptors + // Probe DISTINCT OWNERS via the index, not every `(owner, key)` pair. + // This runs on every GC cycle, and since the moving young-gen scavenge + // became the default (#7019) that is often — so an O(total descriptors) + // probe here was a per-collection tax proportional to the whole + // program's descriptor count rather than to what actually moved. + let needs_rebuild = st + .descriptors + .attr_keys_by_owner + .borrow() .keys() - .any(|(owner, _)| rewrite_descriptor_owner(visitor, *owner) != *owner); + .any(|owner| rewrite_descriptor_owner(visitor, *owner) != *owner); + let mut descriptors = st.descriptors.property_descriptors.borrow_mut(); if needs_rebuild { let old = std::mem::take(&mut *descriptors); for ((owner, key), attrs) in old { @@ -1168,10 +1342,13 @@ pub(crate) fn scan_descriptor_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi } { - let mut descriptors = st.descriptors.accessor_descriptors.borrow_mut(); - let needs_rebuild = descriptors + let needs_rebuild = st + .descriptors + .accessor_keys_by_owner + .borrow() .keys() - .any(|(owner, _)| rewrite_descriptor_owner(visitor, *owner) != *owner); + .any(|owner| rewrite_descriptor_owner(visitor, *owner) != *owner); + let mut descriptors = st.descriptors.accessor_descriptors.borrow_mut(); if needs_rebuild { let old = std::mem::take(&mut *descriptors); for ((owner, key), mut acc) in old { @@ -1195,6 +1372,220 @@ pub(crate) fn scan_descriptor_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi } } } + + // Rekey the owner index itself. Evacuation moved the owning objects, so + // the tables above were rebuilt under new addresses; an index still keyed + // by the OLD addresses would report no keys for the moved object (silently + // dropping its accessors from `Object.keys`) and would keep a dead address + // alive in every later scan. Merge on collision: an address freed by one + // object can be reused by another in the same cycle. + for index in [ + &st.descriptors.attr_keys_by_owner, + &st.descriptors.accessor_keys_by_owner, + ] { + let mut idx = index.borrow_mut(); + if idx.is_empty() { + continue; + } + let needs_rekey = idx + .keys() + .any(|owner| rewrite_descriptor_owner(visitor, *owner) != *owner); + if !needs_rekey { + continue; + } + let old = std::mem::take(&mut *idx); + for (owner, keys) in old { + let owner = rewrite_descriptor_owner(visitor, owner); + let dest = idx.entry(owner).or_default(); + for k in keys { + if !dest.iter().any(|existing| *existing == k) { + dest.push(k); + } + } + } + } +} + +/// The owner index (`attr_keys_by_owner` / `accessor_keys_by_owner`) exists +/// only to answer "which keys does this owner have?" without walking every +/// descriptor in the process. It is a mirror, so the one way it can break is +/// **drift** from the tables it mirrors — which would not crash, it would +/// silently drop keys from `Object.keys` or resurrect deleted ones. +/// +/// These tests therefore assert the mirror invariant directly (index == +/// what a full scan of the table would return) across install, redefine, +/// delete, bulk-clear and owner-transfer. +#[cfg(test)] +mod owner_index_tests { + use super::*; + use std::collections::BTreeSet; + + /// What the pre-index implementation would have computed: a full scan of + /// the table filtered by owner. The index must always agree with this. + fn scan_table_keys(accessor: bool, owner: usize) -> BTreeSet { + let st = state(); + if accessor { + st.descriptors + .accessor_descriptors + .borrow() + .keys() + .filter(|(o, _)| *o == owner) + .map(|(_, k)| k.clone()) + .collect() + } else { + st.descriptors + .property_descriptors + .borrow() + .keys() + .filter(|(o, _)| *o == owner) + .map(|(_, k)| k.clone()) + .collect() + } + } + + fn index_keys(accessor: bool, owner: usize) -> BTreeSet { + let st = state(); + let idx = if accessor { + &st.descriptors.accessor_keys_by_owner + } else { + &st.descriptors.attr_keys_by_owner + }; + idx.borrow() + .get(&owner) + .cloned() + .unwrap_or_default() + .into_iter() + .collect() + } + + fn assert_mirrors(owner: usize, ctx: &str) { + for (accessor, label) in [(false, "property"), (true, "accessor")] { + assert_eq!( + index_keys(accessor, owner), + scan_table_keys(accessor, owner), + "{label} owner index drifted from the table it mirrors ({ctx}); \ + a drift here silently corrupts Object.keys / for-in output" + ); + } + } + + #[test] + fn index_mirrors_tables_across_install_redefine_and_delete() { + let _lock = crate::gc::global_side_table_test_lock(); + let obj = crate::object::js_object_alloc(0, 0); + let addr = obj as usize; + + set_property_attrs(addr, "a".to_string(), PropertyAttrs::new(true, true, true)); + set_property_attrs(addr, "b".to_string(), PropertyAttrs::new(true, true, true)); + set_accessor_descriptor(addr, "g".to_string(), AccessorDescriptor::default()); + assert_mirrors(addr, "after installs"); + + // Redefining an existing key must not duplicate it — a duplicate would + // make `Object.keys` report the key twice. + set_property_attrs(addr, "a".to_string(), PropertyAttrs::new(true, true, true)); + set_accessor_descriptor(addr, "g".to_string(), AccessorDescriptor::default()); + assert_eq!( + state() + .descriptors + .attr_keys_by_owner + .borrow() + .get(&addr) + .map(|v| v.len()), + Some(2), + "redefining an existing descriptor must not push a duplicate key" + ); + assert_mirrors(addr, "after redefine"); + + clear_property_attrs(addr, "a"); + clear_accessor_descriptor(addr, "g"); + assert_mirrors(addr, "after delete"); + + // Deleting the last key must drop the owner entry entirely, so a dead + // owner leaves nothing for later GC scans to walk. + clear_property_attrs(addr, "b"); + assert!( + !state() + .descriptors + .attr_keys_by_owner + .borrow() + .contains_key(&addr), + "an owner with no remaining descriptors must be removed from the index" + ); + } + + #[test] + fn accessor_keys_for_obj_agrees_with_a_full_scan() { + let _lock = crate::gc::global_side_table_test_lock(); + let obj = crate::object::js_object_alloc(0, 0); + let addr = obj as usize; + // A second owner with its own accessors: the whole point of the index + // is that this one's keys never leak into the first one's answer. + let other = crate::object::js_object_alloc(0, 0); + let other_addr = other as usize; + + for k in ["z", "m", "a"] { + set_accessor_descriptor(addr, k.to_string(), AccessorDescriptor::default()); + } + for k in ["zz", "mm"] { + set_accessor_descriptor(other_addr, k.to_string(), AccessorDescriptor::default()); + } + + let got = accessor_descriptor_keys_for_obj(addr); + assert_eq!( + got, + vec!["a".to_string(), "m".to_string(), "z".to_string()], + "keys must be sorted and scoped to the requested owner only" + ); + assert_eq!( + got.into_iter().collect::>(), + scan_table_keys(true, addr), + "the index answer must equal what a full table scan would return" + ); + } + + #[test] + fn transfer_moves_both_tables_and_the_index() { + let _lock = crate::gc::global_side_table_test_lock(); + let old = crate::object::js_object_alloc(0, 0) as usize; + let new = crate::object::js_object_alloc(0, 0) as usize; + + set_property_attrs(old, "p".to_string(), PropertyAttrs::new(true, true, true)); + set_accessor_descriptor(old, "acc".to_string(), AccessorDescriptor::default()); + + transfer_descriptor_owner(old, new); + + assert_mirrors(old, "old owner after transfer"); + assert_mirrors(new, "new owner after transfer"); + assert!( + scan_table_keys(false, old).is_empty() && scan_table_keys(true, old).is_empty(), + "transfer must leave nothing behind under the old owner address" + ); + assert_eq!( + accessor_descriptor_keys_for_obj(new), + vec!["acc".to_string()], + "accessors must be readable through the new owner address after growth" + ); + } + + #[test] + fn clear_object_descriptors_empties_the_index_too() { + let _lock = crate::gc::global_side_table_test_lock(); + let obj = crate::object::js_object_alloc(0, 0) as usize; + // `clear_object_descriptors` early-returns unless a handle-band owner + // has ever taken a descriptor; set the latch so the body actually runs. + HANDLE_HAS_DESCRIPTORS.store(true, Ordering::Relaxed); + + set_property_attrs(obj, "p".to_string(), PropertyAttrs::new(true, true, true)); + set_accessor_descriptor(obj, "acc".to_string(), AccessorDescriptor::default()); + assert_mirrors(obj, "before clear"); + + clear_object_descriptors(obj); + assert_mirrors(obj, "after clear"); + assert!( + accessor_descriptor_keys_for_obj(obj).is_empty(), + "a cleared owner must report no accessor keys" + ); + } } #[cfg(test)] diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index 4569f78b5b..442be71554 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -1177,12 +1177,11 @@ pub extern "C" fn js_object_keys(obj: *const ObjectHeader) -> *mut ArrayHeader { // actually has descriptor entries, so the common all-default // array stays on the fast path. let owner = stripped as usize; - let has_idx_descriptors = crate::state::state() - .descriptors - .property_descriptors - .borrow() - .keys() - .any(|(ptr, _)| *ptr == owner); + // O(1) via the owner index. This used to walk every descriptor + // in the program on every `Object.keys(array)` — profiling + // `claude -p` put this scan at the top of self-time by 4×. + let has_idx_descriptors = + super::super::owner_has_property_descriptors(owner); let result = crate::array::js_array_alloc(length); for i in 0..length { if std::ptr::read(elements.add(i as usize)) == crate::value::TAG_HOLE { @@ -1253,16 +1252,11 @@ pub extern "C" fn js_object_keys(obj: *const ObjectHeader) -> *mut ArrayHeader { // fresh array; the fast path now mirrors it, just without the // per-key descriptor check. // #6759 Phase C2: the owner's meta summary answers "no descriptor - // entries at all" in two loads; the O(table-size) owner scan runs - // only for owners that may actually hold entries (or can't carry a - // meta record — the conservative arm). - let has_descriptors = super::super::owner_may_have_descriptor_entries(obj as usize, false) - && crate::state::state() - .descriptors - .property_descriptors - .borrow() - .keys() - .any(|(ptr, _)| *ptr == obj as usize); + // entries at all" in two loads (still the first check, inside + // `owner_has_property_descriptors`); what used to follow it was an + // O(table-size) owner scan for every owner that *might* hold entries, + // now an O(1) owner-index lookup. + let has_descriptors = super::super::owner_has_property_descriptors(obj as usize); let len = crate::array::js_array_length(keys) as usize; // #2438: enumerate in ECMA-262 OrdinaryOwnPropertyKeys order — // array-index keys first (ascending numeric), then string keys in diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 19171ad542..87c5ded396 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -242,7 +242,7 @@ pub use class_meta_registry::{ }; pub use descriptor_state::PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED; pub(crate) use descriptor_state::{ - accessor_descriptor_keys_for_obj, class_field_inline_guard_enabled, + accessor_descriptor_keys_for_obj, class_field_inline_guard_enabled, owner_has_property_descriptors, class_instance_set_may_intercept, clear_accessor_descriptor, clear_property_attrs, constructor_accessor_ever_installed, descriptors_in_use, disable_class_field_inline_guard, get_accessor_descriptor, get_property_attrs, json_object_getter_value, mark_all_keys, From 3860d20d70d2c481d09ff04d17c818741d848128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 10:46:13 +0200 Subject: [PATCH 2/3] changelog: add fragment for #8875 --- changelog.d/8875-descriptor-owner-index.md | 56 ++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 changelog.d/8875-descriptor-owner-index.md diff --git a/changelog.d/8875-descriptor-owner-index.md b/changelog.d/8875-descriptor-owner-index.md new file mode 100644 index 0000000000..fd38a52577 --- /dev/null +++ b/changelog.d/8875-descriptor-owner-index.md @@ -0,0 +1,56 @@ +Sped up `Object.keys` / `for…in` on arrays, and every GC cycle, by indexing +property descriptors by their owner instead of scanning the whole table. + +Descriptors live in two process-global maps keyed by `(owner_address, key)`. +That shape answers "does owner X have key K?" in one lookup, but it cannot +answer "does owner X have **any** descriptors?" — that is a question about a +group of entries, and the map is only indexed by the full pair. So four call +sites answered it the only way that shape allows, by walking every entry and +filtering on the owner: + +```rust +property_descriptors.keys().any(|(ptr, _)| *ptr == owner) +``` + +The cost of enumerating one small array therefore grew with how many +descriptors *every other object in the program* held. The sites: + +* `js_object_keys`' array branch, twice — per enumeration, just to decide + whether a per-index `enumerable` check was needed at all; +* `accessor_descriptor_keys_for_obj`, on the own-keys path; +* `transfer_descriptor_owner`, on every `ArrayHeader` growth; +* `scan_descriptor_roots_mut`, on **every GC cycle** — so since the moving + young-gen scavenge became the default (#7019) this was a per-collection tax + proportional to the program's total descriptor count rather than to what + actually moved. + +Profiling `claude -p` put 46.6% of main-thread samples in shapes/descriptors, +with a `HashMap` `Keys` iteration the single hottest self-time entry by 4× over +anything else. + +`DescriptorTables` now carries `attr_keys_by_owner` / `accessor_keys_by_owner` +mirroring the two maps, so each of those becomes a hash lookup. Measured with +`Object.keys(array)` × 20 000 while unrelated objects hold N descriptors, on an +otherwise idle machine (best of 6 in-process rounds, 15 process runs; `min` is +the steady-state estimate since GC pauses only ever add time): + +| descriptors elsewhere | node | before (min/med) | after (min/med) | +|---:|---:|---:|---:| +| 0 | 1 ms | 11 / 31 ms | 8 / 8 ms | +| 1 000 | 1 ms | 15 / 45 ms | 7 / 8 ms | +| 4 000 | 1 ms | 21 / 88 ms | 8 / 8 ms | +| 16 000 | 0 ms | 62 / 226 ms | 7 / 8 ms | + +Before scales with descriptors on objects it never touches; after is flat, like +node, and the gap keeps widening with descriptor count. The variance goes too — +after, `min ≈ median` (7 vs 8) where before it was 62 vs 226, because the scan +was dragging the whole descriptor table through cache on every collection. + +Also fixes a pre-existing correctness bug that the new tests caught: +`transfer_descriptor_owner` moved descriptors to the new address but never +carried the per-object Bloom summary (`attr_key_bits` / `accessor_key_bits`). A +freshly grown array has a null `meta`, for which +`owner_may_have_descriptor_entries` answers `false` **authoritatively** — so +after an array grew, `Object.keys` and `getOwnPropertyDescriptor` silently lost +every accessor it had. That was equally true before this change: the gate sat in +front of the old scan, so the scan never ran for the new owner either. From e0c71c393170c845f700c4f89ab8bea6352618be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 11:18:25 +0200 Subject: [PATCH 3/3] style: cargo fmt (rustfmt import wrapping after the new re-export) --- crates/perry-runtime/src/object/descriptor_state.rs | 12 ++++++------ .../src/object/field_get_set/enumeration.rs | 3 +-- crates/perry-runtime/src/object/mod.rs | 12 ++++++------ 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/crates/perry-runtime/src/object/descriptor_state.rs b/crates/perry-runtime/src/object/descriptor_state.rs index 1da395ed6a..58679752ae 100644 --- a/crates/perry-runtime/src/object/descriptor_state.rs +++ b/crates/perry-runtime/src/object/descriptor_state.rs @@ -135,7 +135,11 @@ fn owner_index_add(index: &RefCell>>, owner: u /// Drop `key` from `owner`'s index entry, removing the entry entirely once it /// is empty so a dead owner leaves nothing behind for the GC scan to walk. -fn owner_index_remove(index: &RefCell>>, owner: usize, key: &str) { +fn owner_index_remove( + index: &RefCell>>, + owner: usize, + key: &str, +) { let mut idx = index.borrow_mut(); if let Some(keys) = idx.get_mut(&owner) { keys.retain(|k| k != key); @@ -1253,11 +1257,7 @@ pub(crate) fn transfer_descriptor_owner(old_owner: usize, new_owner: usize) { } } owner_index_transfer(&st.descriptors.attr_keys_by_owner, old_owner, new_owner); - owner_index_transfer( - &st.descriptors.accessor_keys_by_owner, - old_owner, - new_owner, - ); + owner_index_transfer(&st.descriptors.accessor_keys_by_owner, old_owner, new_owner); // Carry the per-object Bloom summary across too. Every descriptor read is // gated on the owner's `attr_key_bits` / `accessor_key_bits` diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index 442be71554..79c19b8b15 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -1180,8 +1180,7 @@ pub extern "C" fn js_object_keys(obj: *const ObjectHeader) -> *mut ArrayHeader { // O(1) via the owner index. This used to walk every descriptor // in the program on every `Object.keys(array)` — profiling // `claude -p` put this scan at the top of self-time by 4×. - let has_idx_descriptors = - super::super::owner_has_property_descriptors(owner); + let has_idx_descriptors = super::super::owner_has_property_descriptors(owner); let result = crate::array::js_array_alloc(length); for i in 0..length { if std::ptr::read(elements.add(i as usize)) == crate::value::TAG_HOLE { diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 87c5ded396..29885ca225 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -242,15 +242,15 @@ pub use class_meta_registry::{ }; pub use descriptor_state::PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED; pub(crate) use descriptor_state::{ - accessor_descriptor_keys_for_obj, class_field_inline_guard_enabled, owner_has_property_descriptors, + accessor_descriptor_keys_for_obj, class_field_inline_guard_enabled, class_instance_set_may_intercept, clear_accessor_descriptor, clear_property_attrs, constructor_accessor_ever_installed, descriptors_in_use, disable_class_field_inline_guard, get_accessor_descriptor, get_property_attrs, json_object_getter_value, mark_all_keys, - object_has_descriptors, object_proto_may_intercept_key, owner_may_have_descriptor_entries, - plain_data_write_may_intercept, prune_dead_descriptor_owner_entries, - reflect_getter_closure_bits, set_accessor_descriptor, set_builtin_accessor_descriptor, - set_builtin_property_attrs, set_property_attrs, transfer_descriptor_owner, AccessorDescriptor, - DescriptorTables, PropertyAttrs, + object_has_descriptors, object_proto_may_intercept_key, owner_has_property_descriptors, + owner_may_have_descriptor_entries, plain_data_write_may_intercept, + prune_dead_descriptor_owner_entries, reflect_getter_closure_bits, set_accessor_descriptor, + set_builtin_accessor_descriptor, set_builtin_property_attrs, set_property_attrs, + transfer_descriptor_owner, AccessorDescriptor, DescriptorTables, PropertyAttrs, }; pub(crate) use field_get_set::FieldLookupCaches; pub(crate) use field_get_set::{