The default bug view has changed. See this FAQ.

setNumber / JSDOUBLE_IS_NEGZERO performance regression on OS X

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jandem, Assigned: glandium)

Tracking

({perf, regression})

unspecified
mozilla8
perf, regression
Points:
---

Firefox Tracking Flags

(firefox6-)

Details

(Whiteboard: [landed m-c 7/10])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 640494
(Assignee)

Comment 1

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

Comment 2

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

Updated

6 years ago
Attachment #529947 - Flags: review?(jimb)
(Reporter)

Comment 3

6 years ago
Forgot to mention this earlier but I had to to pull in JSDOUBLE_HI32_SIGNBIT and change u to d to make gcc happy.
(Reporter)

Comment 4

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

x instead of d of course.
(Assignee)

Comment 5

6 years ago
Created attachment 529951 [details] [diff] [review]
Possible patch

Ah yes, obviously, thanks
Assignee: general → mh+mozilla
Attachment #529947 - Attachment is obsolete: true
Attachment #529947 - Flags: review?(jimb)
Attachment #529951 - Flags: review?(jimb)
tracking-firefox6: --- → ?
Keywords: perf, regression

Updated

6 years ago
tracking-firefox6: ? → +
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 8

6 years ago
Comment on attachment 529951 [details] [diff] [review]
Possible patch

Review of attachment 529951 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #529951 - Flags: review?(jimb) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/mozilla-central/rev/64d00c88b3a6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Attachment #529951 - Flags: approval-mozilla-beta?

Comment 10

6 years ago
We'll get to this approval requests on Monday's 2pm PT triage.
Whiteboard: [landed m-c 7/10]

Comment 11

6 years ago
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 12

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

Comment 13

6 years ago
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.
tracking-firefox6: + → -
You need to log in before you can comment on or make changes to this bug.