From 1f4ff6d379dca5ab0c5c346f6546a182335d1ff1 Mon Sep 17 00:00:00 2001 From: Haled Odat Date: Tue, 25 Apr 2023 22:25:57 +0000 Subject: [PATCH] Direct array element access on `ByValue` instructions (#2827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of the time that we have a `ByValue` ( `[ value ]` syntax ) it is for arrays and the value is usually an index. This PR adds a fast path to the instructions (without calling `.borrow()` on the object to check if its an array) For example, this code: ```js let a = [1, 2, 3] for (let i = 0 ; i < 10000000; ++i) { a[i % 3] += a[ (i + 1) % 3 ] } ``` Using `hyperfine`, it ran `1.38` times faster on this PR. ```bash Benchmark 1: ./boa_main test.js Time (mean ± σ): 16.504 s ± 0.192 s [User: 16.440 s, System: 0.020 s] Range (min … max): 16.328 s … 16.938 s 10 runs Benchmark 2: ./boa_direct_array_access test.js Time (mean ± σ): 11.982 s ± 0.038 s [User: 11.939 s, System: 0.013 s] Range (min … max): 11.914 s … 12.035 s 10 runs Summary './boa_direct_array_access test.js' ran 1.38 ± 0.02 times faster than './boa_main test.js' ``` --- boa_engine/src/object/internal_methods/mod.rs | 2 + boa_engine/src/object/jsobject.rs | 9 +-- boa_engine/src/object/property_map.rs | 9 +++ boa_engine/src/vm/opcode/get/property.rs | 35 +++++++++ boa_engine/src/vm/opcode/set/property.rs | 72 ++++++++++++++++++- 5 files changed, 120 insertions(+), 7 deletions(-) diff --git a/boa_engine/src/object/internal_methods/mod.rs b/boa_engine/src/object/internal_methods/mod.rs index 9511173c07..9079630a00 100644 --- a/boa_engine/src/object/internal_methods/mod.rs +++ b/boa_engine/src/object/internal_methods/mod.rs @@ -23,6 +23,8 @@ pub(super) mod integer_indexed; pub(super) mod proxy; pub(super) mod string; +pub(crate) use array::ARRAY_EXOTIC_INTERNAL_METHODS; + impl JsObject { /// Internal method `[[GetPrototypeOf]]` /// diff --git a/boa_engine/src/object/jsobject.rs b/boa_engine/src/object/jsobject.rs index b187dc1857..9e7d3d8bfc 100644 --- a/boa_engine/src/object/jsobject.rs +++ b/boa_engine/src/object/jsobject.rs @@ -3,7 +3,8 @@ //! The `JsObject` is a garbage collected Object. use super::{ - internal_methods::InternalObjectMethods, JsPrototype, NativeObject, Object, PropertyMap, + internal_methods::{InternalObjectMethods, ARRAY_EXOTIC_INTERNAL_METHODS}, + JsPrototype, NativeObject, Object, PropertyMap, }; use crate::{ context::intrinsics::Intrinsics, @@ -338,14 +339,10 @@ impl JsObject { } /// Checks if it's an `Array` object. - /// - /// # Panics - /// - /// Panics if the object is currently mutably borrowed. #[inline] #[track_caller] pub fn is_array(&self) -> bool { - self.borrow().is_array() + std::ptr::eq(self.vtable(), &ARRAY_EXOTIC_INTERNAL_METHODS) } /// Checks if it's a `DataView` object. diff --git a/boa_engine/src/object/property_map.rs b/boa_engine/src/object/property_map.rs index 228f07a3ab..374bd7e221 100644 --- a/boa_engine/src/object/property_map.rs +++ b/boa_engine/src/object/property_map.rs @@ -287,6 +287,15 @@ impl PropertyMap { } } + /// Returns the vec of dense indexed properties if they exist. + pub(crate) fn dense_indexed_properties_mut(&mut self) -> Option<&mut ThinVec> { + if let IndexedProperties::Dense(properties) = &mut self.indexed_properties { + Some(properties) + } else { + None + } + } + /// An iterator visiting all key-value pairs in arbitrary order. The iterator element type is `(PropertyKey, &'a Property)`. /// /// This iterator does not recurse down the prototype chain. diff --git a/boa_engine/src/vm/opcode/get/property.rs b/boa_engine/src/vm/opcode/get/property.rs index b5de8900fb..16910dccb4 100644 --- a/boa_engine/src/vm/opcode/get/property.rs +++ b/boa_engine/src/vm/opcode/get/property.rs @@ -56,6 +56,23 @@ impl Operation for GetPropertyByValue { }; let key = key.to_property_key(context)?; + + // Fast Path + if object.is_array() { + if let PropertyKey::Index(index) = &key { + let object_borrowed = object.borrow(); + if let Some(element) = object_borrowed + .properties() + .dense_indexed_properties() + .and_then(|vec| vec.get(*index as usize)) + { + context.vm.push(element.clone()); + return Ok(CompletionType::Normal); + } + } + } + + // Slow path: let result = object.__get__(&key, value, context)?; context.vm.push(result); @@ -110,6 +127,24 @@ impl Operation for GetPropertyByValuePush { }; let key = key.to_property_key(context)?; + + // Fast path: + if object.is_array() { + if let PropertyKey::Index(index) = &key { + let object_borrowed = object.borrow(); + if let Some(element) = object_borrowed + .properties() + .dense_indexed_properties() + .and_then(|vec| vec.get(*index as usize)) + { + context.vm.push(key); + context.vm.push(element.clone()); + return Ok(CompletionType::Normal); + } + } + } + + // Slow path: let result = object.__get__(&key, value, context)?; context.vm.push(key); diff --git a/boa_engine/src/vm/opcode/set/property.rs b/boa_engine/src/vm/opcode/set/property.rs index 948e79c7c9..3339457b87 100644 --- a/boa_engine/src/vm/opcode/set/property.rs +++ b/boa_engine/src/vm/opcode/set/property.rs @@ -1,3 +1,5 @@ +use boa_macros::utf16; + use crate::{ builtins::function::set_function_name, property::{PropertyDescriptor, PropertyKey}, @@ -31,7 +33,6 @@ impl Operation for SetPropertyByName { let name = context.vm.frame().code_block.names[index as usize]; let name: PropertyKey = context.interner().resolve_expect(name.sym()).utf16().into(); - //object.set(name, value.clone(), context.vm.frame().code.strict, context)?; let succeeded = object.__set__(name.clone(), value.clone(), receiver, context)?; if !succeeded && context.vm.frame().code_block.strict { return Err(JsNativeError::typ() @@ -65,6 +66,75 @@ impl Operation for SetPropertyByValue { }; let key = key.to_property_key(context)?; + + // Fast Path: + 'fast_path: { + if object.is_array() { + if let PropertyKey::Index(index) = &key { + let mut object_borrowed = object.borrow_mut(); + + // Cannot modify if not extensible. + if !object_borrowed.extensible { + break 'fast_path; + } + + let prototype = object_borrowed.prototype().clone(); + + if let Some(dense_elements) = object_borrowed + .properties_mut() + .dense_indexed_properties_mut() + { + let index = *index as usize; + if let Some(element) = dense_elements.get_mut(index) { + *element = value; + context.vm.push(element.clone()); + return Ok(CompletionType::Normal); + } else if dense_elements.len() == index { + // Cannot use fast path if the [[prototype]] is a proxy object, + // because we have to the call prototypes [[set]] on non-existing property, + // and proxy objects can override [[set]]. + if prototype.map_or(false, |x| x.is_proxy()) { + break 'fast_path; + } + + dense_elements.push(value.clone()); + context.vm.push(value); + + let len = dense_elements.len() as u32; + let length_key = PropertyKey::from(utf16!("length")); + let length = object_borrowed + .properties_mut() + .get(&length_key) + .expect("Arrays must have length property"); + + if length.expect_writable() { + // We have to get the max of previous length and len(dense_elements) + 1, + // this is needed if user spacifies `new Array(n)` then adds properties from 0, 1, etc. + let len = length + .expect_value() + .to_u32(context) + .expect("length should have a u32 value") + .max(len); + object_borrowed.insert( + length_key, + PropertyDescriptor::builder() + .value(len) + .writable(true) + .enumerable(length.expect_enumerable()) + .configurable(false) + .build(), + ); + } else if context.vm.frame().code_block.strict { + return Err(JsNativeError::typ().with_message("TypeError: Cannot assign to read only property 'length' of array object").into()); + } + return Ok(CompletionType::Normal); + } + } + } + } + } + + // Slow path: object.set( key, value.clone(),