refactor(ecmascript): cached_set install lookup on push (#820)

* refactor(ecmascript): set_cached install new lookup cache on push

* perf(ecmascript): Optimise CreateUnmappedArgumentsObject

* nit: dedupe PropertyStorage::add / PropertyStorage::push

Author: aapoalas

nova_vm/src/ecmascript/builtins/arguments.rs

@@ -26,26 +26,21 @@
 //!
 //! ECMAScript implementations of arguments exotic objects have historically contained an accessor property named "caller". Prior to ECMAScript 2017, this specification included the definition of a throwing "caller" property on ordinary arguments objects. Since implementations do not contain this extension any longer, ECMAScript 2017 dropped the requirement for a throwing "caller" accessor.
 
+use ahash::AHashMap;
+
 use crate::{
     ecmascript::{
-        abstract_operations::operations_on_objects::{
-            try_create_data_property_or_throw, try_define_property_or_throw,
-        },
-        execution::{ProtoIntrinsics, agent::Agent},
+        execution::agent::Agent,
         types::{
-            BUILTIN_STRING_MEMORY, IntoFunction, IntoValue, Number, Object, PropertyDescriptor,
-            PropertyKey,
+            BUILTIN_STRING_MEMORY, IntoFunction, IntoObject, IntoValue, Number, Object,
+            OrdinaryObject, Value,
         },
     },
-    engine::{
-        context::{Bindable, NoGcScope},
-        unwrap_try,
-    },
-    heap::WellKnownSymbolIndexes,
+    engine::context::{Bindable, NoGcScope},
+    heap::{WellKnownSymbolIndexes, element_array::ElementDescriptor},
 };
 
-use super::ScopedArgumentsList;
-use super::ordinary::ordinary_object_create_with_intrinsics;
+use super::{ScopedArgumentsList, ordinary::shape::ObjectShape};
 
 // 10.4.4.1 [[GetOwnProperty]] ( P )
 
@@ -132,99 +127,92 @@ pub(crate) fn create_unmapped_arguments_object<'a, 'b>(
 ) -> Object<'a> {
     // 1. Let len be the number of elements in argumentsList.
     let len = arguments_list.len(agent);
-    let len_value = Number::from_i64(agent, len as i64, gc)
-        .into_value()
-        .unbind();
+    // SAFETY: GC is not allowed in this scope, and no other scoped values are
+    // accessed during this call. The pointer is not held beyond the current call scope.
+    let arguments_non_null_slice = unsafe { arguments_list.as_non_null_slice(agent) };
+    debug_assert!(len < u32::MAX as usize);
+    let len = len as u32;
+    let len_value = Number::from(len).into_value();
     // 2. Let obj be OrdinaryObjectCreate(%Object.prototype%, « [[ParameterMap]] »).
-    let obj =
-        ordinary_object_create_with_intrinsics(agent, Some(ProtoIntrinsics::Object), None, gc);
-    let Object::Object(obj) = obj else {
-        unreachable!()
-    };
+    let prototype = agent.current_realm_record().intrinsics().object_prototype();
+    let mut shape = ObjectShape::get_shape_for_prototype(agent, Some(prototype.into_object()));
+    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.length.to_property_key());
+    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.callee.into());
+    shape = shape.get_child_shape(agent, WellKnownSymbolIndexes::Iterator.into());
+    for index in 0..len {
+        shape = shape.get_child_shape(agent, index.into());
+    }
+    let obj = OrdinaryObject::create_object_with_shape(agent, shape)
+        .expect("Failed to create Arguments object storage");
+    let array_prototype_values = agent
+        .current_realm_record()
+        .intrinsics()
+        .array_prototype_values()
+        .bind(gc)
+        .into_value();
+    let throw_type_error = agent
+        .current_realm_record()
+        .intrinsics()
+        .throw_type_error()
+        .into_function()
+        .bind(gc);
+    let storage = obj.get_elements_storage_uninit(agent);
+    let values = storage.values;
+    let descriptors = storage.descriptors.or_insert(AHashMap::with_capacity(3));
+
     // 3. Set obj.[[ParameterMap]] to undefined.
     // 4. Perform ! DefinePropertyOrThrow(obj, "length", PropertyDescriptor {
