685358 - Always allow on stack recompilation for methodjit code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Right now OSR can only happen with TI enabled or in debug mode.  Otherwise, stock JM is not guaranteed to fully sync the interpreter stack when calling and we cannot rejoin into the interpreter.  This causes a fair amount of scattered complexity as managing jitcode lifetime is handled differently with/without OSR, prevents us from being able to always release all mjit code on GC, prevents us from able to turn on debug mode with stuff on the stack, and forces GC stuff to need to think about jitcode (JITScript::trace, incremental GC, ...).

The benefits of not fully syncing the stack are a pretty tiny perf win on SS and, I guess, other tests where we go into the VM over and over again.  With TI enabled by default this is kind of moot, though, and it would be good to just always enforce that JM keeps a synced stack and its jitcode is always manged the same way.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 2

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/projects/jaegermonkey/rev/cc2daf6cbaab

Comment 3

[Brian Hackett [Laid off!]]

Attachment: followup

Followup to fix JM+TM integration so that it works in the presence of code discarding on GC.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 559541 [details] [diff] [review]
patch

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

Nice, I like that a whole lot of complex code just went away. I wouldn't be surprised if this improves crash-stats.

::: js/src/methodjit/FrameState.cpp
@@ +1323,5 @@
>       */
>      Registers avail(freeRegs.freeMask & Registers::AvailRegs);
>      Registers temp(Registers::TempAnyRegs);
>  
> +    for (FrameEntry *fe = a->sp - 1; fe >= entries; fe--) {

Please check that this does not regress bug 591836, our compile time exploded when we had to scan all locals on every out-of-line call.

Comment 5

[Brian Hackett [Laid off!]]

Backing out until the tracer is removed.  This is still causing crashes in weird ways when JM invokes the tracer (no problems when this is disabled), I don't know why and keeping this in is destabilizing and delaying things for not much benefit.

https://hg.mozilla.org/projects/jaegermonkey/rev/f933cbe46a03

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per Brian's comments in .js-engine, we probably need this to make chrome jitcode actually discardable at all...

Brian, is the fixed-in-jaegermonkey annotation correct given comment 5?

Comment 7

[Brian Hackett [Laid off!]]

No, this has been backed out of all branches.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: updated (dfa418ec22f1)

Updated patch, green on try.  This differs from the previous patch by removing JM+TM integration, so asking for a re-review on those parts.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 568652 [details] [diff] [review]
updated (dfa418ec22f1)

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

::: js/src/methodjit/InvokeHelpers.cpp
@@ -708,5 @@
> - * it. This will unwind to a given frame, or find and align to an exception
> - * handler in the process.
> - */
> -static inline bool
> -HandleErrorInExcessFrame(VMFrame &f, StackFrame *stopFp, bool searchedTopmostFrame = true)

Goodbye, incredibly complex code!

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/049a08dfadc2

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/049a08dfadc2

Comment 12

[Brendan Eich [:brendan]]

Cause of http://arewefastyet.com/?a=b&machine=9 regression?

/be

Comment 13

[Brian Hackett [Laid off!]]

(In reply to Brendan Eich [:brendan] from comment #12)
> Cause of http://arewefastyet.com/?a=b&machine=9 regression?

Yes.  This patch removes JM+TM integration, so the JM+TM line will track the plain JM line:

http://arewefastyet.com/?a=b&view=regress

the JM, TM, and JM+TI lines did not change.

Comment 14

[Robert Kaiser]

(In reply to Brian Hackett from comment #13)
> Yes.  This patch removes JM+TM integration, so the JM+TM line will track the
> plain JM line

Does that also mean that chrome also has JM+TI on yet instead of JM+TM? And are we going to remove all the TM code in the Firefox 10 cycle as well?

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Robert Kaiser (:[e-mail]) from comment #14)
> (In reply to Brian Hackett from comment #13)
> > Yes.  This patch removes JM+TM integration, so the JM+TM line will track the
> > plain JM line
> 
> Does that also mean that chrome also has JM+TI on yet instead of JM+TM? And
> are we going to remove all the TM code in the Firefox 10 cycle as well?

Chrome code is running just JM.  TM was turned off by default across the board last week without affecting performance metrics.  I don't think either JIT makes much difference for chrome code, though chrome methodjit code was using excessive memory (fixed by this bug).  I think the plan right now is to remove TM early in the Firefox 11 cycle.

Comment 16

[Robert Kaiser]

(In reply to Brian Hackett from comment #15)
> Chrome code is running just JM.

OK. Do we have an open bug to turn on TI for chrome? I think it would make sense to have the same mechanisms in use for chrome and content - if it's safe enough to do that.

Comment 17

[Brian Hackett [Laid off!]]

(In reply to Robert Kaiser (:[e-mail]) from comment #16)
> (In reply to Brian Hackett from comment #15)
> > Chrome code is running just JM.
> 
> OK. Do we have an open bug to turn on TI for chrome? I think it would make
> sense to have the same mechanisms in use for chrome and content - if it's
> safe enough to do that.

No, we don't.  The issues there are that TI hasn't had much testing in the non-compileAndGo case, and it (and JM) are generally much less effective when dealing with non-compileAndGo code.  My hope was that compartment-per-global would rework things so that all code is compileAndGo and simplify these issues, but that is still some months away.