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

[Merged by Bors] - Fix performance bottleneck in VM #1973

Closed
wants to merge 2 commits into from

Conversation

pdogr
Copy link
Contributor

@pdogr pdogr commented Mar 23, 2022

This Pull Request fixes/closes #1972 .

It changes the following:

  • remove format! macro in execute_instruction and replace by &'static str

replace format! macro by &'static str, reducing one allocation per
instruction execution
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #1973 (dfaa29b) into main (e2630fa) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1973      +/-   ##
==========================================
- Coverage   45.90%   45.89%   -0.01%     
==========================================
  Files         206      206              
  Lines       17116    17116              
==========================================
- Hits         7857     7856       -1     
- Misses       9259     9260       +1     
Impacted Files Coverage Δ
boa_engine/src/vm/mod.rs 70.95% <100.00%> (ø)
boa_engine/src/vm/opcode.rs 33.33% <100.00%> (-9.53%) ⬇️
boa_engine/src/string.rs 55.17% <0.00%> (-0.69%) ⬇️
boa_engine/src/builtins/regexp/mod.rs 67.28% <0.00%> (ø)
boa_engine/src/builtins/generator/mod.rs 7.77% <0.00%> (+0.08%) ⬆️
boa_engine/src/builtins/object/for_in_iterator.rs 94.11% <0.00%> (+2.68%) ⬆️

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 e2630fa...dfaa29b. Read the comment docs.

@jasonwilliams
Copy link
Member

great work @pdogr how did you spot that? Will this not be duplication over https://github.com/boa-dev/boa/blob/main/boa_engine/src/vm/opcode.rs#L939 though now?

@pdogr
Copy link
Contributor Author

pdogr commented Mar 24, 2022

great work @pdogr how did you spot that? Will this not be duplication over https://github.com/boa-dev/boa/blob/main/boa_engine/src/vm/opcode.rs#L939 though now?

I noticed that in execute_instruction a lot of time was spent in write_str which meant that string slices were being constantly written somewhere.
image

Yes, this is a duplication of as_str method, as the profiler had an "INST " prefix. We can instead use the as_str method in the profiler.

@jasonwilliams
Copy link
Member

@pdogr did you use https://github.com/boa-dev/boa/blob/main/docs/profiling.md or you had your own setup for that flame graph? Would be curious to hear how you got setup

@pdogr
Copy link
Contributor Author

pdogr commented Mar 24, 2022

No nothing specific just linux-perf and flamegraph. The profiling setup for boa https://github.com/boa-dev/boa/blob/main/docs/profiling.md is great to time bytecode execution though but this bottleneck was before that hence probably not caught.

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 very good! Thank you! I would just document the function, but it's ready to merge from my side :)

boa_engine/src/vm/opcode.rs Show resolved Hide resolved
@HalidOdat
Copy link
Member

HalidOdat commented Mar 24, 2022

The benchmarks look awesome!

Benchmarks

Just execution tests.

name baseDuration changesDuration difference
'Arithmetic operations (Execution)' '1989.1±2.26ns' '679.8±0.83ns' '-66'
'Array access (Execution)' '9.8±0.03µs' '7.5±0.02µs' '-23'
'Array creation (Execution)' '3.2±0.01ms' '2.4±0.00ms' '-25'
'Array pop (Execution)' '1403.2±3.52µs' '1088.6±3.62µs' '-22'
'Boolean Object Access (Execution)' '5.7±0.02µs' '3.9±0.03µs' '-32'
'Clean js (Execution)' '1087.9±7.34µs' '687.4±4.20µs' '-37'
'Dynamic Object Property Access (Execution)' '6.7±0.04µs' '5.0±0.01µs' '-25'
'Fibonacci (Execution)' '1782.5±13.85µs' '1272.5±2.40µs' '-29'
'For loop (Execution)' '42.3±0.11µs' '14.9±0.03µs' '-65'
'Mini js (Execution)' '1001.9±37.22µs' '651.3±4.77µs' '-35'
'Number Object Access (Execution)' '4.5±0.01µs' '3.0±0.01µs' '-33'
'Object Creation (Execution)' '6.0±0.04µs' '4.7±0.02µs' '-21'
'RegExp (Execution)' '12.0±0.06µs' '10.7±0.04µs' '-11'
'RegExp Creation (Execution)' '8.8±0.03µs' '8.1±0.03µs' '-8.3'
'RegExp Literal (Execution)' '12.0±0.07µs' '10.8±0.04µs' '-9.9'
'RegExp Literal Creation (Execution)' '8.8±0.04µs' '8.0±0.02µs' '-9.1'
'Static Object Property Access (Execution)' '6.3±0.02µs' '4.9±0.02µs' '-22'
'String Object Access (Execution)' '7.5±0.02µs' '5.8±0.03µs' '-23'
'String comparison (Execution)' '5.8±0.01µs' '4.2±0.01µs' '-26'
'String concatenation (Execution)' '5.3±0.02µs' '4.0±0.01µs' '-24'
'String copy (Execution)' '4.7±0.03µs' '3.8±0.02µs' '-19'
'Symbols (Execution)' '4.5±0.01µs' '3.8±0.01µs' '-14'

add documentation for as_instruction_str method
@raskad raskad added performance Performance related changes and issues vm Issues and PRs related to the Boa Virtual Machine. labels Mar 24, 2022
@raskad raskad added this to the v0.15.0 milestone Mar 24, 2022
@raskad raskad added the Internal Category for changelog label Mar 24, 2022
@Razican Razican requested a review from HalidOdat March 24, 2022 18:04
@jasonwilliams
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 25, 2022
This Pull Request fixes/closes #1972 .

It changes the following:
- remove `format!` macro in `execute_instruction` and replace by `&'static str`
@bors
Copy link

bors bot commented Mar 25, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title [perf][vm] [Merged by Bors] - [perf][vm] Mar 25, 2022
@bors bors bot closed this Mar 25, 2022
@Razican Razican changed the title [Merged by Bors] - [perf][vm] [Merged by Bors] - Fix performance bottleneck in VM Jun 4, 2022
Razican pushed a commit that referenced this pull request Jun 8, 2022
This Pull Request fixes/closes #1972 .

It changes the following:
- remove `format!` macro in `execute_instruction` and replace by `&'static str`
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 vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible optimization in execute_instruction
5 participants