Last Comment Bug 654664 - setNumber / JSDOUBLE_IS_NEGZERO performance regression on OS X
: setNumber / JSDOUBLE_IS_NEGZERO performance regression on OS X
[landed m-c 7/10]
: perf, regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla8
Assigned To: Mike Hommey [:glandium]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 640494
  Show dependency treegraph
Reported: 2011-05-04 01:07 PDT by Jan de Mooij [:jandem]
Modified: 2013-12-27 14:24 PST (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Possible patch (877 bytes, patch)
2011-05-04 01:26 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Possible patch (1.06 KB, patch)
2011-05-04 02:32 PDT, Mike Hommey [:glandium]
jimb: review+
asa: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2011-05-04 01:07:54 PDT
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.
Comment 1 User image Mike Hommey [:glandium] 2011-05-04 01:26:24 PDT
Created attachment 529947 [details] [diff] [review]
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 User image Jan de Mooij [:jandem] 2011-05-04 02:05:11 PDT
Yes, this fixes the regression on the micro-benchmark and the asm is similar to the (d == 0 && signbit(d)) version.
Comment 3 User image Jan de Mooij [:jandem] 2011-05-04 02:18:09 PDT
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 User image Jan de Mooij [:jandem] 2011-05-04 02:19:37 PDT
(In reply to comment #3)
> and change u to d to make gcc happy.

x instead of d of course.
Comment 5 User image Mike Hommey [:glandium] 2011-05-04 02:32:25 PDT
Created attachment 529951 [details] [diff] [review]
Possible patch

Ah yes, obviously, thanks
Comment 6 User image Johnathan Nightingale [:johnath] 2011-06-09 14:54:49 PDT
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 User image David Mandelin [:dmandelin] 2011-06-29 18:39:57 PDT
Review ping for jimb.
Comment 8 User image Jim Blandy :jimb 2011-07-01 10:15:36 PDT
Comment on attachment 529951 [details] [diff] [review]
Possible patch

Review of attachment 529951 [details] [diff] [review]:
Comment 9 User image Mike Hommey [:glandium] 2011-07-10 23:15:35 PDT
Comment 10 User image Asa Dotzler [:asa] 2011-07-17 20:59:20 PDT
We'll get to this approval requests on Monday's 2pm PT triage.
Comment 11 User image Asa Dotzler [:asa] 2011-07-18 14:26:28 PDT
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 User image Asa Dotzler [:asa] 2011-07-19 14:45:02 PDT
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 User image chris hofmann 2011-07-19 14:45:51 PDT
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 User image Johnny Stenback (:jst, 2011-08-01 15:02:44 PDT
Not tracking for 6 then per previous comments.

Note You need to log in before you can comment on or make changes to this bug.