[Android] Enable -O2 for gfx

[Android] Enable -O2 for gfx

There are a number of key, hot routines in gfx and gfx_geometry. Enable
-O2 for these targets.

BUG=411909, 419051
Committed: https://crrev.com/6e33e2785b76772378a4f8ed33ad782b3f266aee
Cr-Commit-Position: refs/heads/master@{#297475}

[jdduke (slow), 2014-09-26 19:57:35]

jdduke@chromium.org changed reviewers:
+ fdegans@chromium.org

[jdduke (slow), 2014-09-26 19:57:35]

fdegans@: Is there anything else required here? I haven't actually checked the
impact on size, though I suspect it's minimal as both targets are relatively
lightweight.

[Fabrice (no longer in Chrome), 2014-09-30 13:06:24]

fdegans@chromium.org changed reviewers:
+ brettw@chromium.org

[Fabrice (no longer in Chrome), 2014-09-30 13:06:25]

I applied it locally, the binary size increase is exactly 12 kiB.
However, I am a bit reluctant to approve it without having a clear view of the
performance implications. What benchmarks can I run locally to test performance
from Chrome.apk?

+brettw for gn review but it looks fine to me.

[jdduke (slow), 2014-09-30 15:01:42]

On 2014/09/30 13:06:25, Fabrice de Gans wrote:
> I applied it locally, the binary size increase is exactly 12 kiB.
> However, I am a bit reluctant to approve it without having a clear view of the
> performance implications. What benchmarks can I run locally to test
performance
> from Chrome.apk?

The benefit (if any) should appear in any compositor-heavy benchmarks, e.g.,
cc_perftests or thread_times.key_mobile_sites
(thread_renderer_compositor_cpu_time_per_frame).

[brettw, 2014-09-30 16:08:04]

On 2014/09/30 15:01:42, jdduke wrote:
> On 2014/09/30 13:06:25, Fabrice de Gans wrote:
> > I applied it locally, the binary size increase is exactly 12 kiB.
> > However, I am a bit reluctant to approve it without having a clear view of
the
> > performance implications. What benchmarks can I run locally to test
> performance
> > from Chrome.apk?
> 
> The benefit (if any) should appear in any compositor-heavy benchmarks, e.g.,
> cc_perftests or thread_times.key_mobile_sites
> (thread_renderer_compositor_cpu_time_per_frame).

So we're not clear on the benefit?

[jdduke (slow), 2014-09-30 16:12:20]

On 2014/09/30 16:08:04, brettw wrote:
> On 2014/09/30 15:01:42, jdduke wrote:
> > On 2014/09/30 13:06:25, Fabrice de Gans wrote:
> > > I applied it locally, the binary size increase is exactly 12 kiB.
> > > However, I am a bit reluctant to approve it without having a clear view of
> the
> > > performance implications. What benchmarks can I run locally to test
> > performance
> > > from Chrome.apk?
> > 
> > The benefit (if any) should appear in any compositor-heavy benchmarks, e.g.,
> > cc_perftests or thread_times.key_mobile_sites
> > (thread_renderer_compositor_cpu_time_per_frame).
> 
> So we're not clear on the benefit?

Local results using my GN/N4 are somewhat noisy, but I see several percentage
point+ gains in a number of the  compositor perf tests. With just a 12KB size
gain, I think it's worth landing, and using existing perf infrastructure to
further validate the gains.

[brettw, 2014-09-30 17:13:33]

Mechanically, the build files LGTM. I think it would be a good to file a bug on
evaluating the results from this, and link that bug from the places where you do
the settings so it's clear why. Then hopefully we don't forget to follow-up. If
the benefit is like 1% or something else very low, we should probably just
revert it.

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

I do not consider low-level benchmarks like cc_perftests for Partial -O2
candidates.
Local tests do not seem to show a large difference on high-level benchmarks like
thread_times.key_mobile_sites and thread_times.tough_compositor_cases. In both
cases, the improvements are under the noise margin and not 100% consistent.
We could still run an experiment and look at the results on the perf dashboard
and roll back in case there are no improvements. But I do not feel confident
enabling it definitely right now. Add a comment and a link to crbug.com/411909
to make it clear this is temporary for now. We should have results on the
dashboard within a week - Android perf bots are slow to run.
Does that sound good to you?

[no sievers, 2014-09-30 17:35:37]

On 2014/09/30 17:15:20, Fabrice de Gans wrote:
> I do not consider low-level benchmarks like cc_perftests for Partial -O2
> candidates.
> Local tests do not seem to show a large difference on high-level benchmarks
like
> thread_times.key_mobile_sites and thread_times.tough_compositor_cases. In both
> cases, the improvements are under the noise margin and not 100% consistent.
> We could still run an experiment and look at the results on the perf dashboard
> and roll back in case there are no improvements. But I do not feel confident
> enabling it definitely right now. Add a comment and a link to crbug.com/411909
> to make it clear this is temporary for now. We should have results on the
> dashboard within a week - Android perf bots are slow to run.
> Does that sound good to you?

Why are we so reluctant to change our attitude more towards "-O2" by default and
optimize-for-size only for components that don't contain code that is heavily
exercised and/or blow up the binary size by too much?

We know that this change does not increase the binary size by any amount worth
talking about, that the component is generally very small, and the classes are
used all over the performance-critical code. It just seems silly that we leave
room for the compiler to do something that could hurt performance.

[jdduke (slow), 2014-09-30 18:07:40]

On 2014/09/30 17:13:33, brettw wrote:
> Mechanically, the build files LGTM. I think it would be a good to file a bug
on
> evaluating the results from this, and link that bug from the places where you
do
> the settings so it's clear why. Then hopefully we don't forget to follow-up.
If
> the benefit is like 1% or something else very low, we should probably just
> revert it.

Done.

[jdduke (slow), 2014-09-30 18:08:13]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:10:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/612513003/40001

[commit-bot: I haz the power, 2014-09-30 18:22:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:22:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jdduke (slow), 2014-09-30 18:37:49]

jdduke@chromium.org changed reviewers:
+ danakj@chromium.org

[jdduke (slow), 2014-09-30 18:37:49]

danakj@: PTAL, looks like we need an OWNER for the .gn file updates (not sure
why?).

[danakj, 2014-09-30 19:05:30]

LGTm

[Fabrice (no longer in Chrome), 2014-09-30 19:09:37]

lgtm thanks!

[jdduke (slow), 2014-09-30 19:09:40]

The CQ bit was checked by jdduke@chromium.org

[Fabrice (no longer in Chrome), 2014-09-30 19:09:46]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/612513003/40001

[commit-bot: I haz the power, 2014-09-30 19:17:51]

Committed patchset #3 (id:40001) as e1d2e620c72e14af972cfa41cdf340d7dd0d4003

[commit-bot: I haz the power, 2014-09-30 19:18:40]

Patchset 3 (id:??) landed as
https://crrev.com/6e33e2785b76772378a4f8ed33ad782b3f266aee
Cr-Commit-Position: refs/heads/master@{#297475}