882111 - Invoke/RunScript should not push a StackFrame when calling into the JITs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Right now Invoke always pushes a StackFrame, even if RunScript will immediately enter one of the JITs. This is no longer necessary and is not good for performance.

Ideally we'd pass a list of arguments to RunScript and we'd only push a StackFrame when we are going to run in the interpreter.

I hope this will also allow us to remove the Ion FastInvoke hack: ion::Cannon will look a lot more like ion::FastInvoke.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Minor Interpret cleanup

Interpret is only called from RunScript so the patch makes Interpret static.

JM has been removed and Ion no longer bails out to the interpreter, so we can also remove the InterpMode and InterpretStatus enums.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 761394 [details] [diff] [review]
Part 1 - Minor Interpret cleanup

Mmmmm

Comment 3

[Jan de Mooij [:jandem]]

Part 1

https://hg.mozilla.org/integration/mozilla-inbound/rev/1646619b5221

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2

This patch passes a subclass of RunState to RunScript, Interpret and the JITs. RunState contains everything we need to build an interpreter or JIT frame.

Interpret is now responsible for pushing interpreter frames. This is nice because we no longer push an interpreter frame when we immediately enter the JITs, but also because it means we will push the StackFrame and enter the InterpreterActivation at the same time. That's also necessary if we want to mark StackFrames based on the InterpreterActivation: we don't want to GC between pushing the frame and entering the activation..

The patch also adds an EnterJitData struct. This struct can either be filled from a RunState struct or (when we OSR from the interpreter to baseline) a StackFrame. Right now the JITs pass 8 arguments to the EnterJit trampoline, eventually we should directly pass a pointer to the EnterJitData struct. Avoids copying, and multiple arguments are annoying because the way they are passed (registers or stack) is platform dependent.

Although performance is not the main reason for doing this, calling into the JITs from the interpreter or Invoke is about 10% faster now.

Note that this patch still uses FrameGuard, ExecuteType and InitialFrameFlags. I hope bug 881902 will remove FrameGuard etc, but I decided to use them till that happens.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 762729 [details] [diff] [review]
Part 2

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

From the parts I understand, this looks great.  I don't know enough about baseline/ion to review those changes though.  I think Kannan would be the right reviewer, though; plus it'd be good to have more people understanding the way the new Stack system works.

::: js/src/vm/Interpreter.cpp
@@ +314,5 @@
> +    futureState_(futureState),
> +    entered_(false)
> +{ }
> +
> +RunGeneratorState::~RunGeneratorState()

Could you put the GenerateState methods in jsiter.cpp next to GeneratorState::pushInterpreterFrame?

::: js/src/vm/Interpreter.h
@@ +166,5 @@
>  Execute(JSContext *cx, HandleScript script, JSObject &scopeChain, Value *rval);
>  
> +class ExecuteState;
> +class InvokeState;
> +class RunGeneratorState;

For consistency, could you name it just GeneratorState?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1646619b5221

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 762729 [details] [diff] [review]
Part 2

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

Nice.  This patch (or a follow-up) seems like an appropriate place to make Ion OSR jitcode also not rely on the old vm StackFrame layout (with its duplicate formalArgs, actualArgs, and unused fields of StackFrame).. and instead just use a custom layout generated by baseline OSR.

::: js/src/vm/Interpreter.cpp
@@ +1329,5 @@
>  
>              interpReturnOK = (maybeOsr == ion::IonExec_Ok);
>  
>              if (entryFrame != regs.fp())
> +                goto jit_return_popFrame;

Calling the label jit_return_pop_frame seems to fit naming convention better.

@@ +1419,5 @@
>          else
>              Probes::exitScript(cx, script, script->function(), regs.fp());
>  
> +#if defined(JS_ION)
> +  jit_return_popFrame:

see above about jit_return_pop_frame

::: js/src/vm/Interpreter.h
@@ +179,5 @@
> +    Kind kind_;
> +
> +    RootedScript script_;
> +
> +    explicit RunState(JSContext *cx, Kind kind, JSScript *script)

Might be worthwhile to add implicit constructors for RunState from (const ExecuteState &) and (const InvokeState &) and (const RunGeneratorState &), which are marked MOZ_DELETE, to prevent RunStates from being copied from their derived class (and leading to an unexpected truncate).

The use case for this class is to be allocated on stack and then to have references/pointers passed around.  Enforcing that would be good.

Comment 8

[Jan de Mooij [:jandem]]

Pushed with comments from Luke and Kannan addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3efe3f3d2c25

Comment 9

[Alon Zakai (:azakai)]

It looks like this regressed zlib (non-odin) on awfy by 9.5%. The only other changeset in the range is a commit and a backout to that commit.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3efe3f3d2c25