New issue
Advanced search Search tips

Issue 919430 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: ----



Sign in to add a comment

android_webview_unittests failing on chromium.memory/Android CFI

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Jan 7

Issue description

Timeouts on chromium.memory/Android CFI

Builders failed on: 
- Android CFI: 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI

The bot seems to be failing due to timing out.
It sometimes passes after something like 2h59m, while the purple builds are all 3 hours.
 
Description: Show this description
Components: Infra>Client>Chrome
Labels: Infra-Troopers OS-Android
Status: Untriaged (was: Available)
Infra: Could you advise on this issue? Maybe the timeout should simply be increased.
Components: Build
Oh, I just checked and it's the compile step that takes about 2h 30m. I wonder if that's WAI. It's not surprising that the tests doesn't finish in time in that case.
Description: Show this description
I think getting insight into why the compile is taking so long is important.

Build 3893 on December 3 (https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/3893) only took ~1.5 hours for the compile step. Besides arguments that vary per invocation, the only difference in the command and environment between Build 3893 and more recent builds is the addition of the vr_android_unittests target. It seems unlikely that that would be responsible for 30m to 90m of additional compile time.
The GN args haven't had any changes since Build 3893, so it won't be due to any interesting configuration in the args.gn file.
Scanning the builder history, it looks like the compile step jumped ~30 minutes longer from build #3894 to #3895:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/3894
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/3895
Cc: g...@chromium.org
Perhaps it is due to this CL: https://chromium-review.googlesource.com/c/1149061?
gbiv@ can you comment?
I'm temporarily increasing the timeout of the builder to hopefully get it green again, but we should track down why the compile times have changed and take appropriate action. I'll leave tracking down the compile time change to the sheriffs, the change identified by wolenetz@ seems like a reasonable suspect.
Cc: gbeaty@chromium.org
Labels: -Infra-Troopers
Owner: maxmorin@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 7

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

commit 803fdb50f5e28936655f96fa1ea07ea04af94f01
Author: Garrett Beaty <gbeaty@chromium.org>
Date: Mon Jan 07 20:46:26 2019

Temporarily increase the timeout for the Android CFI builder.

Bug: 919430
Change-Id: I5f6a5190330a2caae97d4d49bdc53a68a6150d12
Reviewed-on: https://chromium-review.googlesource.com/c/1399023
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Commit-Queue: Garrett Beaty <gbeaty@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620459}
[modify] https://crrev.com/803fdb50f5e28936655f96fa1ea07ea04af94f01/infra/config/global/cr-buildbucket.cfg

Cc: p...@chromium.org
+pcc, in case this looks fishy to him/in case he has a different opinion.

Unfortunately, ThinLTO optimizations do substantially increase link time, and ThinLTO isn't compatible with goma (yet, though there's a project that aims to make that happen). This increase in link time is exacerbated as we add more ThinLTO link steps, and we appear to be doing a fair few of those, given all of the unittests we're building.

ThinLTO has a cache that aims to speed it up, but if each new cycle has ~3h worth of commits anyway, it seems that the cache wouldn't be super helpful in keeping builds fast.

If we really need this time to go down, it may be an option to compile all unittests with --lto-O0, though it's unclear if we want to compile "release" tests with different optimization settings than a "release" Chrome binary.

Otherwise, until we teach goma and ThinLTO to play more nicely, I think our options are somewhat limited here. The only other mild hack I can think of is increasing the ThinLTO job limit locally for these builds, but if all N CPUs of these boxes are already saturated for the majority of this 2h30m, that'd likely only make it slower.
--lto-O0 seems like the right way to go for this bot. Its purpose is to prevent CFI errors from being introduced, not to produce fully optimized builds. Note that it is configured to produce non-official release builds, which already implies fewer optimizations than official builds. It is also intentionally configured in other ways that differ from production CFI builds (e.g. it enables CFI diagnostics and CFI cast errors). If it were possible, this bot wouldn't be using LTO at all.

The intent of https://chromium-review.googlesource.com/c/chromium/src/+/1149061 was to enable LTO optimizations in official Android builds. But it (unintentionally?) enabled them in non-official builds as well. So I think that all that's needed here is to add an "&& is_official_build" to the thin_lto_enable_optimizations default.
FYI the bot is now running its first build with the increased timeout (4.5 hours): https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/4199
Ah, I didn't realize this wasn't an official build. Great catch :)

https://chromium-review.googlesource.com/c/chromium/src/+/1399335
Since the new timeout (build 4199), the Android CFI builder no longer fails with "Infra failure". I think it's still a good idea to disable LTO for this bot using the solution in comment #13 and go back to the 3 hrs timeout.
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 8

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

commit eb7a50ee373ce8e94c003d3b7d8e18da7708210f
Author: François Doray <fdoray@chromium.org>
Date: Tue Jan 08 15:19:26 2019

[build] Enable LTO optimizations in official builds only.

LTO optimizations increase build time substantially. They are only
needed for official builds.

This CL will substantially reduce the time required to run the
Android CFI bot. See discussion on bug.

Bug: 919430
Change-Id: Id259dd290a3592d4a977f8d2a0eabe20c7029452
Reviewed-on: https://chromium-review.googlesource.com/c/1400355
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620728}
[modify] https://crrev.com/eb7a50ee373ce8e94c003d3b7d8e18da7708210f/build/config/compiler/BUILD.gn

FYI - #17 improved the compile time by about 1 hour (down to 1.5 hours from 2.3 hours):

before #17: 
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/4203

after #17: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/4204
I'm curious why https://chromium-review.googlesource.com/c/1400355 was submitted despite unresolved concerns about flipping the default for CrOS in https://chromium-review.googlesource.com/c/chromium/src/+/1399335 .

We do have external CrOS users who care about thinlto implying thinlto optimizations, and it'd be trivial to teach CrOS to always specify `thin_lto_enable_optimizations = is_chromeos`, but ISTM like a silly place to diverge.
(s/is_chromeos/true/)
Would it make sense to change the chromeos-chrome ebuild to always compile with is_official_build=true (and just have the is_chrome_branded=true part be conditional on chrome_internal)? Nothing enabled by is_official_build=true should depend on internal bits.
Cc: llozano@chromium.org
+llozano; I don't have enough context on the world of CrOS to say one way or another. :)
Labels: -Sheriff-Chromium
Removing bug from SoM radar.

Comment 24 by lloz...@google.com, Jan 16 (6 days ago)

re: #21,
I am not sure about this change. Sound interesting but I don't know all the consequences of that change.
I am not sure what people have been interpreting is_official_build as. I wish that knob had been named differently.
So, I would not recommend doing that change on the Chrome OS side. 

Comment 25 by g...@chromium.org, Jan 16 (6 days ago)

(To clarify, I think our current plan is to add a flag to configure Chrome to have is_official_build independent of is_chrome_branded. It won't be on by default for external users, but AFAIK neither is ThinLTO, so...

Will ping this bug with a patch once it's tested)

Comment 26 by g...@chromium.org, Jan 16 (6 days ago)

Clarifying the clarification: add a flag on the CrOS side*
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 18 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/44393853594ccad5649ffa61794f47d118b7bf43

commit 44393853594ccad5649ffa61794f47d118b7bf43
Author: George Burgess IV <gbiv@google.com>
Date: Fri Jan 18 12:01:31 2019

chromeos-chrome: always use is_official_build

According to the linked bug, Chrome's is_official_build flag should be
perfectly usable by external people. Since, as time moves on,
is_official_build is becoming a gate for enabling optimizations by
default (e.g. ThinLTO's default optimization level now takes
is_official_build into account), it's probably nice to expose this bit
to users who can't build with chrome_internal.

Per the discussion on the CL, there's no apparent reason that people
would want to disable this (simplechrome can always pass extra gn args
manually, and/or have logic for doing so itself). Hence, no USE flag is
provided.

BUG=chromium:919430
TEST=emerge-falco chromeos-chrome gave me is_official_build without
     is_chrome_branded. Chrome built successfully.
Change-Id: I1b478296ec1574608fca580a05bd0e7d1c2d7e93
Reviewed-on: https://chromium-review.googlesource.com/1415589
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Ilja H. Friedel <ihf@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/44393853594ccad5649ffa61794f47d118b7bf43/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

Sign in to add a comment