Closed Bug 1415853 Opened 3 years ago Closed 3 years ago

Use a trampoline for JIT -> interpreter calls

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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);
Severity: normal → enhancement
Priority: -- → P2
Attached patch PatchSplinter Review
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.
Attachment #8926824 - Attachment is obsolete: true
Attachment #8929482 - Flags: review?(bbouvier)
(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 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?
Attachment #8929482 - Flags: review?(bbouvier) → review+
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.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81d21aaf172
Use a trampoline for JIT -> interpreter calls. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/b81d21aaf172
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1419359
Depends on: 1419758
You need to log in before you can comment on or make changes to this bug.