883748 - IonMonkey makes this version of Mersenne Twister 13x slower

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: shell testcase

I made a simple change to Mersenne Twister that I thought would make it faster.  Instead, it made IonMonkey (m-c 36da3cb92193) choke entirely.

function unsigned32 (n1)
{
  // UPPER_MASK is 0x80000000
  // return n1 < 0 ? (n1 ^ UPPER_MASK) + UPPER_MASK : n1;
  return n1 >>> 0;
}


js mt1.js
1276 ms

js --no-ion mt1.js
93 ms

Comment 1

[Hannes Verschore [:h4writer]]

I think that makes sense, since "n1 >>> 0" will bail for every negative number encountered. Since we specialize bitops always to |integer range| and the output also to |integer range|. Since a negative number is bigger than a |integer range|. The output doesn't fit what IM knows and we bail temp. to baseline.

This was done because in most cases ">>>" is used with a lhs that is 1 or bigger. And in that case we indeed always stay in the integer range.

I hadn't thought about this case yet and it seems very stupid to bail for half of the inputs to ">>> 0". I think we need to make it possible to have a double as output to >>>.

Comment 2

[Jesse Ruderman]

This testcase might be a regression from 1684c32be328 (increased inlining) but it sounds like the real problem is older.

Comment 3

[Hannes Verschore [:h4writer]]

Problem is present since the inception of IonMonkey. Probably not that noticeable, because the bailout happens at the very last instruction in this testcase. So the time we are in baseline/jm/interpreter is not that long. Off course when inlining this takes longer, until we can enter IM again. (We can't jump at every location to IM: start of functions and sometimes in loops or when going back to a function still running in IM.)

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #1)
> I think that makes sense, since "n1 >>> 0" will bail for every negative
> number encountered. Since we specialize bitops always to |integer range| and
> the output also to |integer range|. Since a negative number is bigger than a
> |integer range|. The output doesn't fit what IM knows and we bail temp. to
> baseline.

Sounds like a duplicate of Bug 825268.

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Sounds like a duplicate of Bug 825268.

Totally not.

Comment 6

[Jan de Mooij [:jandem]]

Ion can handle this just fine by lowering MUrsh to LUrshD, somehow that's not working. I will take a look.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

We were never specializing MUrsh as double. Patch fixes that (by using the baseline ICs).

For the attached testcase:

before  : 245 ms
--no-ion:  68 ms
after   :  12 ms

Comment 8

[Jan de Mooij [:jandem]]

Pretty bad perf regression from bug 804676, we should fix this on aurora too.

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d0ce331a3a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84d0ce331a3a

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 763603 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 804676
User impact if declined: Bad perf regression in some cases (see comment 0, comment 7)
Testing completed (on m-c, etc.): On m-c
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1db75fb7a968

Comment 13

[Ioana (away)]

How can I run the attached shell testcase on Mac OSX 10.7.5?

Comment 14

[Till Schneidereit [:till]]

You can download the shell from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

just unzip it, put it in the same directory as the testcase file, and run it from the terminal like so:

./js testcase.js

Comment 15

[Ioana (away)]

Thanks for your help Till.

Verified as fixed with the 07/30 shells for Nightly, Aurora and Beta. I got 18-20 ms for runs  with IonMonkey, and 73-75 ms for runs without it.