From 42fe677d2e821fe8d6ba4ba0e08702a906557ac1 Mon Sep 17 00:00:00 2001 From: Haled Odat Date: Fri, 24 Mar 2023 14:38:16 +0100 Subject: [PATCH] Remove unneeded Option in property_table --- boa_engine/src/object/property_map.rs | 23 ++-- boa_engine/src/object/shape/mod.rs | 18 ++- boa_engine/src/object/shape/shared_shape.rs | 142 ++++++++++++-------- 3 files changed, 108 insertions(+), 75 deletions(-) diff --git a/boa_engine/src/object/property_map.rs b/boa_engine/src/object/property_map.rs index cff54430ef1..e7fd4a24dbe 100644 --- a/boa_engine/src/object/property_map.rs +++ b/boa_engine/src/object/property_map.rs @@ -267,13 +267,11 @@ impl PropertyMap { let mut builder = PropertyDescriptor::builder() .configurable(attributes.contains(PropertyDescriptorAttribute::CONFIGURABLE)) .enumerable(attributes.contains(PropertyDescriptorAttribute::ENUMERABLE)); - if attributes.contains(PropertyDescriptorAttribute::HAS_GET) - || attributes.contains(PropertyDescriptorAttribute::HAS_SET) - { - if attributes.contains(PropertyDescriptorAttribute::HAS_GET) { + if attributes.is_accessor_descriptor() { + if attributes.has_get() { builder = builder.get(self.storage[index].clone()); } - if attributes.contains(PropertyDescriptorAttribute::HAS_SET) { + if attributes.has_set() { builder = builder.set(self.storage[index + 1].clone()); } } else { @@ -336,21 +334,18 @@ impl PropertyMap { property_key: key.clone(), attributes, }; - self.shape = self.shape.change_attributes_transition(key); + self.shape = self.shape.change_attributes_transition(key, slot.index); } - // TODO: maybe implement is_accessor/data_descriptor on attributes? - if attributes.contains(PropertyDescriptorAttribute::HAS_GET) - || attributes.contains(PropertyDescriptorAttribute::HAS_SET) - { - if attributes.contains(PropertyDescriptorAttribute::HAS_GET) { + if attributes.is_accessor_descriptor() { + if attributes.has_get() { self.storage[index] = property .get() .cloned() .map(JsValue::new) .unwrap_or_default(); } - if attributes.contains(PropertyDescriptorAttribute::HAS_SET) { + if attributes.has_set() { self.storage[index + 1] = property .set() .cloned() @@ -369,9 +364,7 @@ impl PropertyMap { attributes, }; self.shape = self.shape.insert_property_transition(transition_key); - if attributes.contains(PropertyDescriptorAttribute::HAS_GET) - || attributes.contains(PropertyDescriptorAttribute::HAS_SET) - { + if attributes.is_accessor_descriptor() { self.storage.push( property .get() diff --git a/boa_engine/src/object/shape/mod.rs b/boa_engine/src/object/shape/mod.rs index a2c5fba5741..90255539629 100644 --- a/boa_engine/src/object/shape/mod.rs +++ b/boa_engine/src/object/shape/mod.rs @@ -43,6 +43,22 @@ bitflags! { } } +impl PropertyDescriptorAttribute { + pub const fn is_accessor_descriptor(self) -> bool { + self.contains(Self::HAS_GET) || self.contains(Self::HAS_SET) + } + pub const fn is_data_descriptor(self) -> bool { + !self.is_accessor_descriptor() + } + + pub const fn has_get(self) -> bool { + self.contains(Self::HAS_GET) + } + pub const fn has_set(self) -> bool { + self.contains(Self::HAS_SET) + } +} + #[derive(Debug, Finalize, Clone, PartialEq, Eq, Hash)] pub(crate) struct TransitionKey { pub(crate) property_key: PropertyKey, @@ -118,7 +134,7 @@ impl Shape { } } - pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self { + pub(crate) fn change_attributes_transition(&self, key: TransitionKey, index: u32) -> Self { match &self.inner { Inner::Shared(shape) => Self::shared(shape.change_attributes_transition(key)), Inner::Unique(shape) => Self::unique(shape.change_attributes_transition(key)), diff --git a/boa_engine/src/object/shape/shared_shape.rs b/boa_engine/src/object/shape/shared_shape.rs index 5e1a5abf32c..7b322ccd9ee 100644 --- a/boa_engine/src/object/shape/shared_shape.rs +++ b/boa_engine/src/object/shape/shared_shape.rs @@ -1,6 +1,10 @@ -use std::cell::{Cell, RefCell}; +use std::{ + cell::{Cell, RefCell}, + collections::hash_map::RandomState, +}; use boa_gc::{Finalize, Gc, GcRefCell, Trace, WeakGc}; +use indexmap::{IndexMap, IndexSet}; use rustc_hash::FxHashMap; use crate::property::{Attribute, PropertyDescriptor, PropertyKey}; @@ -12,10 +16,7 @@ use super::{PropertyDescriptorAttribute, Shape, Slot, TransitionKey, TransitionT struct Inner { /// Cached property map, only constucted when a property is accesed through it. #[unsafe_ignore_trace] - property_table: RefCell>>, - - #[unsafe_ignore_trace] - property_access: Cell, + property_table: RefCell>, forward_property_transitions: GcRefCell>>, previous: Option, @@ -37,7 +38,7 @@ pub(crate) struct SharedShape { } impl SharedShape { - const DEBUG: bool = false; + const DEBUG: bool = true; #[inline] pub(crate) fn previous(&self) -> Option<&SharedShape> { @@ -73,8 +74,7 @@ impl SharedShape { #[inline] pub(crate) fn root() -> Self { Self::new(Inner { - property_table: RefCell::new(None), - property_access: Cell::new(0), + property_table: RefCell::default(), forward_property_transitions: GcRefCell::new(FxHashMap::default()), property_key: PropertyKey::Index(0), property_index: 0, @@ -98,8 +98,7 @@ impl SharedShape { self.property_index() + 2 }; let new_inner_shape = Inner { - property_table: RefCell::new(None), - property_access: Cell::new(0), + property_table: RefCell::default(), forward_property_transitions: GcRefCell::new(FxHashMap::default()), property_index: next_property_index, property_key: key.property_key.clone(), @@ -148,14 +147,8 @@ impl SharedShape { key.attributes, ); } - let mut next_property_index = if self.previous().is_none() { - self.property_index() - } else { - self.property_index() + 2 - }; let new_inner_shape = Inner { - property_table: RefCell::new(None), - property_access: Cell::new(0), + property_table: RefCell::default(), forward_property_transitions: GcRefCell::new(FxHashMap::default()), property_index: self.property_index(), property_key: key.property_key.clone(), @@ -200,7 +193,8 @@ impl SharedShape { if Self::DEBUG { println!("Shape: deleting {key}"); } - let mut transitions = Vec::default(); + let mut transitions: IndexMap = + IndexMap::default(); let mut p = Some(self); let mut base = loop { @@ -218,33 +212,46 @@ impl SharedShape { break base; } - transitions.push(TransitionKey { - property_key: shape.property_key().clone(), - attributes: shape.inner.property_attributes, - }); + // Do not add property that we are trying to delete. + // this can happen if a configure was called after inserting it into the shape + if shape.property_key() != key { + // Only take the latest changes to a property. To try to build a smaller tree. + transitions + .entry(shape.property_key().clone()) + .or_insert(shape.inner.property_attributes); + } p = shape.previous(); }; - for transition in transitions.into_iter().rev() { + for (property_key, attributes) in IntoIterator::into_iter(transitions).rev() { + let transition = TransitionKey { + property_key, + attributes, + }; base = base.insert_property_transition(transition); } base } + // TODO: Implement descriptor array. #[inline] pub(crate) fn lookup(&self, key: &PropertyKey) -> Option { if Self::DEBUG { println!("Shape: lookup {key}"); } + + self.previous()?; + // Use cheched property table, if exists - if self.inner.property_access.get() == 4 { + { let property_table = self.inner.property_table.borrow(); - if let Some(property_table) = property_table.as_ref() { + if property_table.len() != 0 { if Self::DEBUG { println!(" Shape: Using cached property table"); } + if let Some((index, attributes)) = property_table.get(key).copied() { return Some(Slot { index, attributes }); } @@ -253,35 +260,44 @@ impl SharedShape { } } - if self.inner.property_access.get() < 4 { - self.inner - .property_access - .set(self.inner.property_access.get() + 1); - - let mut p = Some(self); - while let Some(shape) = p { - // Root has no properties - if shape.is_root() { - break; - } - if shape.property_key() == key { - if Self::DEBUG { - println!( - " Shape: found {} with {} property index", - shape.property_key(), - shape.property_index() - ); - } - return Some(Slot { - index: shape.property_index(), - attributes: shape.inner.property_attributes, - }); - } - p = shape.previous(); - } - - return None; - } + // if self.inner.property_access.get() < 4 { + // self.inner + // .property_access + // .set(self.inner.property_access.get() + 1); + + // let mut p = Some(self); + + // let mut index = 0; + // let mut attributes = None; + // while let Some(shape) = p { + // // Root has no properties + // if shape.is_root() { + // break; + // } + // if shape.property_key() == key { + // if Self::DEBUG { + // println!( + // " Shape: found {} with {} property index", + // shape.property_key(), + // shape.property_index() + // ); + // } + // if attributes.is_none() { + // attributes = Some(shape.inner.property_attributes); + // } + + // if shape.transition_type() == TransitionType::Insert { + // return Some(Slot { + // index: shape.property_index(), + // attributes: attributes.expect("This should have already been set!"), + // }); + // } + // } + // p = shape.previous(); + // } + + // return None; + // } if Self::DEBUG { println!(" Shape: Creating cached property table"); @@ -291,6 +307,7 @@ impl SharedShape { // Creating cached property table let mut result = None; + let mut attributes = None; let mut p = Some(self); while let Some(shape) = p { // Root has no properties @@ -305,16 +322,23 @@ impl SharedShape { shape.property_index() ); } - result = Some(Slot { - index: shape.property_index(), - attributes: shape.inner.property_attributes, - }); + if attributes.is_none() { + attributes = Some(shape.inner.property_attributes); + } + + if shape.transition_type() == TransitionType::Insert { + result = Some(Slot { + index: shape.property_index(), + attributes: attributes.expect("This should have already been set!"), + }); + } } // If we already added a property we don't add the previous versions // which can happens if a configure property transition occurs. property_table .entry(shape.property_key().clone()) + .and_modify(|(index, _)| *index = shape.property_index()) .or_insert((shape.property_index(), shape.inner.property_attributes)); p = shape.previous(); @@ -325,7 +349,7 @@ impl SharedShape { // Put into shape let mut property_table_borrow = self.inner.property_table.borrow_mut(); - property_table_borrow.insert(property_table); + std::mem::replace(&mut *property_table_borrow, property_table); result }