Compile specific parts of the Android build with -O2.

Compile specific parts of the Android build with -O2.

BUG=411909
Committed: https://crrev.com/75b3d0978299e1427b8ce49dae99eda071308c5a
Cr-Commit-Position: refs/heads/master@{#295306}

[Fabrice (no longer in Chrome), 2014-09-10 11:58:37]

fdegans@chromium.org changed reviewers:
+ brettw@chromium.org, pasko@chromium.org, skyostil@chromium.org,
thakis@chromium.org

[Fabrice (no longer in Chrome), 2014-09-10 11:58:37]

There is a report with detailed results incoming. The tl;dr version of it being
we can get +3-4% on high-level bench (thread_times, rasterize_time) with less
than 600 kiB binary increase on release official builds. This makes up almost
all the gains found in a full -O2 build.
I do not plan to land this until next week at the earliest to leave some time
for the perf bots to get data from the GCC 4.9 upgrade and to make sure nothing
broke.

Brett, I tried to edit the gn files to do the same thing, but I found no way to
get cflags -= [ "Os" ] to work anywhere in the BUILD.gn files in base/ cc/ or
skia/.

[Sami, 2014-09-10 12:44:56]

Thanks Fabrice, this is great stuff. Do you have an idea about the code size
contributions of each part here? Or should I just wait for the detailed report?
:)

https://codereview.chromium.org/555373004/diff/1/cc/blink/cc_blink.gyp
File cc/blink/cc_blink.gyp (right):

https://codereview.chromium.org/555373004/diff/1/cc/blink/cc_blink.gyp#newcode6
cc/blink/cc_blink.gyp:6: 'target_defaults': {
Any idea how big the perf improvements are in cc_blink? I'd be suprised if it
makes any difference because this is mostly just plumbing code between cc and
blink.

[Fabrice (no longer in Chrome), 2014-09-10 13:07:53]

On 2014/09/10 12:44:56, Sami wrote:
> Thanks Fabrice, this is great stuff. Do you have an idea about the code size
> contributions of each part here? Or should I just wait for the detailed
report?
> :)
Spoilers ahead.
Base: 128 kiB
CC: 144 kiB
Skia: 312 kiB
They combine pretty well, as the build with all 3 is 580 kiB larger.

[Fabrice (no longer in Chrome), 2014-09-10 14:51:12]

I removed 2 files from the CL, as they seem to have no effect.

https://codereview.chromium.org/555373004/diff/1/cc/blink/cc_blink.gyp
File cc/blink/cc_blink.gyp (right):

https://codereview.chromium.org/555373004/diff/1/cc/blink/cc_blink.gyp#newcode6
cc/blink/cc_blink.gyp:6: 'target_defaults': {
On 2014/09/10 12:44:56, Sami wrote:
> Any idea how big the perf improvements are in cc_blink? I'd be suprised if it
> makes any difference because this is mostly just plumbing code between cc and
> blink.

Done.
After testing it seems to have no effect and saves us 4 kiB.

[Nico, 2014-09-10 14:57:04]

Is there some page that lists which benchmarks we actually care about? We have a
ton of benchmark binaries, and apparently don't really care about the results of
several of them all that much.

[danakj, 2014-09-10 15:14:46]

On Wed, Sep 10, 2014 at 10:57 AM, <thakis@chromium.org> wrote:

> Is there some page that lists which benchmarks we actually care about? We
> have a
> ton of benchmark binaries, and apparently don't really care about the
> results of
> several of them all that much.
>

I think that we care about all of the telemetry perf tests. We also have
microbenchmarks (cc_perftests) but they are harder to get signal about for
changes that aren't directly working on the micro piece they test, and are
more for developers measuring their work than for regression catching.

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

[brettw, 2014-09-10 22:55:31]

This should do it for GN:

if (is_android && !is_debug) {
  configs -= [ "//build/config/compiler:optimize" ]
  configs += [ "//build/config/compiler:optimize_max" ]
}

The optimize max definition does -O2.

[Fabrice (no longer in Chrome), 2014-09-12 11:29:57]

On 2014/09/10 22:55:31, brettw wrote:
> This should do it for GN:
> 
> if (is_android && !is_debug) {
>   configs -= [ "//build/config/compiler:optimize" ]
>   configs += [ "//build/config/compiler:optimize_max" ]
> }
> 
> The optimize max definition does -O2.

Thanks, that should do it.
It's not 100% identical to gyp, mainly because some of the test targets in gyp
share their gyp files with the "main" one (like base).
FYI, with gyp, v8 is compiled with -O2, but not with gn.

[Nico, 2014-09-12 16:21:38]

On Wed, Sep 10, 2014 at 7:57 AM, <thakis@chromium.org> wrote:

> Is there some page that lists which benchmarks we actually care about? We
> have a
> ton of benchmark binaries, and apparently don't really care about the
> results of
> several of them all that much.
>

^^


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

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

[danakj, 2014-09-12 16:38:34]

On Fri, Sep 12, 2014 at 12:21 PM, Nico Weber <thakis@chromium.org> wrote:

> On Wed, Sep 10, 2014 at 7:57 AM, <thakis@chromium.org> wrote:
>
>> Is there some page that lists which benchmarks we actually care about? We
>> have a
>> ton of benchmark binaries, and apparently don't really care about the
>> results of
>> several of them all that much.
>>
>
> ^^
>

I tried to answer this but I guess I missed what benchmarks you're talking
about? Can you clarify?


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

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

[brettw, 2014-09-12 21:42:54]

On Fri, Sep 12, 2014 at 4:29 AM,  <fdegans@chromium.org> wrote:
> FYI, with gyp, v8 is compiled with -O2, but not with gn.

Thanks for noticing, I'll fix that.

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

[Fabrice (no longer in Chrome), 2014-09-15 17:22:27]

Friendly ping everyone.
Branch point is coming, I would like to push this by tomorrow if possible.

[Nico, 2014-09-15 17:23:11]

(I've repeatedly asked a question on this CL that's so far been ignored.)

On Mon, Sep 15, 2014 at 10:22 AM, <fdegans@chromium.org> wrote:

> Friendly ping everyone.
> Branch point is coming, I would like to push this by tomorrow if possible.
>
> https://codereview.chromium.org/555373004/
>

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

[chromium-reviews, 2014-09-15 17:23:53]

Dana's answered you twice.

On Mon, Sep 15, 2014 at 10:23 AM, Nico Weber <thakis@chromium.org> wrote:

> (I've repeatedly asked a question on this CL that's so far been ignored.)
>
> On Mon, Sep 15, 2014 at 10:22 AM, <fdegans@chromium.org> wrote:
>
>> Friendly ping everyone.
>> Branch point is coming, I would like to push this by tomorrow if possible.
>>
>> https://codereview.chromium.org/555373004/
>>
>
>

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

[Nico, 2014-09-15 17:26:42]

Not as far as I can see: http://imgur.com/spLRte7

On Mon, Sep 15, 2014 at 10:23 AM, James Robinson <jamesr@google.com> wrote:

> Dana's answered you twice.
>
> On Mon, Sep 15, 2014 at 10:23 AM, Nico Weber <thakis@chromium.org> wrote:
>
>> (I've repeatedly asked a question on this CL that's so far been ignored.)
>>
>> On Mon, Sep 15, 2014 at 10:22 AM, <fdegans@chromium.org> wrote:
>>
>>> Friendly ping everyone.
>>> Branch point is coming, I would like to push this by tomorrow if
>>> possible.
>>>
>>> https://codereview.chromium.org/555373004/
>>>
>>
>>
>

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

[danakj, 2014-09-15 17:27:39]

On 2014/09/15 17:26:42, Nico (hiding) wrote:
> Not as far as I can see: http://imgur.com/spLRte7

https://codereview.chromium.org/555373004/#msg7

idk :)

[chromium-reviews, 2014-09-15 17:29:17]

Hmm, Dana must be hellbanned.  Oh well!

On Mon, Sep 15, 2014 at 10:27 AM, <danakj@chromium.org> wrote:

> On 2014/09/15 17:26:42, Nico (hiding) wrote:
>
>> Not as far as I can see: http://imgur.com/spLRte7
>>
>
> https://codereview.chromium.org/555373004/#msg7
>
> idk :)
>
> https://codereview.chromium.org/555373004/
>

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

[Sami, 2014-09-15 17:32:35]

:D

In case you still want a list Nico, see here:
https://sites.google.com/a/google.com/silk/home/benchmarks

FWIW cc/ lgtm if James and Dana are happy.

[Fabrice (no longer in Chrome), 2014-09-15 17:56:51]

On 2014/09/15 17:23:11, Nico (hiding) wrote:
> (I've repeatedly asked a question on this CL that's so far been ignored.)

