android_webview_unittests failing on chromium.memory/Android CFI |
|||||||||
Issue descriptionTimeouts 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.
,
Jan 7
Infra: Could you advise on this issue? Maybe the timeout should simply be increased.
,
Jan 7
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.
,
Jan 7
,
Jan 7
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.
,
Jan 7
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.
,
Jan 7
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
,
Jan 7
Perhaps it is due to this CL: https://chromium-review.googlesource.com/c/1149061? gbiv@ can you comment?
,
Jan 7
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.
,
Jan 7
,
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
,
Jan 7
+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.
,
Jan 7
--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.
,
Jan 8
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
,
Jan 8
Ah, I didn't realize this wasn't an official build. Great catch :) https://chromium-review.googlesource.com/c/chromium/src/+/1399335
,
Jan 8
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.
,
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
,
Jan 8
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
,
Jan 8
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.
,
Jan 8
(s/is_chromeos/true/)
,
Jan 8
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.
,
Jan 8
+llozano; I don't have enough context on the world of CrOS to say one way or another. :)
,
Jan 14
Removing bug from SoM radar.
,
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.
,
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)
,
Jan 16
(6 days ago)
Clarifying the clarification: add a flag on the CrOS side*
,
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 |
|||||||||
Comment 1 by maxmorin@chromium.org
, Jan 7