654664 - setNumber / JSDOUBLE_IS_NEGZERO performance regression on OS X

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

AWFY shows there's a huge imaging-gaussian-blur regression (15%, > 800 ms slower) with JM enabled. Bisecting points to http://hg.mozilla.org/tracemonkey/rev/83e8315c5f69, bug 640494.

gaussian-blur calls Math.abs *a lot*. Math.abs calls setNumber and we end up in JSDOUBLE_IS_NEGZERO via JSDOUBLE_IS_INT32.

For a micro-benchmark (calling abs(-3.3) 100000000 times) with/without the change to JSDOUBLE_IS_NEGZERO in jsvalue.h:

-m before: 1891 ms
-m after:  2277 ms

interp before: 6290 ms
interp after:  6819 ms

TM and JM+TI are not affected because they inline Math.abs.

JSDOUBLE_IS_NEGZERO used to do this on OS X:
--
return (d == 0 && signbit(d));
--
0x0009f7f6 <JSDOUBLE_IS_NEGZERO+0>:  ucomisd 0x1e954c(%ebx),%xmm1
0x0009f7fe <JSDOUBLE_IS_NEGZERO+8>:  jne    0x9f810 <JSDOUBLE_IS_INT32+26>
0x0009f800 <JSDOUBLE_IS_NEGZERO+10>: jp     0x9f810 <JSDOUBLE_IS_INT32+26>
0x0009f802 <__inline_signbitd+0>:    movsd  %xmm1,0x20(%esp)
0x0009f808 <JSDOUBLE_IS_NEGZERO+18>: mov    0x24(%esp),%eax
0x0009f80c <JSDOUBLE_IS_NEGZERO+22>: test   %eax,%eax
0x0009f80e <JSDOUBLE_IS_NEGZERO+24>: js     0x9f840 <DOUBLE_TO_JSVAL_IMPL>
--
The common case (x != 0) is very fast. Now we do this:
--
union {
    jsdouble d;
    uint64 bits;
} x;
x.d = d;
return x.bits == JSDOUBLE_SIGNBIT;
--
0x0009f772 <JSDOUBLE_IS_NEGZERO+0>: movsd  %xmm1,0x20(%esp)
0x0009f778 <JSDOUBLE_IS_INT32+6>:   mov    0x24(%esp),%eax
0x0009f77c <JSDOUBLE_IS_INT32+10>:  sub    $0x80000000,%eax
0x0009f781 <JSDOUBLE_IS_INT32+15>:  or     0x20(%esp),%eax
0x0009f785 <JSDOUBLE_IS_INT32+19>:  je     0x9f7b0
--
Like 5 instructions if x != 0 and some loads/stores.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Possible patch

This should be faster. It would be simpler written as return d == 0 && JSDOUBLE_IS_NEG(d), but including jsnum.h in jsvalue.h is a nightmare.

Comment 2

[Jan de Mooij [:jandem]]

Yes, this fixes the regression on the micro-benchmark and the asm is similar to the (d == 0 && signbit(d)) version.

Comment 3

[Jan de Mooij [:jandem]]

Forgot to mention this earlier but I had to to pull in JSDOUBLE_HI32_SIGNBIT and change u to d to make gcc happy.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to comment #3)
> and change u to d to make gcc happy.

x instead of d of course.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Possible patch

Ah yes, obviously, thanks

Comment 6

[Johnathan Nightingale [:johnath]]

Hey JimB - is this patch worthwhile and/or do we have another plan here? This bug is tracking-firefox6, but it's been sleepy for a few weeks.

Comment 7

[David Mandelin [:dmandelin]]

Review ping for jimb.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 529951 [details] [diff] [review]
Possible patch

Review of attachment 529951 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 9

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/64d00c88b3a6

Comment 10

[Asa Dotzler [:asa]]

We'll get to this approval requests on Monday's 2pm PT triage.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 529951 [details] [diff] [review]
Possible patch

Moving the nomination to Aurora. We may end up just taking this on central and eating the regression until 8. Anyone have further input that would cause us to rush this into 7?

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 529951 [details] [diff] [review]
Possible patch

Given that this isn't a terrible regression we're seeing in the wild, the triage team wants to just wait on this to make its way through the channels. Re-nominate if you want to try to make a better case for this.

Comment 13

[chris hofmann]

bc and all,   the suspicion is that this code pattern and regression might not show up widely on the web.  anyone have some harder evidence that it might?

Comment 14

[Johnny Stenback (:jst)]

Not tracking for 6 then per previous comments.