Skip to content

Remove Box<T> from JsValue for JsString#4329

Merged
HalidOdat merged 3 commits intomainfrom
optimization/non-box-string-jsvalue
Jul 10, 2025
Merged

Remove Box<T> from JsValue for JsString#4329
HalidOdat merged 3 commits intomainfrom
optimization/non-box-string-jsvalue

Conversation

@HalidOdat
Copy link
Member

Remove Box<T> wrapper of JsString in nan-boxed JsValue, this requires us to return a clone of a JsString (reference bump) for JsValue::as_string().

@HalidOdat HalidOdat added A-Performance Performance related changes and issues A-Memory PRs and Issues related to the memory management or memory footprint. A-Internal Changes that don't modify execution behaviour labels Jul 10, 2025
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,595 50,595 0
Passed 47,238 47,238 0
Ignored 1,964 1,964 0
Failed 1,393 1,393 0
Panics 0 0 0
Conformance 93.36% 93.36% 0.00%

@HalidOdat
Copy link
Member Author

Benchmark

main

PROGRESS Richards
RESULT Richards 177
PROGRESS DeltaBlue
RESULT DeltaBlue 180
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 145
PROGRESS RayTrace
RESULT RayTrace 391
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 465
PROGRESS RegExp
RESULT RegExp 73.2
PROGRESS Splay
RESULT Splay 706
PROGRESS NavierStokes
RESULT NavierStokes 345
SCORE 249

PR

PROGRESS Richards
RESULT Richards 180
PROGRESS DeltaBlue
RESULT DeltaBlue 182
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 150
PROGRESS RayTrace
RESULT RayTrace 388
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 462
PROGRESS RegExp
RESULT RegExp 74.5
PROGRESS Splay
RESULT Splay 742
PROGRESS NavierStokes
RESULT NavierStokes 351
SCORE 254

@HalidOdat HalidOdat force-pushed the optimization/non-box-string-jsvalue branch from 86aa77d to 3c12a07 Compare July 10, 2025 16:45
@HalidOdat HalidOdat force-pushed the optimization/non-box-string-jsvalue branch from 3c12a07 to 0480d7a Compare July 10, 2025 16:56
@HalidOdat HalidOdat requested a review from a team July 10, 2025 17:20
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Nice optimization! LGTM.

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.

I have not looked into this in detail, but do we potentially run into issues here because JsString is a Tagged pointer?

@hansl
Copy link
Contributor

hansl commented Jul 10, 2025

do we potentially run into issues here because JsString is a Tagged pointer

@raskad NaN-boxed pointers are not tagged on the LSB (which is what JsString uses). We moved away from this early in the development of NaN-boxing values. Storing a LSB-tagged pointer is "safe".

We have enough tests around it that I'm confident if we ever break this assumption we'll assert quite quickly.

@HalidOdat
Copy link
Member Author

Yes, as @hansl mentioned, it's not a issue, and we have more than enough space to store the tag. 😄

Co-authored-by: raskad <32105367+raskad@users.noreply.github.com>
@HalidOdat HalidOdat enabled auto-merge July 10, 2025 19:16
@HalidOdat HalidOdat added this pull request to the merge queue Jul 10, 2025
Merged via the queue into main with commit e51fcd7 Jul 10, 2025
15 checks passed
@HalidOdat HalidOdat deleted the optimization/non-box-string-jsvalue branch July 10, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Internal Changes that don't modify execution behaviour A-Memory PRs and Issues related to the memory management or memory footprint. A-Performance Performance related changes and issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants