diff --git a/boa_engine/src/object/property_map.rs b/boa_engine/src/object/property_map.rs index 9e256f365c1..186b33657e7 100644 --- a/boa_engine/src/object/property_map.rs +++ b/boa_engine/src/object/property_map.rs @@ -7,7 +7,7 @@ use super::{ JsPrototype, PropertyDescriptor, PropertyKey, }; use crate::{property::PropertyDescriptorBuilder, JsString, JsSymbol, JsValue}; -use boa_gc::{custom_trace, Finalize, GcRefCell, Trace}; +use boa_gc::{custom_trace, Finalize, Trace}; use indexmap::IndexMap; use rustc_hash::{FxHashMap, FxHasher}; use std::{collections::hash_map, hash::BuildHasherDefault, iter::FusedIterator}; @@ -241,10 +241,7 @@ impl PropertyMap { pub fn from_prototype_unique_shape(prototype: JsPrototype) -> Self { Self { indexed_properties: IndexedProperties::default(), - shape: Shape::unique(UniqueShape::new( - GcRefCell::new(prototype), - IndexMap::default(), - )), + shape: Shape::unique(UniqueShape::new(prototype, IndexMap::default())), storage: Vec::default(), } } @@ -372,7 +369,6 @@ impl PropertyMap { } else { self.storage .push(property.value().cloned().unwrap_or_default()); - self.storage.push(JsValue::undefined()); } false @@ -384,8 +380,10 @@ impl PropertyMap { return self.indexed_properties.remove(*index); } if let Some(slot) = self.shape.lookup(key) { - // shift all elements - self.storage.remove(slot.index as usize + 1); + // shift all elements when removing. + if slot.attributes.is_accessor_descriptor() { + self.storage.remove(slot.index as usize + 1); + } self.storage.remove(slot.index as usize); self.shape = self.shape.remove_property_transition(key); diff --git a/boa_engine/src/object/shape/shared_shape/mod.rs b/boa_engine/src/object/shape/shared_shape/mod.rs index b8e970760ff..c8fb28ffac5 100644 --- a/boa_engine/src/object/shape/shared_shape/mod.rs +++ b/boa_engine/src/object/shape/shared_shape/mod.rs @@ -67,7 +67,6 @@ struct Inner { previous: Option, - // TODO: add prototype to shape transition_type: TransitionType, } @@ -101,6 +100,13 @@ impl SharedShape { .expect("There should be a property"); (key.clone(), *slot) } + // pub(crate) fn property_slot(&self) -> Slot { + // let properties = self.inner.property_table.properties().borrow(); + // let (_, slot) = properties + // .get_index(self.property_index() as usize) + // .expect("There should be a property"); + // *slot + // } fn transition_type(&self) -> TransitionType { self.inner.transition_type } @@ -128,17 +134,11 @@ impl SharedShape { } fn create_property_transition(&self, key: TransitionKey) -> Self { - let slot_index = self.property_count() - // TODO: implement different sized storage - * 2; - - let (property_index, property_table) = - self.inner.property_table.add_property_deep_clone_if_needed( - key.property_key.clone(), - key.attributes, - slot_index, - self.property_count(), - ); + let property_table = self.inner.property_table.add_property_deep_clone_if_needed( + key.property_key.clone(), + key.attributes, + self.property_count(), + ); let new_inner_shape = Inner { prototype: self.prototype(), forward_transitions: ForwardTransition::default(), @@ -344,6 +344,7 @@ impl SharedShape { let property_count = self.property_count() as usize; + // TOOD: use .chain to concat iterator let mut keys: Vec = property_table .keys() .take(property_count) diff --git a/boa_engine/src/object/shape/shared_shape/property_table.rs b/boa_engine/src/object/shape/shared_shape/property_table.rs index fd5daa26fa0..b2b91256b92 100644 --- a/boa_engine/src/object/shape/shared_shape/property_table.rs +++ b/boa_engine/src/object/shape/shared_shape/property_table.rs @@ -26,9 +26,8 @@ impl PropertyTable { &self, key: PropertyKey, attributes: SlotAttribute, - slot_index: u32, property_count: u32, - ) -> (u32, Self) { + ) -> Self { // TODO: possible optimization if we are the only ones holding a reference to self we can add the property directly. // TOOD: possible optimization if we already have the property in the exact position we want it to be with the same properties, // then just return self, this might happen, if a branch is created than after not being used gets gc collected we still have the @@ -38,40 +37,31 @@ impl PropertyTable { { let mut properties = self.inner.properties.borrow_mut(); if (property_count as usize) == properties.len() && !properties.contains_key(&key) { - // println!( - // "Extending PropertyTable with {key} - Slot {slot_index} - PC {property_count}" - // ); - let (index, value) = properties.insert_full( - key, - Slot { - index: slot_index, - attributes, - }, + let slot = Slot::from_previous(properties.last().map(|x| x.1), attributes); + println!( + "Extending PropertyTable with {key} - Slot {} - PC {property_count}", + slot.index ); + let (index, value) = properties.insert_full(key, slot); debug_assert!(value.is_none()); - return (index as u32, self.clone()); + return self.clone(); } } - // println!( - // "Creating fork PropertyTable with {key} - Slot {slot_index} - PC {property_count}" - // ); - // property is already present need to make deep clone of property table. let this = self.deep_clone(property_count); let index = { let mut properties = this.inner.properties.borrow_mut(); - let (index, value) = properties.insert_full( - key, - Slot { - index: slot_index, - attributes, - }, + let slot = Slot::from_previous(properties.last().map(|x| x.1), attributes); + println!( + "Creating fork PropertyTable with {key} - Slot {} - PC {property_count}", + slot.index ); + let (index, value) = properties.insert_full(key, slot); debug_assert!(value.is_none()); index }; - (index as u32, this) + this } pub(crate) fn deep_clone(&self, count: u32) -> Self { diff --git a/boa_engine/src/object/shape/slot.rs b/boa_engine/src/object/shape/slot.rs index 1580d03e47a..cbd38516c5a 100644 --- a/boa_engine/src/object/shape/slot.rs +++ b/boa_engine/src/object/shape/slot.rs @@ -33,3 +33,27 @@ pub struct Slot { pub index: SlotIndex, pub attributes: SlotAttribute, } + +impl Slot { + pub(crate) fn from_previous( + previous_slot: Option<&Slot>, + new_attributes: SlotAttribute, + ) -> Self { + // If there was no previous lot then return 0 as the index. + let Some(previous_slot) = previous_slot else { + return Self { + index: 0, + attributes: new_attributes, + } + }; + + // accessor take 2 positions in the storage to accomodate for the `get` and `set` fields. + let new_slot_index = + previous_slot.index + 1 + u32::from(previous_slot.attributes.is_accessor_descriptor()); + + Self { + index: new_slot_index, + attributes: new_attributes, + } + } +} diff --git a/boa_engine/src/object/shape/unique_shape.rs b/boa_engine/src/object/shape/unique_shape.rs index b83435568ff..48c2515ffa8 100644 --- a/boa_engine/src/object/shape/unique_shape.rs +++ b/boa_engine/src/object/shape/unique_shape.rs @@ -23,7 +23,7 @@ use super::{shared_shape::TransitionKey, slot::SlotAttribute, JsPrototype, Slot} #[derive(Default, Debug, Trace, Finalize)] struct Inner { #[unsafe_ignore_trace] - property_table: RefCell>, + property_table: RefCell>, prototype: GcRefCell, } @@ -40,14 +40,11 @@ impl UniqueShape { } /// Create a new unique shape. - pub(crate) fn new( - prototype: GcRefCell, - property_table: IndexMap, - ) -> Self { + pub(crate) fn new(prototype: JsPrototype, property_table: IndexMap) -> Self { Self { inner: Gc::new(Inner { property_table: property_table.into(), - prototype, + prototype: GcRefCell::new(prototype), }), } } @@ -56,7 +53,8 @@ impl UniqueShape { pub(crate) fn insert_property_transition(&self, key: TransitionKey) -> Self { { let mut property_table = self.inner.property_table.borrow_mut(); - let (index, inserted) = property_table.insert_full(key.property_key, key.attributes); + let slot = Slot::from_previous(property_table.last().map(|x| x.1), key.attributes); + let (index, inserted) = property_table.insert_full(key.property_key, slot); debug_assert!(inserted.is_none()); debug_assert!(index + 1 == property_table.len()); @@ -72,17 +70,31 @@ impl UniqueShape { return self.clone(); }; - /// Check if it's the last property, that was deleted, - /// if so delete it and return self - if index == property_table.len() { - return self.clone(); - } - // The property that was deleted was not the last property added. // Therefore we need to create a new unique shape, // to invalidate any pointers to this shape i.e inline caches. - let property_table = std::mem::take(&mut *property_table); - let prototype = self.inner.prototype.clone(); + let mut property_table = std::mem::take(&mut *property_table); + + /// If it is not the last property that was deleted, + /// then update all the property slots that are after it. + if index != property_table.len() { + // Get the previous value before the value at index, + // + // NOTE: calling wrapping_sub when usize index is 0 will wrap into usize::MAX + // which will return None, avoiding unneeded checks. + let mut previous_slot = property_table + .get_index(index.wrapping_sub(1)) + .map(|x| x.1) + .copied(); + + // Update all slot positions + for slot in property_table.values_mut().skip(index) { + *slot = Slot::from_previous(previous_slot.as_ref(), slot.attributes); + previous_slot = Some(*slot); + } + } + + let prototype = self.inner.prototype.borrow_mut().take(); Self::new(prototype, property_table) } @@ -90,11 +102,8 @@ impl UniqueShape { pub(crate) fn lookup(&self, key: &PropertyKey) -> Option { // println!("Unique: lookup {key}"); let mut property_table = self.inner.property_table.borrow(); - if let Some((index, _property_key, attributes)) = property_table.get_full(key) { - return Some(Slot { - index: index as u32 * 2, - attributes: *attributes, - }); + if let Some(slot) = property_table.get(key) { + return Some(*slot); } None @@ -103,11 +112,11 @@ impl UniqueShape { pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self { { let mut property_table = self.inner.property_table.borrow_mut(); - let Some(attributes) = property_table.get_mut(&key.property_key) else { + let Some(slot) = property_table.get_mut(&key.property_key) else { unreachable!("Attribute change can only happen on existing property") }; - *attributes = key.attributes; + slot.attributes = key.attributes; } self.clone() } @@ -117,7 +126,7 @@ impl UniqueShape { // We need to create a new unique shape, // to invalidate any pointers to this shape i.e inline caches. let property_table = std::mem::take(&mut *property_table); - Self::new(GcRefCell::new(prototype), property_table) + Self::new(prototype, property_table) } /// Gets all keys first strings then symbols.