Add Link-Time Optimizations support for Android.

Add Link-Time Optimizations support for Android.

TBR=sergeyu, shess, cpu, jln
BUG=407544
Committed: https://crrev.com/d7a59f95af820ebe6c026ac6b3483199f79c08a8
Cr-Commit-Position: refs/heads/master@{#300872}

[Fabrice (no longer in Chrome), 2014-10-10 10:50:08]

fdegans@chromium.org changed reviewers:
+ henrika@chromium.org, jln@chromium.org, pasko@chromium.org,
piman@chromium.org, shess@chromium.org, thakis@chromium.org

[Fabrice (no longer in Chrome), 2014-10-10 10:50:08]

This is for a non-release configuration. Essentially, this CL add two new gyp
flags to conditionally enable LTO and disables LTO on targets identified as
causing link issues.

[henrika (OOO until Aug 14), 2014-10-10 10:57:00]

henrika@chromium.org changed reviewers:
+ tlegrand@chromium.org
- henrika@chromium.org

[henrika (OOO until Aug 14), 2014-10-10 10:57:42]

replacing henrika with tlegrand for opus parts

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

fdegans@chromium.org changed reviewers:
+ cpu@chromium.org, sievers@chromium.org, thestig@chromium.org

[Fabrice (no longer in Chrome), 2014-10-10 11:50:17]

Adding folks who aren't on leave, I think that should cover everything.

[pasko, 2014-10-10 11:59:21]

some thoughts about the hardest thing: naming. And also about communicating what
is being done.

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
File build/android/increase_size_for_speed.gypi (right):

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
build/android/increase_size_for_speed.gypi:21: # Do not merge -Os and -O2 in LTO
s/LTO/LTO./

Also you can briefly explain, like:
All objects that were compiled with LTO will be forced to have the same
optimization flags at link time with a stupid algorithm. Avoid using LTO
simultaneously on -Os and -O2 parts for that reason.

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
build/android/increase_size_for_speed.gypi:22: ['use_lto==1', {
maybe this should be called use_lto_os? I understand that there is code
committed to other repos that name this thing 'use_lto'. Maybe put an 'alias' in
common.gypi to have use_lto_os, and use_lto be aliases? Or maybe just an
explanation in the comment there for the start?

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

https://codereview.chromium.org/646873004/diff/1/build/common.gypi#newcode638
build/common.gypi:638: 'use_lto_o2%': 0,
it's worth explaining what use_lto_o2 is. Does use_lto work? In the issues
description please explain what the current state of the things are: which
targets build, which tests are known to pass/fail in each configuration.

https://codereview.chromium.org/646873004/diff/1/build/common.gypi#newcode2314
build/common.gypi:2314: # This reduces link time
nit:

s/This reduces link time/This reduces link time./

[no sievers, 2014-10-10 18:57:32]

https://codereview.chromium.org/646873004/diff/1/build/android/disable_lto.gypi
File build/android/disable_lto.gypi (right):

https://codereview.chromium.org/646873004/diff/1/build/android/disable_lto.gy...
build/android/disable_lto.gypi:8: 'target_conditions': [
should this also check for os == android?

https://codereview.chromium.org/646873004/diff/1/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/646873004/diff/1/content/content.gyp#newcode154
content/content.gyp:154: # crbug.com/TODO
replace TODO with bug number? :)

[Lei Zhang, 2014-10-10 19:17:41]

On 2014/10/10 11:50:17, Fabrice de Gans wrote:
> Adding folks who aren't on leave, I think that should cover everything.

You may want to be specific about who reviews what. I'm guessing chrome/ for me.

BTW, are there Android builds with GN? If so, you may want to update the
corresponding .gn[i] files.

[cpu_(ooo_6.6-7.5), 2014-10-10 19:53:56]

and for the sandbox I think you mean jln@

[tlegrand1, 2014-10-11 08:23:21]

I reviewed third_party/opus/opus.gyp, but I'm missing some context. What is LTO,
and what does the changes do?

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

https://codereview.chromium.org/646873004/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:109: 'opus_srcs_arm.gypi',
Not needed here?

https://codereview.chromium.org/646873004/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:120: 'opus_srcs_rtcd.gypi',
Or here?

[Fabrice (no longer in Chrome), 2014-10-13 15:45:11]

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

[Fabrice (no longer in Chrome), 2014-10-13 15:45:12]

Regarding gn, I am not doing it for now mainly because gn support for Android is
incomplete. Also because this is not supposed to be a release configuration. If
it ever becomes one or gn becomes the default, I will add the necessary changes.

Regarding who reviews what:
chrome/
  thakis@
  thestig@
content/
  piman@
  sievers@
gpu/
  piman@
  sievers@
sandbox/
  cpu@
  jln@
skia/
  thakis@
  tomhudson@
third_party/opus/
  tlegrand@
third_party/sqlite/
  shess@
You are of course all welcome to leave comments on the core of the changes
themselves, which are in build/

I think that covers everything.

https://codereview.chromium.org/646873004/diff/1/build/android/disable_lto.gypi
File build/android/disable_lto.gypi (right):

https://codereview.chromium.org/646873004/diff/1/build/android/disable_lto.gy...
build/android/disable_lto.gypi:8: 'target_conditions': [
On 2014/10/10 18:57:32, sievers wrote:
> should this also check for os == android?
use_lto should not be used with anything other than android for now. If some
other folks want to test it on other platforms, we'll discuss merging.

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
File build/android/increase_size_for_speed.gypi (right):

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
build/android/increase_size_for_speed.gypi:21: # Do not merge -Os and -O2 in LTO
On 2014/10/10 11:59:21, pasko wrote:
> s/LTO/LTO./
> 
> Also you can briefly explain, like:
> All objects that were compiled with LTO will be forced to have the same
> optimization flags at link time with a stupid algorithm. Avoid using LTO
> simultaneously on -Os and -O2 parts for that reason.

Done.

https://codereview.chromium.org/646873004/diff/1/build/android/increase_size_...
build/android/increase_size_for_speed.gypi:22: ['use_lto==1', {
On 2014/10/10 11:59:21, pasko wrote:
> maybe this should be called use_lto_os? I understand that there is code
> committed to other repos that name this thing 'use_lto'. Maybe put an 'alias'
in
> common.gypi to have use_lto_os, and use_lto be aliases? Or maybe just an
> explanation in the comment there for the start?

I think it's too late for that at this point. The bot uses use_lto, the other
repos use use_lto. I added a note in common.gypi explaining the difference
between the two.

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

https://codereview.chromium.org/646873004/diff/1/build/common.gypi#newcode638
build/common.gypi:638: 'use_lto_o2%': 0,
On 2014/10/10 11:59:21, pasko wrote:
> it's worth explaining what use_lto_o2 is. Does use_lto work? In the issues
> description please explain what the current state of the things are: which
> targets build, which tests are known to pass/fail in each configuration.

Done.

https://codereview.chromium.org/646873004/diff/1/build/common.gypi#newcode2314
build/common.gypi:2314: # This reduces link time
On 2014/10/10 11:59:21, pasko wrote:
> nit:
> 
> s/This reduces link time/This reduces link time./

Done.

https://codereview.chromium.org/646873004/diff/1/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/646873004/diff/1/content/content.gyp#newcode154
content/content.gyp:154: # crbug.com/TODO
On 2014/10/10 18:57:32, sievers wrote:
> replace TODO with bug number? :)
Argl, I missed that one!
Done.

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

https://codereview.chromium.org/646873004/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:109: 'opus_srcs_arm.gypi',
On 2014/10/11 08:23:21, tlegrand1 wrote:
> Not needed here?
No, LTO just is very brittle.
What happens is that if you throw too much data at it, LTO complains and
crashes, which is why some targets are disabled due to "ELF section name out of
range"

https://codereview.chromium.org/646873004/diff/1/third_party/opus/opus.gyp#ne...
third_party/opus/opus.gyp:120: 'opus_srcs_rtcd.gypi',
On 2014/10/11 08:23:21, tlegrand1 wrote:
> Or here?
Same, see above.

[tomhudson, 2014-10-13 16:41:11]

tomhudson@google.com changed reviewers:
+ tomhudson@google.com

[tomhudson, 2014-10-13 16:41:11]

Skia LGTM; nits in build/.

https://codereview.chromium.org/646873004/diff/300001/build/android/increase_...
File build/android/increase_size_for_speed.gypi (right):

https://codereview.chromium.org/646873004/diff/300001/build/android/increase_...
build/android/increase_size_for_speed.gypi:24: # on -Os and -O2 parts for that
reason.
Seems like this comment applies to the above condition, rather than the below
conditions?

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi#newco...
build/common.gypi:639: # Do not merge both options as that will result with the
whole
Not quite clear to me reading this what "merge both options" means (and you've
used the same phrasing elsewhere) - what am I not supposed to do?

Not *use* both options?
Refactor the gyp to combine the two options as one somehow?
Something else?

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi#newco...
build/common.gypi:2317: # Set component build with LTO until all tests pass.
"Until all tests pass" - does this mean they don't pass yet? Or are this comment
or condition block obsolete?

[no sievers, 2014-10-13 18:11:02]

lgtm. Would you mind updating the description with more details regarding the
net effect on the different components and depending on the configuration?

[Lei Zhang, 2014-10-13 19:04:49]

chrome/ lgtm

[tlegrand1, 2014-10-14 08:00:23]

third_party/opus/opus.gyp LGTM

I'm not an owner, though.

[Fabrice (no longer in Chrome), 2014-10-14 14:58:54]

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

[Fabrice (no longer in Chrome), 2014-10-14 14:58:56]

+sergeyu to confirm opus changes.

Thanks for the comments/reviews everyone. Fixed the wording in build/common.gypi
so it should hopefully be clearer.

https://codereview.chromium.org/646873004/diff/300001/build/android/increase_...
File build/android/increase_size_for_speed.gypi (right):

https://codereview.chromium.org/646873004/diff/300001/build/android/increase_...
build/android/increase_size_for_speed.gypi:24: # on -Os and -O2 parts for that
reason.
On 2014/10/13 16:41:11, tomhudson wrote:
> Seems like this comment applies to the above condition, rather than the below
> conditions?
No, it's to the below conditions. We remove the -flto flags for targets compiled
with -O2 if we're using the use_lto gyp flag.
On the other hand, we add the -flto flags if we're using the use_lto_o2 gyp
flag.

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi#newco...
build/common.gypi:639: # Do not merge both options as that will result with the
whole
On 2014/10/13 16:41:11, tomhudson wrote:
> Not quite clear to me reading this what "merge both options" means (and you've
> used the same phrasing elsewhere) - what am I not supposed to do?
> 
> Not *use* both options?
> Refactor the gyp to combine the two options as one somehow?
> Something else?

Do not use both options at the same time, I fail at typing.

https://codereview.chromium.org/646873004/diff/300001/build/common.gypi#newco...
build/common.gypi:2317: # Set component build with LTO until all tests pass.
Yes, there are many code generation and link bugs with LTO. We have a bot in the
works to compile the code and run tests which we are going to use to track
regressions and file bugs against GCC.

[pasko, 2014-10-20 14:12:20]

LGTM with given the comment/consistency nits are fixed.

Thanks!!

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi#newco...
build/common.gypi:635: # Link-Time Optimizations
I think it would be clearer if written like this:

# Variables to control Link-Time Optimizations (LTO).
# Note: the variables must *not* be enabled at the same time. In this case LTO
#       would 'merge' the optimization flags at link-time which would lead to
#       all code be optimized with -O2.
# Enable LTO on the code compiled with -Os.
# See crbug.com/407544
'use_lto%': 0,
# Enable LTO on code compiled with -O2.
'use_lto_o2%': 0,

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi#newco...
build/common.gypi:1192: 'use_lto_o2%': '<use_lto_o2',
parens for consistency please

[Fabrice (no longer in Chrome), 2014-10-22 14:10:42]

Thanks for the review and comments everyone!
TBR'ing the rest because these are mechanical changes.

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi#newco...
build/common.gypi:635: # Link-Time Optimizations
On 2014/10/20 14:12:20, pasko wrote:
> I think it would be clearer if written like this:
> 
> # Variables to control Link-Time Optimizations (LTO).
> # Note: the variables must *not* be enabled at the same time. In this case LTO
> #       would 'merge' the optimization flags at link-time which would lead to
> #       all code be optimized with -O2.
> # Enable LTO on the code compiled with -Os.
> # See crbug.com/407544
> 'use_lto%': 0,
> # Enable LTO on code compiled with -O2.
> 'use_lto_o2%': 0,

Done.

https://codereview.chromium.org/646873004/diff/430001/build/common.gypi#newco...
build/common.gypi:1192: 'use_lto_o2%': '<use_lto_o2',
On 2014/10/20 14:12:20, pasko wrote:
> parens for consistency please

Done.

[Fabrice (no longer in Chrome), 2014-10-22 14:10:50]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2014-10-22 14:11:30]

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

[commit-bot: I haz the power, 2014-10-22 15:02:13]

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

[commit-bot: I haz the power, 2014-10-22 15:02:15]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Fabrice (no longer in Chrome), 2014-10-23 10:54:07]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2014-10-23 10:55:08]

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

[commit-bot: I haz the power, 2014-10-23 11:50:05]

Committed patchset #4 (id:450001)

[commit-bot: I haz the power, 2014-10-23 11:50:45]

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