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

Varying length instruction operands #3253

Merged
merged 3 commits into from
Sep 30, 2023
Merged

Varying length instruction operands #3253

merged 3 commits into from
Sep 30, 2023

Conversation

HalidOdat
Copy link
Member

Depends on #3201

Currently almost all operands are u32 size, this is because it is enough for most cases, but we waste space since most of the time the size is smaller than u8::MAX.

This is an optimization that reduces that size of the operands of bytecode instructions, by default if the instruction is executed then it is assumed that the operands are u8 type. When we need a larger size we prefix the opcode with a modifier (Half for u16 or Wide for u32).

This allows us to greatly reduce the memory footprint of the generated bytecode.

V8 does this optimization, but it is not exclusive to v8 bytecode, since most CISC architectures have this as well (x86 being of of them).

@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 Sep 3, 2023
@HalidOdat HalidOdat changed the title Varying length operand Varying length operands Sep 3, 2023
@HalidOdat HalidOdat changed the title Varying length operands Varying length instruction operands Sep 3, 2023
@HalidOdat
Copy link
Member Author

HalidOdat commented Sep 3, 2023

Ran the full quickjs benchmarks and collected the size of all the bytecode generated.

Main

Total bytecode generated size: 202.411KB

PR

Total bytecode generated size: 110.361KB

This is a 1.83x reduction in size there are still some opcodes that are left once everything is done will make this as ready for review. But feel Feel free to give feedback :)

@jasonwilliams
Copy link
Member

This is awesome! Great optimization

Base automatically changed from instruction-iterator to main September 7, 2023 07:49
@HalidOdat HalidOdat force-pushed the varying-length-operand branch from fcb4182 to af5d660 Compare September 8, 2023 14:11
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,574 95,574 0
Passed 75,193 75,193 0
Ignored 19,482 19,482 0
Failed 899 899 0
Panics 0 0 0
Conformance 78.68% 78.68% 0.00%

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: 660 lines in your changes are missing coverage. Please review.

Comparison is base (70ee050) 49.72% compared to head (1fba8c2) 49.36%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3253      +/-   ##
==========================================
- Coverage   49.72%   49.36%   -0.37%     
==========================================
  Files         443      444       +1     
  Lines       43378    43843     +465     
==========================================
+ Hits        21570    21642      +72     
- Misses      21808    22201     +393     
Files Coverage Δ
boa_engine/src/bytecompiler/expression/unary.rs 100.00% <100.00%> (ø)
boa_engine/src/bytecompiler/utils.rs 78.57% <100.00%> (ø)
boa_engine/src/vm/opcode/nop/mod.rs 0.00% <ø> (ø)
boa_engine/src/bytecompiler/expression/binary.rs 74.62% <0.00%> (ø)
boa_engine/src/bytecompiler/statement/loop.rs 76.26% <75.00%> (ø)
boa_engine/src/module/source.rs 0.00% <0.00%> (ø)
boa_engine/src/bytecompiler/expression/mod.rs 56.58% <80.00%> (-0.22%) ⬇️
boa_engine/src/vm/opcode/modifier.rs 50.00% <50.00%> (ø)
boa_engine/src/vm/opcode/push/literal.rs 76.92% <75.00%> (-23.08%) ⬇️
...gine/src/bytecompiler/expression/object_literal.rs 54.16% <55.55%> (ø)
... and 34 more

... and 1 file with indirect coverage changes

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

@HalidOdat HalidOdat force-pushed the varying-length-operand branch from af5d660 to eca1b75 Compare September 16, 2023 13:03
@HalidOdat HalidOdat requested a review from a team September 16, 2023 13:03
@HalidOdat HalidOdat added this to the v0.18.0 milestone Sep 16, 2023
@HalidOdat HalidOdat marked this pull request as ready for review September 16, 2023 13:03
@HalidOdat HalidOdat force-pushed the varying-length-operand branch from eca1b75 to 1820df1 Compare September 16, 2023 16:12
@HalidOdat HalidOdat force-pushed the varying-length-operand branch 2 times, most recently from a53595f to a1af47c Compare September 28, 2023 14:09
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.

Really nice work. I just have a few comments / questions.

boa_engine/src/vm/flowgraph/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/opcode/modifier.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/opcode/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/opcode/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Really nice optimization work! The only suggestion I have would be to make Opcode::Half/Wide an operand itself. This would simplify the logic a lot, since Operand would remain the same, and only instructions that want to opt-in into the varying operands logic would read the first operand as a VaryingOperandKind, then read the rest of the operands based off of that.
What do you think?

@HalidOdat
Copy link
Member Author

This would be a more simple approach, but it would incurred memory and performance penalties.

It would require an additional byte per varying instruction, additionally we would have to add logic for conditionally branching based on that varying kind (Short, Half, Wide) to read the correct sized operand.

I favored the current approach because by default, it's assumed that the operands are Short a byte no conditions or varying kind needed to be stored and when we prefix with the Half, or Wide modifiers we use the opcode execution table to jump directly, and the Self::operation() call is the last returned so it should be tail call optimized.

So we only pay a price (additional table jump) when we need more than a Short operand.

@jedel1043
Copy link
Member

It would require an additional byte per varying instruction

Ahhh, because we would have to store the varying kind for all varying instructions even if the instruction is Short, right?
Yep, that sounds bad 😅 This design is fine then :)

@HalidOdat HalidOdat force-pushed the varying-length-operand branch from 29974b7 to 58e3a7d Compare September 29, 2023 19:07
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Just some small nitpicks that don't block merging. Really well done!

boa_engine/src/vm/opcode/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/opcode/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat force-pushed the varying-length-operand branch from dc90423 to 1fba8c2 Compare September 30, 2023 11:41
@HalidOdat HalidOdat enabled auto-merge September 30, 2023 11:51
@HalidOdat HalidOdat added this pull request to the merge queue Sep 30, 2023
Merged via the queue into main with commit 332fd65 Sep 30, 2023
10 checks passed
@HalidOdat HalidOdat deleted the varying-length-operand branch September 30, 2023 12:29
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 vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants