Enable FIXED_POINT and -O3 for Opus when building for ARM

Enable FIXED_POINT and -O3 for Opus when building for ARM

The original CL:
https://codereview.chromium.org/315673002/

The revert of the CL above:
https://codereview.chromium.org/349293006/

The reason for the original revert:
Media tests started failing consistently on a tsan bot
(http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28tsan%29%281%29?numbuilds=200) with revision 277414 (the commit of original CL).
Cl was reverted in revision 279198.

This CL solved the problem differently, by not using the release_optimize flag, and instead replace the cflag -Os with -O3.

Opus speed gain with -03 flag on ARM:
ARM32b - 6-8% for encoding, 2% for decoding
ARM64b - 20% for encoding, 17% for decoding

Increased binary size: 

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

[tlegrand1, 2014-06-27 13:51:54]

Hi Sergey,

The CL which enables -O3 flag for Opus was reverted, but I believe it is safe to
re-land. The problem the revert tried to solve seems to have been unrelated.

This CL is a replica of the original, basically. I didn't want to revert the
revert, because it becomes difficult to understand what such CL actually change.

Please help review.

Nico, if you by any chance reads email when away, can you please review this
too?

[tommi (sloooow) - chröme, 2014-06-27 14:27:55]

https://codereview.chromium.org/354903003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/354903003/diff/1/build/common.gypi#newcode3446
build/common.gypi:3446: '-O>(release_optimize)',
would it make sense to commit this in a separate CL just to address this
particular issue? (since it doesn't seem specific to the opus change)

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:10: 'release_optimize': '3',
is a check for ARM not needed/appropriate?

[Nico, 2014-06-27 15:36:50]

I'm relatively sure the problems weren't unrelated; the bots went green
after the revert. Why do you think they were unrelated?
On Jun 27, 2014 7:27 AM, <tommi@chromium.org> wrote:

>
> https://codereview.chromium.org/354903003/diff/1/build/common.gypi
> File build/common.gypi (right):
>
> https://codereview.chromium.org/354903003/diff/1/build/
> common.gypi#newcode3446
> build/common.gypi:3446: '-O>(release_optimize)',
> would it make sense to commit this in a separate CL just to address this
> particular issue? (since it doesn't seem specific to the opus change)
>
> https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> File third_party/opus/opus.gyp (right):
>
> https://codereview.chromium.org/354903003/diff/1/third_
> party/opus/opus.gyp#newcode10
> third_party/opus/opus.gyp:10: 'release_optimize': '3',
> is a check for ARM not needed/appropriate?
>
> https://codereview.chromium.org/354903003/
>

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

[tlegrand1, 2014-06-27 16:08:25]

On 2014/06/27 15:36:50, Nico (away) wrote:
> I'm relatively sure the problems weren't unrelated; the bots went green
> after the revert. Why do you think they were unrelated?


Thanks for answering!

I can't find the logs, but if I recall correctly the bots went green after your
revert. But that doesn't necessarily mean it was related. Now with the bot gone,
how do we proceed? Did the failure occur on any other bot? Could it be that the
old bot was configured differently? IF the change passes all bots this time,
should we care about the old bot? Do you remember what failed?

If you build on Linux today, and try to change the optimization setting for
Ffmpeg, it will not have any effect. Same for Opus, and that is why we need to
change the common.gypi. With the change made in this CL it is possible to change
the opt settings, both for Opus and Ffmpeg.

On Jun 27, 2014 7:27 AM, <mailto:tommi@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/354903003/diff/1/build/common.gypi
> > File build/common.gypi (right):
> >
> > https://codereview.chromium.org/354903003/diff/1/build/
> > common.gypi#newcode3446
> > build/common.gypi:3446: '-O>(release_optimize)',
> > would it make sense to commit this in a separate CL just to address this
> > particular issue? (since it doesn't seem specific to the opus change)
> >
> > https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> > File third_party/opus/opus.gyp (right):
> >
> > https://codereview.chromium.org/354903003/diff/1/third_
> > party/opus/opus.gyp#newcode10
> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> > is a check for ARM not needed/appropriate?
> >
> > https://codereview.chromium.org/354903003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-06-27 16:13:55]

Hm, if the bot is gone it's cool I guess.

Please do what tommi asked for though. Also, that character was explicitly
changed in https://chromiumcodereview.appspot.com/10828084 about 2 years ago, so
please check that the bug that that change fixed back then isn't reintroduced.

[tlegrand1, 2014-06-27 16:37:53]

Tommi's comments:

https://codereview.chromium.org/354903003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/354903003/diff/1/build/common.gypi#newcode3446
build/common.gypi:3446: '-O>(release_optimize)',
Yes, I think it does. I'll create a new CL for this change.

On 2014/06/27 14:27:54, tommi wrote:
> would it make sense to commit this in a separate CL just to address this
> particular issue? (since it doesn't seem specific to the opus change)

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:10: 'release_optimize': '3',
No, not needed. We want the -O3 flag on all platforms. The comment just says
that the perf gain is substantial on ARM.

On 2014/06/27 14:27:54, tommi wrote:
> is a check for ARM not needed/appropriate?

[tommi (sloooow) - chröme, 2014-06-28 16:33:15]

Thanks Tina.  The opus.gyp change looks good.  I'll wait for the next patch set.

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:10: 'release_optimize': '3',
On 2014/06/27 16:37:53, tlegrand1 wrote:
> No, not needed. We want the -O3 flag on all platforms. The comment just says
> that the perf gain is substantial on ARM.
> 
> On 2014/06/27 14:27:54, tommi wrote:
> > is a check for ARM not needed/appropriate?
> 

Ah I see.  That wasn't clear to me from the comment so can you add that info? 
E.g. something like:

# In release builds we always want -O3 optimization, which is not the Chrome
default, on all platforms.
# On ARM (v7,v8) in particular, performance gains of using -O3 have been shown
to be substantial.

[Nico, 2014-06-29 21:40:21]

On Sat, Jun 28, 2014 at 9:33 AM, <tommi@chromium.org> wrote:

> Thanks Tina.  The opus.gyp change looks good.  I'll wait for the next
> patch set.
>
>
>
> https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> File third_party/opus/opus.gyp (right):
>
> https://codereview.chromium.org/354903003/diff/1/third_
> party/opus/opus.gyp#newcode10
> third_party/opus/opus.gyp:10: 'release_optimize': '3',
> On 2014/06/27 16:37:53, tlegrand1 wrote:
>
>> No, not needed. We want the -O3 flag on all platforms. The comment
>>
> just says
>
>> that the perf gain is substantial on ARM.
>>
>
>  On 2014/06/27 14:27:54, tommi wrote:
>> > is a check for ARM not needed/appropriate?
>>
>
>
> Ah I see.  That wasn't clear to me from the comment so can you add that
> info?  E.g. something like:
>
> # In release builds we always want -O3 optimization, which is not the
> Chrome default, on all platforms.
> # On ARM (v7,v8) in particular, performance gains of using -O3 have been
> shown to be substantial.
>

BTW, the reason we -Os by default on Android is binary size. What's the
size impact of changing this here?


>
> https://codereview.chromium.org/354903003/
>

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

[tlegrand1, 2014-06-30 13:35:53]

On 2014/06/29 21:40:21, Nico (away) wrote:
> On Sat, Jun 28, 2014 at 9:33 AM, <mailto:tommi@chromium.org> wrote:
> 
> > Thanks Tina.  The opus.gyp change looks good.  I'll wait for the next
> > patch set.
> >
> >
> >
> > https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> > File third_party/opus/opus.gyp (right):
> >
> > https://codereview.chromium.org/354903003/diff/1/third_
> > party/opus/opus.gyp#newcode10
> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> > On 2014/06/27 16:37:53, tlegrand1 wrote:
> >
> >> No, not needed. We want the -O3 flag on all platforms. The comment
> >>
> > just says
> >
> >> that the perf gain is substantial on ARM.
> >>
> >
> >  On 2014/06/27 14:27:54, tommi wrote:
> >> > is a check for ARM not needed/appropriate?
> >>
> >
> >
> > Ah I see.  That wasn't clear to me from the comment so can you add that
> > info?  E.g. something like:
> >
> > # In release builds we always want -O3 optimization, which is not the
> > Chrome default, on all platforms.
> > # On ARM (v7,v8) in particular, performance gains of using -O3 have been
> > shown to be substantial.
> >
> 
> BTW, the reason we -Os by default on Android is binary size. What's the
> size impact of changing this here?
> 

There is a 2.77% increase in binary size, from 6227084 to 6399648.

> 
> >
> > https://codereview.chromium.org/354903003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-06-30 16:46:02]

(aurimas: See "^^^" below)

On Mon, Jun 30, 2014 at 6:35 AM, <tlegrand@chromium.org> wrote:

> On 2014/06/29 21:40:21, Nico (away) wrote:
>
>  On Sat, Jun 28, 2014 at 9:33 AM, <mailto:tommi@chromium.org> wrote:
>>
>
>  > Thanks Tina.  The opus.gyp change looks good.  I'll wait for the next
>> > patch set.
>> >
>> >
>> >
>> > https://codereview.chromium.org/354903003/diff/1/third_
>> party/opus/opus.gyp
>> > File third_party/opus/opus.gyp (right):
>> >
>> > https://codereview.chromium.org/354903003/diff/1/third_
>> > party/opus/opus.gyp#newcode10
>> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
>> > On 2014/06/27 16:37:53, tlegrand1 wrote:
>> >
>> >> No, not needed. We want the -O3 flag on all platforms. The comment
>> >>
>> > just says
>> >
>> >> that the perf gain is substantial on ARM.
>> >>
>> >
>> >  On 2014/06/27 14:27:54, tommi wrote:
>> >> > is a check for ARM not needed/appropriate?
>> >>
>> >
>> >
>> > Ah I see.  That wasn't clear to me from the comment so can you add that
>> > info?  E.g. something like:
>> >
>> > # In release builds we always want -O3 optimization, which is not the
>> > Chrome default, on all platforms.
>> > # On ARM (v7,v8) in particular, performance gains of using -O3 have been
>> > shown to be substantial.
>> >
>>
>
>  BTW, the reason we -Os by default on Android is binary size. What's the
>> size impact of changing this here?
>>
>
>
> There is a 2.77% increase in binary size, from 6227084 to 6399648.


^^^ I'm not sure if that's considered acceptable. Aurimas either knows, or
knows who knows.


>
>
>
>  >
>> > https://codereview.chromium.org/354903003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/354903003/
>

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

[aurimas (slooooooooow), 2014-06-30 16:54:43]

> # In release builds we always want -O3 optimization, which is not the
> Chrome default, on all platforms.
> # On ARM (v7,v8) in particular, performance gains of using -O3 have been
> shown to be substantial.

Can you please put a number to "substantial" performance gains?

> There is a 2.77% increase in binary size, from 6227084 to 6399648.

On Chrome for Android we try to be very careful about binary size and only
sacrifice size for "important" performance improvements. 2.77% is a large
increase for a non-core browser feature.

+yfriedman

[tommi (sloooow) - chröme, 2014-06-30 18:27:41]

Tina is going to check this again. She looked at numbers from Fredrik but
it's not clear if this is an increase of 2.77% for Chrome or just this one
build target.
On Jun 30, 2014 6:54 PM, <aurimas@chromium.org> wrote:

> # In release builds we always want -O3 optimization, which is not the
>> Chrome default, on all platforms.
>> # On ARM (v7,v8) in particular, performance gains of using -O3 have been
>> shown to be substantial.
>>
>
> Can you please put a number to "substantial" performance gains?
>
>  There is a 2.77% increase in binary size, from 6227084 to 6399648.
>>
>
> On Chrome for Android we try to be very careful about binary size and only
> sacrifice size for "important" performance improvements. 2.77% is a large
> increase for a non-core browser feature.
>
> +yfriedman
>
>
> https://codereview.chromium.org/354903003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[tlegrand1, 2014-07-01 08:00:18]

I'll get back with numbers on performance gain and new measures of code size. I
understand your concerns, and an increase of total size of 2.77% sound a lot to
me too.


I had a look at the issue linked to in comment #5, and search the history of
common.gypi.

"release_optimize" was originally introduced to speed up valgrind bots (using
early expansion):
http://src.chromium.org/viewvc/chrome?view=revision&revision=21169

I didn't search for all changes, but apparently this parameter, and
debug_optimize, have been changed back and forth a number of times.

The most resent change is from two years ago, and even though the title says
"Use early expansion for debug_optimize and release_optimize", the CL only
changes release_optimize. After this change, debug_optimize and release_optimize
uses different expansion rules:
https://chromiumcodereview.appspot.com/10828084

Before that there was an attempt to change from late to early expansion three
years ago (both debug_optimize and release_optimize):
https://codereview.chromium.org/6267004
But it got immediately reverted because it broke clang/linux:
http://src.chromium.org/viewvc/chrome?view=revision&revision=73329

It must have been changed at least one more time before this. Now I'm confused.
What is the intention with this parameter?

[tlegrand1, 2014-07-01 10:47:37]

I don't think we should use release_optimize for compiling Opus with -O3. I will
upload new files soonish.

Regarding the performance improvement and increase in size, I have copied the
data from the original CL and pasted here. I *think* the 2.8% increase for arm32
in executable size is for the specific speed test used to measure performance.
I'm taking over this work from a colleague who's on leave, in case you think I'm
unorganized. :)

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.
arm64: Speed increase up to 18% (encode) and 25% (decode).
arm64: 7.4% increase in executable size.

[tommi (sloooow) - chröme, 2014-07-01 11:55:48]

On 2014/07/01 10:47:37, tlegrand1 wrote:
> I don't think we should use release_optimize for compiling Opus with -O3. I
will
> upload new files soonish.
> 
> Regarding the performance improvement and increase in size, I have copied the
> data from the original CL and pasted here. I *think* the 2.8% increase for
arm32
> in executable size is for the specific speed test used to measure performance.
> I'm taking over this work from a colleague who's on leave, in case you think
I'm
> unorganized. :)
> 
> 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.
> arm64: Speed increase up to 18% (encode) and 25% (decode).
> arm64: 7.4% increase in executable size.

Nico - Just to be clear, none of these size numbers mean anything for the Chrome
binary size.  That number is on the way and will most likely be order(s) of
magnitude smaller.

[Nico, 2014-07-01 15:21:19]

Then let's wait for the chrome/android size number :-)
On Jul 1, 2014 4:55 AM, <tommi@chromium.org> wrote:

> On 2014/07/01 10:47:37, tlegrand1 wrote:
>
>> I don't think we should use release_optimize for compiling Opus with -O3.
>> I
>>
> will
>
>> upload new files soonish.
>>
>
>  Regarding the performance improvement and increase in size, I have copied
>> the
>> data from the original CL and pasted here. I *think* the 2.8% increase for
>>
> arm32
>
>> in executable size is for the specific speed test used to measure
>> performance.
>> I'm taking over this work from a colleague who's on leave, in case you
>> think
>>
> I'm
>
>> unorganized. :)
>>
>
>  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.
>> arm64: Speed increase up to 18% (encode) and 25% (decode).
>> arm64: 7.4% increase in executable size.
>>
>
> Nico - Just to be clear, none of these size numbers mean anything for the
> Chrome
> binary size.  That number is on the way and will most likely be order(s) of
> magnitude smaller.
>
> https://codereview.chromium.org/354903003/
>

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

[tlegrand1, 2014-07-02 17:15:16]

I don't think we should change the expansion order for release_optimize, and
instead I have limited the change to only use the -O3 on ARM, where we really
need it.

You can disregard my previous number of 2.77% increase in binary size. That
number was only for a webrtc test executable, and is not what we are interested
in here. I have trouble measuring the increase in binary size, and would be
happy for some advice. I built on Linux with -O3 for Opus and saw a size
increase for out/Release/chrome of 0.04%. Is that a correct way of measuring?

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
File third_party/opus/opus.gyp (right):

https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:10: 'release_optimize': '3',
I decided to skip using the release_optimize flag, and only use -O3 on ARM for
now.

[tlegrand1, 2014-07-02 17:16:38]

Forgot to say: I have updated the name of this CL, as well as the description,
to match my changes in Patch Set 2.

[aurimas (slooooooooow), 2014-07-02 17:33:05]

On 2014/07/02 17:15:16, tlegrand1 wrote:
> I don't think we should change the expansion order for release_optimize, and
> instead I have limited the change to only use the -O3 on ARM, where we really
> need it.
> 
> You can disregard my previous number of 2.77% increase in binary size. That
> number was only for a webrtc test executable, and is not what we are
interested
> in here. I have trouble measuring the increase in binary size, and would be
> happy for some advice. I built on Linux with -O3 for Opus and saw a size
> increase for out/Release/chrome of 0.04%. Is that a correct way of measuring?
> 
> https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> File third_party/opus/opus.gyp (right):
> 
>
https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
> third_party/opus/opus.gyp:10: 'release_optimize': '3',
> I decided to skip using the release_optimize flag, and only use -O3 on ARM for
> now.

Is opus built automatically when building Chrome for Android? I can locally try
building with your patch to see the size change.

[tlegrand1, 2014-07-03 13:00:10]

On 2014/07/02 17:33:05, aurimas wrote:
> On 2014/07/02 17:15:16, tlegrand1 wrote:
> > I don't think we should change the expansion order for release_optimize, and
> > instead I have limited the change to only use the -O3 on ARM, where we
really
> > need it.
> > 
> > You can disregard my previous number of 2.77% increase in binary size. That
> > number was only for a webrtc test executable, and is not what we are
> interested
> > in here. I have trouble measuring the increase in binary size, and would be
> > happy for some advice. I built on Linux with -O3 for Opus and saw a size
> > increase for out/Release/chrome of 0.04%. Is that a correct way of
measuring?
> > 
> > https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp
> > File third_party/opus/opus.gyp (right):
> > 
> >
>
https://codereview.chromium.org/354903003/diff/1/third_party/opus/opus.gyp#ne...
> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> > I decided to skip using the release_optimize flag, and only use -O3 on ARM
for
> > now.
> 
> Is opus built automatically when building Chrome for Android? I can locally
try
> building with your patch to see the size change.

Hmm, I thought it was, but now I'm not so sure. Would be great if you could try
it, if you have time!
On Linux the binary size increases with 0.07% comparing a chromium build with
Opus build with -O3 compared to -Os. Same for my Chromium Android checkout.

[Nico, 2014-07-03 19:55:35]

On Thu, Jul 3, 2014 at 6:00 AM, <tlegrand@chromium.org> wrote:

> On 2014/07/02 17:33:05, aurimas wrote:
>
>> On 2014/07/02 17:15:16, tlegrand1 wrote:
>> > I don't think we should change the expansion order for
>> release_optimize, and
>> > instead I have limited the change to only use the -O3 on ARM, where we
>>
> really
>
>> > need it.
>> >
>> > You can disregard my previous number of 2.77% increase in binary size.
>> That
>> > number was only for a webrtc test executable, and is not what we are
>> interested
>> > in here. I have trouble measuring the increase in binary size, and
>> would be
>> > happy for some advice. I built on Linux with -O3 for Opus and saw a size
>> > increase for out/Release/chrome of 0.04%. Is that a correct way of
>>
> measuring?
>
>> >
>> > https://codereview.chromium.org/354903003/diff/1/third_
>> party/opus/opus.gyp
>> > File third_party/opus/opus.gyp (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/354903003/diff/1/third_
> party/opus/opus.gyp#newcode10
>
>> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
>> > I decided to skip using the release_optimize flag, and only use -O3 on
>> ARM
>>
> for
>
>> > now.
>>
>
>  Is opus built automatically when building Chrome for Android? I can
>> locally
>>
> try
>
>> building with your patch to see the size change.
>>
>
> Hmm, I thought it was, but now I'm not so sure. Would be great if you
> could try
> it, if you have time!
> On Linux the binary size increases with 0.07% comparing a chromium build
> with
> Opus build with -O3 compared to -Os. Same for my Chromium Android checkout.
>

(Please post absolute numbers, not percentages. "From X kB to Y kB, an
increase of z kB.". Linux uses -O2 by default for all files, not -Os,
Android uses -Os.)


>
>
> https://codereview.chromium.org/354903003/
>

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

[tlegrand1, 2014-07-04 14:12:19]

On 2014/07/03 19:55:35, Nico (away) wrote:
> On Thu, Jul 3, 2014 at 6:00 AM, <mailto:tlegrand@chromium.org> wrote:
> 
> > On 2014/07/02 17:33:05, aurimas wrote:
> >
> >> On 2014/07/02 17:15:16, tlegrand1 wrote:
> >> > I don't think we should change the expansion order for
> >> release_optimize, and
> >> > instead I have limited the change to only use the -O3 on ARM, where we
> >>
> > really
> >
> >> > need it.
> >> >
> >> > You can disregard my previous number of 2.77% increase in binary size.
> >> That
> >> > number was only for a webrtc test executable, and is not what we are
> >> interested
> >> > in here. I have trouble measuring the increase in binary size, and
> >> would be
> >> > happy for some advice. I built on Linux with -O3 for Opus and saw a size
> >> > increase for out/Release/chrome of 0.04%. Is that a correct way of
> >>
> > measuring?
> >
> >> >
> >> > https://codereview.chromium.org/354903003/diff/1/third_
> >> party/opus/opus.gyp
> >> > File third_party/opus/opus.gyp (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/354903003/diff/1/third_
> > party/opus/opus.gyp#newcode10
> >
> >> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> >> > I decided to skip using the release_optimize flag, and only use -O3 on
> >> ARM
> >>
> > for
> >
> >> > now.
> >>
> >
> >  Is opus built automatically when building Chrome for Android? I can
> >> locally
> >>
> > try
> >
> >> building with your patch to see the size change.
> >>
> >
> > Hmm, I thought it was, but now I'm not so sure. Would be great if you
> > could try
> > it, if you have time!
> > On Linux the binary size increases with 0.07% comparing a chromium build
> > with
> > Opus build with -O3 compared to -Os. Same for my Chromium Android checkout.
> >
> 
> (Please post absolute numbers, not percentages. "From X kB to Y kB, an
> increase of z kB.". Linux uses -O2 by default for all files, not -Os,
> Android uses -Os.)
> 

The Linux numbers are not important since this CL now only affects Android on
ARM.
I've tried to build Chrome on Android, but didn't manage. aurimas@ offered to
help.

How much is considered an acceptable increase? 

> 
> >
> >
> > https://codereview.chromium.org/354903003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[aurimas (slooooooooow), 2014-07-05 22:09:48]

On 2014/07/04 14:12:19, tlegrand1 wrote:
> On 2014/07/03 19:55:35, Nico (away) wrote:
> > On Thu, Jul 3, 2014 at 6:00 AM, <mailto:tlegrand@chromium.org> wrote:
> > 
> > > On 2014/07/02 17:33:05, aurimas wrote:
> > >
> > >> On 2014/07/02 17:15:16, tlegrand1 wrote:
> > >> > I don't think we should change the expansion order for
> > >> release_optimize, and
> > >> > instead I have limited the change to only use the -O3 on ARM, where we
> > >>
> > > really
> > >
> > >> > need it.
> > >> >
> > >> > You can disregard my previous number of 2.77% increase in binary size.
> > >> That
> > >> > number was only for a webrtc test executable, and is not what we are
> > >> interested
> > >> > in here. I have trouble measuring the increase in binary size, and
> > >> would be
> > >> > happy for some advice. I built on Linux with -O3 for Opus and saw a
size
> > >> > increase for out/Release/chrome of 0.04%. Is that a correct way of
> > >>
> > > measuring?
> > >
> > >> >
> > >> > https://codereview.chromium.org/354903003/diff/1/third_
> > >> party/opus/opus.gyp
> > >> > File third_party/opus/opus.gyp (right):
> > >> >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/354903003/diff/1/third_
> > > party/opus/opus.gyp#newcode10
> > >
> > >> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> > >> > I decided to skip using the release_optimize flag, and only use -O3 on
> > >> ARM
> > >>
> > > for
> > >
> > >> > now.
> > >>
> > >
> > >  Is opus built automatically when building Chrome for Android? I can
> > >> locally
> > >>
> > > try
> > >
> > >> building with your patch to see the size change.
> > >>
> > >
> > > Hmm, I thought it was, but now I'm not so sure. Would be great if you
> > > could try
> > > it, if you have time!
> > > On Linux the binary size increases with 0.07% comparing a chromium build
> > > with
> > > Opus build with -O3 compared to -Os. Same for my Chromium Android
checkout.
> > >
> > 
> > (Please post absolute numbers, not percentages. "From X kB to Y kB, an
> > increase of z kB.". Linux uses -O2 by default for all files, not -Os,
> > Android uses -Os.)
> > 
> 
> The Linux numbers are not important since this CL now only affects Android on
> ARM.
> I've tried to build Chrome on Android, but didn't manage. aurimas@ offered to
> help.
> 
> How much is considered an acceptable increase? 
> 
> > 
> > >
> > >
> > > https://codereview.chromium.org/354903003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

I did a local release build of chrome_apk target and the .so binary size went
from 34144514 to 34177282 bytes or 32768 byte increase. I will leave it to
yfriendman@ to decide what is a reasonable increase.

[tommi (sloooow) - chröme, 2014-07-05 22:21:18]

Thanks! :)  32kb is a lot less at least.  Tina is on vacation as of now so
if this is acceptable as is, I think we should land it via the cq.


On Sun, Jul 6, 2014 at 12:09 AM, <aurimas@chromium.org> wrote:

> On 2014/07/04 14:12:19, tlegrand1 wrote:
>
>> On 2014/07/03 19:55:35, Nico (away) wrote:
>> > On Thu, Jul 3, 2014 at 6:00 AM, <mailto:tlegrand@chromium.org> wrote:
>> >
>> > > On 2014/07/02 17:33:05, aurimas wrote:
>> > >
>> > >> On 2014/07/02 17:15:16, tlegrand1 wrote:
>> > >> > I don't think we should change the expansion order for
>> > >> release_optimize, and
>> > >> > instead I have limited the change to only use the -O3 on ARM,
>> where we
>> > >>
>> > > really
>> > >
>> > >> > need it.
>> > >> >
>> > >> > You can disregard my previous number of 2.77% increase in binary
>> size.
>> > >> That
>> > >> > number was only for a webrtc test executable, and is not what we
>> are
>> > >> interested
>> > >> > in here. I have trouble measuring the increase in binary size, and
>> > >> would be
>> > >> > happy for some advice. I built on Linux with -O3 for Opus and saw a
>>
> size
>
>> > >> > increase for out/Release/chrome of 0.04%. Is that a correct way of
>> > >>
>> > > measuring?
>> > >
>> > >> >
>> > >> > https://codereview.chromium.org/354903003/diff/1/third_
>> > >> party/opus/opus.gyp
>> > >> > File third_party/opus/opus.gyp (right):
>> > >> >
>> > >> >
>> > >>
>> > >
>> > > https://codereview.chromium.org/354903003/diff/1/third_
>> > > party/opus/opus.gyp#newcode10
>> > >
>> > >> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
>> > >> > I decided to skip using the release_optimize flag, and only use
>> -O3 on
>> > >> ARM
>> > >>
>> > > for
>> > >
>> > >> > now.
>> > >>
>> > >
>> > >  Is opus built automatically when building Chrome for Android? I can
>> > >> locally
>> > >>
>> > > try
>> > >
>> > >> building with your patch to see the size change.
>> > >>
>> > >
>> > > Hmm, I thought it was, but now I'm not so sure. Would be great if you
>> > > could try
>> > > it, if you have time!
>> > > On Linux the binary size increases with 0.07% comparing a chromium
>> build
>> > > with
>> > > Opus build with -O3 compared to -Os. Same for my Chromium Android
>>
> checkout.
>
>> > >
>> >
>> > (Please post absolute numbers, not percentages. "From X kB to Y kB, an
>> > increase of z kB.". Linux uses -O2 by default for all files, not -Os,
>> > Android uses -Os.)
>> >
>>
>
>  The Linux numbers are not important since this CL now only affects
>> Android on
>> ARM.
>> I've tried to build Chrome on Android, but didn't manage. aurimas@
>> offered to
>> help.
>>
>
>  How much is considered an acceptable increase?
>>
>
>  >
>> > >
>> > >
>> > > https://codereview.chromium.org/354903003/
>> > >
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> I did a local release build of chrome_apk target and the .so binary size
> went
> from 34144514 to 34177282 bytes or 32768 byte increase. I will leave it to
> yfriendman@ to decide what is a reasonable increase.
>
> https://codereview.chromium.org/354903003/
>

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

[Yaron, 2014-07-07 16:36:52]

On 2014/07/05 22:21:18, tommi wrote:
> Thanks! :)  32kb is a lot less at least.  Tina is on vacation as of now so
> if this is acceptable as is, I think we should land it via the cq.
> 
> 
> On Sun, Jul 6, 2014 at 12:09 AM, <mailto:aurimas@chromium.org> wrote:
> 
> > On 2014/07/04 14:12:19, tlegrand1 wrote:
> >
> >> On 2014/07/03 19:55:35, Nico (away) wrote:
> >> > On Thu, Jul 3, 2014 at 6:00 AM, <mailto:tlegrand@chromium.org> wrote:
> >> >
> >> > > On 2014/07/02 17:33:05, aurimas wrote:
> >> > >
> >> > >> On 2014/07/02 17:15:16, tlegrand1 wrote:
> >> > >> > I don't think we should change the expansion order for
> >> > >> release_optimize, and
> >> > >> > instead I have limited the change to only use the -O3 on ARM,
> >> where we
> >> > >>
> >> > > really
> >> > >
> >> > >> > need it.
> >> > >> >
> >> > >> > You can disregard my previous number of 2.77% increase in binary
> >> size.
> >> > >> That
> >> > >> > number was only for a webrtc test executable, and is not what we
> >> are
> >> > >> interested
> >> > >> > in here. I have trouble measuring the increase in binary size, and
> >> > >> would be
> >> > >> > happy for some advice. I built on Linux with -O3 for Opus and saw a
> >>
> > size
> >
> >> > >> > increase for out/Release/chrome of 0.04%. Is that a correct way of
> >> > >>
> >> > > measuring?
> >> > >
> >> > >> >
> >> > >> > https://codereview.chromium.org/354903003/diff/1/third_
> >> > >> party/opus/opus.gyp
> >> > >> > File third_party/opus/opus.gyp (right):
> >> > >> >
> >> > >> >
> >> > >>
> >> > >
> >> > > https://codereview.chromium.org/354903003/diff/1/third_
> >> > > party/opus/opus.gyp#newcode10
> >> > >
> >> > >> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
> >> > >> > I decided to skip using the release_optimize flag, and only use
> >> -O3 on
> >> > >> ARM
> >> > >>
> >> > > for
> >> > >
> >> > >> > now.
> >> > >>
> >> > >
> >> > >  Is opus built automatically when building Chrome for Android? I can
> >> > >> locally
> >> > >>
> >> > > try
> >> > >
> >> > >> building with your patch to see the size change.
> >> > >>
> >> > >
> >> > > Hmm, I thought it was, but now I'm not so sure. Would be great if you
> >> > > could try
> >> > > it, if you have time!
> >> > > On Linux the binary size increases with 0.07% comparing a chromium
> >> build
> >> > > with
> >> > > Opus build with -O3 compared to -Os. Same for my Chromium Android
> >>
> > checkout.
> >
> >> > >
> >> >
> >> > (Please post absolute numbers, not percentages. "From X kB to Y kB, an
> >> > increase of z kB.". Linux uses -O2 by default for all files, not -Os,
> >> > Android uses -Os.)
> >> >
> >>
> >
> >  The Linux numbers are not important since this CL now only affects
> >> Android on
> >> ARM.
> >> I've tried to build Chrome on Android, but didn't manage. aurimas@
> >> offered to
> >> help.
> >>
> >
> >  How much is considered an acceptable increase?
> >>
> >
> >  >
> >> > >
> >> > >
> >> > > https://codereview.chromium.org/354903003/
> >> > >
> >> >
> >> > To unsubscribe from this group and stop receiving emails from it, send
> >> an
> >> email
> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > I did a local release build of chrome_apk target and the .so binary size
> > went
> > from 34144514 to 34177282 bytes or 32768 byte increase. I will leave it to
> > yfriendman@ to decide what is a reasonable increase.
> >
> > https://codereview.chromium.org/354903003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

lgtm

[minyue, 2014-07-08 08:29:05]

The CQ bit was checked by minyue@chromium.org

[commit-bot: I haz the power, 2014-07-08 08:29:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tlegrand@chromium.org/354903003/20001

[henrika (OOO until Aug 14), 2014-07-08 11:34:47]

LGTM

[tommi (sloooow) - chröme, 2014-07-08 11:48:37]

The CQ bit was unchecked by tommi@chromium.org

[tommi (sloooow) - chröme, 2014-07-08 11:48:47]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-07-08 11:49:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tlegrand@chromium.org/354903003/20001

[commit-bot: I haz the power, 2014-07-08 12:20:34]

Change committed as 281729

[tlegrand1, 2014-07-09 08:49:04]

Thank you aurimas@ for helping out getting the numbers! Highly appreciated!



On Sun, Jul 6, 2014 at 12:09 AM, <aurimas@chromium.org> wrote:

> On 2014/07/04 14:12:19, tlegrand1 wrote:
>
>> On 2014/07/03 19:55:35, Nico (away) wrote:
>> > On Thu, Jul 3, 2014 at 6:00 AM, <mailto:tlegrand@chromium.org> wrote:
>> >
>> > > On 2014/07/02 17:33:05, aurimas wrote:
>> > >
>> > >> On 2014/07/02 17:15:16, tlegrand1 wrote:
>> > >> > I don't think we should change the expansion order for
>> > >> release_optimize, and
>> > >> > instead I have limited the change to only use the -O3 on ARM,
>> where we
>> > >>
>> > > really
>> > >
>> > >> > need it.
>> > >> >
>> > >> > You can disregard my previous number of 2.77% increase in binary
>> size.
>> > >> That
>> > >> > number was only for a webrtc test executable, and is not what we
>> are
>> > >> interested
>> > >> > in here. I have trouble measuring the increase in binary size, and
>> > >> would be
>> > >> > happy for some advice. I built on Linux with -O3 for Opus and saw a
>>
> size
>
>> > >> > increase for out/Release/chrome of 0.04%. Is that a correct way of
>> > >>
>> > > measuring?
>> > >
>> > >> >
>> > >> > https://codereview.chromium.org/354903003/diff/1/third_
>> > >> party/opus/opus.gyp
>> > >> > File third_party/opus/opus.gyp (right):
>> > >> >
>> > >> >
>> > >>
>> > >
>> > > https://codereview.chromium.org/354903003/diff/1/third_
>> > > party/opus/opus.gyp#newcode10
>> > >
>> > >> > third_party/opus/opus.gyp:10: 'release_optimize': '3',
>> > >> > I decided to skip using the release_optimize flag, and only use
>> -O3 on
>> > >> ARM
>> > >>
>> > > for
>> > >
>> > >> > now.
>> > >>
>> > >
>> > >  Is opus built automatically when building Chrome for Android? I can
>> > >> locally
>> > >>
>> > > try
>> > >
>> > >> building with your patch to see the size change.
>> > >>
>> > >
>> > > Hmm, I thought it was, but now I'm not so sure. Would be great if you
>> > > could try
>> > > it, if you have time!
>> > > On Linux the binary size increases with 0.07% comparing a chromium
>> build
>> > > with
>> > > Opus build with -O3 compared to -Os. Same for my Chromium Android
>>
> checkout.
>
>> > >
>> >
>> > (Please post absolute numbers, not percentages. "From X kB to Y kB, an
>> > increase of z kB.". Linux uses -O2 by default for all files, not -Os,
>> > Android uses -Os.)
>> >
>>
>
>  The Linux numbers are not important since this CL now only affects
>> Android on
>> ARM.
>> I've tried to build Chrome on Android, but didn't manage. aurimas@
>> offered to
>> help.
>>
>
>  How much is considered an acceptable increase?
>>
>
>  >
>> > >
>> > >
>> > > https://codereview.chromium.org/354903003/
>> > >
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> I did a local release build of chrome_apk target and the .so binary size
> went
> from 34144514 to 34177282 bytes or 32768 byte increase. I will leave it to
> yfriendman@ to decide what is a reasonable increase.
>
> https://codereview.chromium.org/354903003/
>

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