1654465 - 7.57 - 9.16% build times (linux64-shippable, osx-shippable) regression on pushrange 034864174a - bb79da633a (Mon July 20 2020)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Perfherder has detected a build_metrics performance regression from push range 034864174a6f8d7f7042e0e4b42abf8201e4c1c8 - bb79da633a2efe12f7a0a6e89d925961b8b444a7. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

9% build times linux64-shippable opt nightly taskcluster-c5d.4xlarge 2,725.86 -> 2,975.56
9% build times osx-shippable opt nightly taskcluster-c5d.4xlarge 2,716.07 -> 2,958.21
8% build times osx-shippable opt nightly taskcluster-m5.4xlarge 2,877.22 -> 3,094.93

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Nathan Froyd [:froydnj]]

At least on:

https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&series=autoland,1922228,1,2&timerange=1209600&zoom=1595243980108,1595325648931,2248.1108341503304,3468.9556517868855

and

https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&selected=1930882,1190654374&series=autoland,1930882,1,2&timerange=1209600&zoom=1595265331434,1595425670054,2364.934281813436,3603.163981766394

It looks like this is probably bug 1640982.

I'm not sure where the regression range from comment 1 comes from.

Comment 2

[Mike Hommey [:glandium]]

It's due to -C lto now being passed to rustc for at least http3server, which was not the case before.

Comment 3

[Mike Hommey [:glandium]]

And other more annoying things that also lead to bug 1654458

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

It turns out setting CARGO_PROFILE_RELEASE_LTO has unwanted side
effects.

First it's not actually strictly equivalent to using cargo rustc -- -Clto. For instance, it apparently also enables cross-language LTO in
newer versions of cargo.

Second, it changes the rust computed hash for all the dependencies of
the crate being built with the variable set, which makes them diverge
from when the same dependencies are built through another crate in the
tree that is not LTOed. This effectively makes us build a lot of
crates twice, many of which are not cacheable.

Since the original problem is that cargo >= 1.45 passes extra flags (-C embed-bitcode=no) to rustc that are incompatible with -Clto, and while
it knows to adjust based on the lto setting in the build profile
(which CARGO_PROFILE_RELEASE_LTO overrides the default of), cargo
ignores flags passed via cargo rustc -- ... when making those
adjustments.

So, we need to override with -C embed-bitcode=yes on our own at the
same time we pass -Clto. But doing that through cargo rustc -- ...
is not enough because all the dependencies of the crate built with
-Clto need to be built with -C embed-bitcode=yes. So we need to
override with RUSTFLAGS, which will affect all the dependencies.
But we also need to do this consistently across all crates, not only the
dependencies of crates built with -Clto, otherwise we'd still end up
building crates twice (once with and once without the override).

Unfortunately, the -C embed-bitcode=* flag is also not supported in
versions older than 1.45, so we have to avoid adding it on older
versions.

We unfortunately support a large range of versions of rustc (albeit only
for tools/crashreporter), but we actually need to upgrade the smaller
supported version because rustc < 1.38 doesn't support our top-level
Cargo.lock. This makes the version check slightly less awful.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5d2a6d5187b
Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO. r=firefox-build-system-reviewers,rstewart

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e5d2a6d5187b

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

Beta/Release Uplift Approval Request

• User impact if declined: Slower builds after bug 1640982
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch reverts a "small" side effect of bug 1640982 that increased build times, and adds something that has no effect on mozilla CI builds because it only applies on newer versions of the rust compiler we're not using yet.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See Beta/Release uplift approval request
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

approved for 80.0b2

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5049f067f189

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #26621 (as of Wed, 29 Jul 2020 17:10:55 GMT) ==

Improvements:

18% build times linux64 debug base-toolchains-clang taskcluster-m5.4xlarge 1,959.48 -> 1,598.27
13% build times linux64-shippable opt nightly taskcluster-c5.4xlarge 2,995.51 -> 2,613.45
12% build times linux64-shippable opt nightly taskcluster-m5.4xlarge 3,021.78 -> 2,644.77
12% build times osx-shippable opt nightly taskcluster-m5.4xlarge 2,995.90 -> 2,630.89
11% build times linux64-shippable opt nightly taskcluster-c5d.4xlarge 2,803.46 -> 2,483.17
11% build times osx-shippable opt nightly taskcluster-c5.4xlarge 2,972.54 -> 2,639.70
11% build times osx-shippable opt nightly taskcluster-c5d.4xlarge 2,814.39 -> 2,513.19
9% build times linux64 debug base-toolchains-clang taskcluster-c5d.4xlarge 1,625.79 -> 1,481.19

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26621

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

Approved for 78.2esr and 68.12esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

This needs a rebased patch for ESR68.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/9181214a2308

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

The backport for esr68 requires more version exclusions.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

This needs a rebased patch for ESR68.

https://phabricator.services.mozilla.com/D85788 is the version for ESR68.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9167748 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

Carrying over previous esr68 approval.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/6b2d278ed3f0

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 9165606 [details]
Bug 1654465 - Set -Cembed-bitcode=yes instead of CARGO_PROFILE_RELEASE_LTO.

bug 1640982 wasn't uplifted to 79