558830 - TM: trace stopped: 12849: script getter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Humphrey (:humph)]

Spin-off from bug Bug 480192 comment 24:

thinking:processing-no-with dave$ cat -n ./getter-setter2.js
     1	(function () {
     2	  var o = {
     3	    x: 0,
     4	    get value()    { return this.x; },
     5	    set value(val) { this.x = val   }
     6	  };
     7	
     8	  for (var i = 0; i < 100000; i++) {
     9	    o.value = o.value + i;
    10	  }
    11	})();
thinking:processing-no-with dave$ TMFLAGS=stats,abort ~/moz/mozilla-central/src/objdir-debug/dist/bin/js -j ./getter-setter2.js
trace stopped: 12849: script getter
Abort recording of tree ./getter-setter2.js:8@34 at ./getter-setter2.js:9@38: getlocalprop.
trace stopped: 12849: script getter
Abort recording of tree ./getter-setter2.js:8@34 at ./getter-setter2.js:9@38: getlocalprop.
trace stopped: 12849: script getter
Abort recording of tree ./getter-setter2.js:8@34 at ./getter-setter2.js:9@38: getlocalprop.
recorder: started(3), aborted(3), completed(0), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0), merged loop exits(0), unstableInnerCalls(0), blacklisted(1)
monitor: exits(0), timeouts(0), type mismatch(0), triggered(0), global mismatch(0), flushed(0)

Comment 1

[Brendan Eich [:brendan]]

How does this bug differ from bug 480192? If you want to split that into setters and getters, is that one now only about setters? Or is it a metabug?

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This patch only traces GETPROP, CALLPROP, and GET{THIS,ARG,LOCAL}PROP with script getters. So it doesn't make the test case in comment 0 trace.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Same as WIP 1, basically.

With this patch, the test case instead says:
  trace stopped: 11289: can't trace JavaScript function setter

Bug 559813 fixes that. With the whole stack of patches, this test goes from 696msec to 19msec.

Comment 4

[Brendan Eich [:brendan]]

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

>+function simulate_cfg(igroup, imacro, depth, i) {
>+    let any_group_opcode;
>+    for (any_group_opcode in igroup.ops)
>+        break;
>+
>+    let opi = opinfo[any_group_opcode];
>+    let expected_depth = opi.pops < 0 ? undefined : opi.pushes - opi.pops;

Nit: parenthesize the ?: condition.

>+    for (let opcode in igroup.ops) {
>+        let opi = opinfo[opcode];
>+        if (expected_depth === undefined) {
>+            if (opi.pops >= 0)
>+                throw Error("imacro shared by constant- and variable-stack-defs/uses instructions");
>+        } else {
>+            if (opi.pops < 0)
>+                throw Error("imacro shared by constant- and variable-stack-defs/uses instructions");
>+            if (opi.pushes - opi.pops != expected_depth)
>+                throw Error("imacro shared by instructions with different stack depths");
>+        }
>+    }

Could you combine the two for loops? The second has to check for expected_depth === undefined anyway, but if you use one state variable (any_group_opcode would do) to take special first-time-through-the-loop action, you could then keep opinfo[any_group_opcode] in a separate let binding from opi = opinfo[opcode], and this would enable a single if for the "imacro shared by constant- and variable-stack-defs/uses instructions" exception.

In any event having inner let opi shadow outer seems not so good. It could cause a warning some day, at least from some jslint successor if not from a full JS implementation.

>+TraceRecorder::canCallImacro() const
>+{
>+    /* We cannot nest imacros. */
>+    return !cx->fp->imacpc;
>+}
>+
> JS_REQUIRES_STACK RecordingStatus
> TraceRecorder::call_imacro(jsbytecode* imacro)
> {
>+    return canCallImacro() ? call_imacro_infallibly(imacro) : RECORD_STOP;
>+}
>+
>+JS_REQUIRES_STACK RecordingStatus
>+TraceRecorder::call_imacro_infallibly(jsbytecode* imacro)

Time to rename call_imacro to callImacro, and call_imacro_infallibly to callImacroInfallibly? No time like the present!

>+TraceRecorder::getPropertyWithScriptGetter(JSObject *obj, LIns* obj_ins, JSScopeProperty* sprop)
>+{
>+    if (!canCallImacro())
>+        RETURN_STOP("cannot trace script getter, already in imacro");
>+
>+    // Rearrange the stack in preparation for the imacro, taking care to adjust
>+    // the interpreter state and the tracker in the same way. This adjustment
>+    // is noted in imacros.jsasm with .fixup tags.
>+    jsval getter = sprop->getterValue();
>+    jsval*& sp = cx->fp->regs->sp;
>+    switch (*cx->fp->regs->pc) {
>+      case JSOP_GETPROP:
>+        sp++;
>+        sp[-1] = sp[-2];
>+        set(&sp[-1], get(&sp[-2]));
>+        sp[-2] = getter;
>+        set(&sp[-2], INS_CONSTOBJ(JSVAL_TO_OBJECT(getter)));
>+        return call_imacro_infallibly(getprop_imacros.scriptgetter);
>+
>+      case JSOP_CALLPROP:
>+        sp += 2;
>+        sp[-2] = getter;
>+        set(&sp[-2], INS_CONSTOBJ(JSVAL_TO_OBJECT(getter)));
>+        sp[-1] = sp[-3];
>+        set(&sp[-1], get(&sp[-3]));
>+        return call_imacro_infallibly(callprop_imacros.scriptgetter);
>+
>+      case JSOP_GETTHISPROP:
>+      case JSOP_GETARGPROP:
>+      case JSOP_GETLOCALPROP:
>+        sp += 2;
>+        sp[-2] = getter;
>+        set(&sp[-2], INS_CONSTOBJ(JSVAL_TO_OBJECT(getter)));
>+        sp[-1] = OBJECT_TO_JSVAL(obj);
>+        set(&sp[-1], obj_ins);
>+        return call_imacro_infallibly(getthisprop_imacros.scriptgetter);

Cool -- reorder imacros.jsasm to match the order of cases here? Nit for sure.

Good use of imacros and extension of the assembler! Hand-updating the tracker is a bit gnarly, though. Did you try JSOP_DUP or kin at all?

/be

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to comment #4)
> Good use of imacros and extension of the assembler! Hand-updating the tracker
> is a bit gnarly, though. Did you try JSOP_DUP or kin at all?

The first thing I tried was adding a custom-purpose bytecode instruction to push the getter function-object onto the operand stack. (No existing instruction does it.) That was pretty awful.

So I decided to sneak the getter onto the interpreter stack from the tracer. I could have stopped there and used DUP/PICK in the imacros, but since I was already hacking the interpreter stack at that point, it was only a couple more lines to arrange the stack exactly how the imacros want it.

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/7fa9e28cdc33

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7fa9e28cdc33