Implement varying width property storage

boa_engine/src/object/property_map.rs

@@ -2,7 +2,7 @@ use super::{
     shape::{
         shared_shape::TransitionKey,
         slot::{Slot, SlotAttribute},
-        Shape, UniqueShape,
+        ChangeTransitionAction, Shape, UniqueShape,
     },
     JsPrototype, PropertyDescriptor, PropertyKey,
 };
@@ -321,7 +321,18 @@ impl PropertyMap {
                     property_key: key.clone(),
                     attributes,
                 };
-                self.shape = self.shape.change_attributes_transition(key);
+                let transition = self.shape.change_attributes_transition(key);
+                self.shape = transition.shape;
+                match transition.action {
+                    ChangeTransitionAction::Nothing => {}
+                    ChangeTransitionAction::Remove => {
+                        self.storage.remove(slot.index as usize + 1);
+                    }
+                    ChangeTransitionAction::Insert => {
+                        // insert after index which is (index + 1).
+                        self.storage.insert(index, JsValue::undefined());
+                    }
+                }
             }
 
             if attributes.is_accessor_descriptor() {

boa_engine/src/object/shape/mod.rs

@@ -38,6 +38,33 @@ use self::{
 
 use super::JsPrototype;
 
+/// Action to be performed after a property attribute change
+//
+// Example: of { get/set x() { ... }, y: ... } into { x: ..., y: ... }
+//
+//                 0       1       2
+//    Storage: | get x | set x |   y   |
+//
+// We delete at position of x which is index 0 (it spans two elements) + 1:
+//
+//                 0      1
+//    Storage: |   x  |   y   |
+pub(crate) enum ChangeTransitionAction {
+    /// Do nothing to storage.
+    Nothing,
+
+    /// Remove element at (index + 1) from storage.
+    Remove,
+
+    /// Insert element at (index + 1) into storage.
+    Insert,
+}
+
+pub(crate) struct ChangeTransition {
+    pub(crate) shape: Shape,
+    pub(crate) action: ChangeTransitionAction,
+}
+
 #[derive(Debug, Trace, Finalize, Clone)]
 enum Inner {
     Unique(UniqueShape),
@@ -81,10 +108,10 @@ impl Shape {
         }
     }
 
-    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self {
+    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> ChangeTransition {
         match &self.inner {
-            Inner::Shared(shape) => Self::shared(shape.change_attributes_transition(key)),
-            Inner::Unique(shape) => Self::unique(shape.change_attributes_transition(key)),
+            Inner::Shared(shape) => shape.change_attributes_transition(key),
+            Inner::Unique(shape) => shape.change_attributes_transition(key),
         }
     }
 

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

@@ -21,7 +21,7 @@ use self::{forward_transition::ForwardTransition, property_table::PropertyTable}
 
 use super::{
     slot::{SlotAttribute, SlotIndex},
-    Shape, Slot,
+    ChangeTransition, ChangeTransitionAction, Shape, Slot,
 };
 
 #[derive(Debug, Finalize, Clone, PartialEq, Eq, Hash)]
@@ -203,53 +203,87 @@ impl SharedShape {
         self.create_property_transition(key)
     }
 
-    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self {
+    /// Returns the change transition shape and a boolean to indicate if we should shift the storage.
+    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> ChangeTransition {
         let property_table = self.inner.property_table.deep_clone_all();
-        property_table.set_attributes(&key.property_key, key.attributes);
-        let new_inner_shape = Inner {
-            forward_transitions: ForwardTransition::default(),
-            prototype: self.prototype(),
-            property_table,
-            property_count: self.property_count(),
-            previous: Some(self.clone()),
-            transition_type: TransitionType::Configure,
-        };
-        let new_shape = Self::new(new_inner_shape);
 
-        self.forward_transitions()
-            .insert_property(key, &new_shape.inner);
+        let (index, slot) = property_table.get_expect(&key.property_key);
+
+        // The attribute change transitions, didn't change from accessor to data property or vice-versa.
+        //
+        // Both false, or both true.
+        if slot.attributes.is_accessor_descriptor() == key.attributes.is_accessor_descriptor() {
+            property_table.set_attributes_at_index(index, key.attributes);
+            let inner_shape = Inner {
+                forward_transitions: ForwardTransition::default(),
+                prototype: self.prototype(),
+                property_table,
+                property_count: self.property_count(),
+                previous: Some(self.clone()),
+                transition_type: TransitionType::Configure,
+            };
+            let shape = Self::new(inner_shape);
 
-        new_shape
-    }
+            self.forward_transitions()
+                .insert_property(key, &shape.inner);
 
-    // Rollback to the transition before the propery was inserted.
-    //
-    // For example with the following chain:
-    //
-    //        INSERT(x)             INSERT(y)                INSERT(z)
-    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
-    //
-    // Then we call delete on `y`. We rollback to before the property was added and we put
-    // the transitions in a array for reconstruction of the new branch:
-    //
-    //        INSERT(x)             INSERT(y)                INSERT(z)
-    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
-    //                       ^
-    //                       \--- base ( with array of transitions to be performed: INSERT(z) )
-    //
-    // Then we apply transitions (z):
-    //
-    //        INSERT(x)             INSERT(y)                INSERT(z)
-    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
-    //                       |
-    //                       |      INSERT(z)
-    //                       \---------------->  { x, z }  <----- The shape we return :)
-    //
-    pub(crate) fn remove_property_transition(&self, key: &PropertyKey) -> Self {
-        if Self::DEBUG {
-            println!("Shape: deleting {key}");
+            return ChangeTransition {
+                shape: Shape::shared(shape),
+                action: ChangeTransitionAction::Nothing,
+            };
+        }
+
+        /// Rollback before adding
+        let (mut base, prototype, transitions) = self.rollback_before(&key.property_key);
+
+        // Apply prototype transition, if it was found.
+        if let Some(prototype) = prototype {
+            base = base.change_prototype_transition(prototype);
         }
 
+        base = base.insert_property_transition(key);
+
+        for (property_key, attributes) in transitions.into_iter().rev() {
+            let transition = TransitionKey {
+                property_key,
+                attributes,
+            };
+            base = base.insert_property_transition(transition);
+        }
+
+        let action = if slot.attributes.is_accessor_descriptor() {
+            // Accessor property --> Data property
+            ChangeTransitionAction::Remove
+        } else {
+            // Data property --> Accessor property
+            ChangeTransitionAction::Insert
+        };
+
+        ChangeTransition {
+            shape: Shape::shared(base),
+            action,
+        }
+    }
+
+    /// Rollback to shape before the insertion of the [`PropertyKey`] that is provided.
+    ///
+    /// This returns the shape before the insertion, if it sees a prototype transition it will return the lastest one,
+    /// ignoring any others, [`None`] otherwise. It also will return the property transitions ordered from
+    /// latest to oldest that it sees.
+    ///
+    /// NOTE: In the transitions it does not include the property that we are rolling back.
+    ///
+    /// NOTE: The prototype transitions if it sees a property insert and then later an attribute change it will condense
+    /// into one property insert transition with the new attribute in the change attribute transition,
+    /// in the same place that the property was inserted initially.
+    fn rollback_before(
+        &self,
+        key: &PropertyKey,
+    ) -> (
+        SharedShape,
+        Option<JsPrototype>,
+        IndexMap<PropertyKey, SlotAttribute>,
+    ) {
         let mut prototype = None;
         let mut transitions: IndexMap<PropertyKey, SlotAttribute, RandomState> =
             IndexMap::default();
@@ -297,6 +331,39 @@ impl SharedShape {
             current = current_shape.previous();
         };
 
+        (base, prototype, transitions)
+    }
+
+    // Rollback to the transition before the propery was inserted.
+    //
+    // For example with the following chain:
+    //
+    //        INSERT(x)             INSERT(y)                INSERT(z)
+    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
+    //
+    // Then we call delete on `y`. We rollback to before the property was added and we put
+    // the transitions in a array for reconstruction of the new branch:
+    //
+    //        INSERT(x)             INSERT(y)                INSERT(z)
+    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
+    //                       ^
+    //                       \--- base ( with array of transitions to be performed: INSERT(z) )
+    //
+    // Then we apply transitions (z):
+    //
+    //        INSERT(x)             INSERT(y)                INSERT(z)
+    // { }  ------------>  { x }  ------------>  { x, y }  ------------>  { x, y, z }
+    //                       |
+    //                       |      INSERT(z)
+    //                       \---------------->  { x, z }  <----- The shape we return :)
+    //
+    pub(crate) fn remove_property_transition(&self, key: &PropertyKey) -> Self {
+        if Self::DEBUG {
+            println!("Shape: deleting {key}");
+        }
+
+        let (mut base, prototype, transitions) = self.rollback_before(key);
+
         // Apply prototype transition, if it was found.
         if let Some(prototype) = prototype {
             base = base.change_prototype_transition(prototype);

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

@@ -91,15 +91,19 @@ impl PropertyTable {
         }
     }
 
-    pub(crate) fn set_attributes(
-        &self,
-        property_key: &PropertyKey,
-        property_attributes: SlotAttribute,
-    ) {
+    pub(crate) fn set_attributes_at_index(&self, index: usize, property_attributes: SlotAttribute) {
+        let mut properties = self.inner.properties.borrow_mut();
+        let Some((_key, slot)) = properties.get_index_mut(index) else {
+            unreachable!("There should already be a property!")
+        };
+        slot.attributes = property_attributes;
+    }
+
+    pub(crate) fn get_expect(&self, key: &PropertyKey) -> (usize, Slot) {
         let mut properties = self.inner.properties.borrow_mut();
-        let Some(Slot { attributes, .. }) = properties.get_mut(property_key) else {
+        let Some((index, _key, slot)) = properties.get_full(key) else {
             unreachable!("There should already be a property!")
         };
-        *attributes = property_attributes;
+        (index, *slot)
     }
 }

boa_engine/src/object/shape/unique_shape.rs

@@ -17,7 +17,10 @@ use crate::{
     JsString,
 };
 
-use super::{shared_shape::TransitionKey, slot::SlotAttribute, JsPrototype, Slot};
+use super::{
+    shared_shape::TransitionKey, slot::SlotAttribute, ChangeTransition, ChangeTransitionAction,
+    JsPrototype, Shape, Slot,
+};
 
 /// TODO: doc
 #[derive(Default, Debug, Trace, Finalize)]
@@ -109,16 +112,57 @@ impl UniqueShape {
         None
     }
 
-    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> Self {
-        {
-            let mut property_table = self.inner.property_table.borrow_mut();
-            let Some(slot) = property_table.get_mut(&key.property_key) else {
-                unreachable!("Attribute change can only happen on existing property")
-            };
+    pub(crate) fn change_attributes_transition(&self, key: TransitionKey) -> ChangeTransition {
+        let mut property_table = self.inner.property_table.borrow_mut();
+        let Some(slot) = property_table.get_mut(&key.property_key) else {
+            unreachable!("Attribute change can only happen on existing property")
+        };
 
+        if slot.attributes.is_accessor_descriptor() == key.attributes.is_accessor_descriptor() {
             slot.attributes = key.attributes;
+            return ChangeTransition {
+                shape: Shape::unique(self.clone()),
+                action: ChangeTransitionAction::Nothing,
+            };
+        }
+
+        slot.attributes = key.attributes;
+
+        // TODO: maybe refactor into method
+        let action = if key.attributes.is_accessor_descriptor() {
+            // Data --> Accessor
+            ChangeTransitionAction::Insert
+        } else {
+            // Accessor --> Data
+            ChangeTransitionAction::Remove
+        };
+
+        let index = slot.index as usize;
+
+        // 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 mut property_table = std::mem::take(&mut *property_table);
+
+        // 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).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();
+        let shape = Self::new(prototype, property_table);
+
+        ChangeTransition {
+            shape: Shape::unique(shape),
+            action,
         }
-        self.clone()
     }
     pub(crate) fn change_prototype_transition(&self, prototype: JsPrototype) -> Self {
         let mut property_table = self.inner.property_table.borrow_mut();