Port SincResampler's ConvolveBenchmark, as well as VectorMath's benchmarks to media_perftests.

Port SincResampler's ConvolveBenchmark, as well as VectorMath's FMUL and FMAC benchmarks to media_perftests.

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

[rileya (GONE FROM CHROMIUM), 2013-10-28 23:38:56]

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
File media/base/sinc_resampler_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:36: float (*convolve_fn)(const float*,
const float*, const float*, double),
Here and in the vector math tests I pass a function pointer into a testing
function to save duplicating some code. However, given that these are
performance tests, there is some chance that the overhead of potentially
dereferencing the function pointer on each iteration could make things slightly
slower artificially. I imagine most compilers will be clever enough with
optimizations to avoid issues here (some informal testing on Linux didn't show
any appreciable difference in performance), but still...

Making the test function take the function pointer as a template parameter, and
thus generating a copy of the test function for each function pointer, might
make it more likely for the compiler to optimize the dereference out, but that's
a) uglier than just using the fn ptr as a regular param and b) still not a
complete guarantee the compiler will be smart...

[scherkus (not reviewing), 2013-10-29 00:13:45]

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
File media/base/sinc_resampler_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:24: MOCK_METHOD2(ProvideInput, void(int
frames, float* destination));
it doesn't look like this class is doing anything or has any gmock expectations
set

can you try replacing it with:

static void DoNothing(int frames, float* destination) {}

... and base::Bind(&DoNothing) below?

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:34: void RunConvolveBenchmark(
static

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:35: SincResampler& resampler,
the only time we pass as reference is when its const-ref, otherwise use pointers

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:36: float (*convolve_fn)(const float*,
const float*, const float*, double),
On 2013/10/28 23:38:57, rileya wrote:
> Here and in the vector math tests I pass a function pointer into a testing
> function to save duplicating some code. However, given that these are
> performance tests, there is some chance that the overhead of potentially
> dereferencing the function pointer on each iteration could make things
slightly
> slower artificially. I imagine most compilers will be clever enough with
> optimizations to avoid issues here (some informal testing on Linux didn't show
> any appreciable difference in performance), but still...
> 
> Making the test function take the function pointer as a template parameter,
and
> thus generating a copy of the test function for each function pointer, might
> make it more likely for the compiler to optimize the dereference out, but
that's
> a) uglier than just using the fn ptr as a regular param and b) still not a
> complete guarantee the compiler will be smart...

I'd wager the overhead is dwarfed by the convolving code itself. For fun you can
try sticking in some slow code inside the convolving code and seeing if we can
measure any difference

(my assembly is rusty, but I'm curious what the resulting code looks like. I'd
guess w/ function pointers it ends up doing a call instruction w/ the address
stored in a register or on the stack)

https://codereview.chromium.org/41633002/diff/30001/media/base/vector_math_pe...
File media/base/vector_math_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/30001/media/base/vector_math_pe...
media/base/vector_math_perftest.cc:90: sizeof(float)), 0U);
indent here should be aligned at (

https://codereview.chromium.org/41633002/diff/30001/media/base/vector_math_pe...
media/base/vector_math_perftest.cc:121: sizeof(float)), 0U);
ditto

[rileya (GONE FROM CHROMIUM), 2013-10-29 01:23:29]

Alrighty, I think I addressed everything.

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
File media/base/sinc_resampler_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:24: MOCK_METHOD2(ProvideInput, void(int
frames, float* destination));
On 2013/10/29 00:13:45, scherkus wrote:
> it doesn't look like this class is doing anything or has any gmock
expectations
> set
> 
> can you try replacing it with:
> 
> static void DoNothing(int frames, float* destination) {}
> 
> ... and base::Bind(&DoNothing) below?

Seems to work, done.

https://codereview.chromium.org/41633002/diff/30001/media/base/sinc_resampler...
media/base/sinc_resampler_perftest.cc:36: float (*convolve_fn)(const float*,
const float*, const float*, double),
On 2013/10/29 00:13:45, scherkus wrote:
> On 2013/10/28 23:38:57, rileya wrote:
> > Here and in the vector math tests I pass a function pointer into a testing
> > function to save duplicating some code. However, given that these are
> > performance tests, there is some chance that the overhead of potentially
> > dereferencing the function pointer on each iteration could make things
> slightly
> > slower artificially. I imagine most compilers will be clever enough with
> > optimizations to avoid issues here (some informal testing on Linux didn't
show
> > any appreciable difference in performance), but still...
> > 
> > Making the test function take the function pointer as a template parameter,
> and
> > thus generating a copy of the test function for each function pointer, might
> > make it more likely for the compiler to optimize the dereference out, but
> that's
> > a) uglier than just using the fn ptr as a regular param and b) still not a
> > complete guarantee the compiler will be smart...
> 
> I'd wager the overhead is dwarfed by the convolving code itself. For fun you
can
> try sticking in some slow code inside the convolving code and seeing if we can
> measure any difference
> 
> (my assembly is rusty, but I'm curious what the resulting code looks like. I'd
> guess w/ function pointers it ends up doing a call instruction w/ the address
> stored in a register or on the stack) 

Yeah that was what I thought, I'll try to take a look at what the generated code
does.

[scherkus (not reviewing), 2013-10-29 02:16:50]

https://codereview.chromium.org/41633002/diff/110007/media/base/sinc_resample...
File media/base/sinc_resampler_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/110007/media/base/sinc_resample...
media/base/sinc_resampler_perftest.cc:21: // Helper function to ensure
ChunkedResample() functions properly.
this comment refers to a unit test:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/sinc_re...

it should be updated to reflect its purpose for the benchmarks (e.g., providing
no input to SincResampler since we're only benchmarking the convolving
algorithm)

https://codereview.chromium.org/41633002/diff/110007/media/base/sinc_resample...
media/base/sinc_resampler_perftest.cc:38:
convolve_fn(resampler.get_kernel_for_testing() + (aligned ? 0 : 1),
shouldn't these resampler calls use -> notation?

https://codereview.chromium.org/41633002/diff/110007/media/base/sinc_resample...
media/base/sinc_resampler_perftest.cc:56: // Initialize a dummy resampler.
comment is useless -- remove

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_p...
File media/base/vector_math_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_p...
media/base/vector_math_perftest.cc:14: // Default test values.
move these down to after kBenchmarkIterations

(even though they're static we should still keep the code in the media
namespace)

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_p...
media/base/vector_math_perftest.cc:28: static const int kVectorSize = 8192;
I'd move this out along w/ kScale -- there's no reason to keep this split out
from other benchmarking parameters

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_p...
media/base/vector_math_perftest.cc:38: void FillTestVectors(float input, float
output) {
unless there's a good reason to keep around, being a performance test we're not
actually verifying the inputs and outputs (the unit tests do that)

we can simplify a lot of this by doing the following:
  1) Remove k{Input,Output}FillValue and use a value like 0.0 or 1.0
  2) Move fill() calls into ctor after constructing
  3) Remove cal to FillTestVectors() in RunBenchmark()

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_p...
media/base/vector_math_perftest.cc:66: scoped_ptr_malloc<float,
base::ScopedPtrAlignedFree> input_vector;
being member variables they should have a _ suffix (e.g., input_vector_)

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_u...
File media/base/vector_math_unittest.cc (left):

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_u...
media/base/vector_math_unittest.cc:27: // Default test values.
might as well clean these up too by moving into media namespace

https://codereview.chromium.org/41633002/diff/110007/media/base/vector_math_u...
media/base/vector_math_unittest.cc:36: static const int kVectorSize = 8192;
ditto

[rileya (GONE FROM CHROMIUM), 2013-10-29 17:59:51]

Alright, think I addressed everything.

[scherkus (not reviewing), 2013-10-29 18:56:19]

https://codereview.chromium.org/41633002/diff/250001/media/base/sinc_resample...
File media/base/sinc_resampler_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/250001/media/base/sinc_resample...
media/base/sinc_resampler_perftest.cc:21: // Helper function to provide no input
to SincResampler's Convolve benchmark
comments should end w/ periods

https://codereview.chromium.org/41633002/diff/250001/media/base/vector_math_p...
File media/base/vector_math_perftest.cc (right):

https://codereview.chromium.org/41633002/diff/250001/media/base/vector_math_p...
media/base/vector_math_perftest.cc:25: 
remove extra line here

https://codereview.chromium.org/41633002/diff/250001/media/base/vector_math_u...
File media/base/vector_math_unittest.cc (right):

https://codereview.chromium.org/41633002/diff/250001/media/base/vector_math_u...
media/base/vector_math_unittest.cc:19: 
remove extra blank line

[rileya (GONE FROM CHROMIUM), 2013-10-29 19:10:30]

Fixed the 3 issues.

[scherkus (not reviewing), 2013-10-29 19:11:47]

lgtm
On Oct 29, 2013 12:10 PM, <rileya@chromium.org> wrote:

> Fixed the 3 issues.
>
>
https://codereview.chromium.**org/41633002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-29 20:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/41633002/330001

[commit-bot: I haz the power, 2013-10-30 00:21:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-30 17:33:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/41633002/540001

[DaleCurtis, 2013-10-30 20:13:38]

lgtm

[commit-bot: I haz the power, 2013-10-30 20:32:20]

Change committed as 231912