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

Refactor environment, exception handling and jumping in VM #3059

Merged
merged 16 commits into from
Jul 29, 2023

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Jun 22, 2023

Depends on #3053

Fixes #2424

This PR refactors environment handling and jumping with break, continues, etc in the VM. Currently the opcodes are very complex, they dynamically search for the environments, for this we also need to keep a lot of state in the CallFrame, this has a performance penalty as well as being hard to implement optimizers (since it has to mirror the state, so it does not change behaviour).

Since this the bytecompiler has full knowledge of where it needs to jump, how many environments to pop, etc. This PR aims to move that opcode logic to bytecompiler, generating opcodes that are "dumber"/simple, hence easier to optimize ( using #3037 ).

This still needs a lot of work!

It changes the following:

  • Removes opcodes: FinallyStart, FinallyEnd, Break, Continue, LoopStart, LoopEnd, LabelledStart, LabelledEnd, TryStart, TryEnd, IteratorLoopStart,GeneratorResumeReturn, GeneratorSetReturn, GeneratorJumpOnResumeKind
  • Add opcodes: ReThrow, Exception, JumpTable, JumpIfNotResumeKind, IteratorNextWithoutPop, IteratorValueWithoutPop
  • Replaces Breaks and Continues with simple unconditional Jump.
  • Remove EnvEntry type and env entry stack
  • Remove abrupt_completion record from CallFrame
  • Remove generator_resume_kind from CallFrame
  • Remove yield field from CallFrame, added CompletionType::Yield

TODO:

  • Figure out fp on exception handlers
  • Fix bytecode flowgraph creation
  • Add more documentation.

@HalidOdat HalidOdat added performance Performance related changes and issues technical debt labels Jun 22, 2023
@HalidOdat HalidOdat changed the title WIP Refactor environment, exception handling and jumping in VM Jun 22, 2023
@HalidOdat HalidOdat force-pushed the simplify-exception-handling branch from bcb7631 to fed05d7 Compare June 22, 2023 04:55
@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch from 7a50cc6 to 3ea7f1a Compare June 25, 2023 02:13
Base automatically changed from simplify-exception-handling to main June 25, 2023 19:22
@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch from 3ea7f1a to b5b2a5a Compare June 26, 2023 07:13
@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch 2 times, most recently from a087128 to ab575f7 Compare June 27, 2023 05:06
@github-actions
Copy link

github-actions bot commented Jun 27, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,260 95,260 0
Passed 74,910 74,918 +8
Ignored 19,230 19,230 0
Failed 1,120 1,112 -8
Panics 0 0 0
Conformance 78.64% 78.65% +0.01%
Fixed tests (8):
test/language/statements/for-of/iterator-close-via-return.js [strict mode] (previously Failed)
test/language/statements/for-of/iterator-close-via-return.js (previously Failed)
test/language/statements/for-of/generator-close-via-return.js [strict mode] (previously Failed)
test/language/statements/for-of/generator-close-via-return.js (previously Failed)
test/language/statements/class/subclass/derived-class-return-override-for-of-arrow.js [strict mode] (previously Failed)
test/language/statements/class/subclass/derived-class-return-override-for-of-arrow.js (previously Failed)
test/language/statements/class/subclass/derived-class-return-override-for-of.js [strict mode] (previously Failed)
test/language/statements/class/subclass/derived-class-return-override-for-of.js (previously Failed)

@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch from ab575f7 to ec4d598 Compare June 28, 2023 07:48
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #3059 (f88af46) into main (33e8c51) will decrease coverage by 0.09%.
Report is 6 commits behind head on main.
The diff coverage is 72.19%.

@@            Coverage Diff             @@
##             main    #3059      +/-   ##
==========================================
- Coverage   50.50%   50.41%   -0.09%     
==========================================
  Files         443      436       -7     
  Lines       42712    42340     -372     
==========================================
- Hits        21572    21347     -225     
+ Misses      21140    20993     -147     
Files Changed Coverage Δ
boa_engine/src/bytecompiler/expression/mod.rs 55.88% <0.00%> (-0.84%) ⬇️
boa_engine/src/bytecompiler/statement/labelled.rs 68.42% <ø> (-5.50%) ⬇️
boa_engine/src/bytecompiler/statement/switch.rs 100.00% <ø> (ø)
boa_engine/src/module/source.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/flowgraph/mod.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/mod.rs 69.23% <ø> (ø)
boa_engine/src/vm/opcode/pop/mod.rs 100.00% <ø> (ø)
boa_engine/src/vm/opcode/push/environment.rs 92.30% <ø> (-0.38%) ⬇️
boa_engine/src/vm/code_block.rs 57.62% <28.84%> (-2.00%) ⬇️
boa_engine/src/bytecompiler/class.rs 15.56% <33.33%> (-0.10%) ⬇️
... and 26 more

... and 26 files with indirect coverage changes

@HalidOdat
Copy link
Member Author

Ran the quickjs benchmarks:

Main

PROGRESS Richards
RESULT Richards 30.1
PROGRESS DeltaBlue
RESULT DeltaBlue 35.5
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 46.1
PROGRESS RayTrace
RESULT RayTrace 124
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 112
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 120
PROGRESS NavierStokes
RESULT NavierStokes 11.8
SCORE 51.5
Uncaught Error: Benchmark had 1 errors

PR


PROGRESS Richards
RESULT Richards 31.1
PROGRESS DeltaBlue
RESULT DeltaBlue 36.7
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 46.9
PROGRESS RayTrace
RESULT RayTrace 126
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 114
ERROR RegExp TypeError: not a constructor
undefined
PROGRESS RegExp
PROGRESS Splay
RESULT Splay 127
PROGRESS NavierStokes
RESULT NavierStokes 11.7
SCORE 52.7
Uncaught Error: Benchmark had 1 errors

There is performance increase while greatly simplifying the execution :)

There are still some to-dos left (updated in the PR comment) that I have to do, in the mean time making this ready for review :)

@HalidOdat HalidOdat marked this pull request as ready for review July 21, 2023 00:19
@HalidOdat HalidOdat requested a review from a team July 21, 2023 00:19
@HalidOdat HalidOdat added this to the v0.18.0 milestone Jul 21, 2023
@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch from e79a846 to 91986f1 Compare July 22, 2023 01:40
@raskad
Copy link
Member

raskad commented Jul 22, 2023

I really like this change. The logic is much simpler than before. Amazing work in this refactor! As soon as all todos are done, I will give it a second review :)

Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Hey! This is all looking pretty amazing! Thought I'd provide some early feedback on a couple things.

Sort of an aside: I'm curious what you were thinking about regarding Yield and GeneratorResumeKind given the changes. The one's in this PR are fine, but I keep thinking that they could probably be cleaned up by adding something like an execution context stack to the vm. Maybe that's the wrong approach though.

boa_engine/src/vm/code_block.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/code_block.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler/expression/mod.rs Show resolved Hide resolved
boa_engine/src/vm/code_block.rs Outdated Show resolved Hide resolved
@HalidOdat
Copy link
Member Author

This PR is now complete! Ready for review/merge :)

Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Great work on this! Just one question I wanted to double check 😄

boa_engine/src/bytecompiler/jump_control.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested a review from nekevss July 25, 2023 04:46
@HalidOdat HalidOdat force-pushed the refactor-opcodes-environments branch from 95d3566 to b653639 Compare July 25, 2023 05:30
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

One more small misspelling that I noticed in docs. 😄 But overall this looks great!

boa_engine/src/bytecompiler/jump_control.rs Outdated Show resolved Hide resolved
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.

Amazing refactor!

@raskad raskad added this pull request to the merge queue Jul 29, 2023
Merged via the queue into main with commit be055a3 Jul 29, 2023
9 checks passed
@HalidOdat HalidOdat deleted the refactor-opcodes-environments branch July 29, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related changes and issues technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JumpIfFalse skips over PopEnvironment instruction
4 participants