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

Prevent allocation of field names #2901

Merged
merged 1 commit into from May 5, 2023
Merged

Prevent allocation of field names #2901

merged 1 commit into from May 5, 2023

Conversation

HalidOdat
Copy link
Member

This PR makes the CodeBlock store JsStrings in the names field instead of Identifier (which is a wrapper around Sym), This is done because once we resolve the sym and get the &[u16] we allocate a JsString on every field access in the worst case and at best if it's part of the static strings array we do a HashMap lookup.

Ran the benchmarks locally and there was a 2% to 10% increase in performance in some of the benchmarks.

The performance increase can be better seen by running the quickjs benches:

Main

PROGRESS Richards
RESULT Richards 19.8
PROGRESS DeltaBlue
RESULT DeltaBlue 21.9
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 27.9
PROGRESS RayTrace
RESULT RayTrace 76.8
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 67.8
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 99.4
PROGRESS NavierStokes
RESULT NavierStokes 6.28
SCORE 32.6
Uncaught Error: Benchmark had 1 errors

PR

PROGRESS Richards
RESULT Richards 21.0
PROGRESS DeltaBlue
RESULT DeltaBlue 23.0
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 28.4
PROGRESS RayTrace
RESULT RayTrace 81.2
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 69.2
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 101
PROGRESS NavierStokes
RESULT NavierStokes 6.33
SCORE 33.7
Uncaught Error: Benchmark had 1 errors

@HalidOdat HalidOdat added performance Performance related changes and issues execution Issues or PRs related to code execution labels May 5, 2023
@HalidOdat HalidOdat requested a review from a team May 5, 2023 16:15
@github-actions
Copy link

github-actions bot commented May 5, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,601 94,601 0
Passed 73,298 73,298 0
Ignored 17,540 17,540 0
Failed 3,763 3,763 0
Panics 0 0 0
Conformance 77.48% 77.48% 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.

Looks like a good change. Nice improvement on the benchmarks.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good, nice optimization!

@Razican Razican enabled auto-merge May 5, 2023 16:31
@Razican Razican added this pull request to the merge queue May 5, 2023
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #2901 (d0a1a9a) into main (e3d2056) will decrease coverage by 0.01%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
- Coverage   51.90%   51.90%   -0.01%     
==========================================
  Files         428      428              
  Lines       43113    43097      -16     
==========================================
- Hits        22378    22368      -10     
+ Misses      20735    20729       -6     
Impacted Files Coverage Δ
boa_engine/src/vm/code_block.rs 56.84% <0.00%> (ø)
boa_engine/src/vm/flowgraph/mod.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/define/class/getter.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/define/class/method.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/define/class/setter.rs 0.00% <0.00%> (ø)
boa_engine/src/bytecompiler/mod.rs 63.85% <100.00%> (+0.05%) ⬆️
boa_engine/src/vm/opcode/define/own_property.rs 87.50% <100.00%> (-0.60%) ⬇️
boa_engine/src/vm/opcode/delete/mod.rs 80.55% <100.00%> (-1.03%) ⬇️
boa_engine/src/vm/opcode/get/property.rs 85.71% <100.00%> (-0.50%) ⬇️
boa_engine/src/vm/opcode/set/property.rs 70.12% <100.00%> (-0.89%) ⬇️

@HalidOdat HalidOdat added this to the v0.17.0 milestone May 5, 2023
Merged via the queue into main with commit d20304e May 5, 2023
9 checks passed
@jedel1043 jedel1043 deleted the optimization/field-name branch May 5, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants