-
-
Notifications
You must be signed in to change notification settings - Fork 408
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 ordinary VM calling #3295
Conversation
Test262 conformance changes
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3295 +/- ##
==========================================
+ Coverage 45.71% 45.78% +0.06%
==========================================
Files 482 483 +1
Lines 49182 49349 +167
==========================================
+ Hits 22486 22595 +109
- Misses 26696 26754 +58
☔ View full report in Codecov by Sentry. |
c414191
to
4d2a14b
Compare
a73be22
to
b72857a
Compare
cbd24a5
to
855fe43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice.
Just to be clear, we still depend on the native call stack size when we call ordinary functions from native functions right? For example the following code will extend the call native call stack:
[0].map((e) => { return ++e})
It will extend the native call stack only as much as our rust implementation of So the following code will have the same effect as the previous on the native call stack: const fact = (n) => {
return n == 0 ? 1 : (n * fact(n - 1))
}
[100].map((e) => { return fact(e) }) |
855fe43
to
1b68a7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Still need to review it in a bit more detail, but thought I'd add a few comments from my initial review.
boa_engine/src/vm/call_frame/mod.rs
Outdated
|
||
pub(crate) realm: Realm, | ||
|
||
pub(crate) exit_early: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: Would it be better to have a singular bitflag
with more states eventually over a exit_early
and construct
?
I'd always hoped to eventually return to CompletionType
and remove CompletionType::Yield
in favor of the spec's SuspendedYield
state. This may be a bit out of scope on this PR, but I thought I'd bring it up on the off chance you had run across something while working on this or may have had some thoughts about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: Would it be better to have a singular
bitflag
with more states eventually over aexit_early
andconstruct
?
I was going to add that but, forgot 😄 , I don't think this would save space though, due to structure padding. Will add it anyway, if we add other flags, it will be easier.
I'd always hoped to eventually return to
CompletionType
and removeCompletionType::Yield
in favor of the spec'sSuspendedYield
state. This may be a bit out of scope on this PR, but I thought I'd bring it up on the off chance you had run across something while working on this or may have had some thoughts about it.
Hmmm. I think it's fine as long as we match the spec behaviour, we don't have to match the spec steps (this PR goes against spec, since it should recurses when calling any function). As long as we document why we don't do things according to spec, it should be fine :).
I wanted to also refactor generators, because they use the native stack on each .next()
call, and many opcodes that use .call()
.
2ffe401
to
217b17b
Compare
- Prevent recursing in `__call__` and `__construct__` internal methods
6d9a22f
to
3c398d2
Compare
3c398d2
to
2ae0a96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this! Just came across a couple things I was curious about 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
* Refactor ordinary VM calling - Prevent recursing in `__call__` and `__construct__` internal methods * Apply review * Refactor `Context::run()` * Fix typo * Apply review
This PR is a refactor of ordinary vm function calls, currently the function calls for JavaScript functions recurse, which causes a stack overflow.
Now we are no longer bound on native call stack size :)
__call__
and__construct__
internal methods__call__
and__construct__
function