[Merged by Bors] - Improve `JsString` performance #2042

YXL76 · 2022-04-23T05:41:20Z

It changes the following:

The current JsString implementation has a lot of duplicate heap allocations. For static strings, JsString only needs to store the index of CONSTANTS_ARRAY. ~~I let JsString can store pointer to heap or static area. The two are distinguished by the first bit. (This implementation is rough, maybe we should put flag into Inner, like arcstr)~~
I also added more string constants, which are always used.

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

YXL76 · 2022-04-23T05:49:54Z

At present, the Rc feature of JsString is very limited, just like the function, it always performs two heap allocations instead of increasing the reference count. (See here)

Since we always do hashing, wouldn't it be better to use get_or_insert_with instead of get?

codecov · 2022-04-23T06:35:42Z

Codecov Report

Merging #2042 (698a517) into main (5b46d12) will increase coverage by 0.23%.
The diff coverage is 48.93%.

@@            Coverage Diff             @@
##             main    #2042      +/-   ##
==========================================
+ Coverage   43.90%   44.14%   +0.23%     
==========================================
  Files         212      212              
  Lines       18734    18841     +107     
==========================================
+ Hits         8226     8318      +92     
- Misses      10508    10523      +15

Impacted Files	Coverage Δ
boa_engine/src/bigint.rs	`37.59% <0.00%> (ø)`
boa_engine/src/string.rs	`53.93% <51.11%> (-5.09%)`	⬇️
...oa_engine/src/syntax/parser/statement/block/mod.rs	`46.34% <0.00%> (-6.30%)`	⬇️
...ngine/src/syntax/parser/statement/try_stm/catch.rs	`45.67% <0.00%> (-4.33%)`	⬇️
...src/syntax/parser/statement/declaration/lexical.rs	`30.59% <0.00%> (-3.60%)`	⬇️
...syntax/parser/statement/iteration/for_statement.rs	`38.82% <0.00%> (-3.52%)`	⬇️
.../statement/declaration/hoistable/class_decl/mod.rs	`24.13% <0.00%> (-0.83%)`	⬇️
...a_engine/src/syntax/parser/statement/switch/mod.rs	`39.53% <0.00%> (-0.47%)`	⬇️
boa_engine/src/object/mod.rs	`20.68% <0.00%> (-0.41%)`	⬇️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b46d12...698a517. Read the comment docs.

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

YXL76 · 2022-04-23T11:09:13Z

Simplify it, now inner only stores the index of CONSTANTS_ARRAY

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

raskad

The principle used here is a tagged pointer right? There should be an explicit mention of that in the comments, so that everyone knows what is being done.

I'm not too familiar with the platform requirements for tagged pointers. I assume this would fail on 16 bit platforms? Are there any other requirements that should be noted?
I think if we implement this, we would have to at least mention these requirements, at best prevent boa from compiling if any of the requirements are not met.

boa_engine/src/string.rs

boa_engine/src/value/mod.rs

boa_engine/src/bigint.rs

jedel1043

I like the idea of this PR very much! However, I think it would be good to check with miri if the pointer provenance of our Inner pointer is preserved between casts.

YXL76 · 2022-04-24T00:40:46Z

The principle used here is a tagged pointer right? There should be an explicit mention of that in the comments, so that everyone knows what is being done.

I'm not too familiar with the platform requirements for tagged pointers. I assume this would fail on 16 bit platforms? Are there any other requirements that should be noted? I think if we implement this, we would have to at least mention these requirements, at best prevent boa from compiling if any of the requirements are not met.

It works on 16 bit machine, because we only use the first bit. So far there is no side effect of introducing this technique, since rust-gc also uses this technique.

https://github.com/Manishearth/rust-gc/blob/0278c6b895d53153a4eb63cfc56e59d97698e5ed/gc/src/lib.rs#L102-L107

https://github.com/Manishearth/rust-gc/blob/0278c6b895d53153a4eb63cfc56e59d97698e5ed/gc/src/lib.rs#L114-L118

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

boa_engine/src/string.rs

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

YXL76 · 2022-04-25T07:40:07Z

A larger MAX_CONSTANT_STRING_LENGTH may have some side effects, we have to do more hash table queries, and the hit rate is low.

This goes back to my previous question, should CONSTANTS be a fixed size HashSet? If so, then we should strictly limit the size of MAX_CONSTANT_STRING_LENGTH, if not (ie as a global cache), then there are no such problems.

raskad · 2022-04-25T12:23:54Z

A larger MAX_CONSTANT_STRING_LENGTH may have some side effects, we have to do more hash table queries, and the hit rate is low.

I think this is a performance property that we would have to adjust based on real-word benchmarks. In theory a longer MAX_CONSTANT_STRING_LENGTH leads to more lookups at JsString creation time, but how does that impact the performance of the runtime overall? Do real-world programs benefit more from static strings or from less hashmap lookups?

Maybe there is some research already done in other engines that we could base a decision upon, but I think for now this is totally fine.

This goes back to my previous question, should CONSTANTS be a fixed size HashSet? If so, then we should strictly limit the size of MAX_CONSTANT_STRING_LENGTH, if not (ie as a global cache), then there are no such problems.

I think inserting into CONSTANTS is not realistic. We either leak memory by never deleting those strings or would have to implement reference counting in CONSTANTS to make removing from CONSTANTS possible which seems like a bad idea for dealloc performance.

jedel1043 · 2022-04-26T06:55:59Z

As expected, running MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-disable-stacked-borrows" cargo +nightly miri test --package boa_engine --lib -- string::tests --skip builtins fails with the message:

error: Undefined Behavior: dereferencing pointer failed: 0xdb4690 is not a valid pointer

Meaning the provenance of the pointer is not preserved. The fix is pretty easy, we just need to use NonNull<Inner> instead of NonZeroUsize, casting the tagged indexes to invalid pointers with index as *const Inner. That should preserve the provenance of every valid pointer.

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Razican · 2022-04-26T16:54:35Z

Benchmarks

Click to view benchmark

name	baseDuration	changesDuration	difference
Arithmetic operations (Compiler)	493.7±1.24ns	526.0±0.90ns	+7.0 %
Arithmetic operations (Execution)	445.5±1.31ns	487.7±1.37ns	+9.0 %
Arithmetic operations (Parser)	6.1±0.02µs	6.2±0.04µs	+1.0 %
Array access (Compiler)	1338.8±5.43ns	1374.3±6.68ns	+3.0 %
Array access (Execution)	8.5±0.02µs	8.5±0.08µs	0.0 %
Array access (Parser)	13.9±0.04µs	14.1±0.06µs	+2.0 %
Array creation (Compiler)	1961.4±19.27ns	1945.9±6.06ns	-0.99 %
Array creation (Execution)	2.4±0.00ms	2.5±0.00ms	+3.0 %
Array creation (Parser)	15.7±0.04µs	15.8±0.22µs	0.0 %
Array pop (Compiler)	3.7±0.01µs	3.8±0.01µs	+4.0 %
Array pop (Execution)	1075.4±13.56µs	1107.8±11.13µs	+3.0 %
Array pop (Parser)	157.2±0.25µs	157.2±0.18µs	0.0 %
Boolean Object Access (Compiler)	1110.1±5.17ns	1126.9±3.66ns	+2.0 %
Boolean Object Access (Execution)	4.6±0.02µs	4.5±0.01µs	-2.0 %
Boolean Object Access (Parser)	16.2±0.03µs	16.7±0.06µs	+3.0 %
Clean js (Compiler)	3.7±0.01µs	3.7±0.01µs	0.0 %
Clean js (Execution)	703.3±6.39µs	681.9±3.80µs	-2.9 %
Clean js (Parser)	33.8±0.21µs	34.1±0.05µs	+1.0 %
Create Realm	265.1±0.37ns	273.5±1.32ns	+3.0 %
Dynamic Object Property Access (Compiler)	1752.9±6.21ns	1769.6±4.42ns	+1.0 %
Dynamic Object Property Access (Execution)	5.7±0.04µs	5.6±0.01µs	-0.99 %
Dynamic Object Property Access (Parser)	12.1±0.06µs	12.3±0.08µs	+2.0 %
Fibonacci (Compiler)	2.4±0.00µs	2.4±0.00µs	0.0 %
Fibonacci (Execution)	1400.4±2.33µs	1445.6±14.19µs	+3.0 %
Fibonacci (Parser)	18.5±0.07µs	19.0±0.03µs	+3.0 %
For loop (Compiler)	2.1±0.00µs	2.2±0.03µs	+5.0 %
For loop (Execution)	15.2±0.06µs	15.4±0.07µs	+1.0 %
For loop (Parser)	16.0±0.05µs	16.1±0.05µs	0.0 %
Mini js (Compiler)	3.6±0.01µs	3.5±0.01µs	-2.0 %
Mini js (Execution)	657.5±4.75µs	649.1±4.81µs	-0.99 %
Mini js (Parser)	30.0±0.12µs	30.4±0.05µs	+1.0 %
Number Object Access (Compiler)	1029.0±2.34ns	1060.2±4.30ns	+3.0 %
Number Object Access (Execution)	3.5±0.02µs	3.6±0.01µs	+2.0 %
Number Object Access (Parser)	12.8±0.11µs	13.0±0.04µs	+2.0 %
Object Creation (Compiler)	1434.7±3.62ns	1492.4±4.71ns	+4.0 %
Object Creation (Execution)	5.2±0.02µs	5.2±0.07µs	-0.99 %
Object Creation (Parser)	10.6±0.06µs	10.8±0.05µs	+2.0 %
RegExp (Compiler)	1725.3±3.85ns	1691.2±29.10ns	-2.0 %
RegExp (Execution)	11.7±0.06µs	11.9±0.06µs	+1.0 %
RegExp (Parser)	11.8±0.25µs	11.9±0.06µs	+1.0 %
RegExp Creation (Compiler)	1531.4±3.00ns	1502.1±5.63ns	-2.0 %
RegExp Creation (Execution)	8.8±0.02µs	8.8±0.04µs	0.0 %
RegExp Creation (Parser)	9.7±0.11µs	9.8±0.03µs	+2.0 %
RegExp Literal (Compiler)	1754.2±38.01ns	1684.2±15.68ns	-3.8 %
RegExp Literal (Execution)	11.6±0.05µs	11.9±0.06µs	+2.0 %
RegExp Literal (Parser)	9.5±0.04µs	9.7±0.02µs	+2.0 %
RegExp Literal Creation (Compiler)	1510.7±10.96ns	1512.1±5.35ns	0.0 %
RegExp Literal Creation (Execution)	8.9±0.03µs	8.8±0.03µs	-0.99 %
RegExp Literal Creation (Parser)	7.5±0.05µs	7.7±0.02µs	+2.0 %
Static Object Property Access (Compiler)	1486.9±4.70ns	1470.8±4.04ns	-0.99 %
Static Object Property Access (Execution)	5.4±0.06µs	5.3±0.02µs	-0.99 %
Static Object Property Access (Parser)	11.4±0.06µs	11.5±0.06µs	+1.0 %
String Object Access (Compiler)	1550.9±23.24ns	1546.9±6.29ns	0.0 %
String Object Access (Execution)	6.1±0.02µs	6.7±0.13µs	+9.0 %
String Object Access (Parser)	15.9±0.02µs	16.5±0.02µs	+4.0 %
String comparison (Compiler)	2.2±0.01µs	2.2±0.01µs	-3.8 %
String comparison (Execution)	4.7±0.03µs	4.8±0.06µs	+2.0 %
String comparison (Parser)	12.5±0.04µs	12.8±0.04µs	+3.0 %
String concatenation (Compiler)	1735.4±4.99ns	1673.5±3.89ns	-3.8 %
String concatenation (Execution)	4.6±0.03µs	4.6±0.04µs	0.0 %
String concatenation (Parser)	8.7±0.03µs	8.9±0.01µs	+2.0 %
String copy (Compiler)	1366.5±5.89ns	1339.6±5.52ns	-2.0 %
String copy (Execution)	4.3±0.02µs	4.3±0.04µs	+2.0 %
String copy (Parser)	6.5±0.03µs	6.7±0.03µs	+3.0 %
Symbols (Compiler)	941.6±3.70ns	983.6±3.43ns	+4.0 %
Symbols (Execution)	4.4±0.01µs	4.3±0.03µs	-2.0 %
Symbols (Parser)	5.0±0.02µs	5.2±0.02µs	+4.0 %

jedel1043

Some more comments about the implementation. I also tinkered a bit with the documentation to make it a bit more clearer.

boa_engine/src/string.rs

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

jedel1043

Nice work! There are still some nitpicks I have, but it's looking pretty good nonetheless :)

boa_engine/src/string.rs

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

jedel1043

Looks good to me! Let's wait until the ones who left comments approve 😄

boa_engine/src/string.rs

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

HalidOdat

Looks good to me! :)

raskad · 2022-05-01T17:17:50Z

bors r+

It changes the following: - The current `JsString` implementation has a lot of duplicate heap allocations. For static strings, `JsString` only needs to store the index of `CONSTANTS_ARRAY`. ~~I let `JsString` can store pointer to heap or static area. The two are distinguished by the first bit. (This implementation is rough, maybe we should put flag into `Inner`, like [arcstr](https://github.com/thomcc/arcstr/blob/70ba2fac19d3efe8d2e0231daaf74f9987c04b8a/src/arc_str.rs#L751-L757))~~ - I also added more string constants, which are always used.

bors · 2022-05-01T17:26:51Z

Pull request successfully merged into main.

Build succeeded:

It changes the following: - The current `JsString` implementation has a lot of duplicate heap allocations. For static strings, `JsString` only needs to store the index of `CONSTANTS_ARRAY`. ~~I let `JsString` can store pointer to heap or static area. The two are distinguished by the first bit. (This implementation is rough, maybe we should put flag into `Inner`, like [arcstr](https://github.com/thomcc/arcstr/blob/70ba2fac19d3efe8d2e0231daaf74f9987c04b8a/src/arc_str.rs#L751-L757))~~ - I also added more string constants, which are always used.

YXL76 added 2 commits April 23, 2022 11:39

perf(string): No more heap allocation of static strings

e8ef21d

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Add more elements to CONSTANTS_ARRAY

3598379

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Remove StaticStr

574f722

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

fix: Clippy lints

effcaa4

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

raskad reviewed Apr 23, 2022

View reviewed changes

boa_engine/src/string.rs Outdated Show resolved Hide resolved

boa_engine/src/value/mod.rs Outdated Show resolved Hide resolved

boa_engine/src/bigint.rs Show resolved Hide resolved

boa_engine/src/bigint.rs Show resolved Hide resolved

raskad added the enhancement New feature or request label Apr 23, 2022

raskad requested review from HalidOdat, jasonwilliams, jedel1043, RageKnify and Razican April 23, 2022 17:06

jedel1043 requested changes Apr 23, 2022

View reviewed changes

YXL76 added 4 commits April 24, 2022 10:40

Make meaning of inner more clear

119e547

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Fix clippy lints

1b2236e

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

revert effcaa4

c0bb509

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Fix clippy and add some tests

34494a1

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

HalidOdat reviewed Apr 24, 2022

View reviewed changes

boa_engine/src/string.rs Outdated Show resolved Hide resolved

Use get_unchecked

31feb14

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Fix miri error

46f7f8b

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

jedel1043 requested changes Apr 26, 2022

View reviewed changes

YXL76 added 2 commits April 27, 2022 10:15

Clearer documentation

29fadb2

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

Change CONSTANTS

9349a08

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

jedel1043 requested changes Apr 27, 2022

View reviewed changes

boa_engine/src/string.rs Outdated Show resolved Hide resolved

boa_engine/src/string.rs Outdated Show resolved Hide resolved

Better type

456cd19

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

jedel1043 approved these changes Apr 27, 2022

View reviewed changes

HalidOdat requested changes Apr 28, 2022

View reviewed changes

boa_engine/src/string.rs Show resolved Hide resolved

Fix memory leak

698a517

Signed-off-by: YXL <chenxin.lan.76@gmail.com>

HalidOdat approved these changes Apr 30, 2022

View reviewed changes

raskad approved these changes May 1, 2022

View reviewed changes

bors bot changed the title ~~Improve JsString performance~~ [Merged by Bors] - Improve JsString performance May 1, 2022

bors bot closed this May 1, 2022

YXL76 deleted the string branch May 2, 2022 05:38

raskad added this to the v0.15.0 milestone May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Improve `JsString` performance #2042

[Merged by Bors] - Improve `JsString` performance #2042

YXL76 commented Apr 23, 2022 •

edited

Loading

YXL76 commented Apr 23, 2022 •

edited

Loading

codecov bot commented Apr 23, 2022 •

edited

Loading

YXL76 commented Apr 23, 2022

raskad left a comment

jedel1043 left a comment •

edited

Loading

YXL76 commented Apr 24, 2022 •

edited

Loading

YXL76 commented Apr 25, 2022

raskad commented Apr 25, 2022

jedel1043 commented Apr 26, 2022 •

edited

Loading

Razican commented Apr 26, 2022

jedel1043 left a comment

jedel1043 left a comment

jedel1043 left a comment

HalidOdat left a comment

raskad commented May 1, 2022

bors bot commented May 1, 2022

[Merged by Bors] - Improve JsString performance #2042

[Merged by Bors] - Improve JsString performance #2042

Conversation

YXL76 commented Apr 23, 2022 • edited Loading

YXL76 commented Apr 23, 2022 • edited Loading

codecov bot commented Apr 23, 2022 • edited Loading

Codecov Report

YXL76 commented Apr 23, 2022

raskad left a comment

Choose a reason for hiding this comment

jedel1043 left a comment • edited Loading

Choose a reason for hiding this comment

YXL76 commented Apr 24, 2022 • edited Loading

YXL76 commented Apr 25, 2022

raskad commented Apr 25, 2022

jedel1043 commented Apr 26, 2022 • edited Loading

Razican commented Apr 26, 2022

Benchmarks

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

raskad commented May 1, 2022

bors bot commented May 1, 2022

[Merged by Bors] - Improve `JsString` performance #2042

[Merged by Bors] - Improve `JsString` performance #2042

YXL76 commented Apr 23, 2022 •

edited

Loading

YXL76 commented Apr 23, 2022 •

edited

Loading

codecov bot commented Apr 23, 2022 •

edited

Loading

jedel1043 left a comment •

edited

Loading

YXL76 commented Apr 24, 2022 •

edited

Loading

jedel1043 commented Apr 26, 2022 •

edited

Loading