1415853 - Use a trampoline for JIT -> interpreter calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

To make a scripted call from JIT code, we currently load and null-check script->baselineOrIonRaw. It would be much nicer if this pointer was always non-null - if the script has no JIT code it could point to a trampoline that calls into C++. Then we can eliminate the null check and we can also simplify regalloc a bit in some places.

The attached patch implements this and it works well. I didn't expect this to improve perf that much, but it's actually a pretty big win. For the micro-benchmark below I get the following with --ion-inlining=off:

before: 420 ms
after:  369 ms

The one downside here is that we now need to create the JitRuntime in JSRuntime::init. This doesn't matter too much for the main thread because we create a JitRuntime anyway when starting Firefox, but it could affect workers. I want to look into this more - if needed we could allocate most of the JitRuntime trampolines lazily.

---

function g() { return 1; };
function f() {
    var res = 0;
    for (var i = 0; i < 100000000; i++)
        res += g();
    return res;
}
var t = new Date;
f();
print(new Date - t);

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Benjamin, think you could review this one? nbp already got a lot of review requests from me this week, and this patch touches a lot of code you modified recently. The rest is very similar to the lazy link stub code.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> The one downside here is that we now need to create the JitRuntime in
> JSRuntime::init. This doesn't matter too much for the main thread because we
> create a JitRuntime anyway when starting Firefox, but it could affect
> workers. I want to look into this more - if needed we could allocate most of
> the JitRuntime trampolines lazily.

I fixed bug 1416572, bug 1416592, bug 1417398 this week to speed up JitRuntime::init, so I hope we can do this now.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8929482 [details] [diff] [review]
Patch

Review of attachment 8929482 [details] [diff] [review]:
-----------------------------------------------------------------

Looks plausible to me, thanks!

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1713,5 @@
>      Address setterAddr(stubAddress(reader.stubOffset()));
>      ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
>  
> +    // First, ensure our setter is non-lazy. This also loads the callee in
> +    // scratch1.

nit: second sentence can be removed

::: js/src/jit/BaselineIC.cpp
@@ +2205,5 @@
>          // Likewise, if the callee is a class constructor, we have to throw.
>          if (!constructing && fun->isClassConstructor())
>              return true;
>  
> +        if (!fun->hasScript()) {

This is dead code: we're within an `if (fun->hasScript())` body.

::: js/src/jit/CodeGenerator.cpp
@@ +8046,5 @@
> +    masm.enterFakeExitFrame(temp0, temp0, ExitFrameType::InterpreterStub);
> +    masm.moveStackPtrTo(temp1);
> +
> +    masm.setupUnalignedABICall(temp0);
> +    masm.loadJSContext(temp0);

Could we use another temporary register to memoize JSContext and avoid this second load?

@@ +8057,5 @@
> +    masm.leaveExitFrame();
> +
> +    // InvokeFromInterpreterStub stores the return value in argv[0], where the
> +    // caller stored |this|.
> +    masm.loadValue(Address(masm.getStackPointer(), JitFrameLayout::Size()), JSReturnOperand);

nit: use JitFrameLayout::offsetOfThis() instead of JitFrameLayout::Size()? Same effect, better blackboxing.

::: js/src/jit/JitFrames.cpp
@@ +1145,5 @@
>          return;
>      }
>  
>      if (frame.isExitFrameLayout<LazyLinkExitFrameLayout>()) {
> +        auto layout = frame.exitFrame()->as<LazyLinkExitFrameLayout>();

nit: auto*

@@ +1155,3 @@
>  
> +    if (frame.isExitFrameLayout<InterpreterStubExitFrameLayout>()) {
> +        auto layout = frame.exitFrame()->as<InterpreterStubExitFrameLayout>();

ditto

::: js/src/jit/JitFrames.h
@@ +880,5 @@
>  }
>  
> +template <>
> +inline InterpreterStubExitFrameLayout*
> +ExitFrameLayout::as<InterpreterStubExitFrameLayout>()

nit: maybe put the InterpreterSutbExitFrameLayout class just above this method? Seems to be the way it's done in this file (and it's putting back the specialization for LazyLinkExitFrameLayout next to its class)

::: js/src/jit/VMFunctions.cpp
@@ +135,5 @@
> +    // Ion's CallKnown might have bypassed the arguments rectifier. Ensure
> +    // new.target immediately follows the actual arguments. See also
> +    // InvokeFunctionShuffleNewTarget.
> +    if (constructing && numActualArgs < fun->nargs())
> +        argv[1 + numActualArgs] = argv[1 + fun->nargs()];

I imagine the caller always puts 1 + fun->nargs() value slots even if it did bypass the arguments rectifier, right?

Comment 4

[Jan de Mooij [:jandem]]

Great review comments!

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> > +    // First, ensure our setter is non-lazy. This also loads the callee in
> > +    // scratch1.
> 
> nit: second sentence can be removed

I'd like to keep this part because the code after this depends on the callee being in scratch1 still :)

> This is dead code: we're within an `if (fun->hasScript())` body.

Oops, right, removed. I think we can do a bit better here, will file a follow-up bug.

> Could we use another temporary register to memoize JSContext and avoid this
> second load?

Yeah, done. I think we have at least 3 volatile regs on all platforms...

> I imagine the caller always puts 1 + fun->nargs() value slots

Yeah the arguments rectifier adds padding. I changed the comment to:

    // Ensure new.target immediately follows the actual arguments (the arguments
    // rectifier added padding). See also InvokeFunctionShuffleNewTarget.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81d21aaf172
Use a trampoline for JIT -> interpreter calls. r=bbouvier

Comment 6

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/b81d21aaf172