diff --git a/moli-core/src/dom/tests.rs b/moli-core/src/dom/tests.rs index 330cf3f983..dfa1c48cf2 100644 --- a/moli-core/src/dom/tests.rs +++ b/moli-core/src/dom/tests.rs @@ -1,6 +1,6 @@ use url::Url; -use crate::{parser::HtmlParser, renderer::ReflectorRegistry}; +use crate::parser::HtmlParser; use super::native::{NativeDom, NodeType}; @@ -187,19 +187,6 @@ fn native_dom_treats_noscript_contents_as_raw_text_when_scripting_is_enabled() { ); } -#[test] -fn native_dom_node_ids_can_be_interned_by_reflector_registry() { - let native_dom = parse_fixture("

ok

"); - let body_node_id = native_dom.body_node_id().expect("body should exist"); - - let mut registry = ReflectorRegistry::default(); - let first = registry.intern(body_node_id); - let second = registry.intern(body_node_id); - - assert_eq!(first.id(), second.id()); - assert_eq!(registry.len(), 1); -} - #[test] fn native_dom_exposes_node_traversal_and_equality_helpers() { let native_dom = parse_fixture( diff --git a/moli-core/src/renderer.rs b/moli-core/src/renderer.rs index 9e94698407..e96675a724 100644 --- a/moli-core/src/renderer.rs +++ b/moli-core/src/renderer.rs @@ -76,7 +76,5 @@ pub(crate) fn materialize_page_created_reply_with_side_effect( }) } -#[cfg(test)] -pub(crate) use moli_renderer_v8::ReflectorRegistry; #[cfg(test)] pub(crate) use moli_renderer_v8::{PageId, RendererPageTestingHandle, RendererPageView}; diff --git a/moli-renderer-v8/src/lib.rs b/moli-renderer-v8/src/lib.rs index 44db7e74a0..2290b9cbd7 100644 --- a/moli-renderer-v8/src/lib.rs +++ b/moli-renderer-v8/src/lib.rs @@ -82,7 +82,6 @@ mod parser_script; mod queue_microtask; mod range_boundary; mod referrer_policy; -pub(crate) mod reflector; mod render_runtime; mod renderer_resource_scheduler; mod resource_owner; @@ -214,7 +213,6 @@ pub use host::{ }; pub use local_executor::is_on_js_local_executor; pub use native_bridge::element::ClientRect as RendererClientRect; -pub use reflector::ReflectorRegistry; pub use runtime::RendererRuntimeInspectorMessageResponseOrder; pub use runtime::{ DetachedParserScriptFetchContinuation, DevToolsSessionKey, ExternalRawDocumentBodyStream, diff --git a/moli-renderer-v8/src/native_bridge/bindings.rs b/moli-renderer-v8/src/native_bridge/bindings.rs index d3b6731dcf..6ab85cba2c 100644 --- a/moli-renderer-v8/src/native_bridge/bindings.rs +++ b/moli-renderer-v8/src/native_bridge/bindings.rs @@ -6,8 +6,9 @@ use moli_webapi_declare::WebApiObject; use super::super::context_bootstrap::bridge_descriptor::{ WrapperKind, node_bridge_descriptor, node_bridge_descriptors, }; -use super::super::reflector::ReflectorId; -use super::{BridgeHandle, JsContextHost, collections, document, element, traversal, window}; +use super::{ + BridgeHandle, JsContextHost, ReflectorId, collections, document, element, traversal, window, +}; mod native_template; mod node_template; diff --git a/moli-renderer-v8/src/native_bridge/bridge/wrappers.rs b/moli-renderer-v8/src/native_bridge/bridge/wrappers.rs index 9bb06ddfd2..6de7b49d88 100644 --- a/moli-renderer-v8/src/native_bridge/bridge/wrappers.rs +++ b/moli-renderer-v8/src/native_bridge/bridge/wrappers.rs @@ -23,7 +23,7 @@ impl NativeDomBridge { ) -> Option> { let reflector_id = self .identity - .existing_reflector_id(BridgeHandle::Node(handle))?; + .existing_reflector_id(&BridgeHandle::Node(handle))?; self.identity.cached_wrapper(scope, reflector_id) } @@ -49,7 +49,7 @@ impl NativeDomBridge { host_ptr: *mut JsContextHost, handle: BridgeHandle, ) -> Option> { - let reflector_id = self.identity.reflector_id(handle.clone()); + let reflector_id = self.identity.reflector_id(&handle); if let Some(wrapper) = self.identity.cached_wrapper(scope, reflector_id) { if !matches!(&handle, BridgeHandle::Window) { self.bindings diff --git a/moli-renderer-v8/src/native_bridge/identity.rs b/moli-renderer-v8/src/native_bridge/identity.rs index db4d0a551b..aa6abb341e 100644 --- a/moli-renderer-v8/src/native_bridge/identity.rs +++ b/moli-renderer-v8/src/native_bridge/identity.rs @@ -6,12 +6,39 @@ use std::{ rc::Rc, }; -use super::super::{ - document_runtime::DomHandle, - reflector::{DomPtr, ReflectorId, ReflectorRegistry}, -}; +use indexmap::IndexSet; + +use super::super::document_runtime::DomHandle; use super::element::{control_label_handles, form_control_elements}; use super::{JsContextHost, RuntimeObservableContextToken}; +use dense_reflector_map::DenseReflectorMap; + +mod dense_reflector_map; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(super) struct ReflectorId(u64); + +impl ReflectorId { + pub(super) fn from_raw(raw: u64) -> Self { + Self(raw) + } + + fn from_index(index: usize) -> Self { + let raw = u64::try_from(index) + .ok() + .and_then(|index| index.checked_add(1)) + .expect("reflector id overflow"); + Self(raw) + } + + fn index(self) -> Option { + usize::try_from(self.0.checked_sub(1)?).ok() + } + + pub(super) fn raw(self) -> u64 { + self.0 + } +} #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub(super) enum BridgeHandle { @@ -394,105 +421,6 @@ struct BridgeContextWrapperCache { live_collection_wrappers: HashMap, } -const MAX_DENSE_REFLECTOR_ID_GAP: u64 = 64; - -#[derive(Debug)] -struct DenseReflectorMap { - dense_base: Option, - dense: Vec>, - sparse: HashMap, -} - -impl Default for DenseReflectorMap { - fn default() -> Self { - Self { - dense_base: None, - dense: Vec::new(), - sparse: HashMap::new(), - } - } -} - -impl DenseReflectorMap { - fn dense_index(&self, id: ReflectorId) -> Option { - let offset = id.raw().checked_sub(self.dense_base?)?; - let index = usize::try_from(offset).ok()?; - (index < self.dense.len()).then_some(index) - } - - fn get(&self, id: &ReflectorId) -> Option<&V> { - self.dense_index(*id) - .and_then(|index| self.dense[index].as_ref()) - .or_else(|| self.sparse.get(id)) - } - - fn insert(&mut self, id: ReflectorId, value: V) -> Option { - let raw = id.raw(); - let base = self.dense_base.get_or_insert(raw); - let dense_end = base.saturating_add(self.dense.len() as u64); - if raw >= *base && raw <= dense_end.saturating_add(MAX_DENSE_REFLECTOR_ID_GAP) { - let index = usize::try_from(raw - *base).expect("dense reflector index overflow"); - if index >= self.dense.len() { - self.dense.resize_with(index + 1, || None); - } - let sparse = self.sparse.remove(&id); - return self.dense[index].replace(value).or(sparse); - } - self.sparse.insert(id, value) - } - - fn clear(&mut self) { - self.dense_base = None; - self.dense.clear(); - self.sparse.clear(); - } - - fn retain(&mut self, mut keep: impl FnMut(ReflectorId, &mut V) -> bool) { - if let Some(base) = self.dense_base { - for (index, slot) in self.dense.iter_mut().enumerate() { - let Some(value) = slot.as_mut() else { - continue; - }; - let raw = base - .checked_add(index as u64) - .expect("dense reflector id overflow"); - if !keep(ReflectorId::from_raw(raw), value) { - *slot = None; - } - } - if let Some(first_live) = self.dense.iter().position(Option::is_some) { - if first_live != 0 { - self.dense.drain(..first_live); - self.dense_base = Some( - base.checked_add(first_live as u64) - .expect("dense reflector id overflow"), - ); - } - let retained_len = self - .dense - .iter() - .rposition(Option::is_some) - .map_or(0, |index| index + 1); - self.dense.truncate(retained_len); - } else { - self.dense.clear(); - self.dense_base = None; - } - } - self.sparse.retain(|id, value| keep(*id, value)); - } - - #[cfg(test)] - fn len(&self) -> usize { - self.dense.iter().flatten().count() + self.sparse.len() - } - - #[cfg(test)] - fn values(&self) -> impl Iterator { - self.dense.iter().flatten().chain(self.sparse.values()) - } -} - #[derive(Debug)] struct SharedDefaultWorldWrapperCache; @@ -607,7 +535,7 @@ pub(crate) fn clear_context_wrapper_cache_for_teardown( #[derive(Debug, Default)] pub(super) struct BridgeIdentityStore { - reflectors: ReflectorRegistry, + reflector_handles: IndexSet, live_collections: LiveCollectionStore, static_handle_collections: StaticHandleCollectionStore, default_world_wrapper_cache: Rc>, @@ -619,22 +547,26 @@ impl BridgeIdentityStore { let _ = context.set_slot(Rc::new(SharedDefaultWorldWrapperCache)); } - fn reflector_id_for(&mut self, handle: BridgeHandle) -> ReflectorId { - self.reflectors.root(DomPtr::new(handle)).reflector_id() + pub(super) fn reflector_id(&mut self, handle: &BridgeHandle) -> ReflectorId { + if let Some(id) = self.existing_reflector_id(handle) { + return id; + } + + let (index, inserted) = self.reflector_handles.insert_full(handle.clone()); + debug_assert!(inserted); + ReflectorId::from_index(index) } - pub(super) fn reflector_id(&mut self, handle: BridgeHandle) -> ReflectorId { - self.reflector_id_for(handle) - } - - pub(super) fn existing_reflector_id(&self, handle: BridgeHandle) -> Option { - self.reflectors - .existing(handle) - .map(|reflector| reflector.id()) + pub(super) fn existing_reflector_id(&self, handle: &BridgeHandle) -> Option { + self.reflector_handles + .get_index_of(handle) + .map(ReflectorId::from_index) } pub(super) fn bridge_handle(&self, reflector_id: ReflectorId) -> Option { - self.reflectors.key_for_id(reflector_id) + self.reflector_handles + .get_index(reflector_id.index()?) + .cloned() } pub(super) fn cached_wrapper<'s>( @@ -738,7 +670,7 @@ impl BridgeIdentityStore { mod tests { use std::mem::size_of; - use super::{BridgeCachedWrapper, BridgeHandle, DenseReflectorMap, ReflectorId}; + use super::{BridgeCachedWrapper, BridgeHandle, BridgeIdentityStore, ReflectorId}; #[test] fn bridge_handle_stays_compact_for_per_wrapper_identity_tables() { @@ -755,25 +687,15 @@ mod tests { } #[test] - fn dense_reflector_map_keeps_sequential_ids_dense_and_large_gaps_sparse() { - let mut entries = DenseReflectorMap::default(); - assert_eq!(entries.insert(ReflectorId::from_raw(50_000), 1_u32), None); - assert_eq!(entries.insert(ReflectorId::from_raw(50_001), 2_u32), None); - assert_eq!(entries.insert(ReflectorId::from_raw(100_000), 3_u32), None); + fn bridge_identity_store_reuses_and_resolves_reflector_ids() { + let mut identities = BridgeIdentityStore::default(); - assert_eq!(entries.dense_base, Some(50_000)); - assert_eq!(entries.dense.len(), 2); - assert_eq!(entries.sparse.len(), 1); - assert_eq!(entries.get(&ReflectorId::from_raw(50_001)), Some(&2)); - assert_eq!(entries.get(&ReflectorId::from_raw(100_000)), Some(&3)); - assert_eq!(entries.len(), 3); + let first = identities.reflector_id(&BridgeHandle::Window); + let second = identities.reflector_id(&BridgeHandle::Window); - entries.retain(|id, _| id.raw() != 50_000); - assert_eq!(entries.dense_base, Some(50_001)); - assert_eq!(entries.values().copied().collect::>(), vec![2, 3]); - - entries.clear(); - assert_eq!(entries.len(), 0); - assert_eq!(entries.dense_base, None); + assert_eq!(first, second); + assert_eq!(first.raw(), 1); + assert_eq!(identities.bridge_handle(first), Some(BridgeHandle::Window)); + assert_eq!(identities.bridge_handle(ReflectorId::from_raw(0)), None); } } diff --git a/moli-renderer-v8/src/native_bridge/identity/dense_reflector_map.rs b/moli-renderer-v8/src/native_bridge/identity/dense_reflector_map.rs new file mode 100644 index 0000000000..a9aaf28a94 --- /dev/null +++ b/moli-renderer-v8/src/native_bridge/identity/dense_reflector_map.rs @@ -0,0 +1,143 @@ +use std::collections::HashMap; + +use super::ReflectorId; + +/// Stores realm-local wrapper values keyed by store-wide reflector IDs. +/// +/// Reflector IDs are allocated monotonically by `BridgeIdentityStore`, while a +/// V8 context only caches the wrappers materialized in that context. Most +/// contexts therefore observe contiguous ID runs, but a late-created or +/// isolated context can start at a large ID or skip a wide range. The first +/// observed ID becomes `dense_base`, so it never creates empty slots below +/// that ID. Small later gaps stay in the dense vector; large gaps use the +/// sparse map instead of expanding the vector with mostly empty slots. +/// +/// An ID is present in at most one backing store. Dense slot `index` always +/// represents `dense_base + index`. +#[derive(Debug)] +pub(super) struct DenseReflectorMap { + dense_base: Option, + dense: Vec>, + sparse: HashMap, +} + +/// Maximum number of empty dense slots accepted to avoid a hash-map entry. +const MAX_DENSE_REFLECTOR_ID_GAP: u64 = 64; + +impl Default for DenseReflectorMap { + fn default() -> Self { + Self { + dense_base: None, + dense: Vec::new(), + sparse: HashMap::new(), + } + } +} + +impl DenseReflectorMap { + fn dense_index(&self, id: ReflectorId) -> Option { + let offset = id.raw().checked_sub(self.dense_base?)?; + let index = usize::try_from(offset).ok()?; + (index < self.dense.len()).then_some(index) + } + + pub(super) fn get(&self, id: &ReflectorId) -> Option<&V> { + self.dense_index(*id) + .and_then(|index| self.dense[index].as_ref()) + .or_else(|| self.sparse.get(id)) + } + + pub(super) fn insert(&mut self, id: ReflectorId, value: V) -> Option { + let raw = id.raw(); + let base = self.dense_base.get_or_insert(raw); + let dense_end = base.saturating_add(self.dense.len() as u64); + if raw >= *base && raw <= dense_end.saturating_add(MAX_DENSE_REFLECTOR_ID_GAP) { + let index = usize::try_from(raw - *base).expect("dense reflector index overflow"); + if index >= self.dense.len() { + self.dense.resize_with(index + 1, || None); + } + let sparse = self.sparse.remove(&id); + return self.dense[index].replace(value).or(sparse); + } + self.sparse.insert(id, value) + } + + pub(super) fn clear(&mut self) { + self.dense_base = None; + self.dense.clear(); + self.sparse.clear(); + } + + pub(super) fn retain(&mut self, mut keep: impl FnMut(ReflectorId, &mut V) -> bool) { + if let Some(base) = self.dense_base { + for (index, slot) in self.dense.iter_mut().enumerate() { + let Some(value) = slot.as_mut() else { + continue; + }; + let raw = base + .checked_add(index as u64) + .expect("dense reflector id overflow"); + if !keep(ReflectorId::from_raw(raw), value) { + *slot = None; + } + } + if let Some(first_live) = self.dense.iter().position(Option::is_some) { + if first_live != 0 { + self.dense.drain(..first_live); + self.dense_base = Some( + base.checked_add(first_live as u64) + .expect("dense reflector id overflow"), + ); + } + let retained_len = self + .dense + .iter() + .rposition(Option::is_some) + .map_or(0, |index| index + 1); + self.dense.truncate(retained_len); + } else { + self.dense.clear(); + self.dense_base = None; + } + } + self.sparse.retain(|id, value| keep(*id, value)); + } + + #[cfg(test)] + pub(super) fn len(&self) -> usize { + self.dense.iter().flatten().count() + self.sparse.len() + } + + #[cfg(test)] + pub(super) fn values(&self) -> impl Iterator { + self.dense.iter().flatten().chain(self.sparse.values()) + } +} + +#[cfg(test)] +mod tests { + use super::{DenseReflectorMap, ReflectorId}; + + #[test] + fn keeps_sequential_ids_dense_and_large_gaps_sparse() { + let mut entries = DenseReflectorMap::default(); + assert_eq!(entries.insert(ReflectorId::from_raw(50_000), 1_u32), None); + assert_eq!(entries.insert(ReflectorId::from_raw(50_001), 2_u32), None); + assert_eq!(entries.insert(ReflectorId::from_raw(100_000), 3_u32), None); + + assert_eq!(entries.dense_base, Some(50_000)); + assert_eq!(entries.dense.len(), 2); + assert_eq!(entries.sparse.len(), 1); + assert_eq!(entries.get(&ReflectorId::from_raw(50_001)), Some(&2)); + assert_eq!(entries.get(&ReflectorId::from_raw(100_000)), Some(&3)); + assert_eq!(entries.len(), 3); + + entries.retain(|id, _| id.raw() != 50_000); + assert_eq!(entries.dense_base, Some(50_001)); + assert_eq!(entries.values().copied().collect::>(), vec![2, 3]); + + entries.clear(); + assert_eq!(entries.len(), 0); + assert_eq!(entries.dense_base, None); + } +} diff --git a/moli-renderer-v8/src/native_bridge/mod.rs b/moli-renderer-v8/src/native_bridge/mod.rs index b5888fb540..a1f1173303 100644 --- a/moli-renderer-v8/src/native_bridge/mod.rs +++ b/moli-renderer-v8/src/native_bridge/mod.rs @@ -26,7 +26,6 @@ mod window; use super::{ document_runtime::DomHandle, - reflector::ReflectorId, util::{callback_arg_string, v8_string}, }; @@ -36,7 +35,7 @@ pub(crate) use history_queue::{ PendingHistoryTraversalAction, PendingNavigationApiTaskAction, PendingNavigationFinishedResult, PendingNavigationResult, }; -use identity::{BridgeHandle, BridgeIdentityStore, DomTokenListKind}; +use identity::{BridgeHandle, BridgeIdentityStore, DomTokenListKind, ReflectorId}; use identity::{CollectionKind, LiveCollectionDescriptor, LiveCollectionQueryKind}; pub(crate) use identity::{ ComputedStyleDescriptor, ComputedStylePseudoKey, ComputedStyleTargetKey, diff --git a/moli-renderer-v8/src/reflector.rs b/moli-renderer-v8/src/reflector.rs deleted file mode 100644 index e22c0e8116..0000000000 --- a/moli-renderer-v8/src/reflector.rs +++ /dev/null @@ -1,166 +0,0 @@ -use std::{collections::HashMap, hash::Hash}; - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct ReflectorId(u64); - -impl ReflectorId { - pub fn from_raw(raw: u64) -> Self { - Self(raw) - } - - pub fn raw(self) -> u64 { - self.0 - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct Reflector { - id: ReflectorId, - key: K, -} - -impl Reflector { - pub fn id(&self) -> ReflectorId { - self.id - } - - pub fn key(&self) -> K { - self.key.clone() - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct DomPtr(K); - -impl DomPtr { - pub fn new(key: K) -> Self { - Self(key) - } - - pub fn key(&self) -> K { - self.0.clone() - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct DomRoot { - reflector: Reflector, -} - -impl DomRoot { - pub fn reflector_id(&self) -> ReflectorId { - self.reflector.id() - } -} - -#[derive(Debug, Clone)] -pub struct ReflectorRegistry { - next_id: u64, - ids_by_key: HashMap, - keys_by_id: Vec, -} - -impl Default for ReflectorRegistry { - fn default() -> Self { - Self { - next_id: 0, - ids_by_key: HashMap::new(), - keys_by_id: Vec::new(), - } - } -} - -impl ReflectorRegistry -where - K: Clone + Eq + Hash, -{ - pub fn intern(&mut self, key: K) -> Reflector { - if let Some(existing) = self.ids_by_key.get(&key).copied() { - return Reflector { id: existing, key }; - } - - self.next_id = self.next_id.checked_add(1).expect("reflector id overflow"); - let id = ReflectorId(self.next_id); - self.ids_by_key.insert(key.clone(), id); - self.keys_by_id.push(key.clone()); - Reflector { id, key } - } - - pub fn existing(&self, key: K) -> Option> { - self.ids_by_key - .get(&key) - .copied() - .map(|id| Reflector { id, key }) - } - - pub fn root(&mut self, ptr: DomPtr) -> DomRoot { - DomRoot { - reflector: self.intern(ptr.key()), - } - } - - pub fn key_for_id(&self, id: ReflectorId) -> Option { - let index = usize::try_from(id.raw().checked_sub(1)?).ok()?; - self.keys_by_id.get(index).cloned() - } - - pub fn len(&self) -> usize { - self.ids_by_key.len() - } - - pub fn is_empty(&self) -> bool { - self.ids_by_key.is_empty() - } -} - -#[cfg(test)] -mod tests { - use super::ReflectorRegistry; - - #[test] - fn reflector_registry_reuses_identity_for_same_key() { - let mut registry = ReflectorRegistry::default(); - - let first = registry.intern(7_u32); - let second = registry.intern(7_u32); - - assert_eq!(first.id(), second.id()); - assert_eq!(first.key(), second.key()); - assert_eq!(first.id().raw(), 1); - assert_eq!(registry.len(), 1); - assert_eq!(registry.key_for_id(first.id()), Some(7_u32)); - } - - #[test] - fn reflector_registry_supports_non_copy_keys() { - let mut registry = ReflectorRegistry::default(); - - let first = registry.intern("highlight(name)".to_owned()); - let second = registry.intern("highlight(name)".to_owned()); - let other = registry.intern("highlight(other)".to_owned()); - - assert_eq!(first.id(), second.id()); - assert_ne!(first.id(), other.id()); - assert_eq!( - registry.key_for_id(first.id()), - Some("highlight(name)".to_owned()) - ); - } - - #[test] - fn reflector_registry_allocates_distinct_identity_for_distinct_keys() { - let mut registry = ReflectorRegistry::default(); - - let first = registry.intern(1_u32); - let second = registry.intern(2_u32); - - assert_ne!(first.id(), second.id()); - assert_eq!(registry.existing(1_u32), Some(first)); - assert_eq!(registry.existing(2_u32), Some(second)); - assert!(registry.existing(99_u32).is_none()); - assert_eq!(registry.key_for_id(first.id()), Some(1_u32)); - assert_eq!(registry.key_for_id(second.id()), Some(2_u32)); - assert_eq!(registry.len(), 2); - assert!(!registry.is_empty()); - } -}