1184388 - 270% Kraken crypto-aes regression caused by eager unboxing analysis for large array literals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

A few other tests maybe also regressed, but this one certainly did.

Details:
A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion, unboxed objects

Regression(s)/Improvement(s):
- kraken: 14.93% (regression)
- asmjs-ubench: life: -0.73% (improvement)
- kraken: gaussian-blur: -5.15% (improvement)
- kraken: darkroom: 2.73% (regression)
- kraken: desaturate: 5.99% (regression)
- kraken: crypto-aes: 273.12% (regression)
- octane: Richards: 1.83% (improvement)
- octane: Crypto: -2.28% (regression)
- octane: Typescript: -2.55% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=53ffaa2b1ce2&tochange=b32e6d86fd89

More details: http://arewefastyet.com/regressions/#/regression/1792290

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

We're using unboxed arrays in more places in this benchmark now, and some hot access sites are polymorphic on arrays that are either unboxed int32 arrays or unboxed double arrays.  We end up emitting element caches for these, which are really slow (even if everything happens in jitcode).  There isn't a good, easy fix for this, unfortunately.  A more specialized instruction that can get elements from arrays with multiple different layouts would be faster than caches, but still a lot slower than plain inline accesses.  If we could generate multiple copies of a loop depending on the layouts of the arrays it manipulates that would be really nifty, but also a big departure from IonMonkey's design (though maybe not too different from loop unrolling, which we already support).  This patch goes for an easier solution, though, which just converts unboxed arrays to their native representation if we find multiple different array layouts at an access site during Ion compilation.  This reduces the slowdown for --unboxed-arrays vs. normal behavior on kraken-stanford-crypto-aes to 30% or so (x86 10.9).  I'll write more patches to try to address the remaining regression.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8645191 [details] [diff] [review]
patch

After more tinkering, I don't think this is a good approach; we should try to keep things unboxed where possible and doing it more often opens up a can of worms.

The reason we are seeing polymorphic sites in this benchmark seems to be because the benchmark creates a giant literal which contains objects which can have either unboxed int32 arrays or unboxed double arrays for certain properties.  I'm going to try a more targeted fix that tries to give such structures more homogenous contents.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> After more tinkering, I don't think this is a good approach; we should try
> to keep things unboxed where possible and doing it more often opens up a can
> of worms.

Bleh, "doing it" == "converting objects to natives"

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Here's a more targeted patch to improve parsing of literals like:

[ [1,2,3,4], [1.5,2.5,3.5,4.5] ]

and:

[ {p:[1,2,3,4]}, {p:[1.5,2.5,3.5,4.5]} ]

So that the inner arrays in the multidimensional array have a consistent group and unboxed/native layout.  With this change, stanford-crypto-aes runs at about the same speed with/without unboxed arrays.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b53d012913

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25b53d012913

Comment 7

[Brian Hackett [Laid off!]]

Attachment: followup

Here is a followup patch to fix a crash that is now happening on the kraken benchmark with --unboxed-arrays.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8651845 [details] [diff] [review]
followup

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

Makes sense.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60ea93665ccd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60ea93665ccd

Comment 11

[Brian Hackett [Laid off!]]

Attachment: followup #2

In another followup, the original patch transposed the arguments to GiveObjectGroup at all its call sites.  CombinePlainObjectPropertyTypes would work as expected when running stanford-crypto-aes by itself, but in the full shell harness the structures would still end up with heterogenous layouts.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8a68592a31

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b8a68592a31