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

Register VM #3798

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Register VM #3798

wants to merge 5 commits into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Apr 9, 2024

This PR transitions the current stack based VM to a register based VM.

There are a many changes in this, but I will try to highlight the most important ones.

  • The register allocation machanism in the bytecompiler has already been implemented in Implement register allocation #3942
  • I moved the register storage at runtime out of the Vm struct into a dedicated Registers struct that is passed to the vm execution functions together with the Context. The reason for this is mostly to be able to get references to register values without borrowing the Context. In some opcodes, the Clone rate of JsObjects significantly increased with the move to registers, because I had to clone all register values before operating on them, because the operation required a &mut Context.
  • Most logic in the bytecompiler is (IMO) way easier to understand now. I could eliminate 13 opcodes, and most importantly Dup, Swap, RotateLeft and RotateRight are gone. Working with registers turns out to be way more intuitive than working on a stack.
  • There is a lot of boilerplate in the opcode implementation now, since many more opcodes now read the register locations and need implementations for u8, u16 and u32 sizes. I think we should probably look into reworking the opcode generation macros, to possibly add both emit and read_operands functions. This should also make the code a lot safer, since right now we have to manually implement all operand reading and writing.
  • There is still some usage of the stack left. In particular all Call / New opcodes use the stack for all arguments and the return values. Also the return value and returned resume kind for yielding opcodes (Await, etc.) still use the stack. Currently I'm not sure how / if we want to change this. There are some significant challanges here, especially with how our function calls currently work.
  • I tried to unify most opcode descriptions with their relevant input / output values to make those more clear. I also adjusted the instruction_operands print function for all opcodes. A significant amout of lines changed is just that.
  • Performance had initially regressed notably until I moved the registers to their dedicated struct. As far as I can tell, this was mainly from cloning JsObjects and the associated gc. Currently the object clone rate is still higher than with a the stack vm. I measured this on the EarleyBoyer benchmark and last time I checked, the clone rate is ~1,35B vs ~1,42B. In my local benchmark runs the performance regression is between ~1% and ~3%.

@HalidOdat HalidOdat added execution Issues or PRs related to code execution Internal Category for changelog labels Apr 9, 2024
Copy link

github-actions bot commented Apr 9, 2024

Test262 conformance changes

Test result main count PR count difference
Total 48,625 48,625 0
Passed 43,616 43,618 +2
Ignored 1,471 1,471 0
Failed 3,538 3,536 -2
Panics 0 0 0
Conformance 89.70% 89.70% +0.00%
Fixed tests (2):
test/language/expressions/super/prop-expr-getsuperbase-before-topropertykey-putvalue-increment.js (previously Failed)
test/language/expressions/super/prop-expr-getsuperbase-before-topropertykey-putvalue-compound-assign.js (previously Failed)

@HalidOdat HalidOdat force-pushed the refactor/register-vm branch from 7351c87 to 039bd2b Compare June 27, 2024 20:05
@HalidOdat HalidOdat force-pushed the refactor/register-vm branch 2 times, most recently from f98cd87 to 1e0bf01 Compare July 5, 2024 15:26
@HalidOdat
Copy link
Member Author

The failing test is due to #3907 , in any case I'll work on always returning i32 for increment.

@HalidOdat HalidOdat force-pushed the refactor/register-vm branch 2 times, most recently from 5fd2167 to 50e8339 Compare August 3, 2024 11:07
@raskad
Copy link
Member

raskad commented Oct 10, 2024

@HalidOdat Do you have the time to work on this at the moment? If not, I can see if I can make some progress on the PR.

@HalidOdat
Copy link
Member Author

I'm quite busy at the moment, but feel free to work on it! 😊

@raskad raskad force-pushed the refactor/register-vm branch from 50e8339 to 2bab5ac Compare December 10, 2024 00:22
@raskad raskad added this to the next-release milestone Dec 11, 2024
@raskad raskad marked this pull request as ready for review December 11, 2024 19:07
@raskad raskad requested a review from a team December 11, 2024 19:07
@raskad raskad force-pushed the refactor/register-vm branch from a2631ed to 0cecbb6 Compare December 20, 2024 03:19
HalidOdat and others added 5 commits December 20, 2024 03:27
- Add todo for storing fp in CallFrame
- Start on moving to register VM
- Add more binary ops
- Add helpter transition methods
- Register based logical ops
- Register InPrivate
- Register GetFunction
- Some class opcodes
- Register post and pre increment and decrement
@raskad raskad force-pushed the refactor/register-vm branch from 0cecbb6 to aeb2e2f Compare December 20, 2024 04:24
@raskad
Copy link
Member

raskad commented Dec 26, 2024

@jedel1043 @HalidOdat Could you give this a look? I was looking into making some some changes to some opcodes, but didn't want to rebase this all the time, so it would be nice if we could merge it soon.

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 Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants