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

Optimization fast local variables #3194

Closed
wants to merge 3 commits into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Aug 1, 2023

Depends on #3185

This PR is a WIP, it implements an optimization on function local variables, functions that do not call eval directly or variables that are not escaped, we can keep then on the stack after frame pointer and use and offset to index them (index = fp + offset). This allows us to eliminate the creation of some of the gc collected environments. Which gives us huge performance improvements!

Benchmarks

Main

Uncaught Error: Benchmark had 1 errors
PROGRESS Richards
RESULT Richards 36.0
PROGRESS DeltaBlue
RESULT DeltaBlue 42.2
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 63.2
PROGRESS RayTrace
RESULT RayTrace 147
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 136
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 118
PROGRESS NavierStokes
RESULT NavierStokes 136
SCORE 84.6
Uncaught Error: Benchmark had 1 errors

PR

PROGRESS Richards
RESULT Richards 42.3
PROGRESS DeltaBlue
RESULT DeltaBlue 46.5
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 102
PROGRESS RayTrace
RESULT RayTrace 164
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 158
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 124
PROGRESS NavierStokes
RESULT NavierStokes 246
SCORE 107

Almost 2x improvment on NavierStokes :)

@HalidOdat HalidOdat added performance Performance related changes and issues execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine. labels Aug 1, 2023
@HalidOdat HalidOdat changed the base branch from main to use-one-stack August 1, 2023 14:13
@HalidOdat HalidOdat marked this pull request as draft August 1, 2023 14:17
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,574 95,574 0
Passed 75,273 75,273 0
Ignored 19,482 19,482 0
Failed 819 819 0
Panics 0 0 0
Conformance 78.76% 78.76% 0.00%

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Attention: Patch coverage is 66.73152% with 171 lines in your changes missing coverage. Please review.

Project coverage is 49.60%. Comparing base (a51581b) to head (d53a0c4).
Report is 514 commits behind head on main.

Files with missing lines Patch % Lines
boa_engine/src/bytecompiler/declarations.rs 36.20% 37 Missing ⚠️
boa_engine/src/vm/opcode/get/name.rs 36.58% 26 Missing ⚠️
boa_engine/src/bytecompiler/mod.rs 81.03% 22 Missing ⚠️
boa_engine/src/vm/opcode/delete/mod.rs 11.11% 16 Missing ⚠️
boa_engine/src/vm/opcode/set/name.rs 44.44% 15 Missing ⚠️
boa_engine/src/vm/opcode/control_flow/return.rs 57.14% 12 Missing ⚠️
boa_ast/src/operations.rs 68.96% 9 Missing ⚠️
boa_engine/src/vm/code_block.rs 40.00% 9 Missing ⚠️
boa_engine/src/module/source.rs 0.00% 6 Missing ⚠️
boa_engine/src/bytecompiler/expression/mod.rs 0.00% 5 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3194      +/-   ##
==========================================
+ Coverage   49.53%   49.60%   +0.06%     
==========================================
  Files         446      446              
  Lines       43722    44070     +348     
==========================================
+ Hits        21659    21861     +202     
- Misses      22063    22209     +146     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from use-one-stack to main August 12, 2023 19:09
@HalidOdat HalidOdat force-pushed the optimization/fast-local-variables branch from 8f055b5 to 2d454cb Compare September 16, 2023 16:05
@HalidOdat HalidOdat force-pushed the optimization/fast-local-variables branch 3 times, most recently from 933900d to fedf269 Compare September 28, 2023 14:06
@HalidOdat HalidOdat force-pushed the optimization/fast-local-variables branch 2 times, most recently from 8fa5cb8 to b7c9230 Compare October 4, 2023 12:49
@HalidOdat HalidOdat force-pushed the optimization/fast-local-variables branch from b7c9230 to e23ec27 Compare October 4, 2023 12:53
@HalidOdat HalidOdat added the run-benchmark Label used to run banchmarks on PRs label Oct 4, 2023
@jedel1043 jedel1043 added the blocked Waiting for another code change label Nov 29, 2023
@jedel1043
Copy link
Member

Blocked on #3490

@raskad
Copy link
Member

raskad commented Aug 3, 2024

@HalidOdat I would like to take a look at continuing development on local variables. We talked about having environment aware bytecode generation for this with analyzed variable scopes. Do you have any more work or ideas on this or should I just start from the current state of this PR? Also, should I keep anything in mind regarding #3798?

@HalidOdat
Copy link
Member Author

Do you have any more work or ideas on this or should I just start from the current state of this PR?

The issue I have with the current implementation is that it does not account for many cases (and it's very hacked together).

My idea for a more proper solution would be to move the compile time environment logic to the parser and have the AST nodes like *Function*, Block contain a compile time environment, LibJS does this. It removes the need for a second pass after parsing (though makes parsing slower if we want to fail fast, but most scripts are valid so I don't think this is an issue).

The second idea was to perform a second pass after the AST generation this would generate a data structure that contains all the scope data that is passed to the compiler, and add a ID to the AST nodes that is None initially and filled in with the scope it points to, making mapping back to the scopes more efficient. This is a simpler approach and I was experimenting with it.

I began working on this but didn't make much progress, you can find the code on my fork (hope it helps): https://github.com/HalidOdat/boa/tree/scope-analysis

Also, should I keep anything in mind regarding #3798?

Hmmm.. The logic on how to allocate register/local variables in the compiler will be shared, besides that I think both tasks can be worked on simultaneously.

I'll create a PR and extract that logic so we can merge it into main.

@raskad
Copy link
Member

raskad commented Aug 30, 2024

Quick update on this. I have a working version with scope analysis at parse time. All tests pass and local variables work. I'm now in the process of cleaning up the implementation and removing some hacks.
The benchmark score increases are similar to the ones in the PR here. Overall score + ~23% for my first benchmarks.

I ran into the issue, of needing to track uninitialized variables, since we cannot track if a variable in initialized just with the stack value. Right now I'm just using a call frame local map to track this, but I think I can reduce that to an array to get some better performance.

@jedel1043
Copy link
Member

@raskad Tracking uninit variables may be something that is solved by #3037 IIRC

@raskad
Copy link
Member

raskad commented Sep 22, 2024

Superseded by #3988

@raskad raskad closed this Sep 22, 2024
@HalidOdat HalidOdat deleted the optimization/fast-local-variables branch September 23, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Waiting for another code change execution Issues or PRs related to code execution performance Performance related changes and issues run-benchmark Label used to run banchmarks on PRs vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants