Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

The code in Compiler::stricteq claims that undefined and null get a fast path. This is only true for null; undefined is compiled to 'getgname "undefined"' so it always goes through the stub call.

For this benchmark:
----
function f(a) {
    for(var i=0; i<10000000; i++) {
	if(a === undefined) {	}
    }
}
var t0 = new Date;
f();
print(new Date - t0);
----
V8: 59 ms
JM: 185 ms (3.13x)

f(4) instead of f():
V8: 67 ms
JM: 192 ms (2.86x)

f(4) and === null:
V8: 25 ms
JM: 41 ms (1.64x)

Can we do something faster here like guarding that "undefined" is the real undefined value? Type inference may also help, but in the meantime the code is still misleading.

This could also help v8-earley-boyer a bit, it does "if (x === undefined)" a lot.
I thought you cant change undefined anymore, so this could get its on byte code like null?
Forget this...
Yes, undefined is non-writable and non-configurable now, per ES5. The compiler knows when it is being referenced, too (i.e., knows when it might be or is shadowed by any inner binding of the same name). So we should be able to optimize has hard as we do for null.

/be
Posted patch WIP (obsolete) — Splinter Review
This special-cases undefined for JSOP_GETGNAME; I hope this is the right approach? The benchmark in comment 0 runs in 42 ms, 4x faster.

I need to do the same in TM, and for NaN and Infinity. What's the best way to compare a JSAtom* to the lazy NaN and Infinity atoms?
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Keep it simple, make them un-lazy.  (I expect we'll eliminate lazy atoms and just make them all eager at some point to simplify code, and we just haven't done it yet.)
Posted patch Patch (obsolete) — Splinter Review
This unlazifies the NaN and Infinity atoms, like Waldo suggested. I tried to add the same optimization to the tracejit, but it didn't help there and the same number of LIR-instructions was generated for a simple test case. So I assume this is not needed there, or am I missing something?
Attachment #486613 - Attachment is obsolete: true
Attachment #487176 - Flags: review?(dmandelin)
Nice patch.

What did your tracejit patch look like?

/be
brendan, here is the TM patch for undefined and NaN. With -j the benchmark below runs in 22 ms, the patch makes no difference. With JM this goes from 42 to 29 ms. 
------
function f() {
    var a;
    for(var i=0; i<10000000; i++) {
	a = undefined;
    }
    return a;
}
var t0 = new Date;
f();
print(new Date - t0);
The tracer copies all globals onto the stack before entering trace, so it won't do a name lookup for "undefined" except at recording time.
Comment on attachment 487176 [details] [diff] [review]
Patch

The patch looks good, but please add at least one test case for each of 'undefined', 'Infinity', and 'NaN'.
Posted patch Patch v2Splinter Review
Added a number of tests which I think are relevant. If it's overkill please let me know which tests can be removed and which are relevant.
Attachment #487176 - Attachment is obsolete: true
Attachment #487413 - Flags: review?(dmandelin)
Attachment #487176 - Flags: review?(dmandelin)
Comment on attachment 487413 [details] [diff] [review]
Patch v2

(In reply to comment #11)
> Created attachment 487413 [details] [diff] [review]
> Patch v2
> 
> Added a number of tests which I think are relevant. If it's overkill please let
> me know which tests can be removed and which are relevant.

Thanks for the tests. They are good--I don't think there is such a thing as overkill when testing a compiler.
Attachment #487413 - Flags: review?(dmandelin) → review+
Keywords: checkin-needed
Anybody willing to commit this, if possible? I think the patch is quite safe and I'm afraid it will bitrot otherwise.
http://hg.mozilla.org/tracemonkey/rev/c47a52df435d
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/c47a52df435d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.