Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {perf, testcase, verified1.9.1})

Trunk
perf, testcase, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

10 years ago
Waldo, anything to say in your defense? I thought ::cmp is complete now :)
(Assignee)

Comment 2

10 years ago
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED

Comment 3

10 years ago
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.
(Reporter)

Comment 4

10 years ago
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? ;)
(Assignee)

Comment 5

10 years ago
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.
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
Flags: wanted1.9.1?
Flags: blocking1.9.1?

Updated

10 years ago
Severity: minor → major
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 7

10 years ago
Patch in a bit!
Summary: null==undefined does not trace: "unsupported operand types for cmp" → Never abort on ==
(Assignee)

Comment 8

10 years ago
Created attachment 354374 [details] [diff] [review]
I am astounded how readable this is for determining spec-compliance

All hail imacros!
Attachment #354374 - Flags: review?(gal)
(Assignee)

Comment 9

10 years ago
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!

Updated

10 years ago
Attachment #354374 - Flags: review?(gal) → review+

Comment 10

10 years ago
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 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
Attachment #354374 - Flags: review+ → review?(gal)
(Assignee)

Comment 12

10 years ago
Created attachment 354383 [details] [diff] [review]
Recursive rather than iterative, alas, comment on recursion bottoming out
Attachment #354374 - Attachment is obsolete: true
Attachment #354383 - Flags: review?(gal)
Attachment #354374 - Flags: review?(gal)

Updated

10 years ago
Attachment #354383 - Flags: review?(gal) → review+

Comment 13

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

Comment 14

10 years ago
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).
(Reporter)

Comment 15

10 years ago
How about "looseEquality" to contrast with the "strict equality" of ===?
ES1-3 avoid "loose", we should too.

fooHelper is an extant naming convention in SpiderMonkey. Worksforme.

/be

Comment 17

10 years ago
I think fooHelper is horrible, but Brendan is the style & naming police around here so go for it :)
Schemers like fooHelper, nyah nyah! And ho ho ho! :-P

/be
(Assignee)

Comment 19

9 years ago
Checked into TM with equalityHelper:

http://hg.mozilla.org/tracemonkey/rev/3bd2a3f41a2c
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Whiteboard: fixed-in-tracemonkey → [needs 1.9.1 landing]

Comment 20

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/89c91656464b
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]

Comment 21

9 years ago
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
Flags: in-testsuite+
Flags: in-litmus-

Comment 22

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

9 years ago
Igor, what would hashjit look like?

Comment 24

9 years ago
(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

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

Comment 26

9 years ago
v 1.9.1, 1.9.2 modulo x64
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.