-
-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dense array storage variants for i32
and f64
#3760
Conversation
Test262 conformance changes
Fixed tests (1):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice optimization!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I just have some suggestions which are just nitpicks, but I'm approving anyways since those aren't essential per se.
This PR adds dense variants for array storage (
i32
andf64
) this is mostly a memory optimization, this optimization is also implemented by v8 (they have even more granular types like sparsei32
andf64
variants).Storing number
i32
numbers in a dense array now takes6x
less memory and3x
less memory forf64
(sinceJsValue
is 24 bytes)This PR also applies the changes in #3744 , because with this PR it complicates that fast path too much. Since the patch introduces a regression for setting new properties in arrays, this PR amortizes the regression, it's pretty much the same with the current performance in quickjs benchmarks.
Even though I implemented that fast path, I was never happy with it 😅 (because it has too much duplicate logic), it was a quick way to improve performance, but I think the better option is to improve the internal object methods, which should give overall better performance.