1048754 - 15% regression in kraken-fft on MacOSX 32bit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

On July 25th awfy is reporting a 15% regression on kraken-fft on MacOSX 32bit:
http://arewefastyet.com/#machine=11&view=single&suite=kraken&subtest=fft&start=1406231660&end=1406289474

Possibly bug 1039965?

Comment 1

[Nicholas Nethercote [inactive]]

Sigh. I'll take a look.

Comment 2

[Nicholas Nethercote [inactive]]

This is another edge case involving arrays with power-of-two lengths. In bug
bug 1039965 I added a hack whereby if an array would have its capacity
increased to a size that's just below its length, we nudge the capacity up
slightly so the length will fit without requiring another resizing. That fixed
the 8192-length arrays in audio-oscillator.

In audio-fft it's similar, but we have arrays of length 1024. 1024 is less than
EagerAllocationMaxLength, so we allocate their elements immediately rather than
growing up, and we allocate 2048. To fix this, we can use a slightly different
hack: in this case, if the requested capacity is equal to the length, then just
use the requested capacity exactly as is, because the array probably won't grow
any more.

As well as speeding up audio-fft, this should reduce memory usage in general,
assuming that if you create an array with a particular length it's unusual to
then lengthen it subsequently.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Handle creation of small power-of-two-sized arrays better

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8468218 [details] [diff] [review]
Handle creation of small power-of-two-sized arrays better

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

::: js/src/jsobj.cpp
@@ +3234,5 @@
>  // higher, but we waste less space.
>  //
> +// There are a couple of exceptions that help speed up certain interesting
> +// cases. See below.
> +//

I think this comment should describe how reqAllocated and length relate to each other.  Reading the function body is kind of confusing, especially since there is a missing piece in the heuristics (EagerAllocationMaxLength) which you might want to add a reference to also.

@@ +3249,5 @@
> +        //   var a = new Array(1024);
> +        //
> +        // This is likely to be the final size of the array, so just allocate
> +        // the full length immediately.
> +        goodAllocated = reqAllocated;

I think this would read better if you changed the |if| to 'reqCapacity == length && reqAllocated >= SLOT_CAPACITY_MIN', added a premature return here, and avoided putting all the stuff below inside an |else| block.  For arrays with less than SLOT_CAPACITY_MIN allocated values we should be using the fixed elements instead of dynamically allocated ones anyways.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0441c39f9380

Comment 6

[Nicholas Nethercote [inactive]]

Fingers crossed this fixes the regression on AWFY.

Comment 7

[Nicholas Nethercote [inactive]]

I measured a 64-bit shell build on my Mac (having trouble with 32-bit builds) and this change makes audio-fft about 1.20x faster. Yay.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Fingers crossed this fixes the regression on AWFY.

Yes it does. And also improves some other kraken benchmarks. Thanks!

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0441c39f9380