363530 - Optimize JS function/method calling bytecodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See bug 363529 for data.  This bug is about consolidating bytecode to compute the callee and |this| parameter, currently done via bytecode for an expression ending in a JSOP_NAME, JSOP_GETMETHOD, JSOP_GETPROP, or JSOP_GETELEM followed by a JSOP_PUSHOBJ.  One good effect of this fix will be to eliminate JSOP_PUSHOBJ and avoid carrying the |this| parameter around the interpreter loop in the |obj| VM register, which taxes all "leaf" bytecodes.

/be

Comment 1

[Brendan Eich [:brendan]]

Igor is going to take this one, I think.

/be

Comment 2

[Brendan Eich [:brendan]]

Potentially conflicting patch in bug 363536 -- incentive to review the patch there before patching this bug ;-).

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Potentially conflicting patch in bug 363536 -- incentive to review the patch
> there before patching this bug ;-).

Should I based the patch based on the patch in bug 363536?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > Potentially conflicting patch in bug 363536 -- incentive to review the patch
> > there before patching this bug ;-).
> 
> Should I based the patch based on the patch in bug 363536?

Sorry, I just committed that bug's last patch.  You should be good to go now!

/be

Comment 5

[Igor Bukanov]

Attachment: Implementation v0

An initial implementation that covers the case of various name() bytecodes.

Comment 6

[Igor Bukanov]

Attachment: Implementation v0.2

Here is a patch that removes pushobj and instead adds all the necessary call operations. The patch regresses in the decompiler since my initial idea to map there various callsomething into something and simplify code is not straightforward code change and somehow affects let blocks.

Comment 7

[Igor Bukanov]

Attachment: Implementation v1

With this patch there is no regressions against the test suite.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 253686 [details] [diff] [review]
Implementation v1

Waiting for new patch -- Igor, can blocking bug(s) be noted in this one?

/be

Comment 9

[Igor Bukanov]

Resolving bug 369740 should simplify the patch for this bug.

Comment 10

[Igor Bukanov]

Attachment: Implementation v2 (quilt patch)

Here is rewritten and shrunken implementation that takes into account the work done for bug 369740. This is a quilt patch as it depends on the patch from bug 370016 applied.

Comment 11

[Igor Bukanov]

Attachment: Implementation v2

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 256116 [details] [diff] [review]
Implementation v2

>+EmitName(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn,
>+         JSBool callContext)

Nit: Slight pref for EmitNameOp rather than EmitName.

>+{
>+    JSOp op;
>+    jsatomid atomIndex;
>+
>+    if (!BindNameToSlot(cx, &cg->treeContext, pn, JS_FALSE))
>+        return JS_FALSE;
>+    op = pn->pn_op;
>+
>+    if (callContext) {
>+        switch (op) {
>+          case JSOP_NAME:
>+            op = JSOP_CALLNAME;
>+            break;

Just making sure this covers all cases: JSOP_XMLNAME? JSOP_SETCALL (test it.item(i) = j in the js shell).

>+          case JSOP_GETVAR:
>+            op = JSOP_CALLVAR;
>+            break;
>+          case JSOP_GETGVAR:
>+            op = JSOP_CALLGVAR;
>+            break;
>+          case JSOP_GETARG:
>+            op = JSOP_CALLARG;
>+            break;
>+          case JSOP_GETLOCAL:
>+            op = JSOP_CALLLOCAL;
>+            break;
>+          default:
>+            JS_ASSERT(op == JSOP_ARGUMENTS);
>+            break;
>+        }
>+    }
>+
>+    if (op == JSOP_ARGUMENTS) {
>+        if (js_Emit1(cx, cg, op) < 0)
>+            return JS_FALSE;
>+        if (callContext && js_Emit1(cx, cg, JSOP_NULL) < 0)

Shouldn't the bytecode here be JSOP_EXPTHIS, not JSOP_NULL? (See below for more on that opcode).

>+            return JS_FALSE;
>+    } else {
>+        if (pn->pn_slot >= 0) {
>+            atomIndex = (jsatomid) pn->pn_slot;
>+            EMIT_UINT16_IMM_OP(op, atomIndex);

s/atomIndex/slot/g -- its type should be jsuint, I guess. Or just use pn->pn_slot as the actual parameter to EMIT_UINT16_IMM_OP.


> JSBool
>+js_EmitUnary(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn,
>+             JSBool callContext)

Non-nit: This function should be a static, and (nit alert) since it does more than the postfix op emission, EmitUnary is a fine name, better than EmitUnaryOp (which sounds like it does just the op) and EmitUnaryExpr (which while symmetric with the parser function UnaryExpr is unnecessarily long).

>+{
>+    JSParseNode *pn2, *pn3;
>+    JSOp op;
>+    uintN oldflags;
>+
>+    pn2 = pn->pn_kid;
>+    op = pn->pn_op;
>+    if (op == JSOP_TYPEOF) {
>+        for (pn3 = pn2; pn3->pn_type == TOK_RP; pn3 = pn3->pn_kid)
>+            continue;
>+        if (pn3->pn_type != TOK_NAME)
>+            op = JSOP_TYPEOFEXPR;
>+    }
>+    oldflags = cg->treeContext.flags;
>+    cg->treeContext.flags &= ~TCF_IN_FOR_INIT;
>+    if (!js_EmitTree(cx, cg, pn2))
>+        return JS_FALSE;
>+    cg->treeContext.flags |= oldflags & TCF_IN_FOR_INIT;
>+#if JS_HAS_XML_SUPPORT
>+    if (op == JSOP_XMLNAME) {
>+        if (js_NewSrcNote2(cx, cg, SRC_PCBASE,
>+                           CG_OFFSET(cg) - pn2->pn_offset) < 0) {
>+            return JS_FALSE;
>+        }
>+        if (callContext)
>+            return js_Emit1(cx, cg, JSOP_CALLXMLNAME) >= 0;
>+    }
>+#endif
>+
>+    return js_Emit1(cx, cg, op) >= 0 &&
>+           (!callContext || js_Emit1(cx, cg, JSOP_EXPTHIS) >= 0);

Could we avoid JSOP_EXPTHIS by using JSOP_NULL and having the decompiler suppress null this decompilation?

/be

Comment 13

[Brendan Eich [:brendan]]

The other alternative to avoiding JSOP_EXPTHIS just for the decompiler (we try to avoid nop variants just for the decompiler; mrbkap added a few for E4X but I think they could be eliminated too):

JSOP_NULL with a SRC_HIDDEN note.

Still want interpreter to win and decompiler to lose when there's a tradeoff like this to be made.

/be

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> The other alternative to avoiding JSOP_EXPTHIS just for the decompiler (we try
> to avoid nop variants just for the decompiler; mrbkap added a few for E4X but I
> think they could be eliminated too):
> 
> JSOP_NULL with a SRC_HIDDEN note.

I think a better alternative would be to have a special call/new bytecodes like expcall/expnew for call expressions that expect just single function slot on the stack.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> I think a better alternative would be to have a special call/new bytecodes like
> expcall/expnew for call expressions that expect just single function slot on
> the stack.

And how would these ops satisfy the public API of argv[-2] to fetch the callee? The stack layout wants argv[-1] to be the this value. The API is concrete in this case and I don't see how we can change it.

But to create another call code path that pushes callee and arguments with no this is an idea that needs more justification. IIRC Tamarin does it for class method calls because this is bound (class functions are bound methods), and of course there's no public API constraint.

So all things equal, with SpiderMonkey I would prefer to see the one true calling convention remain in place. Call expressions are pretty rare AFAICT (from the instrumentation in bug 365851).

/be

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> And how would these ops satisfy the public API of argv[-2] to fetch the callee?
> The stack layout wants argv[-1] to be the this value. The API is concrete in
> this case and I don't see how we can change it.

Right, the stack space has to be allocated in any case. Plus that would lead to proliferation of bytecodes as alternatives has to be provided for call, new setcall and eval.

So I will replace expthis by null. That does not even require a source note as the decompiler pops this argument in any case. 

Comment 17

[Igor Bukanov]

(In reply to comment #12)
> 
> Just making sure this covers all cases: JSOP_XMLNAME? JSOP_SETCALL (test
> it.item(i) = j in the js shell).

But JSOP_XMLNAME is not a name opertaion. It is a unary with arbitrary expression child. Also JSOP_SETCALL is a variant of JSOP_CALL and should work as is with the new bytecodes.

Comment 18

[Igor Bukanov]

Attachment: Implementation v3

The new patch uses straight JSOP_NULL to push null as this object. AFAICS there is no need to annotate it in the decompiler as it pops and ignores any this objects. The only potentially problematic area is the following lines in the patch:

               case JSOP_GROUP:
                 cs = &js_CodeSpec[lastop];
                 if ((cs->prec != 0 &&
                      cs->prec == js_CodeSpec[pc[JSOP_GROUP_LENGTH]].prec) ||
-                    pc[JSOP_GROUP_LENGTH] == JSOP_PUSHOBJ ||
+                    pc[JSOP_GROUP_LENGTH] == JSOP_NULL ||
                     pc[JSOP_GROUP_LENGTH] == JSOP_DUP) {
                     /*
                      * Force parens if this JSOP_GROUP forced re-association
                      * against precedence, or if this is a call or constructor
                      * expression, or if it is destructured (JSOP_DUP).
                      *

I do not know if it is necessary to detect here that this a synthetic null, not a real one representing null keyword.

Comment 19

[Brendan Eich [:brendan]]

With the latest patch, how does

function f() { null.foo() }

decompile?

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 256734 [details] [diff] [review]
Implementation v3

Argh, text/x-patch again -- doesn't do the right thing in bugzilla (is it a common mime type for patches?).

/be

Comment 21

[Brendan Eich [:brendan]]

Quite nit, pet peeve: I hope to avoid Xml spelling of XML -- yeah, XMLName runs together a lot of capital letters, but the XML stand out better (perhaps I am in favor of ugly shouting to mark E4X intrusions into the code ;-). Rhino seems to have XML and Xml mixed up, which inconsistency I think is worse than either.

/be

Comment 22

[Igor Bukanov]

(In reply to comment #19)
> With the latest patch, how does
> 
> function f() { null.foo() }
> 
> decompile?

With the patch:

js> function f() { null.foo(); }
function f() { null.foo(); }
js> print(f)
print(f)
function f() {
    null.foo();
}
js> dis(f)
dis(f)
main:
00000:  null
00001:  callprop "foo"
00004:  call 0
00007:  pop
00008:  stop

Source notes:
  0:     1 [   1] pcbase   offset 1
  2:     4 [   3] pcbase   offset 4
js> 

js> function g() { null(); }
function g() { null(); }
js> dis(g);
dis(g);
main:
00000:  null
00001:  null
00002:  call 0
00005:  pop
00006:  stop

Source notes:
  0:     2 [   2] pcbase   offset 2
js> print(g)
print(g)
function g() {
    null();
}

(In reply to comment #20)
> (From update of attachment 256734 [details] [diff] [review])
> Argh, text/x-patch again -- doesn't do the right thing in bugzilla (is it a
> common mime type for patches?).

Sorry, I forgot to mark the attachment as a patch.

Comment 23

[Igor Bukanov]

Attachment: Implementation v3b

The new version uses EmitXMLName.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 256743 [details] [diff] [review]
Implementation v3b

Thanks, this looks great! r=me.

/be

Comment 25

[Igor Bukanov]

Attachment: Implementation v3c

Here is the patch to commit with xdr version bumped.

Comment 26

[Igor Bukanov]

I committed the patch from comment 25 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.239; previous revision: 3.238
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.336; previous revision: 3.335
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.211; previous revision: 3.210
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.42; previous revision: 3.41
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.88; previous revision: 3.87
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.26; previous revision: 1.25
done