From c013caca22497f3a6637b442ac84bb102900d23c Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Mon, 29 May 2023 18:39:55 +0200 Subject: [PATCH] Prune collected shared shapes (#2941) * Prune garbage collected `SharedShape`s * Prune on insertion limit --- .../shape/shared_shape/forward_transition.rs | 95 ++++++++++++++- .../src/object/shape/shared_shape/mod.rs | 9 ++ .../src/object/shape/shared_shape/tests.rs | 109 ++++++++++++++++++ boa_gc/src/pointers/ephemeron.rs | 7 ++ boa_gc/src/pointers/weak.rs | 5 + 5 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 boa_engine/src/object/shape/shared_shape/tests.rs diff --git a/boa_engine/src/object/shape/shared_shape/forward_transition.rs b/boa_engine/src/object/shape/shared_shape/forward_transition.rs index ba9c9488c8..b23b0bf320 100644 --- a/boa_engine/src/object/shape/shared_shape/forward_transition.rs +++ b/boa_engine/src/object/shape/shared_shape/forward_transition.rs @@ -1,3 +1,5 @@ +use std::fmt::Debug; + use boa_gc::{Finalize, Gc, GcRefCell, Trace, WeakGc}; use rustc_hash::FxHashMap; @@ -6,7 +8,34 @@ use crate::object::JsPrototype; use super::{Inner as SharedShapeInner, TransitionKey}; /// Maps transition key type to a [`SharedShapeInner`] transition. -type TransitionMap = FxHashMap>; +#[derive(Debug, Trace, Finalize)] +struct TransitionMap { + map: FxHashMap>, + + /// This counts the number of insertions after a prune operation. + insertion_count_since_prune: u8, +} + +impl Default for TransitionMap { + fn default() -> Self { + Self { + map: FxHashMap::default(), + insertion_count_since_prune: 0, + } + } +} + +impl TransitionMap { + fn get_and_increment_count(&mut self) -> u8 { + let result = self.insertion_count_since_prune; + + // NOTE(HalidOdat): This is done so it overflows to 0, on every 256 insertion + // operations. Fulfills the prune condition every 256 insertions. + self.insertion_count_since_prune = self.insertion_count_since_prune.wrapping_add(1); + + result + } +} /// The internal representation of [`ForwardTransition`]. #[derive(Default, Debug, Trace, Finalize)] @@ -17,7 +46,7 @@ struct Inner { /// Holds a forward reference to a previously created transition. /// -/// The reference is weak, therefore it can be garbage collected if it is not in use. +/// The reference is weak, therefore it can be garbage collected, if it's not in use. #[derive(Default, Debug, Trace, Finalize)] pub(super) struct ForwardTransition { inner: GcRefCell, @@ -28,14 +57,24 @@ impl ForwardTransition { pub(super) fn insert_property(&self, key: TransitionKey, value: &Gc) { let mut this = self.inner.borrow_mut(); let properties = this.properties.get_or_insert_with(Box::default); - properties.insert(key, WeakGc::new(value)); + + if properties.get_and_increment_count() == u8::MAX { + properties.map.retain(|_, v| v.is_upgradable()); + } + + properties.map.insert(key, WeakGc::new(value)); } /// Insert a prototype transition. pub(super) fn insert_prototype(&self, key: JsPrototype, value: &Gc) { let mut this = self.inner.borrow_mut(); let prototypes = this.prototypes.get_or_insert_with(Box::default); - prototypes.insert(key, WeakGc::new(value)); + + if prototypes.get_and_increment_count() == u8::MAX { + prototypes.map.retain(|_, v| v.is_upgradable()); + } + + prototypes.map.insert(key, WeakGc::new(value)); } /// Get a property transition, return [`None`] otherwise. @@ -44,7 +83,7 @@ impl ForwardTransition { let Some(transitions) = this.properties.as_ref() else { return None; }; - transitions.get(key).cloned() + transitions.map.get(key).cloned() } /// Get a prototype transition, return [`None`] otherwise. @@ -53,6 +92,50 @@ impl ForwardTransition { let Some(transitions) = this.prototypes.as_ref() else { return None; }; - transitions.get(key).cloned() + transitions.map.get(key).cloned() + } + + /// Prunes the [`WeakGc`]s that have been garbage collected. + pub(super) fn prune_property_transitions(&self) { + let mut this = self.inner.borrow_mut(); + let Some(transitions) = this.properties.as_deref_mut() else { + return; + }; + + transitions.insertion_count_since_prune = 0; + transitions.map.retain(|_, v| v.is_upgradable()); + } + + /// Prunes the [`WeakGc`]s that have been garbage collected. + pub(super) fn prune_prototype_transitions(&self) { + let mut this = self.inner.borrow_mut(); + let Some(transitions) = this.prototypes.as_deref_mut() else { + return; + }; + + transitions.insertion_count_since_prune = 0; + transitions.map.retain(|_, v| v.is_upgradable()); + } + + #[cfg(test)] + pub(crate) fn property_transitions_count(&self) -> (usize, u8) { + let this = self.inner.borrow(); + this.properties.as_ref().map_or((0, 0), |transitions| { + ( + transitions.map.len(), + transitions.insertion_count_since_prune, + ) + }) + } + + #[cfg(test)] + pub(crate) fn prototype_transitions_count(&self) -> (usize, u8) { + let this = self.inner.borrow(); + this.prototypes.as_ref().map_or((0, 0), |transitions| { + ( + transitions.map.len(), + transitions.insertion_count_since_prune, + ) + }) } } diff --git a/boa_engine/src/object/shape/shared_shape/mod.rs b/boa_engine/src/object/shape/shared_shape/mod.rs index 02931998d0..0eff31474f 100644 --- a/boa_engine/src/object/shape/shared_shape/mod.rs +++ b/boa_engine/src/object/shape/shared_shape/mod.rs @@ -1,6 +1,9 @@ mod forward_transition; pub(crate) mod template; +#[cfg(test)] +mod tests; + use std::{collections::hash_map::RandomState, hash::Hash}; use bitflags::bitflags; @@ -192,6 +195,8 @@ impl SharedShape { if let Some(inner) = shape.upgrade() { return Self { inner }; } + + self.forward_transitions().prune_prototype_transitions(); } let new_inner_shape = Inner { forward_transitions: ForwardTransition::default(), @@ -217,6 +222,8 @@ impl SharedShape { if let Some(inner) = shape.upgrade() { return Self { inner }; } + + self.forward_transitions().prune_property_transitions(); } let property_table = self.property_table().add_property_deep_clone_if_needed( @@ -266,6 +273,8 @@ impl SharedShape { action, }; } + + self.forward_transitions().prune_property_transitions(); } // The attribute change transitions, didn't change from accessor to data property or vice-versa. diff --git a/boa_engine/src/object/shape/shared_shape/tests.rs b/boa_engine/src/object/shape/shared_shape/tests.rs new file mode 100644 index 0000000000..68d74e1b8a --- /dev/null +++ b/boa_engine/src/object/shape/shared_shape/tests.rs @@ -0,0 +1,109 @@ +use crate::{object::shape::slot::SlotAttributes, property::PropertyKey, JsObject, JsSymbol}; + +use super::{SharedShape, TransitionKey}; + +#[test] +fn test_prune_property_on_counter_limit() { + let shape = SharedShape::root(); + + for i in 0..255 { + assert_eq!( + shape.forward_transitions().property_transitions_count(), + (i, i as u8) + ); + + shape.insert_property_transition(TransitionKey { + property_key: PropertyKey::Symbol(JsSymbol::new(None).unwrap()), + attributes: SlotAttributes::all(), + }); + } + + assert_eq!( + shape.forward_transitions().property_transitions_count(), + (255, 255) + ); + + boa_gc::force_collect(); + + { + shape.insert_property_transition(TransitionKey { + property_key: PropertyKey::Symbol(JsSymbol::new(None).unwrap()), + attributes: SlotAttributes::all(), + }); + } + + assert_eq!( + shape.forward_transitions().property_transitions_count(), + (1, 0) + ); + + { + shape.insert_property_transition(TransitionKey { + property_key: PropertyKey::Symbol(JsSymbol::new(None).unwrap()), + attributes: SlotAttributes::all(), + }); + } + + assert_eq!( + shape.forward_transitions().property_transitions_count(), + (2, 1) + ); + + boa_gc::force_collect(); + + assert_eq!( + shape.forward_transitions().property_transitions_count(), + (2, 1) + ); +} + +#[test] +fn test_prune_prototype_on_counter_limit() { + let shape = SharedShape::root(); + + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (0, 0) + ); + + for i in 0..255 { + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (i, i as u8) + ); + + shape.change_prototype_transition(Some(JsObject::with_null_proto())); + } + + boa_gc::force_collect(); + + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (255, 255) + ); + + { + shape.change_prototype_transition(Some(JsObject::with_null_proto())); + } + + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (1, 0) + ); + + { + shape.change_prototype_transition(Some(JsObject::with_null_proto())); + } + + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (2, 1) + ); + + boa_gc::force_collect(); + + assert_eq!( + shape.forward_transitions().prototype_transitions_count(), + (2, 1) + ); +} diff --git a/boa_gc/src/pointers/ephemeron.rs b/boa_gc/src/pointers/ephemeron.rs index c63da65fd7..f7dfd50a47 100644 --- a/boa_gc/src/pointers/ephemeron.rs +++ b/boa_gc/src/pointers/ephemeron.rs @@ -31,6 +31,13 @@ impl Ephemeron { // `inner_ptr`. unsafe { self.inner_ptr.get().as_ref().value().cloned() } } + + /// Checks if the [`Ephemeron`] has a value. + pub fn has_value(&self) -> bool { + // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer + // `inner_ptr`. + unsafe { self.inner_ptr.get().as_ref().value().is_some() } + } } impl Ephemeron { diff --git a/boa_gc/src/pointers/weak.rs b/boa_gc/src/pointers/weak.rs index edeadd7add..6bbb9762d3 100644 --- a/boa_gc/src/pointers/weak.rs +++ b/boa_gc/src/pointers/weak.rs @@ -23,6 +23,11 @@ impl WeakGc { pub fn upgrade(&self) -> Option> { self.inner.value() } + + /// Check if the [`WeakGc`] can be upgraded. + pub fn is_upgradable(&self) -> bool { + self.inner.has_value() + } } impl Clone for WeakGc {