I'll answer part of it - though Dana did reply.
I targeted relatively high-level benchmarks like thread_times for
key_mobile_sites and rasterize_and_record_micro for key_silk_cases.
We do lack a page referencing what we "should" care about. I try to keep up with
what the various teams are interested in and that is not too noisy to track
regressions/improvements.

[Nico, 2014-09-15 18:14:13]

Thanks.

Is there any plan for making sure that these settings don't bitrot? What
makes sure that people randomly add this to their targets since they've
seen it on others and thought it looked good? (I'm not opposed to this
change, but I'd like you to think about this if you haven't.)

Code-wise, for the gyp part, maybe the repeated block could be in some gypi
file that's included in the targets instead? Maybe it could have a name
that's not too attractive to prevent cargo-culting it too much
(trade_bloat_for_perf.gypi or similar)

On Mon, Sep 15, 2014 at 10:56 AM, <fdegans@chromium.org> wrote:

> On 2014/09/15 17:23:11, Nico (hiding) wrote:
>
>> (I've repeatedly asked a question on this CL that's so far been ignored.)
>>
>
> I'll answer part of it - though Dana did reply.
> I targeted relatively high-level benchmarks like thread_times for
> key_mobile_sites and rasterize_and_record_micro for key_silk_cases.
> We do lack a page referencing what we "should" care about. I try to keep
> up with
> what the various teams are interested in and that is not too noisy to track
> regressions/improvements.
>
> https://codereview.chromium.org/555373004/
>

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

[Fabrice (no longer in Chrome), 2014-09-15 19:51:54]

On 2014/09/15 18:14:13, Nico (hiding) wrote:
> Thanks.
> 
> Is there any plan for making sure that these settings don't bitrot? What
> makes sure that people randomly add this to their targets since they've
> seen it on others and thought it looked good? (I'm not opposed to this
> change, but I'd like you to think about this if you haven't.)

Bitrot is a good point and was one of my main concerns. The rationale with this
change is that we are almost getting the improvements from a full -O2 build with
these modules. We are also talking about full modules, not specific targets or
files so I do not think people will "hoard" them for their new files since these
modules have a clearly defined role. On a related note I also tested compiling
v8 with -Os recently (it is compiled with -O2), and the improvements are still
there. Unless there is a dramatic change in our codebase or in the toolchain, it
shouldn't be a problem.
Regarding people randomly adding this to their targets, I obviously can't answer
for them or forbid them from doing this :) I had some requests to add Blink to
the mix but the trade-off simply isn't worth it, mainly because Blink is too big
and we do not high-level benchmarks showing enough of an improvement to justify
switching it to -O2 too. I hope I made it clear in the design doc that I only
picked the modules that showed significant improvements.
One last point, this is also a short-term improvement. We want (and are working
on) a more robust system with profile-guided optimizations. Once this is done, I
am hopeful we will be able to remove these module-specific flags, including in
v8.

> Code-wise, for the gyp part, maybe the repeated block could be in some gypi
> file that's included in the targets instead? Maybe it could have a name
> that's not too attractive to prevent cargo-culting it too much
> (trade_bloat_for_perf.gypi or similar)

We seem to already have some amount of -O2 here and there for other builds,
albeit per-target rather than per-module (except for v8). I am not against
having a specific generic gypi file for that case but it would be inconsistent
with the rest of the codebase.

[Nico, 2014-09-15 20:00:48]

On Mon, Sep 15, 2014 at 12:51 PM, <fdegans@chromium.org> wrote:

> On 2014/09/15 18:14:13, Nico (hiding) wrote:
>
>> Thanks.
>>
>
>  Is there any plan for making sure that these settings don't bitrot? What
>> makes sure that people randomly add this to their targets since they've
>> seen it on others and thought it looked good? (I'm not opposed to this
>> change, but I'd like you to think about this if you haven't.)
>>
>
> Bitrot is a good point and was one of my main concerns. The rationale with
> this
> change is that we are almost getting the improvements from a full -O2
> build with
> these modules. We are also talking about full modules, not specific
> targets or
> files so I do not think people will "hoard" them for their new files since
> these
> modules have a clearly defined role. On a related note I also tested
> compiling
> v8 with -Os recently (it is compiled with -O2), and the improvements are
> still
> there. Unless there is a dramatic change in our codebase or in the
> toolchain, it
> shouldn't be a problem.
>

Ok.


> Regarding people randomly adding this to their targets, I obviously can't
> answer
> for them or forbid them from doing this :)


