515806 - TM: Rename JSOP_LOOP to JSOP_TRACE, emit at function start

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

This is needed for both recursion and tracing methods.

Comment 1

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

Attachment: patch

Emits JSOP_TRACE at the start of scripts. Passes trace-tests and jstests

Comment 2

[Brendan Eich [:brendan]]

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

>diff --git a/js/src/imacros.c.out b/js/src/imacros.c.out
>--- a/js/src/imacros.c.out
>+++ b/js/src/imacros.c.out
>@@ -907,7 +907,7 @@ uint8 js_opcode2extra[JSOP_LIMIT] = {
>     0,  /* JSOP_DEFLOCALFUN_FC */
>     0,  /* JSOP_LAMBDA_FC */
>     0,  /* JSOP_OBJTOP */
>-    0,  /* JSOP_LOOP */
>+    0,  /* JSOP_TRACE */
>     0,  /* JSOP_GETUPVAR_DBG */
>     0,  /* JSOP_CALLUPVAR_DBG */
>     0,  /* JSOP_DEFFUN_DBGFC */
>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp
>--- a/js/src/jsemit.cpp
>+++ b/js/src/jsemit.cpp
>@@ -1,5 +1,5 @@
> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>- * vim: set ts=8 sw=4 et tw=99:
>+ * vim: set ts=4 sw=4 et tw=99:

Sigh -- some files have ts=4 with tw=99, some have ts=8. The et should make it a matter of indifference but if you hit <tab> you want four. Maybe leave this for a separate bug? Or at least file that bug so we get a cleanup patch for blame isolation/minimization.

>@@ -3531,6 +3531,9 @@ bad:
> JSBool
> js_EmitFunctionScript(JSContext *cx, JSCodeGenerator *cg, JSParseNode *body)
> {
>+    if (js_Emit1(cx, cg, JSOP_TRACE) < 0)
>+        return JS_FALSE;
>+
>     if (cg->flags & TCF_FUN_IS_GENERATOR) {
>         /* JSOP_GENERATOR must be the first instruction. */

Comment contradicted by your change. Wonder if you want to put the JSOP_TRACE emission in the else if this if (generator) {...} statement. Eventually, no, but for now it would be safer.

>@@ -4976,6 +4976,11 @@ js_DecompileFunction(JSPrinter *jp)
>         endpc = pc + fun->u.i.script->length;
>         ok = JS_TRUE;
> 
>+        /* Skip JSOP_TRACE or JSOP_NOP if they appear first */
>+        JS_STATIC_ASSERT(JSOP_TRACE_LENGTH == JSOP_NOP_LENGTH);
>+        if (*pc == JSOP_TRACE || *pc == JSOP_NOP)
>+            pc += JSOP_NOP_LENGTH;

Don't skip JSOP_NOP, it can have a srcnote and stand for a nested function definition that must be decompiled. (It can stand for other things important to decompile, but not IIRC at the start of script->code.)

Now that we've talked I don't think you need to skip JSOP_TRACE either unless it breaks something like destructuring formal parameters or generators (see above on avoiding that conundrum). Decompile handles a stack-neutral op for which it has no switch case by skipping it.

>diff --git a/js/src/jsops.cpp b/js/src/jsops.cpp
>--- a/js/src/jsops.cpp
>+++ b/js/src/jsops.cpp
>@@ -4244,8 +4244,8 @@
>           END_CASE(JSOP_ARRAYPUSH)
> #endif /* JS_HAS_GENERATORS */
> 
>-          BEGIN_CASE(JSOP_LOOP)
>-          END_CASE(JSOP_LOOP)
>+          BEGIN_CASE(JSOP_TRACE)
>+          END_CASE(JSOP_TRACE)

EMPTY_CASE, rather.

>@@ -941,8 +941,8 @@ AttemptCompilation(JSContext *cx, JSTrac
>                    uint32 argc)
> {
>     /* If we already permanently blacklisted the location, undo that. */
>-    JS_ASSERT(*(jsbytecode*)pc == JSOP_NOP || *(jsbytecode*)pc == JSOP_LOOP);
>-    *(jsbytecode*)pc = JSOP_LOOP;
>+    JS_ASSERT(*(jsbytecode*)pc == JSOP_NOP || *(jsbytecode*)pc == JSOP_TRACE);
>+    *(jsbytecode*)pc = JSOP_TRACE;

No need for all those (jsbytecode*) casts, right?

>@@ -3959,10 +3959,10 @@ TraceRecorder::closeLoop(SlotMap& slotMa
> {
>     /*
>      * We should have arrived back at the loop header, and hence we don't want
>-     * to be in an imacro here and the opcode should be either JSOP_LOOP or, in
>+     * to be in an imacro here and the opcode should be either JSOP_TRACE or, in
>      * case this loop was blacklisted in the meantime, JSOP_NOP.
>      */
>-    JS_ASSERT((*cx->fp->regs->pc == JSOP_LOOP || *cx->fp->regs->pc == JSOP_NOP) && !cx->fp->imacpc);
>+    JS_ASSERT((*cx->fp->regs->pc == JSOP_TRACE || *cx->fp->regs->pc == JSOP_NOP) && !cx->fp->imacpc);

Since you are changing this, how about JS_ASSERT_IF(!cx->fp->imacpc, ...) instead.

/be

Comment 3

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

Attachment: v2

Comment 4

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

Actually, wait, is that JS_ASSERT_IF comment right? It seems like we don't want an imacpc there at all.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 400128 [details] [diff] [review]
v2

>+ Only emit a trace hint opcode if not in a generator, since generators

s/Only emit a trace hint opcode if/Emit a trace hint opcode only if/

>+        /* Skip trace-hint, or nop'd trace-hint, if it appears here. */

Shouldn't this be trapped trace-hint, not nop'ed trace-hint, and s/JSOP_NOP/JSOP_TRACE/g accordingly?

/be

Comment 6

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

Attachment: addresses new comments

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 400607 [details] [diff] [review]
addresses new comments

Thanks!

/be

Comment 8

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

http://hg.mozilla.org/tracemonkey/rev/61f41c7f987f

Comment 9

[Jeff Walden [:Waldo]]

For completeness I note that Function.prototype hasn't been updated to start with JSOP_TRACE (it's a scripted function -- seriously).  David tells me this doesn't matter, but it conceivably might at some point, so I'm pointing it out here so people are at least aware of this oddity.

Comment 10

[Brendan Eich [:brendan]]

We will never trace Function.prototype. Probably we shouldn't trace functions or scripts shorter than 10 (or some N based on measuring average JIT trace recording overhead vs. payoff) bytecodes, for that matter.

/be

Comment 11

[Jeff Walden [:Waldo]]

I mentioned it under the assumption that having a function not start with JSOP_TRACE (generators excepted) might someday be bad, or at least break assumptions; it's certainly true we wouldn't want to start a trace in Function.prototype()!

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/61f41c7f987f

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b0970edf57ff