633929 - TM does not work without JM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

On a normal tracemonkey build, the "-j" option is broken. TM does work when "-m" is also specified. 

I think this is a regression from bug 631951.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Hack

This fix works, but I don't have time to test it extensively and I don't know this part of the code very well.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 512135 [details] [diff] [review]
Hack

Renaming patch. I'm sure there's a better fix, this is just quick-and-dirty until MV wakes up.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Bill: can you take this rq?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I'll take a look. It looks like -m and also -j regressed on AWFY, and I think it's probably related to this region of code as well.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

Jan's patch was correct for fixing -j. The shrinking patch assumed if there's no recorder or profiler active, then there's no reason to call MonitorLoopEdge (since the methodjit handles all this itself via MonitorTracePoint). However, this is not true for -j.

The reason -m wasn't working is that we weren't calling MONITOR_BRANCH_METHODJIT unless tracing was turned on. This is easy to fix.

This brings up another problem: if a method can't be methodjitted, then we will no longer apply the tracer to it either. A fairly easy fix would be to add another interpreter mode, like JSINTERP_FALLBACK, to signal that the reason we're interpreting is that we cannot methodjit at all. I'll look into this.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: better fix

This also fixes the problem where the methodjit isn't able to compile.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: and another

Fixed a stupid bug.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 512272 [details] [diff] [review]
and another

>         if (status == mjit::Compile_Error)                                    \
>             goto error;                                                       \
>         if (status == mjit::Compile_Okay) {                                   \

No else after goto, nice!

>             void *ncode =                                                     \
>                 script->nativeCodeForPC(regs.fp->isConstructing(), regs.pc);  \
>             interpReturnOK = mjit::JaegerShotAtSafePoint(cx, ncode);          \
>             if (inlineCallCount)                                              \
>                 goto jit_return;                                              \
>             goto leave_on_safe_point;                                         \
>+        } else if (status == mjit::Compile_Abort) {                           \

D'oh!

>@@ -2383,39 +2387,45 @@ Interpret(JSContext *cx, JSStackFrame *e
>         if (cx->isExceptionPending())                                         \
>             goto error;                                                       \
>     JS_END_MACRO
> 
> #define MONITOR_BRANCH()                                                      \
>     JS_BEGIN_MACRO                                                            \
>         if (TRACING_ENABLED(cx)) {                                            \
>             if (!TRACE_RECORDER(cx) && !TRACE_PROFILER(cx) &&                 \
>-                interpMode == JSINTERP_NORMAL)                                \
>+                interpMode == JSINTERP_NORMAL &&                              \
>+                cx->methodJitEnabled && !methodJitFailed)                     \

That's one fugly condition (no offense, I've written worse). Prevailing style favors each conjunct on its own line once you start overflowing, but either way: fugly. Wonder if down the road there's a way to combine things so fewer variables need to be tested.

/be

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/1e38ca07f02f

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix for --disable-methodjit

Thanks to philor's nifty new tbpl thing, I can now immediately recognize that I broke --disable-methodjit. This also cleans up the condition that Brendan complained about.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/b84a3c09482d

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/1e38ca07f02f