(Random example of folks blindly copying things like this around:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/g...
)


> I had some requests to add Blink to
> the mix but the trade-off simply isn't worth it, mainly because Blink is
> too big
> and we do not high-level benchmarks showing enough of an improvement to
> justify
> switching it to -O2 too. I hope I made it clear in the design doc that I
> only
> picked the modules that showed significant improvements.
> One last point, this is also a short-term improvement. We want (and are
> working
> on) a more robust system with profile-guided optimizations. Once this is
> done, I
> am hopeful we will be able to remove these module-specific flags,
> including in
> v8.
>
>  Code-wise, for the gyp part, maybe the repeated block could be in some
>> gypi
>> file that's included in the targets instead? Maybe it could have a name
>> that's not too attractive to prevent cargo-culting it too much
>> (trade_bloat_for_perf.gypi or similar)
>>
>
> We seem to already have some amount of -O2 here and there for other builds,
> albeit per-target rather than per-module (except for v8). I am not against
> having a specific generic gypi file for that case but it would be
> inconsistent
> with the rest of the codebase.
>

From what I can tell, it's pretty rare, and it's not identical blocks in
many files either:
https://code.google.com/p/chromium/codesearch#search/&q=%5CbO2%20file:%5C.gyp...


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

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

[Fabrice (no longer in Chrome), 2014-09-16 11:10:08]

I think we can kill two birds with one stone.
I added specific gypi file for this scenario with a link to the bug and a short
message explaining it shouldn't be used.

[Fabrice (no longer in Chrome), 2014-09-16 11:11:40]

fdegans@chromium.org changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[pasko, 2014-09-16 11:41:34]

LGTM

looks even better to me with optimize_speed_template.gypi

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
File build/android/optimize_speed_template.gypi (right):

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
build/android/optimize_speed_template.gypi:8: # to be included everywhere. If
you want to use it for one target,
This procedure looks unusual to me. I would prefer reopening "bugs" only if they
were not well fixed originally. I would say:

"""
Before adding the template to another target, please ask in chromium-dev@. See
http://crbug.com/411909
"""

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
build/android/optimize_speed_template.gypi:20: 'cflags': ['-O2'],
stupid idea: can we list all "legitimate" uses of this template in PRESUBMIT?