-    let key = PropertyKey::from(BUILTIN_STRING_MEMORY.length);
-    unwrap_try(try_define_property_or_throw(
-        agent,
-        obj,
-        key,
-        PropertyDescriptor {
-            // [[Value]]: 𝔽(len),
-            value: Some(len_value),
-            // [[Writable]]: true,
-            writable: Some(true),
-            // [[Enumerable]]: false,
-            enumerable: Some(false),
-            // [[Configurable]]: true }).
-            configurable: Some(true),
-            ..Default::default()
+    // [[Value]]: 𝔽(len),
+    // [[Writable]]: true,
+    // [[Enumerable]]: false,
+    // [[Configurable]]: true
+    // }).
+
+    // "length"
+    values[0] = Some(len_value.unbind());
+    // "callee"
+    values[1] = None;
+    // Iterator
+    values[2] = Some(array_prototype_values.unbind());
+    // "length"
+    descriptors.insert(0, ElementDescriptor::WritableUnenumerableConfigurableData);
+    // "callee"
+    descriptors.insert(
+        1,
+        ElementDescriptor::ReadWriteUnenumerableUnconfigurableAccessor {
+            get: throw_type_error.unbind(),
+            set: throw_type_error.unbind(),
         },
-        gc,
-    ))
-    .unwrap();
+    );
+    // Iterator
+    descriptors.insert(2, ElementDescriptor::WritableUnenumerableConfigurableData);
     // 5. Let index be 0.
     // 6. Repeat, while index < len,
     for index in 0..len {
         // a. Let val be argumentsList[index].
         // b. Perform ! CreateDataPropertyOrThrow(obj, ! ToString(𝔽(index)), val).
-        debug_assert!(index < u32::MAX as usize);
-        let index = index as u32;
-        let key = PropertyKey::Integer(index.into());
-        let val = arguments_list.get(agent, index, gc);
-        unwrap_try(try_create_data_property_or_throw(agent, obj, key, val, gc)).unwrap();
+        // SAFETY: arguments slice valid in this call stack and we've not
+        // performed GC or touched other scoped data.
+        let val = unsafe { arguments_non_null_slice.as_ref() }
+            .get(index as usize)
+            .cloned()
+            .unwrap_or(Value::Undefined);
+        values[index as usize + 3] = Some(val);
         // c. Set index to index + 1.
     }
+    agent[obj].set_len(len + 3);
     // 7. Perform ! DefinePropertyOrThrow(obj, @@iterator, PropertyDescriptor {
-    let key = PropertyKey::Symbol(WellKnownSymbolIndexes::Iterator.into());
-    unwrap_try(try_define_property_or_throw(
-        agent,
-        obj,
-        key,
-        PropertyDescriptor {
-            // [[Value]]: %Array.prototype.values%,
-            value: Some(
-                agent
-                    .current_realm_record()
-                    .intrinsics()
-                    .array_prototype_values()
-                    .into_value(),
-            ),
-            // [[Writable]]: true,
-            writable: Some(true),
-            // [[Enumerable]]: false,
-            enumerable: Some(false),
-            // [[Configurable]]: true }).
-            configurable: Some(true),
-            ..Default::default()
-        },
-        gc,
-    ))
-    .unwrap();
-    let throw_type_error = agent
-        .current_realm_record()
-        .intrinsics()
-        .throw_type_error()
-        .bind(gc);
+    // [[Value]]: %Array.prototype.values%,
+    // [[Writable]]: true,
+    // [[Enumerable]]: false,
+    // [[Configurable]]: true
+    // }).
     // 8. Perform ! DefinePropertyOrThrow(obj, "callee", PropertyDescriptor {
-    let key = PropertyKey::from(BUILTIN_STRING_MEMORY.callee);
-    unwrap_try(try_define_property_or_throw(
-        agent,
-        obj,
-        key,
-        PropertyDescriptor {
-            // [[Get]]: %ThrowTypeError%,
-            get: Some(Some(throw_type_error.into_function())),
-            // [[Set]]: %ThrowTypeError%,
-            set: Some(Some(throw_type_error.into_function())),
-            // [[Enumerable]]: false,
-            enumerable: Some(false),
-            // [[Configurable]]: false }).
-            configurable: Some(false),
-            ..Default::default()
-        },
-        gc,
-    ))
-    .unwrap();
+    // [[Get]]: %ThrowTypeError%,
+    // [[Set]]: %ThrowTypeError%,
+    // [[Enumerable]]: false,
+    // [[Configurable]]: false
+    // }).
     // 9. Return obj.
     Object::Arguments(obj)
 }

nova_vm/src/ecmascript/builtins/array.rs

