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

Implement Tail Call Optimization #3510

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Implement Tail Call Optimization #3510

wants to merge 4 commits into from

Conversation

RageKnify
Copy link
Member

@RageKnify RageKnify commented Dec 6, 2023

This Pull Request implements Tail Call Optimization as described in the spec: tail-position-calls

It changes the following:

  • introduce a new instruction TailCall that can is compatible with Call and will perform TCO if possible
  • introduce ByteCompiler::optimize that introduces optimizations to the bytecode code

While the spec says that TCO should be implemented most engines do not currently support it. Apple's JavaScriptCore is the only mainstream implementation as far as I am aware. When it was first introduced V8 implemented it but after Firefox and Internet Explorer pushed back and it was clear not all browsers would have it V8 dropped it to avoid maintaining unnecessary code.

Given that I wanted to get all of @boa-dev/maintainers opinions on implementing these parts of the spec that aren't technically required. I did it mostly as a challenge since it is a topic I find interesting so I understand if the choice is to not incorporate the code.

Copy link

github-actions bot commented Dec 7, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,960 95,960 0
Passed 76,534 76,555 +21
Ignored 18,477 18,477 0
Failed 949 928 -21
Panics 0 0 0
Conformance 79.76% 79.78% +0.02%
Fixed tests (21):
test/language/statements/block/tco-stmt.js [strict mode] (previously Failed)
test/language/statements/block/tco-stmt-list.js [strict mode] (previously Failed)
test/language/statements/switch/tco-case-body-dflt.js [strict mode] (previously Failed)
test/language/statements/switch/tco-case-body.js [strict mode] (previously Failed)
test/language/statements/switch/tco-dftl-body.js [strict mode] (previously Failed)
test/language/statements/if/tco-else-body.js [strict mode] (previously Failed)
test/language/statements/if/tco-if-body.js [strict mode] (previously Failed)
test/language/statements/try/tco-catch.js [strict mode] (previously Failed)
test/language/statements/labeled/tco.js [strict mode] (previously Failed)
test/language/statements/return/tco.js [strict mode] (previously Failed)
test/language/expressions/tco-pos.js [strict mode] (previously Failed)
test/language/expressions/comma/tco-final.js [strict mode] (previously Failed)
test/language/expressions/tagged-template/tco-call.js [strict mode] (previously Failed)
test/language/expressions/tagged-template/tco-member.js [strict mode] (previously Failed)
test/language/expressions/logical-or/tco-right.js [strict mode] (previously Failed)
test/language/expressions/logical-and/tco-right.js [strict mode] (previously Failed)
test/language/expressions/call/tco-member-args.js [strict mode] (previously Failed)
test/language/expressions/call/tco-call-args.js [strict mode] (previously Failed)
test/language/expressions/conditional/tco-pos.js [strict mode] (previously Failed)
test/language/expressions/coalesce/tco-pos-undefined.js [strict mode] (previously Failed)
test/language/expressions/coalesce/tco-pos-null.js [strict mode] (previously Failed)

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

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

Comparison is base (279caab) 49.01% compared to head (11fc090) 49.05%.
Report is 24 commits behind head on main.

Files Patch % Lines
core/engine/src/vm/opcode/call/mod.rs 65.51% 10 Missing ⚠️
core/engine/src/vm/code_block.rs 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3510      +/-   ##
==========================================
+ Coverage   49.01%   49.05%   +0.03%     
==========================================
  Files         469      469              
  Lines       48186    48245      +59     
==========================================
+ Hits        23618    23666      +48     
- Misses      24568    24579      +11     

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

@jedel1043 jedel1043 added discussion Issues needing more discussion vm Issues and PRs related to the Boa Virtual Machine. labels Dec 25, 2023
@raskad
Copy link
Member

raskad commented Dec 28, 2023

The implementation looks pretty straight forward. I like it. @RageKnify do you know the more detailed reasons why V8 droped it as a maintanance burden? With our current VM it seems to work pretty well right? @HalidOdat do you think there could be future issues with this?

I would be in favor of merging this. If it becomes hard to maintain or blocks features in the future we could always remove for the same reason V8 did it.

@jedel1043
Copy link
Member

jedel1043 commented Dec 28, 2023

I'm in favour of merging this, but only if we add a way to disable the optimization.

I'm mostly worried about our recursion limit, which could be bypassed by this feature. Bonus points if we document this on the recursion limit setter/getter

EDIT: alternatively, if we want to always do the optimization, we could consider it as a "pseudo-loop", and make it count for the loop limit, but I'm not sure how feasible that would be with indirect recursion.

EDIT2: Found a source about how JSC preserves stack traces with PTC using a shadow stack (see "Debugging PTC with ShadowChicken") https://webkit.org/blog/6240/ecmascript-6-proper-tail-calls-in-webkit/

@Razican Razican changed the title Implment Tail Call Optimization Implement Tail Call Optimization Dec 28, 2023
@raskad
Copy link
Member

raskad commented Dec 29, 2023

EDIT: alternatively, if we want to always do the optimization, we could consider it as a "pseudo-loop", and make it count for the loop limit, but I'm not sure how feasible that would be with indirect recursion.

I like this idea. If we can make that work that would be my prefered solution.

@HalidOdat
Copy link
Member

@HalidOdat do you think there could be future issues with this?

Correct stack traces is the biggest concern (which @jedel1043 proposed a solution). I would prefer either waiting for the stack trace or we implement the JSC shadow stack (In this PR or a follow up PR) since if we implement stack traces we will have implement it anyway.

EDIT: alternatively, if we want to always do the optimization, we could consider it as a "pseudo-loop", and make it count for the loop limit, but I'm not sure how feasible that would be with indirect recursion.

It should be easy since we have access to the previous frame that is popped and we just increment and check.

NOTE: In general I would prefer the users use the budget functions to restrict the compute, how i see it is the recursion limit is more of a call frame limit, and tail call does not add any frames.

@raskad raskad mentioned this pull request Mar 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues needing more discussion 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