boa icon indicating copy to clipboard operation
boa copied to clipboard

Implement WIP Ascii JsString

Open HalidOdat opened this issue 1 year ago • 2 comments

This fixes #3097

This is a very WIP implementation, but feedback is welcomed :)

This PR encodes strings that can be represented as ASCII as a byte array, instead of u16 array, this cuts ASCII strings size by 2x.

We also no longer need to convert &str to &[u16] with utf16! macro, allowing conversion from &str to JsString. This cleans the places where we need JsString from &str.

This may also have some interesting optimization that could be applied when we know that the string is in the ASCII range.

HalidOdat avatar Nov 03 '23 22:11 HalidOdat

Test262 conformance changes

Test result main count PR count difference
Total 50,731 50,731 0
Passed 42,973 42,973 0
Ignored 1,395 1,395 0
Failed 6,363 6,363 0
Panics 18 18 0
Conformance 84.71% 84.71% 0.00%

github-actions[bot] avatar Nov 03 '23 22:11 github-actions[bot]

Codecov Report

Attention: Patch coverage is 62.01084% with 631 lines in your changes are missing coverage. Please review.

Project coverage is 44.47%. Comparing base (6ddc2b4) to head (52d192a). Report is 148 commits behind head on main.

:exclamation: Current head 52d192a differs from pull request most recent head bc980e4. Consider uploading reports for the commit bc980e4 to get more accurate results

Files Patch % Lines
boa_cli/src/debug/string.rs 0.00% 47 Missing :warning:
boa_engine/src/builtins/string/mod.rs 66.41% 44 Missing :warning:
boa_engine/src/string/mod.rs 79.27% 40 Missing :warning:
boa_engine/src/builtins/temporal/calendar/mod.rs 47.76% 35 Missing :warning:
boa_engine/src/string/slice.rs 65.11% 30 Missing :warning:
boa_engine/src/builtins/intl/collator/mod.rs 10.00% 27 Missing :warning:
boa_engine/src/builtins/regexp/mod.rs 66.66% 27 Missing :warning:
boa_engine/src/string/str.rs 52.08% 23 Missing :warning:
boa_engine/src/builtins/intl/segmenter/mod.rs 15.00% 17 Missing :warning:
boa_engine/src/builtins/intl/plural_rules/mod.rs 15.78% 16 Missing :warning:
... and 75 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3450      +/-   ##
==========================================
- Coverage   47.24%   44.47%   -2.77%     
==========================================
  Files         476      490      +14     
  Lines       46892    50360    +3468     
==========================================
+ Hits        22154    22398     +244     
- Misses      24738    27962    +3224     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 04 '23 17:11 codecov[bot]

This is ready for review/merge :partying_face:

In terms of performance it's pretty much the same as on main, a very slight regression that doesn't effect the overall score.

I did some analysis on what is the percentage of latin1 and u16 through out the combined.js execution for allocation strings (so static strings are not counted) and found that ~99.63 are latin1 strings, some methods still converts to u16, we could reduce this number even more by not eagerly converting to u16.

latin1: 9459373,
latin1_size: 64732594 (in bytes),
u16: 34755,
u16_size: 4346430 (in bytes),

There was also a reduction on binary size by ~64KB!

Checked locally and there is not difference between main and this PR in conformance, it seems that data repo's test262 data is not being updated?

HalidOdat avatar Apr 01 '24 15:04 HalidOdat

Checked locally and there is not difference between main and this PR in conformance, it seems that data repo's test262 data is not being updated?

That's weird. I checked the output of more recent PRs such as dependabot's and everything looks fine.

jedel1043 avatar Apr 01 '24 15:04 jedel1043

That's weird. I checked the output of more recent PRs such as dependabot's and everything looks fine.

I reran the test262 workflow and now it shows the correct conformance! :)

HalidOdat avatar Apr 01 '24 16:04 HalidOdat