[Merged by Bors] - Implement constant folding optimization #2679
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HalidOdat
added
performance
HalidOdat
changed the title
Test262 conformance changes
|
Benchmark for 5a33439Click to view benchmark
|
|
I enabled optimizations by default for testing, so that's why there is an improvement in benchmarks (especially 50% improvement in arithmetic execution), there also seems to be some improvement in byte compiling probably because it has to generate less. Optimizations should probably be disabled when benchmarking |
jedel1043
reviewed
Mar 16, 2023
jedel1043
requested changes
Mar 16, 2023
HalidOdat
force-pushed
the
optimization/constant-folding
branch
from
19664ea to
29abb90
Compare
Hmmm it depends what do we want to benchmark. I would like to see optimized vs non-optimized end-to-end benchmarking of real-life code. An arithmetic expression that is compile-time known will of course be optimized away a lot. |
Benchmark for 7a157f0Click to view benchmark
|
Benchmark for 10ebc36Click to view benchmark
|
Codecov Report
@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
+ Coverage 50.81% 52.01% +1.20%
==========================================
Files 400 397 -3
Lines 40121 39952 -169
==========================================
+ Hits 20387 20781 +394
+ Misses 19734 19171 -563
... and 54 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jedel1043
reviewed
Mar 17, 2023
nekevss
reviewed
Mar 17, 2023
.github/workflows/test262.yml
Outdated
| @@ -45,7 +45,7 @@ jobs: | |||
| run: | | |||
| cd boa | |||
| mkdir ../results | |||
| cargo run --release --bin boa_tester -- run -v -o ../results/test262 | |||
| cargo run --release --bin boa_tester -- run -O -v -o ../results/test262 | |||
There was a problem hiding this comment.
This looks really cool! I do still need to dig into this more, but I had a question when I saw this. At least on any optimizer updates, should we run the tests on both optimized and non-optimized in our CI to insure that an update to one doesn't break something in another?
There was a problem hiding this comment.
The enabled optimization in test262 is just for testing for now.
At least on any optimizer updates, should we run the tests on both optimized and non-optimized in our CI to insure that an update to one doesn't break something in another?
Yes, completely agree! maybe we can configure to start test262 with optimizations when change to the optimizer module/directory happens (maybe reviewpad mentioned by @Razican can be used for this, since it has a nice way of checking if a directory contents are changed). And we should probably run it on main push and assert that the non-optimized and optimized are equal, just to be sure :)
Benchmark for f8e7ae0Click to view benchmark
|
HalidOdat
force-pushed
the
optimization/constant-folding
branch
from
d451d75 to
0f07439
Compare
|
This is now ready for review/merge :) 🎉 Also implemented statistics for optimizations which will be printed with the cargo run -- -O --optimizer-statistics>>> (1 + 2 * 3) + '00'
Optimizer {
constant folding: 1 run(s), 2 pass(es) (1 mutating, 1 checking)
} |
jedel1043
reviewed
Mar 18, 2023
Benchmark for 7c6bab0Click to view benchmark
|
Benchmark for da43591Click to view benchmark
|
raskad
approved these changes
Mar 19, 2023
nekevss
reviewed
Mar 20, 2023
HalidOdat
commented
Mar 20, 2023
Benchmark for 46ebdbdClick to view benchmark
|
Benchmark for ccf84f4Click to view benchmark
|
Benchmark for 6827dd9Click to view benchmark
|
Benchmark for 66f8dc5Click to view benchmark
|
|
Looking at the benchmarks, it seems that opting-out makes things slower than before |
I disabled optimizations in the benchmarks. The benchmarks don't seem right, benchmarks like The opt-out only adds one condition check that is called once at the top level (parse_script function), its not something to cause that big of change in the benchmarks. |
HalidOdat
force-pushed
the
optimization/constant-folding
branch
from
97633b6 to
3617c8d
Compare
Benchmark for 84abb56Click to view benchmark
|
HalidOdat
force-pushed
the
optimization/constant-folding
branch
from
3617c8d to
bd251bd
Compare
Benchmark for 3c883b4Click to view benchmark
|
- Optimizer statistics - Make optimization opt-out and disable them in benchmarks
HalidOdat
force-pushed
the
optimization/constant-folding
branch
from
bd251bd to
7637200
Compare
Razican
approved these changes
Mar 30, 2023
|
@jedel1043 are your concerns solved? |
Benchmark for 39e9b10Click to view benchmark
|
jedel1043
approved these changes
Mar 30, 2023
There was a problem hiding this comment.
@jedel1043 are your concerns solved?
Yep, no more comments from my part. We can merge this :)
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 30, 2023
This PR implements an optimizer, It currently implements the [constant folding optimization][cfo]. this optimization is responsible for "folding"/evaluating constant expressions. For example: ```js let x = ((1 + 2 + -4) * 8) << 4 ``` Generates the following instruction(s) (`cargo run -- -t`): ``` 000000 0000 PushOne 000001 0001 PushInt8 2 000003 0002 Add 000004 0003 PushInt8 4 000006 0004 Neg 000007 0005 Add 000008 0006 PushInt8 8 000010 0007 Mul 000011 0008 PushInt8 4 000013 0009 ShiftLeft 000014 0010 DefInitLet 0000: 'x' ``` With constant folding it generates the following instruction(s) (`cargo run -- -t -O`): ``` 000000 0000 PushInt8 -128 000002 0001 DefInitLet 0000: 'x' ``` It changes the following: - Implement ~~WIP~~ constant folding optimization, ~~only works with integers for now~~ - Add `--optimize, -O` flag to boa_cli - Add `--optimizer-statistics` flag to boa_cli for optimizer statistics - Add `--optimize, -O` flag to boa_tester After I finish with this, will try to implement other optimizations :) [cfo]: https://en.wikipedia.org/wiki/Constant_folding
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements an optimizer, It currently implements the constant folding optimization. this optimization is responsible for "folding"/evaluating constant expressions.
For example:
Generates the following instruction(s) (
cargo run -- -t):With constant folding it generates the following instruction(s) (
cargo run -- -t -O):It changes the following:
WIPconstant folding optimization,only works with integers for now--optimize, -Oflag to boa_cli--optimizer-statisticsflag to boa_cli for optimizer statistics--optimize, -Oflag to boa_testerAfter I finish with this, will try to implement other optimizations :)