Closed Bug 363536 Opened 16 years ago Closed 16 years ago

Combine JSOP_THIS and JSOP_GETPROP, e.g., for faster instance var access

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: brendan, Assigned: brendan)

References

Details

Attachments

(4 files, 3 obsolete files)

See bug 363529 for data.  SpiderMonkey's bytecode was factored at the expense of performance, to separate base object from property get or set opcodes.  The common this.foo pattern deserves a single-issue instruction.

/be
Not convinced we need to do more at this point, based on measurements of Firefox. I could be sold on SETTHISPROP based on data from other apps and/or workloads.  I don't want to bother with ++/-- ops.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #249186 - Flags: review?(igor.bukanov)
Comment on attachment 249186 [details] [diff] [review]
optimize get of this.foo easy cases

>+          BEGIN_CASE(JSOP_GETTHISPROP)
>+            atom = GET_ATOM(cx, script, pc);
>+            id = ATOM_TO_JSID(atom);
>+            obj = fp->thisp;
>+            CACHED_GET(OBJ_GET_PROPERTY(cx, obj, id, &rval));
>+            if (!ok)
>+                goto out;
>+            PUSH_OPND(rval);
>+          END_CASE(JSOP_GETTHISPROP)
>+

Here SAVE_SP_AND_PC(fp) is missing. 

IMO NEW_EQ -> NEW_STRICTEQ rename should go to a separated patch as one may want to apply it to 1.8.1 to prevent merge conflicts for security patches if any or at least be able to verify that the merge conflict just due to the rename.
Not keen on pushing any of this bytecode optimization work to 1.8 branches.

Remind me never to name anything NEW (it seemed a good idea in 1997).

/be
Attachment #249189 - Flags: review?(igor.bukanov)
With SAVE_SP_AND_PC fix. Thanks,

/be
Attachment #249186 - Attachment is obsolete: true
Attachment #249192 - Flags: review?(igor.bukanov)
Attachment #249186 - Flags: review?(igor.bukanov)
Attachment #249189 - Flags: review?(igor.bukanov) → review+
Attachment #249192 - Flags: review?(igor.bukanov) → review+
NEW_ => STRICT patch landed:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.311; previous revision: 3.310
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.307; previous revision: 3.306
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.196; previous revision: 3.195
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.82; previous revision: 3.81
done
Checking in jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.40; previous revision: 3.39
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.117; previous revision: 3.116
done

/be
Attachment #249304 - Flags: review?(igor.bukanov)
Comment on attachment 249192 [details] [diff] [review]
optimize get of this.foo easy cases

Checked into the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.231; previous revision: 3.230
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.312; previous revision: 3.311
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.197; previous revision: 3.196
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.83; previous revision: 3.82
done

/be
The Firefox gmail mini-session graph shows heavy edges into getprop from getarg and getvar as well as from the |this| bytecode:

http://wiki.mozilla.org/images/e/e5/Filtered_gmail_bytecode_graph.png

Not sure how much this will help tinderbox'ed perf metrics; JS bytecodes seem not to matter there so much as GC.

/be
Attachment #249321 - Flags: review?(igor.bukanov)
Attachment #249304 - Flags: review?(igor.bukanov) → review+
Comment on attachment 249321 [details] [diff] [review]
get{arg,var,local} -> getprop compound bytecodes

>-#define NEW_EQUALITY_OP(OP)                                                   \
>+#define STRICT_EQUALITY_OP(OP)                                                \

This is the part of a separated rename patch. The rest seems to be OK, but it is too late here for a judgement ;)
The renaming patch landed, this patch is updated to top of trunk.

/be
Attachment #249321 - Attachment is obsolete: true
Attachment #249327 - Flags: review?(igor.bukanov)
Attachment #249321 - Flags: review?(igor.bukanov)
Comment on attachment 249327 [details] [diff] [review]
get{arg,var,local} -> getprop compound bytecodes, updated

>+              case JSOP_GETARG:
>+              case JSOP_GETVAR:
>+              case JSOP_GETLOCAL:
>+              {
>+                JSAtomListElement *ale;
>+                jsatomid atomIndex;
>+
>+                ale = js_IndexAtom(cx, pn->pn_atom, &cg->atomList);
>+                if (!ale)
>+                    return JS_FALSE;
>+                atomIndex = ALE_INDEX(ale);
>+                return EmitIndexConstOp(cx,
>+                                        (pn2->pn_op == JSOP_GETARG)
>+                                        ? JSOP_GETARGPROP
>+                                        : (pn2->pn_op == JSOP_GETVAR)
>+                                        ? JSOP_GETVARPROP
>+                                        : JSOP_GETLOCALPROP,
>+                                        pn2->pn_slot, atomIndex, cg);
>+              }
>+
>+              default:;

That ?:?: looks ugly IMO, the following that follow the pattern from the interpreter looks better:

case JSOP_GETARG:
  op = JSOP_GETARGPROP;
  goto xxx;
case JSOP_GETVAR:
  op = JSOP_GETVARPROP;
  goto xxx;
case JSOP_GETLOCAL:
  op = JSOP_GETLOCALPROP;
xxx: {
  JSAtomListElement *ale;

  ale = js_IndexAtom(cx, pn->pn_atom, &cg->atomList);
  if (!ale)
    return JS_FALSE;
  return EmitIndexConstOp(cx, op, pn2->pn_slot, 
                                          ALE_INDEX(ale), cg);
}
Attachment #249327 - Flags: review?(igor.bukanov) → review+
Chained ?: is common in SpiderMonkey, especially in jsemit.c, so I don't think it's a matter of "looks better" -- we may have to disagree on issues of taste. But on issues of code size and runtime, you are right that retesting the switch cases is a small (calculated) loss.  I'll redo it to save at the price of gotos.

/be

Comment on attachment 249327 [details] [diff] [review]
get{arg,var,local} -> getprop compound bytecodes, updated

The decompiler needs more work to handle diagnostics as before, e.g. (function (a){return a.foo})(null)).

/be
Attachment #249327 - Attachment is obsolete: true
Attachment #249327 - Flags: review+
Some js_CodeSpec[op].format modes were added as format flag bits in recent years. Mode is meaningful to the decompiler: it distinguishes x from o.p from o[i].  So JOF_XMLNAME for n::x should have been a mode, and now the special forms a.p, v.p, and l.p for arg a, var v, and local l are distinguished via JOF_VARPROP.

The whole format encoding is questionable, especially ignoring the decompiler.  Real and virtual machines often can encode the relevant facts in the bytecodes themselves. But compressing or otherwise optimizing this is for another bug.

/be
Attachment #249497 - Flags: review?(igor.bukanov)
Attachment #249497 - Flags: review?(igor.bukanov) → review+
Fixed on trunk: 

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.232; previous revision: 3.231
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.315; previous revision: 3.314
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.200; previous revision: 3.199
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.38; previous revision: 3.37
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.84; previous revision: 3.83
done

/be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Blocks: 372364
Blocks: 381504
Depends on: 420919
You need to log in before you can comment on or make changes to this bug.