Remove static const from AudioBus interleave methods and optimize!

Remove static const from AudioBus interleave methods and optimize!

Using the new AudioBusTest::InterleaveBench on my Z600 over 5 runs:

Old:
ToInterleaved uint8 took 57.95ms.
FromInterleaved uint8 took 24.76ms.
ToInterleaved int16 took 47.67ms.
FromInterleaved int16 took 23.06ms.
ToInterleaved int32 took 59.64ms.
FromInterleaved int32 took 26.53ms.

New:
ToInterleaved uint8 took 56.09ms.
FromInterleaved uint8 took 21.91ms.
ToInterleaved int16 took 32.68ms.
FromInterleaved int16 took 22.96ms.
ToInterleaved int32 took 42.30ms.
FromInterleaved int32 took 33.45ms.

Results are faster across the board except for FromInterleaved int32
which lost ~10ms with the removal of the static const versus an inline
constant.  I'm writing it off to helpful pipelining.

Overall the new methods are ~13% faster. Benchmarking 10000 calls of
each data type over 480 frames instead of a single block of 120 seconds
of audio yields an even more impressive 2.7x speedup on OSX:

Old: 
[       OK ] AudioBusTest.DISABLED_InterleaveBench (3157 ms)

New:
[       OK ] AudioBusTest.DISABLED_InterleaveBench (1154 ms)

Linux saw only a 10% improvement though -- the discrepancy likely has
to do with OSX's usage of SSE2 instructions (versus no-SSE for Linux).

The new methods are ~15% faster in aggregate on Windows.  It's ~ the same
on ToInterleaved() except in the int32 case where the new method is ~40% faster.
FromInterleaved() is ~ the same except for the uint8 case which is ~70% faster.

BUG=224662
TEST=media_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194467

[scherkus (not reviewing), 2013-04-08 14:48:30]

so the bug is talking about removing statics for raciness not performance

where's the performance gain coming from?

[DaleCurtis, 2013-04-08 16:53:09]

Previously static initialization added a bunch of conditional blocks that
required navigation prior to interleave magic. Now everything is inlined at
compile time.  ToInterleaved() got an extra boost by removing the multiple
conditionals in favor of a single conditional with &.

[scherkus (not reviewing), 2013-04-08 16:55:14]

On 2013/04/08 16:53:09, DaleCurtis wrote:
> Previously static initialization added a bunch of conditional blocks that
> required navigation prior to interleave magic. Now everything is inlined at
> compile time.  ToInterleaved() got an extra boost by removing the multiple
> conditionals in favor of a single conditional with &.

I noticed your benchmark calls each method once. Is it correct to say the
difference is due to not initializing the statics?

What if you looped over those methods thousands of times? Does the ~19% number
hold up?

[DaleCurtis, 2013-04-08 17:13:39]

Actually, in that case it gets a lot faster! Benchmarking 10000 iterations of
each data type over 4800 frames:

Old: 
[       OK ] AudioBusTest.DISABLED_InterleaveBench (30637 ms)

New:
[       OK ] AudioBusTest.DISABLED_InterleaveBench (11340 ms)

A 2.7x speedup.

[scherkus (not reviewing), 2013-04-08 18:37:19]

On 2013/04/08 17:13:39, DaleCurtis wrote:
> Actually, in that case it gets a lot faster! Benchmarking 10000 iterations of
> each data type over 4800 frames:
> 
> Old: 
> [       OK ] AudioBusTest.DISABLED_InterleaveBench (30637 ms)
> 
> New:
> [       OK ] AudioBusTest.DISABLED_InterleaveBench (11340 ms)
> 
> A 2.7x speedup.

Strange. And this is release code? I'm all for performance improvements, but I'm
extremely curious where the gain is coming from. Feel like looking at the
before/after disassembly of the function?

lgtm, but update CL description if you find out why it's so much faster

[DaleCurtis, 2013-04-08 18:58:02]

Yes this is release code. I don't think it's strange. I looked at the assembly
while writing this CL, the old version had a conditional block for static
initialization that needed to be navigated on every call. It makes sense that
the new version is faster since:
1. it avoids that conditional block
2. constants are inlined instead of lookups into static memory.
3. ToInterleaved goes from 3 inner loop conditionals to 1.

[commit-bot: I haz the power, 2013-04-08 19:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/13730004/4001

[commit-bot: I haz the power, 2013-04-08 19:40:27]

Retried try job too often on linux_rel for step(s) media_unittests,
remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[DaleCurtis, 2013-04-08 21:00:30]

https://chromiumcodereview.appspot.com/13730004/diff/4001/media/base/audio_bu...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/13730004/diff/4001/media/base/audio_bu...
media/base/audio_bus.cc:74: sample = static_cast<Fixed>(v * -min) & min;
Sadly, this is actually just plain wrong. I'm not sure the conditionals can be
avoided here. I'll take another look and get a rereview once fixed.

https://chromiumcodereview.appspot.com/13730004/diff/4001/media/base/audio_bu...
media/base/audio_bus.cc:76: sample = static_cast<Fixed>(v * max) & max;
Ditto.

[DaleCurtis, 2013-04-09 00:40:34]

https://codereview.chromium.org/13730004/diff/4001/media/base/audio_bus.cc
File media/base/audio_bus.cc (right):

https://codereview.chromium.org/13730004/diff/4001/media/base/audio_bus.cc#ne...
media/base/audio_bus.cc:74: sample = static_cast<Fixed>(v * -min) & min;
On 2013/04/08 21:00:30, DaleCurtis wrote:
> Sadly, this is actually just plain wrong. I'm not sure the conditionals can be
> avoided here. I'll take another look and get a rereview once fixed.

Fixed. Using the ternary here compiles into a cmov which the original if block
does not. Smaller gains, but correct ones this time :)

[scherkus_google.com, 2013-04-09 02:30:14]

lgtm ... but what about the perf #s on our largest platform?
On Apr 8, 2013 5:40 PM, <dalecurtis@chromium.org> wrote:


https://codereview.chromium.**org/13730004/diff/4001/media/**
base/audio_bus.cc<https://codereview.chromium.org/13730004/diff/4001/media/base/audio_bus.cc>
File media/base/audio_bus.cc (right):

https://codereview.chromium.**org/13730004/diff/4001/media/**
base/audio_bus.cc#newcode74<https://codereview.chromium.org/13730004/diff/4001/media/base/audio_bus.cc#newcode74>

media/base/audio_bus.cc:74: sample = static_cast<Fixed>(v * -min) & min;
On 2013/04/08 21:00:30, DaleCurtis wrote:

> Sadly, this is actually just plain wrong. I'm not sure the
>
conditionals can be

> avoided here. I'll take another look and get a rereview once fixed.
>

Fixed. Using the ternary here compiles into a cmov which the original if
block does not. Smaller gains, but correct ones this time :)

https://codereview.chromium.**org/13730004/<https://codereview.chromium.org/1...

[DaleCurtis, 2013-04-15 20:04:47]

After wrangling with my Windows build and Blink, I finally have builds working
again! tl;dr: The new methods are ~15% faster in aggregate on Windows.  It's ~
the same on ToInterleaved() except in the int32 case where the new method is
~40% faster. FromInterleaved() is ~ the same except for the uint8 case which is
~70% faster.

[scherkus (not reviewing), 2013-04-16 01:30:55]

On 2013/04/15 20:04:47, DaleCurtis wrote:
> After wrangling with my Windows build and Blink, I finally have builds working
> again! tl;dr: The new methods are ~15% faster in aggregate on Windows.  It's ~
> the same on ToInterleaved() except in the int32 case where the new method is
> ~40% faster. FromInterleaved() is ~ the same except for the uint8 case which
is
> ~70% faster.

Thanks for following up here. LGTM++ :)

[DaleCurtis, 2013-04-16 22:21:58]

Committed patchset #2 manually as r194467 (presubmit successful).