914478 - 30% octane-Mandreel regression on Sept 10

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hugh Nougher [:Hughman]]

Just saw the octane-Mandreel regression on AWFY of 30% for both Mac Pros.
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=afa6f48ffe9c&tochange=9ff0b038942a

I don't think bz's change is enough it cause it so I blame Bug 899139.

Comment 1

[Timvde]

There's also a 400% (!) regression on misc-bugs-847389-jpeg2000 for the same changeset. It's probably the same bug, so I don't think it's needed to file a new one, but that benchmark should be checked too after fixing this.

Comment 2

[Jan de Mooij [:jandem]]

Shu, can you take a look? Thanks!

Comment 3

[Jan de Mooij [:jandem]]

There's also a 20% Kraken crypto-ccm regression.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: mandreel-regress.patch

As part of bug 909764, djvj refactored a bunch of calls to |PropertyWriteNeedsTypeBarrier|. There's a typo in |setElemTryCache| resulting in the function basically always returning.

This fixes the mandreel regression, but I have no idea why, since before my patch it was always inserting a CallSetElement, which should be slower.

Comment 5

[Shu-yu Guo [:shu]]

Also, mjrosenb found out that the regression needs --parallel-ion-compile=on to reproduce.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 802179 [details] [diff] [review]
mandreel-regress.patch

Review of attachment 802179 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.

Comment 7

[Shu-yu Guo [:shu]]

What's going on is that the typo would erroneously returned false and thus would've been treated as a compilation error, disabling Ion.

The reason *my* patch triggered this is because |setElemTryCache| before my patch never even reached that typo'd call, due to this change:

-    if (!icInspect.sawDenseWrite())
+    if (!icInspect.sawDenseWrite() && !icInspect.sawTypedArrayWrite())
         return true;

Since in these benchmarks no dense write was ever observed, |setElemTryCache| would've just returned true.

Comment 8

[Shu-yu Guo [:shu]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/255093e2f430

Comment 9

[Jan de Mooij [:jandem]]

AWFY confirms this fixes the Octane-mandreel regression and is a nice win on Octane-gameboy..

The Kraken crypto-ccm regression is still there though, a bit weird because Kraken doesn't use typed arrays at all.

Comment 10

[Shu-yu Guo [:shu]]

I'll look at crypto-ccm, weird...

Comment 11

[Shu-yu Guo [:shu]]

Should note we're also winning a bit on jpeg2000 now.

Comment 12

[Boris Zbarsky [:bzbarsky]]

This also improved Dromaeo-CSS by 6% or so.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Which is odd, since I doubt that uses typed arrays.

Comment 14

[Shu-yu Guo [:shu]]

What does dromaeo-css test mainly? The only other big part of this patch was refactoring of some value-to-int logic into the macro assembler.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> What does dromaeo-css test mainly? 

The behavior of things like $(stuff) in jQuery for various values of stuff, and similar for other JS libraries.

Comment 16

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > What does dromaeo-css test mainly? 
> 
> The behavior of things like $(stuff) in jQuery for various values of stuff,
> and similar for other JS libraries.

This bug fixed a bug introduced by bug 909764 which accidentally disabled all setelem ICs. Maybe that was it?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yes, that would certainly do it.  As in, it fixed bug 913752?

Comment 18

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Yes, that would certainly do it.  As in, it fixed bug 913752?

Well, that I can't say for sure. If dromaeo-css was the only one that regressed, then I'd guess so.

Comment 19

[Shu-yu Guo [:shu]]

Does this look like a fix to bug 913752, djvj?

Comment 20

[Kannan Vijayan [:djvj]]

I'm pretty sure it is.  Great job in tracking this down.  I was trying to track down the dromaeo-css regression but not having a lot of luck.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/255093e2f430