[Android] Keep -O2 gfx optimizations

[Android] Keep -O2 gfx optimizations

The optimizations had minimal impact on library size, and should be kept
permanently.

BUG=419051
Committed: https://crrev.com/1ce56f7c60db7b27e0d13cdd45aa2ed1e74dc7ec
Cr-Commit-Position: refs/heads/master@{#300572}

[jdduke (slow), 2014-10-10 19:44:40]

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

[jdduke (slow), 2014-10-10 19:44:40]

danakj@: PTAL, thanks.

[jdduke (slow), 2014-10-21 15:37:26]

danakj@: Any thoughts here?

[danakj, 2014-10-21 15:40:26]

On 2014/10/21 15:37:26, jdduke wrote:
> danakj@: Any thoughts here?

I recently changed the core gfx classes to allow more inlinong by removing
extern template class. I think the perf/size numbers around -O2 might change a
fair bit since a week ago or so. Do you think we should collect them again?

[jdduke (slow), 2014-10-21 19:32:20]

On 2014/10/21 15:40:26, danakj wrote:
> On 2014/10/21 15:37:26, jdduke wrote:
> > danakj@: Any thoughts here?
> 
> I recently changed the core gfx classes to allow more inlinong by removing
> extern template class. I think the perf/size numbers around -O2 might change a
> fair bit since a week ago or so. Do you think we should collect them again?

If the APK delta was more than just a few K, I would say yes. There are enough
hot paths here (in, say, transform.cc, and potentially in the future) that I
think it's worth having O2 stick as the default.

[danakj, 2014-10-21 19:45:54]

On 2014/10/21 19:32:20, jdduke wrote:
> On 2014/10/21 15:40:26, danakj wrote:
> > On 2014/10/21 15:37:26, jdduke wrote:
> > > danakj@: Any thoughts here?
> > 
> > I recently changed the core gfx classes to allow more inlinong by removing
> > extern template class. I think the perf/size numbers around -O2 might change
a
> > fair bit since a week ago or so. Do you think we should collect them again?
> 
> If the APK delta was more than just a few K, I would say yes. There are enough
> hot paths here (in, say, transform.cc, and potentially in the future) that I
> think it's worth having O2 stick as the default.

I thought the -O2 here didn't change perf. So is it still just a few k? The
number of places these could inline now may be a lot. My CLs to remove templates
each dropped a KB or so off the binary

[jdduke (slow), 2014-10-21 19:50:22]

On 2014/10/21 19:45:54, danakj wrote:
> On 2014/10/21 19:32:20, jdduke wrote:
> > On 2014/10/21 15:40:26, danakj wrote:
> > > On 2014/10/21 15:37:26, jdduke wrote:
> > > > danakj@: Any thoughts here?
> > > 
> > > I recently changed the core gfx classes to allow more inlinong by removing
> > > extern template class. I think the perf/size numbers around -O2 might
change
> a
> > > fair bit since a week ago or so. Do you think we should collect them
again?
> > 
> > If the APK delta was more than just a few K, I would say yes. There are
enough
> > hot paths here (in, say, transform.cc, and potentially in the future) that I
> > think it's worth having O2 stick as the default.
> 
> I thought the -O2 here didn't change perf. So is it still just a few k? The
> number of places these could inline now may be a lot. My CLs to remove
templates
> each dropped a KB or so off the binary

Ah, I can definitely collect the size numbers again.

[danakj, 2014-10-21 19:52:09]

On 2014/10/21 19:45:54, danakj wrote:
> On 2014/10/21 19:32:20, jdduke wrote:
> > On 2014/10/21 15:40:26, danakj wrote:
> > > On 2014/10/21 15:37:26, jdduke wrote:
> > > > danakj@: Any thoughts here?
> > > 
> > > I recently changed the core gfx classes to allow more inlinong by removing
> > > extern template class. I think the perf/size numbers around -O2 might
change
> a
> > > fair bit since a week ago or so. Do you think we should collect them
again?
> > 
> > If the APK delta was more than just a few K, I would say yes. There are
enough
> > hot paths here (in, say, transform.cc, and potentially in the future) that I
> > think it's worth having O2 stick as the default.
> 
> I thought the -O2 here didn't change perf. So is it still just a few k? The
> number of places these could inline now may be a lot. My CLs to remove
templates
> each dropped a KB or so off the binary

I just wanna know that were not doing this blindly after some significant
changes. We've been data driven about this so far.

[jdduke (slow), 2014-10-21 19:55:11]

On 2014/10/21 19:52:09, danakj wrote:
> On 2014/10/21 19:45:54, danakj wrote:
> > On 2014/10/21 19:32:20, jdduke wrote:
> > > On 2014/10/21 15:40:26, danakj wrote:
> > > > On 2014/10/21 15:37:26, jdduke wrote:
> > > > > danakj@: Any thoughts here?
> > > > 
> > > > I recently changed the core gfx classes to allow more inlinong by
removing
> > > > extern template class. I think the perf/size numbers around -O2 might
> change
> > a
> > > > fair bit since a week ago or so. Do you think we should collect them
> again?
> > > 
> > > If the APK delta was more than just a few K, I would say yes. There are
> enough
> > > hot paths here (in, say, transform.cc, and potentially in the future) that
I
> > > think it's worth having O2 stick as the default.
> > 
> > I thought the -O2 here didn't change perf. So is it still just a few k? The
> > number of places these could inline now may be a lot. My CLs to remove
> templates
> > each dropped a KB or so off the binary
> 
> I just wanna know that were not doing this blindly after some significant
> changes. We've been data driven about this so far.

Just to be clear, you want me to revert my original patch and make sure the
changes you landed afterward play nicely both with the reverted flags (Os) and
the existing flags (O2)?

[danakj, 2014-10-21 20:10:49]

On 2014/10/21 19:55:11, jdduke wrote:
> On 2014/10/21 19:52:09, danakj wrote:
> > On 2014/10/21 19:45:54, danakj wrote:
> > > On 2014/10/21 19:32:20, jdduke wrote:
> > > > On 2014/10/21 15:40:26, danakj wrote:
> > > > > On 2014/10/21 15:37:26, jdduke wrote:
> > > > > > danakj@: Any thoughts here?
> > > > > 
> > > > > I recently changed the core gfx classes to allow more inlinong by
> removing
> > > > > extern template class. I think the perf/size numbers around -O2 might
> > change
> > > a
> > > > > fair bit since a week ago or so. Do you think we should collect them
> > again?
> > > > 
> > > > If the APK delta was more than just a few K, I would say yes. There are
> > enough
> > > > hot paths here (in, say, transform.cc, and potentially in the future)
that
> I
> > > > think it's worth having O2 stick as the default.
> > > 
> > > I thought the -O2 here didn't change perf. So is it still just a few k?
The
> > > number of places these could inline now may be a lot. My CLs to remove
> > templates
> > > each dropped a KB or so off the binary
> > 
> > I just wanna know that were not doing this blindly after some significant
> > changes. We've been data driven about this so far.
> 
> Just to be clear, you want me to revert my original patch and make sure the
> changes you landed afterward play nicely both with the reverted flags (Os) and
> the existing flags (O2)?

Oh sorry. I misread :( codereviwwing from phone. I thot we are turning on -O2
here. But it stuck on head already then my testing was with it.

[danakj, 2014-10-21 20:11:13]

On 2014/10/21 20:10:49, danakj wrote:
> On 2014/10/21 19:55:11, jdduke wrote:
> > On 2014/10/21 19:52:09, danakj wrote:
> > > On 2014/10/21 19:45:54, danakj wrote:
> > > > On 2014/10/21 19:32:20, jdduke wrote:
> > > > > On 2014/10/21 15:40:26, danakj wrote:
> > > > > > On 2014/10/21 15:37:26, jdduke wrote:
> > > > > > > danakj@: Any thoughts here?
> > > > > > 
> > > > > > I recently changed the core gfx classes to allow more inlinong by
> > removing
> > > > > > extern template class. I think the perf/size numbers around -O2
might
> > > change
> > > > a
> > > > > > fair bit since a week ago or so. Do you think we should collect them
> > > again?
> > > > > 
> > > > > If the APK delta was more than just a few K, I would say yes. There
are
> > > enough
> > > > > hot paths here (in, say, transform.cc, and potentially in the future)
> that
> > I
> > > > > think it's worth having O2 stick as the default.
> > > > 
> > > > I thought the -O2 here didn't change perf. So is it still just a few k?
> The
> > > > number of places these could inline now may be a lot. My CLs to remove
> > > templates
> > > > each dropped a KB or so off the binary
> > > 
> > > I just wanna know that were not doing this blindly after some significant
> > > changes. We've been data driven about this so far.
> > 
> > Just to be clear, you want me to revert my original patch and make sure the
> > changes you landed afterward play nicely both with the reverted flags (Os)
and
> > the existing flags (O2)?
> 
> Oh sorry. I misread :( codereviwwing from phone. I thot we are turning on -O2
> here. But it stuck on head already then my testing was with it.

LGTM

[jdduke (slow), 2014-10-21 20:25:15]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-10-21 20:26:34]

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

[commit-bot: I haz the power, 2014-10-21 22:38:17]

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

[commit-bot: I haz the power, 2014-10-21 22:38:18]

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

[jdduke (slow), 2014-10-21 22:54:45]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-10-21 22:59:11]

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

[commit-bot: I haz the power, 2014-10-21 23:56:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-21 23:57:58]

Patchset 1 (id:??) landed as
https://crrev.com/1ce56f7c60db7b27e0d13cdd45aa2ed1e74dc7ec
Cr-Commit-Position: refs/heads/master@{#300572}