Skip to content

Commit

Permalink
Dense/Packed JavaScript arrays (#2167)
Browse files Browse the repository at this point in the history
This PR implements an optimization done by V8 and spidermonkey. Which stores indexed properties in two class storage methods dense and sparse. The dense method stores the property in a contiguous array ( `Vec<T>` ) where the index is the property key. This storage method is the default. While on the other hand we have sparse storage method which stores them in a hash map with key `u32` (like we currently do). This storage method is a backup and is slower to access because we have to do a hash map lookup, instead of an array access.

In the dense array we can store only data descriptors that have a value field and are `writable`, `configurable` and `enumerable` (which by default array elements are). Since all the fields of the property descriptor are the same except value field, we can omit them an just store the `JsValue`s in `Vec<JsValue>` this decreases the memory consumption and because it is smaller we are less likely to have cache misses.

There are cases where we have to convert from dense to sparse (the slow case):
- If we insert index elements in a non-incremental way, like `array[1000] = 1000` (inserting an element to an index that is already occupied only replaces it does not make it sparse)
- If we delete from the middle of the array (making a hole), like `delete array[10]`  (only converts if there is actualy an element there, so calling delete on a non-existent index property will do nothing)

Once it becomes sparse is *stays* sparse there is no way to convert it again. (the computation needed to check whether it can be converted outweighs the benefits of this optimization)

I did some local benchmarks and on array creation/pop and push there is ~45% speed up and the impact _should_ be bigger the more elements we have. For example the code below on `main` takes `~21.5s` while with this optimization is `~3.5s` (both on release build).

```js
let array = [];
for (let i = 0; i < 50000; ++i) {
   array[i] = i;
}
```

In addition I also made `Array::create_array_from_list` do a direct setting of the properties (small deviation from spec but it should have the same behaviour), with this #2058 should be fixed, conversion from `Vec` to `JsArray`, not `JsTypedArray` for that I will work on next :)
  • Loading branch information
HalidOdat committed Jul 6, 2022
1 parent d0d7034 commit 5bbc225
Show file tree
Hide file tree
Showing 9 changed files with 313 additions and 55 deletions.
22 changes: 15 additions & 7 deletions boa_engine/src/builtins/array/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,23 @@ impl Array {
// 2. Let array be ! ArrayCreate(0).
let array = Self::array_create(0, None, context)
.expect("creating an empty array with the default prototype must not fail");

// 3. Let n be 0.
// 4. For each element e of elements, do
for (i, elem) in elements.into_iter().enumerate() {
// a. Perform ! CreateDataPropertyOrThrow(array, ! ToString(𝔽(n)), e).
array
.create_data_property_or_throw(i, elem, context)
.expect("new array must be extensible");
// b. Set n to n + 1.
}
// a. Perform ! CreateDataPropertyOrThrow(array, ! ToString(𝔽(n)), e).
// b. Set n to n + 1.
//
// NOTE: This deviates from the spec, but it should have the same behaviour.
let elements: Vec<_> = elements.into_iter().collect();
let length = elements.len();
array
.borrow_mut()
.properties_mut()
.override_indexed_properties(elements);
array
.set("length", length, true, context)
.expect("Should not fail");

// 5. Return array.
array
}
Expand Down
3 changes: 1 addition & 2 deletions boa_engine/src/object/internal_methods/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,7 @@ fn array_set_length(
.borrow()
.properties
.index_property_keys()
.filter(|idx| new_len <= **idx && **idx < u32::MAX)
.copied()
.filter(|idx| new_len <= *idx && *idx < u32::MAX)
.collect();
keys.sort_unstable_by(|x, y| y.cmp(x));
keys
Expand Down
6 changes: 3 additions & 3 deletions boa_engine/src/object/internal_methods/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub(crate) fn global_get_own_property(
// 7. Set D.[[Enumerable]] to the value of X's [[Enumerable]] attribute.
// 8. Set D.[[Configurable]] to the value of X's [[Configurable]] attribute.
// 9. Return D.
Ok(context.realm.global_property_map.get(key).cloned())
Ok(context.realm.global_property_map.get(key))
}

/// Abstract operation `OrdinaryIsExtensible`.
Expand Down Expand Up @@ -202,7 +202,7 @@ pub(crate) fn global_set_no_receiver(
// https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinarysetwithowndescriptor

// 1. Assert: IsPropertyKey(P) is true.
let own_desc = if let Some(desc) = context.realm.global_property_map.get(key).cloned() {
let own_desc = if let Some(desc) = context.realm.global_property_map.get(key) {
desc
}
// c. Else,
Expand Down Expand Up @@ -250,7 +250,7 @@ pub(crate) fn global_set_no_receiver(
};

// 1. Let current be ? O.[[GetOwnProperty]](P).
let current = context.realm.global_property_map.get(key).cloned();
let current = context.realm.global_property_map.get(key);

// 2. Let extensible be ? IsExtensible(O).
let extensible = context.realm.global_extensible;
Expand Down
9 changes: 2 additions & 7 deletions boa_engine/src/object/internal_methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ pub(crate) fn ordinary_get_own_property(
// 7. Set D.[[Enumerable]] to the value of X's [[Enumerable]] attribute.
// 8. Set D.[[Configurable]] to the value of X's [[Configurable]] attribute.
// 9. Return D.
Ok(obj.borrow().properties.get(key).cloned())
Ok(obj.borrow().properties.get(key))
}

/// Abstract operation `OrdinaryDefineOwnProperty`.
Expand Down Expand Up @@ -725,12 +725,7 @@ pub(crate) fn ordinary_own_property_keys(
let mut keys = Vec::new();

let ordered_indexes = {
let mut indexes: Vec<_> = obj
.borrow()
.properties
.index_property_keys()
.copied()
.collect();
let mut indexes: Vec<_> = obj.borrow().properties.index_property_keys().collect();
indexes.sort_unstable();
indexes
};
Expand Down
1 change: 0 additions & 1 deletion boa_engine/src/object/internal_methods/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ pub(crate) fn string_exotic_own_property_keys(
let mut remaining_indices: Vec<_> = obj
.properties
.index_property_keys()
.copied()
.filter(|idx| (*idx as usize) >= len)
.collect();
remaining_indices.sort_unstable();
Expand Down
5 changes: 5 additions & 0 deletions boa_engine/src/object/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1317,6 +1317,11 @@ impl Object {
&self.properties
}

#[inline]
pub(crate) fn properties_mut(&mut self) -> &mut PropertyMap {
&mut self.properties
}

/// Inserts a field in the object `properties` without checking if it's writable.
///
/// If a field was already in the object with the same name, then a `Some` is returned
Expand Down
Loading

0 comments on commit 5bbc225

Please sign in to comment.