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 ordinary VM calling #3295

Merged
merged 5 commits into from
Oct 19, 2023
Merged

Refactor ordinary VM calling #3295

merged 5 commits into from
Oct 19, 2023

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Sep 21, 2023

This PR is a refactor of ordinary vm function calls, currently the function calls for JavaScript functions recurse, which causes a stack overflow.

>> $boa.limits.recursion = 100000
>> function fact(n) { if (n == 0) { return 1} return n * fact(n - 1) } fact(10000)
Uncaught: RuntimeLimit: Maximum call stack size exceeded

Now we are no longer bound on native call stack size :)

  • Non-recursive JavaScript calling
    • Ordinary function call
    • Bound-functions calls
    • Proxy calls
  • Non-recursive JavaScript constructing
    • Ordinary function construct
    • Bound-functions construct
    • Proxy construct (partially done)
  • Pick call/construct arguments in reverse order to avoid reversing
  • Prevent recursing in __call__ and __construct__ internal methods
  • Document calling of __call__ and __construct__ function

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,609 95,609 0
Passed 75,690 75,690 0
Ignored 19,160 19,160 0
Failed 759 759 0
Panics 0 0 0
Conformance 79.17% 79.17% 0.00%

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

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

Comparison is base (a9e3849) 45.71% compared to head (4f41c38) 45.78%.
Report is 3 commits behind head on main.

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     
Files Coverage Δ
boa_engine/src/builtins/eval/mod.rs 66.00% <100.00%> (+1.78%) ⬆️
boa_engine/src/builtins/generator/mod.rs 36.02% <100.00%> (-0.93%) ⬇️
boa_engine/src/builtins/json/mod.rs 85.13% <100.00%> (+0.22%) ⬆️
boa_engine/src/bytecompiler/declarations.rs 51.05% <100.00%> (-0.12%) ⬇️
boa_engine/src/bytecompiler/expression/mod.rs 58.41% <100.00%> (+0.19%) ⬆️
boa_engine/src/bytecompiler/mod.rs 66.92% <ø> (ø)
boa_engine/src/object/internal_methods/mod.rs 87.44% <100.00%> (+2.82%) ⬆️
boa_engine/src/object/operations.rs 52.36% <100.00%> (+3.96%) ⬆️
boa_engine/src/script.rs 82.53% <100.00%> (+1.50%) ⬆️
boa_engine/src/vm/opcode/await/mod.rs 73.75% <100.00%> (+1.38%) ⬆️
... and 22 more

... and 2 files with indirect coverage changes

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

@HalidOdat HalidOdat force-pushed the refactor/vm-function-calling branch 6 times, most recently from c414191 to 4d2a14b Compare September 28, 2023 14:27
@HalidOdat HalidOdat force-pushed the refactor/vm-function-calling branch 3 times, most recently from a73be22 to b72857a Compare October 9, 2023 21:44
@HalidOdat HalidOdat marked this pull request as ready for review October 9, 2023 23:55
@HalidOdat HalidOdat requested a review from a team October 9, 2023 23:55
@HalidOdat HalidOdat added this to the v0.18.0 milestone Oct 9, 2023
@HalidOdat HalidOdat added the execution Issues or PRs related to code execution label Oct 9, 2023
@HalidOdat HalidOdat force-pushed the refactor/vm-function-calling branch 2 times, most recently from cbd24a5 to 855fe43 Compare October 11, 2023 20:25
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.

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})

@raskad raskad requested a review from a team October 14, 2023 14:39
@HalidOdat
Copy link
Member Author

HalidOdat commented Oct 14, 2023

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 map takes, all the ordinary calls don't extend the native call stack, including the ordinary functions that map calls.

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) })

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.

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/object/internal_methods/mod.rs Outdated Show resolved Hide resolved

pub(crate) realm: Realm,

pub(crate) exit_early: bool,
Copy link
Member

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.

Copy link
Member Author

@HalidOdat HalidOdat Oct 16, 2023

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 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 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.

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().

- Prevent recursing in `__call__` and `__construct__` internal methods
boa_engine/src/error.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested review from jedel1043 and a team October 17, 2023 04:38
@HalidOdat HalidOdat requested review from nekevss and a team October 18, 2023 17:22
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.

Nice work on this! Just came across a couple things I was curious about 😄

boa_engine/src/vm/call_frame/mod.rs Show resolved Hide resolved
boa_engine/src/module/source.rs Outdated Show resolved Hide resolved
boa_engine/src/module/synthetic.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested review from nekevss and a team October 19, 2023 08:48
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.

Looks good to me!

@nekevss nekevss added this pull request to the merge queue Oct 19, 2023
Merged via the queue into main with commit 1d66836 Oct 19, 2023
14 checks passed
@HalidOdat HalidOdat deleted the refactor/vm-function-calling branch October 19, 2023 15:59
sam-finch-tezos pushed a commit to trilitech/boa that referenced this pull request Nov 29, 2023
* Refactor ordinary VM calling

- Prevent recursing in `__call__` and `__construct__` internal methods

* Apply review

* Refactor `Context::run()`

* Fix typo

* Apply review
@HalidOdat HalidOdat restored the refactor/vm-function-calling branch December 3, 2023 22:55
@HalidOdat HalidOdat deleted the refactor/vm-function-calling branch December 3, 2023 22:56
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 technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants