New issue
Advanced search Search tips

Issue 807147 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 11
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 883563



Sign in to add a comment

Evaluate ThinLTO optimizations for Chrome on Android

Project Member Reported by g...@chromium.org, Jan 30 2018

Issue description

Tracking bug for figuring out whether we want to turn ThinLTO optimizations on for Chrome on Android (and, if we do, for turning it on).

Accompanying doc: https://docs.google.com/document/d/1t6-RXNHe6LOeTgo941Ngv6dArELMY5kz3jSPxaIinPI/edit?usp=sharing

We do have some ThinLTO support already baked into the build system. This is primarily used to power CFI (and presumably similar security-ish features) in Android. The goal of this work is to make it also give us optimizations without bloating Chrome too much.
 

Comment 1 by g...@chromium.org, Feb 6 2018

FWIW, I'm working on this and issue 799629 together, since some of the workarounds I've found for issue 799629 (namely: "just disable AFDO for this project") stop working with ThinLTO on. I assume this is because we're using profiling data present on *callers* of e.g. Skia when trying to optimize Skia code. If the callers are all considered cold, then it's logical for the "unprofiled" Skia code to be considered cold, as well.

Stay tuned.

Comment 3 by g...@chromium.org, Apr 25 2018

Pinpoint runs will go here, to minimize noise on this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=836550
No pushback on the thread I started about ThinLTO yet. Fingers crossed...

CLs are here:
https://chromium-review.googlesource.com/c/chromium/src/+/1147867
https://chromium-review.googlesource.com/c/chromium/src/+/1148997
https://chromium-review.googlesource.com/c/chromium/src/+/1149061

The intent is to land these after we verify how much (if any) this hurts orderfile generation, and after the compiler roll happens (so we can turn thinlto back on for android/arm32).
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/708d7d6be05885487cb5ccdffeab08a09ab1ea60

commit 708d7d6be05885487cb5ccdffeab08a09ab1ea60
Author: George Burgess IV <gbiv@chromium.org>
Date: Thu Aug 23 00:40:55 2018

[zlib] Tweak optimizations to work better with ThinLTO

ThinLTO emits warnings when linking modules with different target
triples. Android uses armv7-a as its default triple, so the mixing with
armv8-a here is problematic.

Bug:  807147 
Test: md5sum <(objdump -d crc32_simd.o) is the same for Android.
Change-Id: If2bffc4090d3d94f158eea80d19ee9eb827fc837
Reviewed-on: https://chromium-review.googlesource.com/1147867
Commit-Queue: George Burgess <gbiv@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585338}
[modify] https://crrev.com/708d7d6be05885487cb5ccdffeab08a09ab1ea60/third_party/zlib/BUILD.gn
[modify] https://crrev.com/708d7d6be05885487cb5ccdffeab08a09ab1ea60/third_party/zlib/crc32_simd.c

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14039e39f2d10dd5380c6bd235e4093adcc33998

commit 14039e39f2d10dd5380c6bd235e4093adcc33998
Author: George Burgess IV <gbiv@chromium.org>
Date: Fri Aug 24 05:10:45 2018

[build] Allow users to toggle whether ThinLTO optimizations are on

This lets users turn ThinLTO optimizations on with a single gn arg,
and adds some special optimization settings for Android.

No functionality change is intended by this patch.

Bug:  807147 
Test: `ninja`
Change-Id: I7c2da21c1d5c2e779abce598ed775109453c8fa0
Reviewed-on: https://chromium-review.googlesource.com/1148997
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: George Burgess <gbiv@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585689}
[modify] https://crrev.com/14039e39f2d10dd5380c6bd235e4093adcc33998/build/config/compiler/BUILD.gn

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fb96ed606ab45f9346888d88475e94d30b20a49

commit 4fb96ed606ab45f9346888d88475e94d30b20a49
Author: Reid Kleckner <rnk@google.com>
Date: Sat Aug 25 00:44:01 2018

Move logic for ThinLTO optimization level into non-is_win block

Avoids linker errors from LLD about -Wl, not being a recognized command
line flag.  This will fix the Windows CFI and ThinLTO FYI bots.

Factor out a variable for the optimization level so we need one less
conditional.

TBR=agreive@chromium.org, agrieve@chromium.org, gbiv@chromium.org, pcc@chromium.org

Bug:  807147 
Change-Id: I8a928eaab93ec181ba057cdda4cb635389f30926
Reviewed-on: https://chromium-review.googlesource.com/1188879
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Reviewed-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586083}
[modify] https://crrev.com/4fb96ed606ab45f9346888d88475e94d30b20a49/build/config/compiler/BUILD.gn

Cc: bartfab@chromium.org
This broke Chrome compilation in a Chrome OS chroot. It fails with:

chromeos-chrome-70.0.3532.0_rc-r1: ERROR at //build/config/compiler/BUILD.gn:639:23: Assignment had no effect.
chromeos-chrome-70.0.3532.0_rc-r1:       lto_opt_level = 2
chromeos-chrome-70.0.3532.0_rc-r1:                       ^
chromeos-chrome-70.0.3532.0_rc-r1: You set the variable "lto_opt_level" here and it was unused before it went
chromeos-chrome-70.0.3532.0_rc-r1: out of scope.
chromeos-chrome-70.0.3532.0_rc-r1: See //build/config/BUILDCONFIG.gn:522:3: which caused the file to be included.
chromeos-chrome-70.0.3532.0_rc-r1:   "//build/config/compiler:afdo",
chromeos-chrome-70.0.3532.0_rc-r1:   ^-----------------------------
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b4f3135e8c1254b16cf74fb5a395ef0ac193a37

commit 1b4f3135e8c1254b16cf74fb5a395ef0ac193a37
Author: George Burgess IV <gbiv@chromium.org>
Date: Mon Aug 27 18:42:49 2018

Fix ThinLTO for CrOS

This fixes a bug in I8a928eaab93ec181ba057cdda4cb635389f30926, which
breaks ThinLTO builds for CrOS with an unused variable.

Bug:  807147 

TBR=agreive@chromium.org, rnk@chromium.org

Change-Id: I19b7cf81f035009d5390e9fc3f12f6e29413aa24
Reviewed-on: https://chromium-review.googlesource.com/1190904
Reviewed-by: George Burgess <gbiv@chromium.org>
Commit-Queue: George Burgess <gbiv@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586336}
[modify] https://crrev.com/1b4f3135e8c1254b16cf74fb5a395ef0ac193a37/build/config/compiler/BUILD.gn

bartfab - please let me know if it's still broken after r586336, and thanks again for the heads-up. :)
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1621d5e342adebc59a381928186b7847c70a7a1

commit e1621d5e342adebc59a381928186b7847c70a7a1
Author: George Burgess IV <gbiv@chromium.org>
Date: Tue Aug 28 00:32:58 2018

mb: Make Android CFI perf.fyi bots use ThinLTO optimizations

We intend to turn optimizations on by default for Android, but need to
gauge the additional build capacity necessary to support optimizations.
I'm told the most straightforward way to do this is to watch how long
these builders take after we turn opts on for them.

Bug:  807147 
Change-Id: Id608b35b7bfa758d0f202b8269c19cdc8945bcda
Reviewed-on: https://chromium-review.googlesource.com/1187741
Commit-Queue: George Burgess <gbiv@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586521}
[modify] https://crrev.com/e1621d5e342adebc59a381928186b7847c70a7a1/tools/mb/mb_config.pyl

Collecting numbers after ^ (50 builds before/50 builds after), it appears that the average `compile` overhead for *incremental* arm32 builds is on the order of 28.5%. This makes the total `steps` overhead on the order of 8%, so we'll say 10% slower incremental builds for arm32.

For arm64, we see a 72.8% (!) `compile` slowdown for incremental builds. Total `steps` overhead is 34.7%, which we can conservatively round to 35%-40%.

Though it's surprising that arm64 is so much slower than arm32, I'm willing to chalk that up to the aarch64 backend being much slower than arm32. I've heard rumors of this being the case, at least...

Concretely, the perf.fyi 'Android CFI arm64 Builder Perf FYI' bot has seen a mean `steps` time increase from 19m13s to 25m54s, and its arm32 counterpart has seen an increase from 21m32s to 23m18s.

Raw numbers are here: https://docs.google.com/spreadsheets/d/1qortGt6HOzhdiydddeP3SpS0gX6iBVzQX1ctS63erUE/edit?usp=sharing (please feel free to ping me if you can't access it and would like to.)
Cc: -bartfab@chromium.org
Blockedon: 883563
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/743db114a0b50b10a4df1a59aa69679b01690db3

commit 743db114a0b50b10a4df1a59aa69679b01690db3
Author: George Burgess IV <gbiv@chromium.org>
Date: Mon Dec 03 19:46:08 2018

[build] Turn ThinLTO optimizations on for Android

This CL turns on ThinLTO optimizations for Android by default for all
official builds.

This CL is expected to increase link times of official builds by
something around 2.5x CPU/2x wall time. More build capacity was requested
in http://crbug.com/883563.

This is also a pretty wide-reaching CL. A size decrease of a hundred
KB is expected, and we expect that the CPU-bound parts of Chrome will
generally be faster. If there are slowdowns, they should be very few in
number, and very small.

ThinLTO is a type of link-time optimization, which defers actual
code-generation and some optimization until link-time. This is very
beneficial to the optimizer, since it now has visibility into
functions/constants that, at a per-TU level, would be opaque. ThinLTO is
already deployed on Linux and Android, but our default Linux configuration
does *not* use optimizations. This CL specifically enables these
optimizations for Android.

      size/build time measurements.

Bug:  807147 
Test: Perf testing on nexus-5 (crbug.com/836550); manual binary
Change-Id: I7e07d9d9cb1dadd077adf44ca6217c40e13b00a8
Reviewed-on: https://chromium-review.googlesource.com/c/1149061
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: George Burgess <gbiv@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613198}
[modify] https://crrev.com/743db114a0b50b10a4df1a59aa69679b01690db3/build/config/compiler/BUILD.gn

And with that, this is officially landed.

I'm holding on to my party poppers for a few days, since the true test will be whether all of the perf bots are happy (both builders and runners, both functionality-wise and performance-wise).
Status: Fixed (was: Assigned)
🎉🎉🎉🎉

No complaints in 7 days, so I'm closing this out.

Thanks everyone for your help!

Sign in to add a comment