Skip to content
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

Optimize number to PropertyKey conversion #3769

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

HalidOdat
Copy link
Member

In previous implementation when converting a number to PropertyKey, we always converted it to string and only when the conversion to PropertyKey::Index was not possible we returned that string.

Instead of using map_or which expects a ready value we use the _else version that takes a function. Also took the oportunity to optimize the i32 to PropertyKey conversion, since it's the one that's called when we convert from JsValue::Integer. Additionaly added a static string for "length" for internal array methods so we don't convert from &[u16] to PropertyKey.

Because the conversion happens very often this change resulted in big performance improvements :)

Benchmarks

Main

PROGRESS Richards
RESULT Richards 57.0
PROGRESS DeltaBlue
RESULT DeltaBlue 61.6
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 62.7
PROGRESS RayTrace
RESULT RayTrace 188
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 176
PROGRESS Splay
RESULT Splay 185
PROGRESS NavierStokes
RESULT NavierStokes 117
SCORE 107

PR

PROGRESS Richards
RESULT Richards 58.8
PROGRESS DeltaBlue
RESULT DeltaBlue 63.7
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 71.7
PROGRESS RayTrace
RESULT RayTrace 198
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 182
PROGRESS Splay
RESULT Splay 193
PROGRESS NavierStokes
RESULT NavierStokes 158
SCORE 117

In previous implementation when converting a number to PropertyKey, we
always converted it to string and only when the conversion to
PropertyKey::Index was not possible we returned that string.
@HalidOdat HalidOdat added performance Performance related changes and issues Internal Category for changelog labels Mar 26, 2024
@HalidOdat HalidOdat requested a review from a team March 26, 2024 17:43
@HalidOdat HalidOdat added this to the v0.18.1 milestone Mar 26, 2024
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,268 50,268 0
Passed 42,773 42,773 0
Ignored 1,391 1,391 0
Failed 6,104 6,104 0
Panics 18 18 0
Conformance 85.09% 85.09% 0.00%

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice find!

@raskad raskad requested a review from a team March 26, 2024 18:09
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@HalidOdat HalidOdat added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit a1e36fc Mar 26, 2024
13 checks passed
@raskad raskad deleted the optimization/property-key-creation branch March 26, 2024 18:48
@raskad raskad modified the milestones: v0.18.1, v0.19.0 Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants