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

Enable Jit for Generators, Asyncs and Async Generators #6662

Merged
merged 6 commits into from
Mar 25, 2021

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Mar 23, 2021

This PR enables the JIT in both FullJit and SimpleJit mode for Generator Functions, Async Functions and Async Generator Functions (Collectively referred to internally as Coroutines)

Exceptions

  • excludes ARM - I don't have access to an ARM machine to test, and our CI doesn't have ARM so seemed reckless to enable
  • excludes functions containing try/catch, may introduce complexities and not yet tested
  • disabled when a debugger is attached - may introduce complexities and not yet tested

Work Done in this PR

  1. Fix register mismatch in Jit

    • This was a bug in yield* statements within Async Generators
    • the Lifetime of registers used to store the iterator statement would be incorrect
    • fixed by using additional regs to extend the lifetime
  2. Fix handling of For-In

    • For-in loops use an enumerator objector stored in a cache
    • the logic to restore this across yields was not working correctly
    • fix by simplifying/removing a couple of steps and moving the load nearer to the point it is used
  3. Disable BailOnNoProfile in Coroutines

    • BailOnNoProfile results in code after it being marked as Dead
    • Consider the below example:
    • if a BailOnNoProfile is inserted after the yield the ++i is marked as dead
    • as ++i is dead, i is marked as a single definition int - a constant
    • then you get yield 0 -> BailOnNoProfile -> yield 1 (in the interpreter) -> try jitted code again - Bails out with i = 0 as it is a const -> yield 1 (in the interpreter)
for (let i = 0; i < 3; ++i)
{
  yield i;
} 
  1. Remove unnecessary startup yield

    • this is a minor optimisation BUT in the context of jitted generators avoids a compulsory extra bailout
    • also makes code much neater
  2. Enable

    • change flags so it's all enabled
    • add test cases for key bugs mentioned above

Future Work
A. Look at enabling for functions containing try/catch - needs to be explored don't know what issues may arise
B. Create an alternative form of BailOnNoProfile that can be used in Coroutines
C. Look at Jitting loop bodies that don't contain yield - (including loop bodies in Modules)

Fix #5877

- For in loops use an enumerator object
- for optimisation reasons this is cached in a known location
- in nested for-in loops containing yield cache could fail to restore
- remove a step from the process and bring load nearer to usage
- BailOnNoProfile marks all code after it as dead
- if variable is used in a yield BUT incremented after BailOnNoProfile
- Jit would delete the increment and mark variable as constant
- interpreter would increment BUT JIT would reload as const
- would yield same value multiple times

Ideally should introduce Generator/Async specific version of BailOnNoProfile
That doesn't mark code after it as dead - but that would be larger work.
For now disable BailOnNoProfile for generators/Asyncs.
- Extra yield inserted at the top of Generator functions
- so Params with side effects can be executed upon function call
- then stop at the yield untill .next() is used
- in cases where there are no params with side effets his is not needed
- use a condition to only do it when needed - saves several extra ops
- Had to update TTD code for this (it expected all Gens to have state)
- AsyncGenerator parameters needed copying to heap on call
  previously this was done when executing until the startup yield
- exclude ARM which has not been tested
- excludes code running with debugger attached (not tested)
- excludes functions containing try/catch (not tested)
- excludes Module globals (unlikely to benefit)
- add tests for bugs fixed above
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Mar 23, 2021

@ppenzin sorry I know this isn't our top priority but I've been wanting to get it in for so long.... Please could you take a look when you have time.

@ppenzin
Copy link
Member

ppenzin commented Mar 25, 2021

I am glad you got it fixed, agree we should check this while it is still fresh. Do you want this shipping as part of the upcoming release? If not, we can cut the release branch right before merging it in.

Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generators and Async functions are not jitted
2 participants