470739 - Never abort on ==

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

for(i=0;i<500000;++i) { var r = (void 0) == null; }

22% slower with JIT enabled.

abort: 4726: unsupported operand types for cmp
Abort recording (line 1, pc 17): eq.

Comment 1

[Andreas Gal :gal]

Waldo, anything to say in your defense? I thought ::cmp is complete now :)

Comment 2

[Jeff Walden [:Waldo]]

The original goal as I understood it was to rewrite to have a saner, more understandable implementation, so ::cmp is complete now by my understanding.

If on the other hand we want to trace everything under the sun, that's all cool, just need to do a little more work to make it so.

Comment 3

[Andreas Gal :gal]

Yeah, I am not sure its worth it. Comparing undefined against null seems a bit useless. I don't think we have seen concrete examples of this being used.

Comment 4

[Jesse Ruderman]

I'd expect this to be fairly common for developers who don't know about the "in" operator:

if (array.map == null) { ... } else { ... }

Maybe bug 470153 could tell us? ;)

Comment 5

[Jeff Walden [:Waldo]]

It's likely worthwhile to implement at least this particular comparison; the differences between the two values may be somewhat subtle unless you really know the language.

Comment 6

[Brendan Eich [:brendan]]

This should be fixed. Recall that undefined could not be written by that name until ES3 (hence the read/write nature of that property -- it was overwritable to avoid breaking existing web content that set undefind = void 0). You had to write (maybeUndefined == void 0), and no one did that when (maybeUndefined == null) worked.

Null meaning "no reference" and undefined meaning "no value" are == but not ===.

/be

Comment 7

[Jeff Walden [:Waldo]]

Patch in a bit!

Comment 8

[Jeff Walden [:Waldo]]

Attachment: I am astounded how readable this is for determining spec-compliance

All hail imacros!

Comment 9

[Jeff Walden [:Waldo]]

Hm, I, er, "misread" the output on a last run with recorderAborted: 0.  Turns out there are 121 aborts when running that test, but they're not within ::equality.  Looks like some multi-trees runaway trace prevention stuff at work, plus maybe JSOP_CALL failing to trace somehow:

find-waldo-now:~/moz/shell-js/js/src jwalden$ TRACEMONKEY=verbose ./js -j trace-test.js testComparisons  | grep -i abort
Abort recording (line 59, pc 8): deffun.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3438, pc 5208): No compatible inner tree.
Abort recording (line 3438, pc 5208): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3451, pc 5342): Inner tree not suitable for calling.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2654, pc 6): call.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3451, pc 5342): Inner tree not suitable for calling.
Abort recording (line 3467, pc 5488): Inner tree not suitable for calling.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3432, pc 5149): No compatible inner tree.
Abort recording (line 3451, pc 5342): Inner tree not suitable for calling.
Abort recording (line 3432, pc 5149): No compatible inner tree.
Abort recording (line 3438, pc 5208): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3446, pc 5266): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5412): Inner tree is trying to grow, abort outer recording.
Abort recording (line 3451, pc 5342): Inner tree not suitable for calling.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2654, pc 6): call.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2655, pc 6): call.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3451, pc 5342): Inner tree not suitable for calling.
abort: 6038: untraceable native
Abort recording (line 2650, pc 6): call.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2648, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2654, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3438, pc 5208): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2654, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
Abort recording (line 3438, pc 5208): No compatible inner tree.
Abort recording (line 3432, pc 5149): No compatible inner tree.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3446, pc 5257): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
abort: 6038: untraceable native
Abort recording (line 2654, pc 6): call.
Abort recording (line 3462, pc 5403): No compatible inner tree.
Abort recording (line 3438, pc 5208): No compatible inner tree.
abort: 6038: untraceable native
Abort recording (line 2647, pc 6): call.
testComparisons : FAILED: expected string ( "no failures reported!" )  [recorderAborted: 0]  != actual string ( "no failures reported!" )  [recorderAborted: 121] 
Abort recording (line 39, pc 185): No compatible inner tree.
Abort recording (line 39, pc 185): No compatible inner tree.
recorder: started(226), aborted(124), completed(259), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(5), breaks(0), returns(0), unstableInnerCalls(66)

But in any case, ::equality is clearly tracing now!

Comment 10

[Andreas Gal :gal]

Comment on attachment 354374 [details] [diff] [review]
I am astounded how readable this is for determining spec-compliance

The re-enter goto is a bit confusing. Maybe factoring out the code into a function that does the number comparison and is called directly or with some preparation first might be nicer? Not sure.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 354374 [details] [diff] [review]
I am astounded how readable this is for determining spec-compliance

