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

Peel known functions in interpreter so they don't break continuations #1510

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

szegedi
Copy link
Contributor

@szegedi szegedi commented Jun 30, 2024

Fixes #1444

Avoids the interpreter from calling out from its current loop when it needs to invoke an arrow function, a lambda function, a bound function, Function.apply, Function.call, or a NoSuchMethodHandleShim.

Also handles their combinations. Prior to this change, the best-effort for Function.apply, Function.call, and NoSuchMethodHandleShim only worked if their target was directly an InterpretedFunction. With this change, the interpreter will keep drilling down through (or, depending on how you like to visualize it, peel away) the functions (modifying the arguments as those functions would when they're invoked) until it finds an interpreted function to install on its loop stack. If the ultimately invoked function is not interpreted, it will call out to it as before (except it won't call out to the top-level ArrowFunction/BoundFunction/etc but rather it already did the work for unwrapping them, so it'll only correctly call the final irreducible function.

The net effect is that interpreter will not leave its loop even for combinations of functions such as `(a, b, c) =>

@szegedi szegedi marked this pull request as ready for review June 30, 2024 12:03
@gbrail
Copy link
Collaborator

gbrail commented Jul 1, 2024

Looks good to me -- does anyone who is deep in to the continuation stuff have an opinion or want to try it out?

@szegedi
Copy link
Contributor Author

szegedi commented Jul 2, 2024

This doesn't really have to do anything with continuations per se. Rather, it's an improvement in the interpreter that increases the number of function invocation scenarios where the execution remains within a single invocation of the interpreter loop – which is a prerequisite for continuations working. So continuations working better is a consequence of this fix but continuations-related code itself was not touched.

I see @rbri and @tuchida were somewhat active in this code in the recent-ish past, maybe they want to review it.

FWIW, all 120k+ tests still pass and I added some new tests too. (I had lots of test262 failures while I was developing this until I got it right, both in /test262/test/built-ins/Function/prototype/{apply|call} and in other tests heavily using .call() and .apply() so there seems to be relevant safety nets aplenty.)

@gbrail
Copy link
Collaborator

gbrail commented Jul 3, 2024

I agree that this is not a super risky change, it's just such complicated code in general, and the code coverage is very good.

I see a few tiny holes in the interpreter test coverage. If you run

./gradlew testCodeCoverageReport

and look in ./tests/build/reports/jacoco/testCodeCoverageReport/html/index.html

you can see very good coverage but one or two places to improve. For example, it seems like we don't have tests where 'NativeContinuation.isContinuationConstructor' returns true, at least not in the "tests" module.

Would you mind taking a look and seeing if anything jumps out as an area where we might improve coverage?

@szegedi
Copy link
Contributor Author

szegedi commented Jul 4, 2024

The example you mentioned is incidentally not a particularly good one. It's code that existed in exactly the same form before my changes, I just moved it into the loop. I have pushed another commit now that simplifies the code and only keeps those paths in the loop that are absolutely necessary to be in there (those that deal with reducible functions.)
With this change, my changes are 100% covered by tests.

@gbrail
Copy link
Collaborator

gbrail commented Jul 4, 2024

Thank you for considering all that. Looks good to me!

@gbrail gbrail merged commit 955af15 into mozilla:master Jul 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants