Always allow on stack recompilation for methodjit code

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 559541 [details] [diff] [review]
patch
Attachment #559541 - Flags: review?(dvander)
(Assignee)

Comment 2

6 years ago
http://hg.mozilla.org/projects/jaegermonkey/rev/cc2daf6cbaab
Whiteboard: fixed-in-jaegermonkey
(Assignee)

Comment 3

6 years ago
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.
Attachment #560044 - Flags: review?(dvander)
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.
Attachment #559541 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

6 years ago
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
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?
Whiteboard: fixed-in-jaegermonkey → fixed-in-jaegermonkey [MemShrink]
(Assignee)

Comment 7

6 years ago
No, this has been backed out of all branches.
Whiteboard: fixed-in-jaegermonkey [MemShrink] → [MemShrink]

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Updated

6 years ago
Attachment #560044 - Flags: review?(dvander)
(Assignee)

Comment 8

6 years ago
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.
Attachment #559541 - Attachment is obsolete: true
Attachment #560044 - Attachment is obsolete: true
Attachment #568652 - Flags: review?(dvander)
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!
Attachment #568652 - Flags: review?(dvander) → review+
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/049a08dfadc2
https://hg.mozilla.org/mozilla-central/rev/049a08dfadc2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Cause of http://arewefastyet.com/?a=b&machine=9 regression?

/be
(Assignee)

Comment 13

6 years ago
(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

6 years ago
(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?
(Assignee)

Comment 15

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
(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.
Assignee: general → bhackett1024
You need to log in before you can comment on or make changes to this bug.