Closed Bug 654664 Opened 10 years ago Closed 10 years ago

setNumber / JSDOUBLE_IS_NEGZERO performance regression on OS X


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox6 - ---


(Reporter: jandem, Assigned: glandium)



(Keywords: perf, regression, Whiteboard: [landed m-c 7/10])


(1 file, 1 obsolete file)

AWFY shows there's a huge imaging-gaussian-blur regression (15%, > 800 ms slower) with JM enabled. Bisecting points to, 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.
Blocks: 640494
Attached patch Possible patch (obsolete) — Splinter Review
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.
Yes, this fixes the regression on the micro-benchmark and the asm is similar to the (d == 0 && signbit(d)) version.
Attachment #529947 - Flags: review?(jimb)
Forgot to mention this earlier but I had to to pull in JSDOUBLE_HI32_SIGNBIT and change u to d to make gcc happy.
(In reply to comment #3)
> and change u to d to make gcc happy.

x instead of d of course.
Attached patch Possible patchSplinter Review
Ah yes, obviously, thanks
Assignee: general → mh+mozilla
Attachment #529947 - Attachment is obsolete: true
Attachment #529947 - Flags: review?(jimb)
Attachment #529951 - Flags: review?(jimb)
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.
Review ping for jimb.
Comment on attachment 529951 [details] [diff] [review]
Possible patch

Review of attachment 529951 [details] [diff] [review]:
Attachment #529951 - Flags: review?(jimb) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Attachment #529951 - Flags: approval-mozilla-beta?
We'll get to this approval requests on Monday's 2pm PT triage.
Whiteboard: [landed m-c 7/10]
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?
Attachment #529951 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
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.
Attachment #529951 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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?
Not tracking for 6 then per previous comments.
You need to log in before you can comment on or make changes to this bug.