>+    } else if (JSVAL_TAG(l) == JSVAL_BOOLEAN) {
>+        args[0] = l_ins, args[1] = cx_ins;
>+        l_ins = lir->insCall(&js_BooleanOrUndefinedToNumber_ci, args);
>+        l = (l == JSVAL_VOID)
>+            ? DOUBLE_TO_JSVAL(&cx->runtime->jsNaN)
>+            : INT_TO_JSVAL(l == JSVAL_TRUE);
>+        goto reenter;
>+    } else

Nit: else after goto is a non-sequitur. Suggest making a final else {...} containing if(C){... return ...} statements.

>........... if (JSVAL_TAG(r) == JSVAL_BOOLEAN) {
>+        args[0] = r_ins, args[1] = cx_ins;
>+        r_ins = lir->insCall(&js_BooleanOrUndefinedToNumber_ci, args);
>+        r = (r == JSVAL_VOID)
>+            ? DOUBLE_TO_JSVAL(&cx->runtime->jsNaN)
>+            : INT_TO_JSVAL(r == JSVAL_TRUE);
>+        goto reenter;
>+    } else

Ditto.

>........... if ((JSVAL_IS_STRING(l) || isNumber(l)) && !JSVAL_IS_PRIMITIVE(r)) {
>+        return call_imacro(equality_imacros.any_obj);
>+    } else

Same for else after return.

>........... if ((JSVAL_IS_STRING(r) || isNumber(r)) && !JSVAL_IS_PRIMITIVE(l)) {
>+        return call_imacro(equality_imacros.obj_any);
>+    } else {

Note how this one leaves three lines in an else, and many more lines after outside that else, arbitrarily:

>+        l_ins = lir->insImm(0);
>+        r_ins = lir->insImm(1);
>+        cond = false;
>     }
> 

Here and below we must run if we are not returning an imacro or "reentering" ("restart" as that label name seems better to me -- the backward goto is tolerable style-wise for such tail-recursive calls but it would be good to see how convergence is guaranteed, if not to assert it -- and clarifying control flow helps by avoiding unnecessary else-after-transfer and arbitrary indentation-in-else-clause help).

/be

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Recursive rather than iterative, alas, comment on recursion bottoming out

Comment 13

[Andreas Gal :gal]

Comment on attachment 354383 [details] [diff] [review]
Recursive rather than iterative, alas, comment on recursion bottoming out

equalityAlgorithm is a terrible method name. How about just "equality" with overloaded arguments? Anything else is fine too but not the Algorithm thing.

Comment 14

[Jeff Walden [:Waldo]]

Overloading seems generally pretty dangerous for what would be a nested function (or let-loop/named-let!) in any reasonable language; we don't want someone accidentally using the wrong overloaded version without realizing it.

How about equalityHelper?  We're Scheme-y enough around here that I think this is reasonable.  Indeed, it was actually my first thought on a name, except that I slightly dislike the convention of *-helper because it says nothing about what the helper actually *does* (not that "algorithm" is anything more than slightly better, at best).

Comment 15

[Jesse Ruderman]

How about "looseEquality" to contrast with the "strict equality" of ===?

Comment 16

[Brendan Eich [:brendan]]

ES1-3 avoid "loose", we should too.

fooHelper is an extant naming convention in SpiderMonkey. Worksforme.

/be

Comment 17

[Andreas Gal :gal]

I think fooHelper is horrible, but Brendan is the style & naming police around here so go for it :)

Comment 18

[Brendan Eich [:brendan]]

Schemers like fooHelper, nyah nyah! And ho ho ho! :-P

/be

Comment 19

[Jeff Walden [:Waldo]]

Checked into TM with equalityHelper:

http://hg.mozilla.org/tracemonkey/rev/3bd2a3f41a2c

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/89c91656464b

Comment 21

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/411342ac526c
/cvsroot/mozilla/js/tests/js1_8_1/trace/regress-470739.js,v  <--  regress-470739.js
initial revision: 1.1

Comment 22

[Igor Bukanov]

The test case for this bug assumes that jit is always available. This is not the case for 64-bit code which leads to sporadic test case failures. I think the simplest way to fix this would be to add a function to have a function like hasjit and use it in the test case to replace "timejit < timenonjit" with "timejit < timenonjit || !hasjit()". Alternatively the test case can be replaced with "timejit < timenonjit * 1.1" or something. Thoughts?

Comment 23

[Bob Clary [:bc] (inactive)]

Igor, what would hashjit look like?

Comment 24

[Igor Bukanov]

(In reply to comment #23)
> Igor, what would hashjit look like?

Hm, it seems currently the js shell does not expose the necessary functionality to implement a reliable check for the jit presence. We need a function like  hasFeature(feature_name) where feature_name is "threadsafe", "tracer", "gc_zeal" or other compiler-time options. I will file a bug about it.

Comment 25

[Bob Clary [:bc] (inactive)]

modify test to prevent random failures on x64
http://hg.mozilla.org/mozilla-central/rev/3d5294a507b3

Comment 26

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 modulo x64