Closed
Bug 363536
Opened 18 years ago
Closed 18 years ago
Combine JSOP_THIS and JSOP_GETPROP, e.g., for faster instance var access
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: brendan, Assigned: brendan)
References
Details
Attachments
(4 files, 3 obsolete files)
7.93 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
5.23 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
34.53 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•18 years ago
|
||
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
Comment 2•18 years ago
|
||
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.
Assignee | ||
Comment 3•18 years ago
|
||
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)
Assignee | ||
Comment 4•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #249189 -
Flags: review?(igor.bukanov) → review+
Updated•18 years ago
|
Attachment #249192 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 5•18 years ago
|
||
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
Assignee | ||
Comment 6•18 years ago
|
||
Attachment #249304 -
Flags: review?(igor.bukanov)
Assignee | ||
Comment 7•18 years ago
|
||
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
Assignee | ||
Comment 8•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #249304 -
Flags: review?(igor.bukanov) → review+
Comment 9•18 years ago
|
||
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 ;)
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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+
Assignee | ||
Comment 12•18 years ago
|
||
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
Assignee | ||
Comment 13•18 years ago
|
||
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+
Assignee | ||
Comment 14•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #249497 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 15•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•