Closed Bug 883748 Opened 7 years ago Closed 7 years ago

IonMonkey makes this version of Mersenne Twister 13x slower

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox23 + verified
firefox24 + verified

People

(Reporter: jruderman, Assigned: jandem)

Details

(Keywords: perf, testcase)

Attachments

(2 files)

Attached file 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
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 >>>.
This testcase might be a regression from 1684c32be328 (increased inlining) but it sounds like the real problem is older.
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.)
(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.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Sounds like a duplicate of Bug 825268.

Totally not.
Ion can handle this just fine by lowering MUrsh to LUrshD, somehow that's not working. I will take a look.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
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
Attachment #763603 - Flags: review?(bhackett1024)
Pretty bad perf regression from bug 804676, we should fix this on aurora too.
Attachment #763603 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/84d0ce331a3a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
Attachment #763603 - Flags: approval-mozilla-aurora?
Attachment #763603 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
How can I run the attached shell testcase on Mac OSX 10.7.5?
Flags: needinfo?(jruderman)
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
Flags: needinfo?(jruderman)
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: ioana.budnar
You need to log in before you can comment on or make changes to this bug.