Optimize loop condition for SincResampler.

Optimize loop condition for SincResampler.

Improves performance by ~2% - 3% on all tested platforms.  Avoids a 20%
performance hit with clang!  Which seems like a bug, so I filed:
http://llvm.org/bugs/show_bug.cgi?id=16578

Results measured by:
media_unittests --gtest_filter=AudioConverterTest.ConvertBenchmark \
--audio-converter-iterations=500000

Switching from size_t to int is a minor performance win on all platforms
< ~1% and reduces code gen by a few hundred bytes with clang, so why not.

BUG=https://code.google.com/p/webrtc/issues/detail?id=2041
TEST=media_unittests

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

[Ami GONE FROM CHROMIUM, 2013-07-09 23:20:08]

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:269: while (virtual_source_idx_ < block_size_) {
what happens if you try:
while ((const int source_idx = virtual_source_idx) < block_size_)
on the various platforms?

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode988
media/media.gyp:988: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
Why not instead change build/common.gypi to not default arm_neon on on non-arm
platforms?

[DaleCurtis, 2013-07-10 21:06:31]

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:269: while (virtual_source_idx_ < block_size_) {
On 2013/07/09 23:20:08, Ami Fischman wrote:
> what happens if you try:
> while ((const int source_idx = virtual_source_idx) < block_size_)
> on the various platforms?

As discussed offline, this doesn't compile, and moving the decl outside of the
while shows the same performance hit seen elsewhere.

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode988
media/media.gyp:988: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
On 2013/07/09 23:20:08, Ami Fischman wrote:
> Why not instead change build/common.gypi to not default arm_neon on on non-arm
> platforms?

Hmm, that's non-trivial since target_arch isn't defined at the point arm_neon is
set (and consumed by other common.gypi pieces). I also worry that it'll have
negative effects on iOS which specifies a target_arch of x86 while really
targeting arm:

https://code.google.com/p/chromium/issues/detail?id=172682

The check I have hear is what is used elsewhere in the codebase (see skia.gyp).

[Ami GONE FROM CHROMIUM, 2013-07-10 21:18:00]

LGTM % maybe unstupid idea

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:269: while (virtual_source_idx_ < block_size_) {
On 2013/07/10 21:06:31, DaleCurtis wrote:
> On 2013/07/09 23:20:08, Ami Fischman wrote:
> > what happens if you try:
> > while ((const int source_idx = virtual_source_idx) < block_size_)
> > on the various platforms?
> 
> As discussed offline, this doesn't compile

FTR that's only b/c of the "const"; declaring the var in loop scope should still
work.

> and moving the decl outside of the
> while shows the same performance hit seen elsewhere.

:(

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:276: #endif
Perverted sense of perversion leads me to wonder how this performs against

for (int loop_iters = (block_size_ - virtual_source_idx_) /
io_sample_rate_ratio_; loop_iters; --loop_iters)

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode455
media/media.gyp:455: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
FWIW, this will break when targeting ios (device, not simulator) in a ninja
build, when target_arch=="armv7".  I don't know what the right thing to do here
is, but that doesn't need to block this CL.

[DaleCurtis, 2013-07-10 23:01:34]

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:276: #endif
On 2013/07/10 21:18:00, Ami Fischman wrote:
> Perverted sense of perversion leads me to wonder how this performs against
> 
> for (int loop_iters = (block_size_ - virtual_source_idx_) /
> io_sample_rate_ratio_; loop_iters; --loop_iters)
> 

Works well on my N4 w/ gcc and OSX w/ clang. I've pinged the guy with the A9 to
see how it does for him.  Strangely if I try to optimize further by making it
std::min(loop_iters, remaining_frames) I hit the 20% performance regression
previously seen with clang...

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode455
media/media.gyp:455: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
On 2013/07/10 21:18:00, Ami Fischman wrote:
> FWIW, this will break when targeting ios (device, not simulator) in a ninja
> build, when target_arch=="armv7".  I don't know what the right thing to do
here
> is, but that doesn't need to block this CL.

Ugh. How does that even work at all? No code outside of common.gypi (that I can
find in code search) checks for a target_arch of armv7. Is none of the existing
code under target_arch=arm used in iOS?

[Ami GONE FROM CHROMIUM, 2013-07-10 23:04:41]

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode455
media/media.gyp:455: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
On 2013/07/10 23:01:34, DaleCurtis wrote:
> On 2013/07/10 21:18:00, Ami Fischman wrote:
> > FWIW, this will break when targeting ios (device, not simulator) in a ninja
> > build, when target_arch=="armv7".  I don't know what the right thing to do
> here
> > is, but that doesn't need to block this CL.
> 
> Ugh. How does that even work at all? No code outside of common.gypi (that I
can
> find in code search) checks for a target_arch of armv7. Is none of the
existing
> code under target_arch=arm used in iOS?

Very little is in fact built into bling...

[DaleCurtis, 2013-07-11 21:04:34]

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/18566009/diff/2001/media/base/sinc_resampler....
media/base/sinc_resampler.cc:276: #endif
On 2013/07/10 23:01:34, DaleCurtis wrote:
> On 2013/07/10 21:18:00, Ami Fischman wrote:
> > Perverted sense of perversion leads me to wonder how this performs against
> > 
> > for (int loop_iters = (block_size_ - virtual_source_idx_) /
> > io_sample_rate_ratio_; loop_iters; --loop_iters)
> > 
> 
> Works well on my N4 w/ gcc and OSX w/ clang. I've pinged the guy with the A9
to
> see how it does for him.  Strangely if I try to optimize further by making it
> std::min(loop_iters, remaining_frames) I hit the 20% performance regression
> previously seen with clang...

Switched to for(), but needs a ceil() to be accurate.  Still faster than the
existing approach.  Added some DCHECKs() to verify accuracy.

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/18566009/diff/2001/media/media.gyp#newcode455
media/media.gyp:455: ['target_arch=="arm" and arm_version>=7 and arm_neon==1', {
On 2013/07/10 23:04:41, Ami Fischman wrote:
> On 2013/07/10 23:01:34, DaleCurtis wrote:
> > On 2013/07/10 21:18:00, Ami Fischman wrote:
> > > FWIW, this will break when targeting ios (device, not simulator) in a
ninja
> > > build, when target_arch=="armv7".  I don't know what the right thing to do
> > here
> > > is, but that doesn't need to block this CL.
> > 
> > Ugh. How does that even work at all? No code outside of common.gypi (that I
> can
> > find in code search) checks for a target_arch of armv7. Is none of the
> existing
> > code under target_arch=arm used in iOS?
> 
> Very little is in fact built into bling...

I've dropped these changes from the latest patch set.

[Ami GONE FROM CHROMIUM, 2013-07-11 21:16:44]

LGTM %:
- update CL description
- add comment above for loop that this is FOR SPEED! and point to this CR
- FTR: i>0 is no slower than simply testing i (for implicit !=0) in the for
loop.

[commit-bot: I haz the power, 2013-07-13 00:04:47]

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

[commit-bot: I haz the power, 2013-07-13 04:21:27]

Change committed as 211538