https://codereview.chromium.org/555373004/diff/120001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/555373004/diff/120001/cc/BUILD.gn#newcode493
cc/BUILD.gn:493: if (!is_debug && (is_win || is_android)) {
should GN also have a discouraging comment like in optimize_speed_template.gypi?

[no sievers, 2014-09-16 17:35:13]

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp#newcode530
gpu/gpu.gyp:530: 'command_buffer_service.gypi',
I think the change above only affects the static but not component build, which
defines a shared 'gpu' library here with everything and would not use O2 anymore
after your change.

[Nico, 2014-09-16 17:42:39]

lgtm, but as I said I'd name optimize_speed_template.gypi less positively. If
someone sees a target that includes optimize_speed_template.gypi, it's very
likely they'll copy it to their new target ("Hey, speed is good!"). If it's
called "increase_size_for_speed" or similar, that's maybe less likely.

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp#newcode530
gpu/gpu.gyp:530: 'command_buffer_service.gypi',
On 2014/09/16 17:35:12, sievers wrote:
> I think the change above only affects the static but not component build,
which
> defines a shared 'gpu' library here with everything and would not use O2
anymore
> after your change.

Does that matter? The component build can't be used for perf testing anyhoo,
right?

[pasko, 2014-09-16 17:50:47]

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp#newcode530
gpu/gpu.gyp:530: 'command_buffer_service.gypi',
On 2014/09/16 17:42:39, Nico (hiding) wrote:
> On 2014/09/16 17:35:12, sievers wrote:
> > I think the change above only affects the static but not component build,
> which
> > defines a shared 'gpu' library here with everything and would not use O2
> anymore
> > after your change.
> 
> Does that matter? The component build can't be used for perf testing anyhoo,
> right?

+1, I would say it is WAI, since component_build == fastbuild, and -O2 makes the
build slower.

[no sievers, 2014-09-16 18:02:06]

On 2014/09/16 17:50:47, pasko wrote:
> https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp
> File gpu/gpu.gyp (right):
> 
> https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp#newcode530
> gpu/gpu.gyp:530: 'command_buffer_service.gypi',
> On 2014/09/16 17:42:39, Nico (hiding) wrote:
> > On 2014/09/16 17:35:12, sievers wrote:
> > > I think the change above only affects the static but not component build,
> > which
> > > defines a shared 'gpu' library here with everything and would not use O2
> > anymore
> > > after your change.
> > 
> > Does that matter? The component build can't be used for perf testing anyhoo,
> > right?
> 
> +1, I would say it is WAI, since component_build == fastbuild, and -O2 makes
the
> build slower.

Just seems like an unnecessary hidden problem later down the road that is being
introduced.
Why should 'shared_library' secretly imply that the source itself is compiled
with different optimization levels?

I don't feel strongly if it's hard to get it right this way, but is it?

[pasko, 2014-09-16 18:11:47]

On 2014/09/16 18:02:06, sievers wrote:
> > +1, I would say it is WAI, since component_build == fastbuild, and -O2 makes
> the
> > build slower.
> 
> Just seems like an unnecessary hidden problem later down the road that is
being
> introduced.
> Why should 'shared_library' secretly imply that the source itself is compiled
> with different optimization levels?
> 
> I don't feel strongly if it's hard to get it right this way, but is it?

I understand that there can be some non-performance surprises for the component
build growing from the fact that optimization options are different. Those could
be build breakages, some undefined behaviors working differently etc. We hope
that the effect of these "secret differences" would be small and rarely
observable.

As for performance implications of these changes, we should not consider them
for the component build because the performance characteristics of the component
build are expected to be vastly different from our release builds already, and
we are not going to change that.

What is the contract that we should be maintaining for the component build? I
thought that the component build is "easiest hack to make iterative development
faster". Am I missing something?

[no sievers, 2014-09-16 18:21:07]

On 2014/09/16 18:11:47, pasko wrote:
> On 2014/09/16 18:02:06, sievers wrote:
> > > +1, I would say it is WAI, since component_build == fastbuild, and -O2
makes
> > the
> > > build slower.
> > 
> > Just seems like an unnecessary hidden problem later down the road that is
> being
> > introduced.
> > Why should 'shared_library' secretly imply that the source itself is
compiled
> > with different optimization levels?
> > 
> > I don't feel strongly if it's hard to get it right this way, but is it?
> 
> I understand that there can be some non-performance surprises for the
component
> build growing from the fact that optimization options are different. Those
could
> be build breakages, some undefined behaviors working differently etc. We hope
> that the effect of these "secret differences" would be small and rarely
> observable.
> 
> As for performance implications of these changes, we should not consider them
> for the component build because the performance characteristics of the
component
> build are expected to be vastly different from our release builds already, and
> we are not going to change that.
> 
> What is the contract that we should be maintaining for the component build? I
> thought that the component build is "easiest hack to make iterative
development
> faster". Am I missing something?

Can we at least make it consistent so that the speed template either affects all
targets similarly in monolithic and component builds, or only affects the one?
What happens for the other targets with the patch?

[no sievers, 2014-09-16 18:26:19]

On 2014/09/16 18:21:07, sievers wrote:
> On 2014/09/16 18:11:47, pasko wrote:
> > On 2014/09/16 18:02:06, sievers wrote:
> > > > +1, I would say it is WAI, since component_build == fastbuild, and -O2
> makes
> > > the
> > > > build slower.
> > > 
> > > Just seems like an unnecessary hidden problem later down the road that is
> > being
> > > introduced.
> > > Why should 'shared_library' secretly imply that the source itself is
> compiled
> > > with different optimization levels?
> > > 
> > > I don't feel strongly if it's hard to get it right this way, but is it?
> > 
> > I understand that there can be some non-performance surprises for the
> component
> > build growing from the fact that optimization options are different. Those
> could
> > be build breakages, some undefined behaviors working differently etc. We
hope
> > that the effect of these "secret differences" would be small and rarely
> > observable.
> > 
> > As for performance implications of these changes, we should not consider
them
> > for the component build because the performance characteristics of the
> component
> > build are expected to be vastly different from our release builds already,
and
> > we are not going to change that.
> > 
> > What is the contract that we should be maintaining for the component build?
I
> > thought that the component build is "easiest hack to make iterative
> development
> > faster". Am I missing something?
> 
> Can we at least make it consistent so that the speed template either affects
all
> targets similarly in monolithic and component builds, or only affects the one?
> What happens for the other targets with the patch?

and what about the gpu GN build?

[Fabrice (no longer in Chrome), 2014-09-16 19:17:18]

Alright, so I changed the test in the gypi file to not set -O2 when
fastbuild==1.
However, you can still get a component build with similar -O2 settings than the
shipping build by setting component="shared_library". Thus, I added the include
in the gpu shared_library target for consistency.
Regarding gn, it appears it does not seem to support fastbuild yet, so it's -O2
in both cases.
I also renamed the gypi file so it looks scarier and rephrased the "do not use
me" comment.

Does that meet with everyone's approval?

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
File build/android/optimize_speed_template.gypi (right):

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
build/android/optimize_speed_template.gypi:8: # to be included everywhere. If
you want to use it for one target,
On 2014/09/16 11:41:33, pasko wrote:
> This procedure looks unusual to me. I would prefer reopening "bugs" only if
they
> were not well fixed originally. I would say:
> 
> """
> Before adding the template to another target, please ask in chromium-dev@. See
> http://crbug.com/411909
> """

Done.

https://codereview.chromium.org/555373004/diff/120001/build/android/optimize_...
build/android/optimize_speed_template.gypi:20: 'cflags': ['-O2'],
On 2014/09/16 11:41:33, pasko wrote:
> stupid idea: can we list all "legitimate" uses of this template in PRESUBMIT?

I had a quick look but this seems overly complicated.

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/555373004/diff/120001/gpu/gpu.gyp#newcode530
gpu/gpu.gyp:530: 'command_buffer_service.gypi',
On 2014/09/16 17:50:47, pasko wrote:
> On 2014/09/16 17:42:39, Nico (hiding) wrote:
> > On 2014/09/16 17:35:12, sievers wrote:
> > > I think the change above only affects the static but not component build,
> > which
> > > defines a shared 'gpu' library here with everything and would not use O2
> > anymore
> > > after your change.
> > 
> > Does that matter? The component build can't be used for perf testing anyhoo,
> > right?
> 
> +1, I would say it is WAI, since component_build == fastbuild, and -O2 makes
the
> build slower.

Done.

[no sievers, 2014-09-16 19:25:57]

lgtm. thanks! gpu/command_buffer/service/BUILD.gn already does the right thing,
correct?

[Fabrice (no longer in Chrome), 2014-09-16 19:29:18]

On 2014/09/16 19:25:57, sievers wrote:
> lgtm. thanks! gpu/command_buffer/service/BUILD.gn already does the right
thing,
> correct?
Yes I checked, except it's always -O2 because there is no equivalent to
fastbuild.

[brettw, 2014-09-16 19:35:52]

lgtm

https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn#newcode493
cc/BUILD.gn:493: if (!is_debug && (is_win || is_android)) {
Can you think of a reason why Mac & Linux shouldn't have this optimized if
Windows and Android do? I'd be inclined to always do this.

[Fabrice (no longer in Chrome), 2014-09-16 20:05:59]

https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn#newcode493
cc/BUILD.gn:493: if (!is_debug && (is_win || is_android)) {
On 2014/09/16 19:35:51, brettw wrote:
> Can you think of a reason why Mac & Linux shouldn't have this optimized if
> Windows and Android do? I'd be inclined to always do this.
That is a good point, I'm going to bring it up on chromium-dev. However, this is
beyond the scope of this CL.

[brettw, 2014-09-16 20:34:17]

On 2014/09/16 20:05:59, Fabrice de Gans wrote:
> https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
> https://codereview.chromium.org/555373004/diff/140001/cc/BUILD.gn#newcode493
> cc/BUILD.gn:493: if (!is_debug && (is_win || is_android)) {
> On 2014/09/16 19:35:51, brettw wrote:
> > Can you think of a reason why Mac & Linux shouldn't have this optimized if
> > Windows and Android do? I'd be inclined to always do this.
> That is a good point, I'm going to bring it up on chromium-dev. However, this
is
> beyond the scope of this CL.

ok

[Fabrice (no longer in Chrome), 2014-09-17 17:24:39]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2014-09-17 17:25:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555373004/140001

[commit-bot: I haz the power, 2014-09-17 18:36:03]

Committed patchset #8 (id:140001) as 1288aac95a27680d4fe5f2dbff5222806e32bfc5

[commit-bot: I haz the power, 2014-09-17 18:36:39]

Patchset 8 (id:??) landed as
https://crrev.com/75b3d0978299e1427b8ce49dae99eda071308c5a
Cr-Commit-Position: refs/heads/master@{#295306}