1868980 - 12.73% build times (Windows) regression on Tue November 28 2023

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Alex Finder|PTO, back on Nov 25th]

Perfherder has detected a build_metrics performance regression from push 3fd5d9d86b9347530ff25f74c39e4143a6dc96cb. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
13%	build times	windows2012-64	buildbot-unknown plain	1,460.62 -> 1,646.55

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 patch(es) may be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Alex Finder|PTO, back on Nov 25th]

(In reply to Alex Finder from comment #0)

Perfherder has detected a build_metrics performance regression from push 3fd5d9d86b9347530ff25f74c39e4143a6dc96cb. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
13%	build times	windows2012-64	buildbot-unknown plain	1,460.62 -> 1,646.55

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 patch(es) may be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

== Change summary for alert #40598 (as of Fri, 08 Dec 2023 12:20:03 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
57%	sccache requests_not_cacheable	windows-mingw32	32 clang debug	282.00 -> 443.00
56%	sccache requests_not_cacheable	android-5-0-x86_64		286.00 -> 447.00
54%	sccache requests_not_cacheable	linux64	base-toolchains	297.00 -> 458.00
18%	build times	windows2012-64	buildbot-unknown plain	1,404.97 -> 1,659.74
13%	build times	windows2012-64	buildbot-unknown plain	1,460.62 -> 1,646.55
10%	build times	linux64	base-toolchains-clang taskcluster-projects/970387039909/machineTypes/c2-standard-16	2,001.95 -> 2,201.93

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40598

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1866934

Comment 3

[Mike Hommey [:glandium]]

The increase in sccache requests_not_cacheable would certainly explain the build times regressions.

Comment 4

[Mike Hommey [:glandium]]

Actually, it's not the full story, because the builds from comment 0 are not cached. So we have two different problems at hand.

Comment 5

[Mike Hommey [:glandium]]

So we have two different problems at hand.

Actually, we don't. They're the same core problem: some rust crates are being rebuilt.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1868980 - Avoid base include flags spreading to the cc crate.

The cc crate was upgraded in bug 1866934, and one of the changes it
contains is to notify cargo to trigger rebuilds when environment
variables change[1].

As we invoke cargo from different directories, and as we pass the
computed compiler flags through CFLAGS_* variables in rust.mk, and
as those computed compiler flags contain -I flags for includes in
the current directory, each cargo invocation ends up with different
values of the CFLAGS_* variable, which, while it doesn't have a
consequence on the build itself, does now trigger a rebuild of crates
using cc, causing longer build times when building gtest.

Those -I flags, however, are not necessary for Rust, so we eliminate
them, working around the problem. The solution is fragile, but is the
simplest we can do short of doing much more intrusive changes.

1. https://github.com/rust-lang/cc-rs/commit/962af5387b8ea4551be23cf0e0236f42238f2af2

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d70e19bf702c
Avoid base include flags spreading to the cc crate. r=firefox-build-system-reviewers,ahochheiden

Comment 8

[Narcis Beleuzu [:NarcisB]]

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

Comment 9

[Alex Finder|PTO, back on Nov 25th]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d70e19bf702c
Avoid base include flags spreading to the cc crate.
r=firefox-build-system-reviewers,ahochheiden

== Change summary for alert #40659 (as of Wed, 13 Dec 2023 19:49:12 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
10%	build times	linux64	base-toolchains-clang taskcluster-projects/970387039909/machineTypes/c2-standard-16	2,174.66 -> 1,964.88

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=40659