599914 - Kraken: audio-beat-detection and audio-fft computes on NaN and undefined

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mads Ager]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.63 Safari/534.3
Build Identifier: 

The Kraken audio-fft and audio-beat-detection benchmarks use most of their time doing arithmetic on NaN and undefined which does not seem like something JS engines should optimize for. :)

The problem is that reverseTable in these benchmarks is an Array which is initialized with undefined values. The original code uses a Uint32Array which is initialize with 0 (and incorrectly uses an Array when Uint32Arrays are not present). The code should zero initialize the array.

Adding some sort of verification that the benchmark computes the right thing would be good. That would have caught this and it could help catch invalid optimizations in the future for all the JS engine implementors. 

Reproducible: Always

Comment 1

[Nicholas Nethercote [inactive]]

FWIW, audio-fft and audio-beat-detection are almost the same benchmark.  
For example, they both spend roughly 50% of their time in the loop that begins on line 334 of audio-{beat-detection,fft}.js.  Other hot loops look very similar, eg. those on lines 153, 331, 309.

This isn't surprising when you realize that the lines 2--1918 of audio-{beat-detection,fft}.js are identical.  audio-beat-detection is just audio-fft with a negligible bit of extra stuff at the end.  There's a strong case to be made for removing one of the tests.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> FWIW, audio-fft and audio-beat-detection are almost the same benchmark.  

I opened bug 603837 for this.

Comment 3

[Mads Ager]

Has this been fixed in the current repository? I just attempted to find the source at http://hg.mozilla.org/projects/kraken but there is nothing there. I'm probably looking in the wrong place?

If you point me to the right place I would be happy to provide a patch for this benchmark to initialize the array with zeros so it makes sense.

Comment 4

[Julian Seward [:jseward]]

FP arithmetic on non-normalised numbers is also often rumoured to be
very slow, hence potentially impacting the representativeness and/or
portability of this benchmark.  The Intel Optimization Reference
Manual of Nov 09 says

  User/Source Coding Rule 14. (H impact, ML generality)

  Make sure your application stays in range to avoid denormal values,
  underflows..  Out-of-range numbers cause very high overhead.

Comment 5

[Robert Sayre]

This is fixed here:

http://hg.mozilla.org/projects/kraken

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> FP arithmetic on non-normalised numbers is also often rumoured to be
> very slow, hence potentially impacting the representativeness and/or
> portability of this benchmark.

Indeed.  I just timed the old and new versions on my MacBook Pro:
- audio-fft went from 560ms to 380ms.
- audio-beat-detection went from 810ms to 578ms.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> (In reply to comment #4)
> > FP arithmetic on non-normalised numbers is also often rumoured to be
> > very slow, hence potentially impacting the representativeness and/or
> > portability of this benchmark.
> 
> Indeed.  I just timed the old and new versions on my MacBook Pro:
> - audio-fft went from 560ms to 380ms.
> - audio-beat-detection went from 810ms to 578ms.

On further analysis, the instruction counts dropped significantly too, ie. enough to explain the differences.  There are fewer property access operations and fewer number-to-string conversions.  I'm not sure why.

Anyway, it's good that the benchmarks have been fixed!

Comment 8

[Bob Clary [:bc] (inactive)]

According to http://blog.chromium.org/2011/05/updating-javascript-benchmarks-for.html http://krakenbenchmark.mozilla.org/ has not been updated with the fix for this. Google is now hosting kraken at http://kraken-mirror.googlecode.com/svn/trunk/kraken/hosted/index.html

fail.

Comment 9

[Bob Clary [:bc] (inactive)]

sorry, that came across as mean spirited. it just pained me that they would make such publicity hay about it.

Comment 10

[Robert Sayre]

Yeah, it was surprising to me as well (it's not like they emailed me to expedite an update...). I think what I'll do is just release whatever is there now as Kraken 1.1, even though I had planned to make more changes. Then, the rest of the changes can come along as Kraken 1.2.

Comment 11

[Jeff Walden [:Waldo]]

Why not have point releases for small-change stuff like this, and just have 1.1.1 or so?  Seems best of all worlds to me.