559813 - TM: Trace scripted setters

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

This applies on top of the patches in bug 558830 and bug 559653.

Comment 2

[Brendan Eich [:brendan]]

Waiting on patch for bug 559653 before reviewing -- let me know if you want me to review ahead.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Unbitrotted.

Comment 4

[Brendan Eich [:brendan]]

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

>+++ b/js/src/imacros.jsasm
>@@ -735,15 +735,24 @@ 3:      swap                            
>     .imacro scriptgetter        # obj
>         .fixup +2               # obj getter obj
>         call 0                  # obj method
>         swap                    # method obj
>         stop
>     .end
> .end callprop
> 
>+.igroup setprop JSOP_SETPROP,JSOP_SETNAME,JSOP_SETMETHOD
>+    .imacro scriptsetter # obj val
>+        .fixup +2        # val setter obj val
>+        call 1           # val ret
>+        pop              # val

Nit: tabulate comments to the same stop as the other imacros.

>+TraceRecorder::setPropertyWithScriptSetter(JSScopeProperty* sprop)
>+{
>+    if (!canCallImacro())
>+        RETURN_STOP("cannot trace script setter, already in imacro");
>+
>+    // Rearrange the stack in preparation for the imacro. See the comment in
>+    // getPropertyWithScriptGetter.
>+    jsval setter = sprop->setterValue();
>+    JSFrameRegs *regs = cx->fp->regs;
>+    regs->sp += 2;                  // obj val --- ---
>+    jsval *sp = regs->sp;
>+    sp[-2] = sp[-4];                // obj val obj ---
>+    set(&sp[-2], get(&sp[-4]));
>+    sp[-4] = sp[-1] = sp[-3];       // val val obj val
>+    set(&sp[-4], get(&sp[-3]));
>+    set(&sp[-1], get(&sp[-3]));
>+    sp[-3] = setter;                // val setter obj val

Time for a helper to do the assignment and set() call in one coherent method?

r=me with above dealt with. Thanks!

/be

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/69ea8f61ae26

With helper methods, it's like this:

    // Rearrange the stack in preparation for the imacro. See the comment in
    // getPropertyWithScriptGetter.
    JSObject *setterObj = JSVAL_TO_OBJECT(sprop->setterValue());
    cx->fp->regs->sp += 2;                  // obj val --- ---
    stackCopy(-2, -4);                      // obj val obj ---
    stackCopy(-4, -3);                      // val val obj ---
    stackCopy(-1, -3);                      // val val obj val
    stackStoreConstObject(-3, setterObj);   // val setter obj val
    return callImacroInfallibly(setprop_imacros.scriptsetter);

#jsapi was at a loss for names. 'stackCopy' is pretty weak, but the best alternative I came up with was 'moveNowAndLater', 'storeConstObjectNowAndLater'.

Comment 6

[Brendan Eich [:brendan]]

stackCopy is fine.

/be

Comment 7

[Jason Orendorff [:jorendorff]]

(This was backed out, long ago, due to problems with the patch for blocking bug 559653.)

Comment 8

[Jason Orendorff [:jorendorff]]

Re-landed: http://hg.mozilla.org/tracemonkey/rev/ca772ab995c9

Comment 9

[Jason Orendorff [:jorendorff]]

Backed out again, due to more problems (we think) with the patch for blocking bug 559653.

http://hg.mozilla.org/tracemonkey/rev/434e0c17c0ec

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v3

The prerequisite patch finally landed and stuck, so let's try this again.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 517802 [details] [diff] [review]
v3

+Object.defineProperty(this, "p", {get: function () { return n; },
+            set: function (x) { n += x; }});

Nit: indentation is off on the overflow line.

General nit, sorry I missed it (pre-exists in some four RETURN_STOP reason strings): "scripted getter", "scripted setter", and for the StudlyCaps names, similarly (ScriptedSetter in particular for this patch) seem much better than "script getter", etc. Is it a getter returning a script?

Uber-nit, also pre-existing: Waldo went with infallibleAppend in a recent patch to jsvector.h, we have callImacroInfallibly in jstracer.cpp. Hacker jargon favors prefix over adverb suffix. I may be to blame for this one!

r=me with whatever you can stand to fix above.

/be

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.