Last Comment Bug 685358 - Always allow on stack recompilation for methodjit code
: Always allow on stack recompilation for methodjit code
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-07 16:33 PDT by Brian Hackett (:bhackett)
Modified: 2012-01-14 00:34 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (30.88 KB, patch)
2011-09-09 12:31 PDT, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Review
followup (24.23 KB, patch)
2011-09-13 15:04 PDT, Brian Hackett (:bhackett)
no flags Details | Diff | Review
updated (dfa418ec22f1) (87.95 KB, patch)
2011-10-21 08:19 PDT, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2011-09-07 16:33:44 PDT
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 (:bhackett) 2011-09-09 12:31:51 PDT
Created attachment 559541 [details] [diff] [review]
patch
Comment 2 Brian Hackett (:bhackett) 2011-09-10 22:57:28 PDT
http://hg.mozilla.org/projects/jaegermonkey/rev/cc2daf6cbaab
Comment 3 Brian Hackett (:bhackett) 2011-09-13 15:04:33 PDT
Created attachment 560044 [details] [diff] [review]
followup

Followup to fix JM+TM integration so that it works in the presence of code discarding on GC.
Comment 4 David Anderson [:dvander] 2011-09-13 16:18:49 PDT
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 (:bhackett) 2011-09-14 22:15:19 PDT
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 [:bz] 2011-10-02 21:15:34 PDT
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 (:bhackett) 2011-10-02 21:23:50 PDT
No, this has been backed out of all branches.
Comment 8 Brian Hackett (:bhackett) 2011-10-21 08:19:51 PDT
Created attachment 568652 [details] [diff] [review]
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] 2011-10-21 17:36:37 PDT
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 (:bhackett) 2011-10-24 20:47:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/049a08dfadc2
Comment 11 Marco Bonardo [::mak] 2011-10-25 05:04:38 PDT
https://hg.mozilla.org/mozilla-central/rev/049a08dfadc2
Comment 12 Brendan Eich [:brendan] 2011-10-25 12:58:30 PDT
Cause of http://arewefastyet.com/?a=b&machine=9 regression?

/be
Comment 13 Brian Hackett (:bhackett) 2011-10-25 14:40:41 PDT
(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 (not working on stability any more) 2011-10-26 05:57:32 PDT
(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 (:bhackett) 2011-10-26 10:59:07 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 (not working on stability any more) 2011-10-26 11:05:01 PDT
(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 (:bhackett) 2011-10-26 11:13:11 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.

Note You need to log in before you can comment on or make changes to this bug.