Last Comment Bug 599914 - Kraken: audio-beat-detection and audio-fft computes on NaN and undefined
: Kraken: audio-beat-detection and audio-fft computes on NaN and undefined
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Robert Sayre
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 603835
  Show dependency treegraph
 
Reported: 2010-09-27 10:23 PDT by Mads Ager
Modified: 2012-08-19 11:33 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Mads Ager 2010-09-27 10:23:25 PDT
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 [:njn] 2010-09-29 20:08:25 PDT
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 [:njn] 2010-10-12 17:27:12 PDT
(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 2010-11-08 23:47:43 PST
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] 2010-11-10 04:28:34 PST
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 2010-11-10 08:58:11 PST
This is fixed here:

http://hg.mozilla.org/projects/kraken
Comment 6 Nicholas Nethercote [:njn] 2010-11-10 14:51:34 PST
(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 [:njn] 2010-11-10 15:30:45 PST
(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:] 2011-05-05 08:59:52 PDT
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:] 2011-05-05 09:09:17 PDT
sorry, that came across as mean spirited. it just pained me that they would make such publicity hay about it.
Comment 10 Robert Sayre 2011-05-05 11:43:53 PDT
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] (remove +bmo to email) 2011-05-05 15:24:50 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.