@@ -26,7 +26,7 @@ use crate::{
         types::{
             BUILTIN_STRING_MEMORY, Function, GetCachedResult, InternalMethods, InternalSlots,
             IntoFunction, IntoObject, IntoValue, NoCache, Object, OrdinaryObject,
-            PropertyDescriptor, PropertyKey, SetCachedResult, Value,
+            PropertyDescriptor, PropertyKey, SetCachedProps, SetCachedResult, Value,
         },
     },
     engine::{
@@ -53,7 +53,6 @@ use super::{
     ordinary::{
         caches::PropertyLookupCache, ordinary_delete, ordinary_get, ordinary_get_own_property,
         ordinary_has_property, ordinary_try_get, ordinary_try_has_property,
-        shape::ShapeSetCachedProps,
     },
 };
 
@@ -926,20 +925,17 @@ impl<'a> InternalMethods<'a> for Array<'a> {
     fn set_cached<'gc>(
         self,
         agent: &mut Agent,
-        p: PropertyKey,
-        value: Value,
-        receiver: Value,
-        cache: PropertyLookupCache,
+        props: &SetCachedProps,
         gc: NoGcScope<'gc, '_>,
     ) -> ControlFlow<SetCachedResult<'gc>, NoCache> {
         // Cached set of an Array should return directly mutate the Array's
         // internal memory if it can.
-        if p == BUILTIN_STRING_MEMORY.length.to_property_key() {
+        if props.p == BUILTIN_STRING_MEMORY.length.to_property_key() {
             // Length lookup: we find it always.
             if !self.length_writable(agent) {
                 return SetCachedResult::Unwritable.into();
             }
-            if let Value::Integer(value) = value
+            if let Value::Integer(value) = props.value
                 && let Ok(value) = u32::try_from(value.into_i64())
             {
                 let Ok(result) = array_set_length_handling(agent, self, value, None, None, None)
@@ -955,7 +951,7 @@ impl<'a> InternalMethods<'a> for Array<'a> {
             } else {
                 return NoCache.into();
             }
-        } else if let Some(index) = p.into_u32() {
+        } else if let Some(index) = props.p.into_u32() {
             // Indexed lookup: check our slice. First bounds-check.
             if !(0..self.len(agent)).contains(&index) {
                 // We're out of bounds; this need prototype lookups.
@@ -990,7 +986,7 @@ impl<'a> InternalMethods<'a> for Array<'a> {
             // dealing with.
             if let Some(slot) = &mut storage.values[index as usize] {
                 // Writable data property it is! Set its value.
-                *slot = value.unbind();
+                *slot = props.value.unbind();
                 return SetCachedResult::Done.into();
             }
             // Hole! We'll just return NoCache to signify that we can't be
@@ -1000,17 +996,7 @@ impl<'a> InternalMethods<'a> for Array<'a> {
         // If this was a non-Array index or a named property on the Array then
         // we want to perform a normal cached set with the Array's shape.
         let shape = self.object_shape(agent);
-        shape.set_cached(
-            agent,
-            ShapeSetCachedProps {
-                o: self.into_object(),
-                p,
-                receiver,
-            },
-            value,
-            cache,
-            gc,
-        )
+        shape.set_cached(agent, self.into_object(), props, gc)
     }
 }
 

nova_vm/src/ecmascript/builtins/bound_function.rs

@@ -15,8 +15,8 @@ use crate::{
         types::{
             BoundFunctionHeapData, Function, FunctionInternalProperties, GetCachedResult,
             InternalMethods, InternalSlots, IntoFunction, IntoValue, NoCache, Object,
-            OrdinaryObject, PropertyDescriptor, PropertyKey, SetCachedResult, String, Value,
-            function_create_backing_object, function_get_cached,
+            OrdinaryObject, PropertyDescriptor, PropertyKey, SetCachedProps, SetCachedResult,
+            String, Value, function_create_backing_object, function_get_cached,
             function_internal_define_own_property, function_internal_delete, function_internal_get,
             function_internal_get_own_property, function_internal_has_property,
             function_internal_own_property_keys, function_internal_set, function_set_cached,
@@ -307,13 +307,10 @@ impl<'a> InternalMethods<'a> for BoundFunction<'a> {
     fn set_cached<'gc>(
         self,
         agent: &mut Agent,
-        p: PropertyKey,
-        value: Value,
-        receiver: Value,
-        cache: PropertyLookupCache,
+        props: &SetCachedProps,
         gc: NoGcScope<'gc, '_>,
     ) -> ControlFlow<SetCachedResult<'gc>, NoCache> {
-        function_set_cached(self, agent, p, value, receiver, cache, gc)
+        function_set_cached(self, agent, props, gc)
     }
 
     /// ### [10.4.1.1 \[\[Call\]\] ( thisArgument, argumentsList )](https://tc39.es/ecma262/#sec-bound-function-exotic-objects-call-thisargument-argumentslist)

nova_vm/src/ecmascript/builtins/builtin_constructor.rs

@@ -21,7 +21,7 @@ use crate::{
             BUILTIN_STRING_MEMORY, BuiltinConstructorHeapData, Function,
             FunctionInternalProperties, GetCachedResult, InternalMethods, InternalSlots,
             IntoFunction, IntoObject, IntoValue, NoCache, Object, OrdinaryObject,
-            PropertyDescriptor, PropertyKey, SetCachedResult, String, Value,
+            PropertyDescriptor, PropertyKey, SetCachedProps, SetCachedResult, String, Value,
             function_create_backing_object, function_get_cached,
             function_internal_define_own_property, function_internal_delete, function_internal_get,
             function_internal_get_own_property, function_internal_has_property,
@@ -316,13 +316,10 @@ impl<'a> InternalMethods<'a> for BuiltinConstructorFunction<'a> {
     fn set_cached<'gc>(
         self,
         agent: &mut Agent,
-        p: PropertyKey,
-        value: Value,
-        receiver: Value,
-        cache: PropertyLookupCache,
+        props: &SetCachedProps,
         gc: NoGcScope<'gc, '_>,
     ) -> ControlFlow<SetCachedResult<'gc>, NoCache> {
-        function_set_cached(self, agent, p, value, receiver, cache, gc)
+        function_set_cached(self, agent, props, gc)
     }
 
     /// ### [10.3.1 \[\[Call\]\] ( thisArgument, argumentsList )](https://tc39.es/ecma262/#sec-built-in-function-objects-call-thisargument-argumentslist)

