Closed Bug 599035 Opened 14 years ago Closed 14 years ago

JM: disabled PIC for n += 1

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: dvander)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #478074 - Flags: review?(sstangl)
(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
Attached patch fix v2Splinter Review
repatch to the right stub on GC
Attachment #478074 - Attachment is obsolete: true
Attachment #478084 - Flags: review?(sstangl)
Attachment #478074 - Flags: review?(sstangl)
Attachment #478084 - Flags: review?(sstangl) → review+
nice!
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*)'
@sayrer: this patch fixes it by moving the function outside the #ifdef JS_POLYIC block.
Blocks: 601106
http://hg.mozilla.org/mozilla-central/rev/3903088ba08f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 622271
You need to log in before you can comment on or make changes to this bug.