Enable FIXED_POINT and -O3 for Opus when building for ARM.

- Enable FIXED_POINT for Opus when building for arm64. This yields:
  ~21% speed up over floating point.
  2.6% increase in executable size.
- Also switched to late expansion of release_optimize in common.gypi (debug_optimize was already using it). This makes it possible to set optimization level via release_optimize in lower level targets.

- Enable -O3 for Opus when building form arm32/arm64. This yields:
  arm32: Speed increase up to 21% (encode) and 5% (decode).
  arm32: 2.8% increase in executable size.
  arm32: Output is bit exact with the output when -Os is used.
  arm64: Speed increase up to 18% (encode) and 25% (decode).
  arm64: 7.4% increase in executable size.
  arm64: Output is exact within one LSB (max difference is 2) with the arm32 output.

Measured using webrtc/audio_codec_speed_tests.

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

[Sergey Ulanov, 2014-06-04 02:47:24]

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:8: ['((OS=="android" or chromeos==1) and
(target_arch=="arm" or target_arch=="arm64")) or (OS=="ios" and
target_arch=="armv7")', {
Can this be simply 'target_arch=="arm" or target_arch=="arm64" or
target_arch=="armv7"'? AFAIK we don't build for ARM for any other OSes.
The conditions below can probably be cleaned up too.

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:75: 'cflags': ['-O3'],
You can set release_optimize=3 instead of adding flags explicitly. E.g. see
ffmpeg.gyp:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

Any reason we need to limit this flag to ARM? Maybe use O3 on Intel too?

[Solis, 2014-06-04 15:04:04]

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:8: ['((OS=="android" or chromeos==1) and
(target_arch=="arm" or target_arch=="arm64")) or (OS=="ios" and
target_arch=="armv7")', {
On 2014/06/04 02:47:24, Sergey Ulanov wrote:
> Can this be simply 'target_arch=="arm" or target_arch=="arm64" or
> target_arch=="armv7"'? AFAIK we don't build for ARM for any other OSes.
> The conditions below can probably be cleaned up too.

l.776, l.845 of build/common.gypi mentions linux but wouldn't we want to enable
the same optimizations there?

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:75: 'cflags': ['-O3'],
On 2014/06/04 02:47:24, Sergey Ulanov wrote:
> You can set release_optimize=3 instead of adding flags explicitly. E.g. see
> ffmpeg.gyp:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

Setting release_optimize in ffmpeg.gyp doesn't appear to do anything. Try
changing it '3' for instance. .ninja's generated for OS=="android" will still
use -Os and other platforms will use -O2. Setting the variable would be a much
better solution than what is in this CL. If you have any idea how to do it I'm
all ears.

> 
> Any reason we need to limit this flag to ARM? Maybe use O3 on Intel too?

The only reason being I haven't tested it on anything but ARM and AFAIK we don't
have any automatic regression tests at this point... :/

[Sergey Ulanov, 2014-06-04 17:55:55]

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:75: 'cflags': ['-O3'],
On 2014/06/04 15:04:04, Solis wrote:
> On 2014/06/04 02:47:24, Sergey Ulanov wrote:
> > You can set release_optimize=3 instead of adding flags explicitly. E.g. see
> > ffmpeg.gyp:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> 
> Setting release_optimize in ffmpeg.gyp doesn't appear to do anything.

You need to set it in this file, not if ffmpeg.gyp. ffmpeg.gyp is just an
example of how it can be done. And it does work for ffmpeg (I see -O2 in
ffmpeg.ninja). Note that the flag has to be set in the 'target_defaults'
section.

> Try
> changing it '3' for instance. .ninja's generated for OS=="android" will still
> use -Os and other platforms will use -O2. Setting the variable would be a much
> better solution than what is in this CL. If you have any idea how to do it I'm
> all ears. 

release_optimize should work for Android too. I've just verified it.

> > 
> > Any reason we need to limit this flag to ARM? Maybe use O3 on Intel too?
> 
> The only reason being I haven't tested it on anything but ARM and AFAIK we
don't
> have any automatic regression tests at this point... :/

There are some tests we run as part of remoting_unittests.

[Solis, 2014-06-04 19:55:42]

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
third_party/opus/opus.gyp:75: 'cflags': ['-O3'],
On 2014/06/04 17:55:55, Sergey Ulanov wrote:
> On 2014/06/04 15:04:04, Solis wrote:
> > On 2014/06/04 02:47:24, Sergey Ulanov wrote:
> > > You can set release_optimize=3 instead of adding flags explicitly. E.g.
see
> > > ffmpeg.gyp:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> > 
> > Setting release_optimize in ffmpeg.gyp doesn't appear to do anything.
> 
> You need to set it in this file, not if ffmpeg.gyp. ffmpeg.gyp is just an
> example of how it can be done. And it does work for ffmpeg (I see -O2 in
> ffmpeg.ninja). Note that the flag has to be set in the 'target_defaults'
> section.
> 
> > Try
> > changing it '3' for instance. .ninja's generated for OS=="android" will
still
> > use -Os and other platforms will use -O2. Setting the variable would be a
much
> > better solution than what is in this CL. If you have any idea how to do it
I'm
> > all ears. 
> 
> release_optimize should work for Android too. I've just verified it.

As discussed over chat, there was a bug in common.gypi causing this to not
actually work (not for ffmpeg, not for opus).

> 
> > > 
> > > Any reason we need to limit this flag to ARM? Maybe use O3 on Intel too?
> > 
> > The only reason being I haven't tested it on anything but ARM and AFAIK we
> don't
> > have any automatic regression tests at this point... :/
> 
> There are some tests we run as part of remoting_unittests.

[tlegrand1, 2014-06-05 06:55:44]

Great clean-up of the gyp-file!
LGTM

On 2014/06/04 19:55:42, Solis wrote:
> https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gyp
> File third_party/opus/opus.gyp (right):
> 
>
https://codereview.chromium.org/315673002/diff/20001/third_party/opus/opus.gy...
> third_party/opus/opus.gyp:75: 'cflags': ['-O3'],
> On 2014/06/04 17:55:55, Sergey Ulanov wrote:
> > On 2014/06/04 15:04:04, Solis wrote:
> > > On 2014/06/04 02:47:24, Sergey Ulanov wrote:
> > > > You can set release_optimize=3 instead of adding flags explicitly. E.g.
> see
> > > > ffmpeg.gyp:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> > > 
> > > Setting release_optimize in ffmpeg.gyp doesn't appear to do anything.
> > 
> > You need to set it in this file, not if ffmpeg.gyp. ffmpeg.gyp is just an
> > example of how it can be done. And it does work for ffmpeg (I see -O2 in
> > ffmpeg.ninja). Note that the flag has to be set in the 'target_defaults'
> > section.
> > 
> > > Try
> > > changing it '3' for instance. .ninja's generated for OS=="android" will
> still
> > > use -Os and other platforms will use -O2. Setting the variable would be a
> much
> > > better solution than what is in this CL. If you have any idea how to do it
> I'm
> > > all ears. 
> > 
> > release_optimize should work for Android too. I've just verified it.
> 
> As discussed over chat, there was a bug in common.gypi causing this to not
> actually work (not for ffmpeg, not for opus).
> 
> > 
> > > > 
> > > > Any reason we need to limit this flag to ARM? Maybe use O3 on Intel too?
> > > 
> > > The only reason being I haven't tested it on anything but ARM and AFAIK we
> > don't
> > > have any automatic regression tests at this point... :/
> > 
> > There are some tests we run as part of remoting_unittests.

[Sergey Ulanov, 2014-06-05 20:54:53]

lgtm

[tlegrand1, 2014-06-16 07:45:12]

The CQ bit was checked by tlegrand@chromium.org

[commit-bot: I haz the power, 2014-06-16 07:46:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/solenberg@chromium.org/315673002/130001

[commit-bot: I haz the power, 2014-06-16 13:28:10]

Change committed as 277414

[Nico, 2014-06-23 22:08:50]

A revert of this CL has been created in
https://codereview.chromium.org/349293006/ by thakis@chromium.org.

The reason for reverting is: Media tests started failing consistently after
this:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28t...

It doesn't look super related, but the <() / >() change in common.gypi looks a
bit suspicious, so revert this speculatively and see if it helps..

[chromium-reviews, 2014-06-24 10:00:05]

Not entirely unexpected if that's the case, but:

- I don't see why the evaluation order of the optimize flags should be
different for release and debug targets, so either the change in my CL
should go in, or the <() >() for debug_optimize should be changed.
- FFMpeg is one of the libs that explicitly uses a different optimize
setting. If it doesn't work with the flag it uses, it should stick to the
default, hence, ffmpeg's .gypi should be fixed and this CL resubmitted.



On Tue, Jun 24, 2014 at 12:08 AM, <thakis@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/349293006/ by thakis@chromium.org.
>
> The reason for reverting is: Media tests started failing consistently after
> this:
> http://build.chromium.org/p/chromium.memory.fyi/builders/
> Linux%20Tests%20%28tsan%29%281%29?numbuilds=200
>
> It doesn't look super related, but the <() / >() change in common.gypi
> looks a
> bit suspicious, so revert this speculatively and see if it helps..
>
> https://codereview.chromium.org/315673002/
>

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