nova_vm/src/ecmascript/builtins/builtin_function.rs

@@ -17,11 +17,12 @@ use crate::{
             BUILTIN_STRING_MEMORY, BuiltinFunctionHeapData, Function, FunctionInternalProperties,
             GetCachedResult, InternalMethods, InternalSlots, IntoFunction, IntoObject, IntoValue,
             NoCache, Object, OrdinaryObject, PropertyDescriptor, PropertyKey, ScopedValuesIterator,
-            SetCachedResult, String, Value, function_create_backing_object, function_get_cached,
-            function_internal_define_own_property, function_internal_delete, function_internal_get,
-            function_internal_get_own_property, function_internal_has_property,
-            function_internal_own_property_keys, function_internal_set, function_set_cached,
-            function_try_get, function_try_has_property, function_try_set,
+            SetCachedProps, SetCachedResult, String, Value, function_create_backing_object,
+            function_get_cached, function_internal_define_own_property, function_internal_delete,
+            function_internal_get, function_internal_get_own_property,
+            function_internal_has_property, function_internal_own_property_keys,
+            function_internal_set, function_set_cached, function_try_get,
+            function_try_has_property, function_try_set,
         },
     },
     engine::{
@@ -342,6 +343,26 @@ impl<'scope> ScopedArgumentsList<'scope> {
             unreachable!()
         }
     }
+
+    /// Get access to the backing reference slice as a pointer slice.
+    ///
+    /// ## Safety
+    ///
+    /// Garbage collection must not be called while this slice is exposed.
+    ///
+    /// Stack values must not be accessed while this slice is exposed.
+    pub(crate) unsafe fn as_non_null_slice(&self, agent: &Agent) -> NonNull<[Value<'static>]> {
+        if let HeapRootCollectionData::ArgumentsList(args) = agent
+            .stack_ref_collections
+            .borrow()
+            .get(self.index as usize)
+            .unwrap()
+        {
+            *args
+        } else {
+            unreachable!()
+        }
+    }
 }
 
 impl core::fmt::Debug for ScopedArgumentsList<'_> {
@@ -684,13 +705,10 @@ impl<'a> InternalMethods<'a> for BuiltinFunction<'a> {
     fn set_cached<'gc>(
         self,
         agent: &mut Agent,
-        p: PropertyKey,
-        value: Value,
-        receiver: Value,
-        cache: PropertyLookupCache,
+        props: &SetCachedProps,
         gc: NoGcScope<'gc, '_>,
     ) -> ControlFlow<SetCachedResult<'gc>, NoCache> {
-        function_set_cached(self, agent, p, value, receiver, cache, gc)
+        function_set_cached(self, agent, props, gc)
     }
 
     /// ### [10.3.1 \[\[Call\]\] ( thisArgument, argumentsList )](https://tc39.es/ecma262/#sec-built-in-function-objects-call-thisargument-argumentslist)