-
Notifications
You must be signed in to change notification settings - Fork 861
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
Conversation
Looks good to me -- does anyone who is deep in to the continuation stuff have an opinion or want to try it out? |
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 |
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
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? |
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.) |
Thank you for considering all that. Looks good to me! |
Commit f3c64ee removed `ObjArray` and replaced its usage with standard JDK classes. In `Interpreter`, in particular, an `ArrayDeque` is now used to store `cx.previousInterpreterInvocations`, which is used to generate the stack frame information. However, there is one place where `toArray` is done, and the behavior for `ObjArray` and `ArrayDeque` are different (swapped). Unfortunately no tests actually ends up exercising this difference due to the interpreter function peeling optimization done in mozilla#1510. We have discovered this problem because, in ServiceNow's fork, we currently need to disable the function peeling optimization.
Commit f3c64ee removed `ObjArray` and replaced its usage with standard JDK classes. In `Interpreter`, in particular, an `ArrayDeque` is now used to store `cx.previousInterpreterInvocations`, which is used to generate the stack frame information. However, there is one place where `toArray` is done, and the behavior for `ObjArray` and `ArrayDeque` are different (swapped). Unfortunately no tests actually ends up exercising this difference due to the interpreter function peeling optimization done in #1510. We have discovered this problem because, in ServiceNow's fork, we currently need to disable the function peeling optimization.
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 aNoSuchMethodHandleShim
.Also handles their combinations. Prior to this change, the best-effort for
Function.apply
,Function.call
, andNoSuchMethodHandleShim
only worked if their target was directly anInterpretedFunction
. 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-levelArrowFunction
/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) =>