[Merged by Bors] - Dense/Packed JavaScript arrays #2167
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HalidOdat
added
performance
Test262 conformance changesVM implementation
|
Codecov Report
@@ Coverage Diff @@
## main #2167 +/- ##
==========================================
- Coverage 42.32% 42.28% -0.05%
==========================================
Files 228 228
Lines 21047 21176 +129
==========================================
+ Hits 8909 8954 +45
- Misses 12138 12222 +84
Continue to review full report at Codecov.
|
HalidOdat
requested review from
jedel1043,
Razican,
raskad,
jasonwilliams and
RageKnify
jedel1043
requested changes
Jul 6, 2022
- Optimized `Array::create_array_from_list()`
Benchmark for 7e7af8eClick to view benchmark
|
Benchmark for 3f569d2Click to view benchmark
|
raskad
approved these changes
Jul 6, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jul 6, 2022
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 :)
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Aug 17, 2022
Currently we only spread spread-expressions if they are the last argument in the function call. With this fix all arguments are spread if needed. The downside is that an array object is allocated to store all arguments if the arguments contain a spread-expression. But with dense indexed properties inplemented in #2167 this should be reasonably fast.
20 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 keyu32(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,configurableandenumerable(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 theJsValues inVec<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):
array[1000] = 1000(inserting an element to an index that is already occupied only replaces it does not make it sparse)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
maintakes~21.5swhile with this optimization is~3.5s(both on release build).In addition I also made
Array::create_array_from_listdo a direct setting of the properties (small deviation from spec but it should have the same behaviour), with this #2058 should be fixed, conversion fromVectoJsArray, notJsTypedArrayfor that I will work on next :)