JM: make '=== undefined' faster

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 4

8 years ago
Created attachment 486613 [details] [diff] [review]
WIP

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

Comment 5

8 years ago
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.)
(Assignee)

Comment 6

8 years ago
Created attachment 487176 [details] [diff] [review]
Patch

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
(Assignee)

Comment 8

8 years ago
Created attachment 487178 [details] [diff] [review]
Tracejit patch

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'.
(Assignee)

Comment 11

8 years ago
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.
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+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

8 years ago
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

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c47a52df435d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.