...

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);

boa_engine/src/object/shape/shared_shape/mod.rs

@@ -67,7 +67,6 @@ struct Inner {
 
     previous: Option<SharedShape>,
 
-    // 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<PropertyKey> = property_table
             .keys()
             .take(property_count)

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 {

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,
+        }
+    }
+}

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<IndexMap<PropertyKey, SlotAttribute>>,
+    property_table: RefCell<IndexMap<PropertyKey, Slot>>,
 
     prototype: GcRefCell<JsPrototype>,
 }
@@ -40,14 +40,11 @@ impl UniqueShape {
     }
 
     /// Create a new unique shape.
-    pub(crate) fn new(
-        prototype: GcRefCell<JsPrototype>,
-        property_table: IndexMap<PropertyKey, SlotAttribute>,
-    ) -> Self {
+    pub(crate) fn new(prototype: JsPrototype, property_table: IndexMap<PropertyKey, Slot>) -> 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,29 +70,40 @@ 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)
     }
 
     /// TODO: doc
     pub(crate) fn lookup(&self, key: &PropertyKey) -> Option<Slot> {
         // 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.