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.

Sign up for GitHub

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

Jump to bottom

JIT Generator (and Async func) loops #6990

Merged
merged 3 commits into from
May 16, 2024
Merged

JIT Generator (and Async func) loops #6990

merged 3 commits into from
May 16, 2024

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Apr 29, 2024 •
edited
Loading

Jitting of generator and async functions is buggy and has some potential performance cliffs. As an alternative this PR enables Jitting of loops that don't contain await or yield.

e.g. In this examples if the profiler found them to be hot loop 2 and loop 4 would be jitted:

async function boo()
{
  while (something) {  //loop 1
    while (stuff) {  // loop 2
      // do sums
    }
    while (otherStuff) { //loop 3
      await x
    }
  }
  while (moreStuff) { //loop 4
    //do more sums
  }
}

Whilst jitting the whole function may in theory offer better performance it's not actually clear that it would in light of the overheads of yielding out of jitted code AND the problems yield brings to optimising jitted code.

This alternative is much simpler; hopefully far more stable AND should target the most optimisable parts of these functions.

@rhuanjl rhuanjl marked this pull request as draft April 30, 2024 06:27
@ppenzin
Copy link
Member

ppenzin commented May 1, 2024

I think it is a pretty good idea.

@rhuanjl rhuanjl marked this pull request as ready for review May 2, 2024 17:33
@rhuanjl rhuanjl requested a review from ppenzin May 2, 2024 17:43
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented May 2, 2024 •
edited
Loading

I've moved all generator JIT test cases into a new folder and added additional test cases for testing loop body jit.

Note, with this PR we now have:

Default behaviour (no flag): do not Jit generator (or async functions) BUT do jit hot loops inside them that don't contain yield or await.

Flagged behaviour -JitEs6Generators: turns back on attempting to JIT the generator/async functions AND disables this loop body Jit, having both working together may be a distant goal BUT for now the function jit is going to be disabled; and their interaction has additional bugs vs just the function jit on its own.

@rhuanjl rhuanjl mentioned this pull request May 2, 2024
Open
5 tasks
rhuanjl
rhuanjl commented May 2, 2024
View reviewed changes
lib/Backend/IRBuilder.cpp
@@ -118,7 +118,7 @@ IRBuilder::DoBailOnNoProfile()
return false;
}

if (m_func->GetTopFunc()->GetJITFunctionBody()->IsCoroutine())
if (m_func->GetTopFunc()->GetJITFunctionBody()->IsCoroutine() && !m_func->IsLoopBody())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BailOnNoProfile caused problems when put in a loop containing a Yield; not a problem when jitting loop bodies that cannot contain yield.

lib/Backend/IRBuilder.cpp
@@ -441,7 +441,7 @@ IRBuilder::Build()
// Note that for generators, we insert the bailout after the jump table to allow
// the generator's execution to proceed before bailing out. Otherwise, we would always
// bail to the beginning of the function in the interpreter, creating an infinite loop.
if (m_func->IsJitInDebugMode() && !this->m_func->GetJITFunctionBody()->IsCoroutine())
if (m_func->IsJitInDebugMode() && (!this->m_func->GetJITFunctionBody()->IsCoroutine() || this->IsLoopBody()))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different debug break point for Generator functions BUT not relevant for LoopBodies.

lib/Backend/Lower.cpp
@@ -5467,7 +5467,7 @@ Lowerer::LowerPrologEpilog()
instr = m_func->m_exitInstr;
AssertMsg(instr->IsExitInstr(), "Last instr isn't an ExitInstr...");

if (m_func->GetJITFunctionBody()->IsCoroutine())
if (m_func->GetJITFunctionBody()->IsCoroutine() && !m_func->IsLoopBody())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generator function required custom Epilog, irrelevant for a loopBody.

lib/Backend/Lower.cpp
@@ -11544,7 +11545,7 @@ Lowerer::LowerArgIn(IR::Instr *instrArgIn)
if (argIndex == 1)
{
// The "this" argument is not source-dependent and doesn't need to be checked.
if (m_func->GetJITFunctionBody()->IsCoroutine())
if (m_func->GetJITFunctionBody()->IsCoroutine() && !m_func->IsLoopBody())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generator function arguments loaded from a different location; not relevant for LoopBodies.

rhuanjl
rhuanjl commented May 2, 2024
View reviewed changes
test/es6GeneratorJit/rlexe.xml
<files>jit-gen-loop-body.js</files>
<compile-flags>-testtrace:Backend</compile-flags>
<baseline>jit-gen-loop-body.baseline</baseline>
<tags>exclude_test, exclude_nonative, exclude_dynapogo</tags>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to exclude test builds from this test and 2 more of the below as the -testrace:backend flag that allows verification of which loops were jitted only functions in a debug build.

ppenzin
ppenzin approved these changes May 16, 2024
View reviewed changes
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.

Sorry, took a while to get to it.

@rhuanjl rhuanjl merged commit 615a614 into chakra-core:master May 16, 2024
22 of 24 checks passed
@rhuanjl rhuanjl deleted the jitGenLoops branch May 16, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppenzin ppenzin ppenzin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rhuanjl @ppenzin