[Merged by Bors] - Fix internal vm tests #1718
Closed
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
raskad
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify and
jedel1043
Codecov Report
@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 53.38% 53.08% -0.30%
==========================================
Files 200 200
Lines 16967 17059 +92
==========================================
- Hits 9057 9056 -1
- Misses 7910 8003 +93
Continue to review full report at Codecov.
|
Test262 conformance changesNon-VM implementation
VM implementation
Fixed tests (1154):Broken tests (7):Fixed panics (177): |
jasonwilliams
approved these changes
Dec 3, 2021
jedel1043
requested changes
Dec 3, 2021
There was a problem hiding this comment.
I'm seeing there's a lot of OpCodes that are used only once. Would it be feasible to create a OpCode::CallRs operation that takes a function pointer from the stack, then casts it to a fn(&mut CodeBlock, &mut usize) -> String and calls it with function(self, pc)?
Maybe it would impact the performance a bit because of the indirection, but it would reduce the complexity of instruction_operands and OpCode a lot :)
Razican
reviewed
Dec 4, 2021
Razican
reviewed
Dec 4, 2021
I also though about this a bit, but I think it's probably better to just create the opcodes. I looked at how SpiderMonkey deals with some of this. For example they also have a |
|
I think we're better with individual opcodes then creating a special one that actually invokes N other functions. That's just moving the indirection to a later point in execution, I think it would make it harder to understand the code and most likely not help performance. |
RageKnify
reviewed
Dec 7, 2021
RageKnify
reviewed
Dec 7, 2021
raskad
force-pushed
the
vm-fix-rs-tests
branch
from
1b05e64 to
980e719
Compare
raskad
force-pushed
the
vm-fix-rs-tests
branch
from
980e719 to
3dfd0a7
Compare
|
@HalidOdat I've got a question for you. I want to fix the individual opcode documentation, as some of them differ. What order should the stack values have in the doc comments? e.g.: What is the appropriate order / where is the top of the stack? But I'm not sure how to best represent this. |
|
Hmmm... IMO the |
RageKnify
approved these changes
Dec 7, 2021
HalidOdat
approved these changes
Dec 10, 2021
|
bors r+ |
|
👎 Rejected by code reviews |
jedel1043
approved these changes
Dec 11, 2021
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Dec 11, 2021
This PR fixes some vm implementation code. All our internal tests should now pass with the vm enabled. There are only a few (~100) 262 tests left that currently break with the vm, that previously worked.
I’m also fine with this, would be good to focus on that going forward |
|
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 fixes some vm implementation code. All our internal tests should now pass with the vm enabled.
There are only a few (~100) 262 tests left that currently break with the vm, that previously worked.