Closed
Bug 599035
Opened 14 years ago
Closed 14 years ago
JM: disabled PIC for n += 1
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: dvander)
References
(Blocks 1 open bug)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files, 1 obsolete file)
12.22 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
Details | Diff | Splinter Review |
Consider this benchmark: ------ function manyTimes(func) { for(var i = 0; i < 1000; i++) { func(); } } function funcA() { var n = 0; var f = function() { n += 1; }; manyTimes(f); } function f() { var t0 = new Date; for(var i = 0; i < 1000; i++) { funcA(); } print(new Date - t0); } f(); ------ The line n += 1 makes us slow here. If I change it I get the following times for js -m: n += 1 : 83 ms n = n + 1 : 19 ms n++ : 19 ms Bytecode for n += 1: ----- 00000: bindname "n" 00003: dup 00004: getxprop "n" 00007: one 00008: add 00009: setname "n" 00012: pop 00013: stop ----- JM pics output shows "getprop disabled: getter" I noticed this when looking at some attachment of bug 460050, dvander asked me to file this.
Assignee | ||
Comment 1•14 years ago
|
||
Explanation: For assignments to scope variables, the LHS must be bound before the RHS is evaluated. We do this by emitting a BINDNAME op on the LHS, evaluating the RHS, then emitting a SETNAME op. BINDNAME gives you the object on the scope chain for which the name should be bound. In some cases, the BINDNAME flows into a GETXPROP ("get extant property", maybe?). It's basically the same thing as just a NAME op, except we've already computed which scope object the property is on. It's used for n += 1, because we can avoid a scope walk: BINDNAME "n" DUP GETXPROP "n" ONE ADD SETPROP "n" We were making GETXPROP a GETPROP, but GetPropCompiler doesn't special case scope chain objects. This patch redirects GETXPROP to ScopeNameCompiler. With this, the attached benchmark runs in 13ms instead of 50ms.
Comment 2•14 years ago
|
||
(In reply to comment #1) > BINDNAME gives you the object on the scope chain for which the name should be > bound. In some cases, the BINDNAME flows into a GETXPROP ("get extant > property", maybe?) No need to ask. From jsopcode.tbl: /* * Get an extant property value, throwing ReferenceError if the identified * property does not exist. */ OPDEF(JSOP_GETXPROP, 196,"getxprop", NULL, 3, 1, 1, 18, JOF_ATOM|JOF_PROP) > With this, the attached benchmark runs in 13ms instead of 50ms. Awesome! Good easy fix. b7 fodder. /be
Assignee | ||
Comment 3•14 years ago
|
||
repatch to the right stub on GC
Attachment #478074 -
Attachment is obsolete: true
Attachment #478084 -
Flags: review?(sstangl)
Attachment #478074 -
Flags: review?(sstangl)
Updated•14 years ago
|
Attachment #478084 -
Flags: review?(sstangl) → review+
Comment 4•14 years ago
|
||
nice!
Assignee | ||
Comment 5•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/3903088ba08f
Whiteboard: fixed-in-tracemonkey
Comment 6•14 years ago
|
||
this broke ARM: /builds/slave/tracemonkey-mobile-browser-android-r7-build/build/tracemonkey/js/src/methodjit/Compiler.cpp:1542: undefined reference to `js::mjit::Compiler::jsop_xname(JSAtom*)'
Reporter | ||
Comment 7•14 years ago
|
||
@sayrer: this patch fixes it by moving the function outside the #ifdef JS_POLYIC block.
Assignee | ||
Comment 8•14 years ago
|
||
minimal bustage fix http://hg.mozilla.org/tracemonkey/rev/8b70fd2b2a74
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3903088ba08f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•