1765746 - 10185.71 - 71.21% sccache cache_write_errors / compiler_metrics num_static_constructors + 2 more (Android, Linux) regression on Tue April 19 2022

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Andra Esanu (needinfo me)]

Perfherder has detected a build_metrics performance regression from push c7840f8abde3f03b60bd6db156336c4b6b6f5983. 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)
10186%	sccache cache_write_errors	linux64	asan opt	2.04 -> 210.00
1693%	sccache cache_write_errors	android-5-0-x86_64	isolated-process	12.00 -> 215.17
71%	compiler_metrics num_static_constructors	linux64	base-toolchains	323.00 -> 553.00
71%	compiler_metrics num_static_constructors	linux64	base-toolchains	323.00 -> 553.00

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

[Emilio Cobos Álvarez (:emilio)]

What does cache_write_errors measure?

The static constructors thing I can look into, I have a good idea of what's happening there.

Comment 2

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

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1765746 - Tweak contain bitflag definition order to avoid static constructors. r=dshin,#style,#layout-reviewers

This has no behavior change otherwise. The STRICT definition depended on
SIZE, which was defined later. That's fine in Rust, but in C++ it causes
the initialization to be dynamic because it doesn't have the definition
of SIZE yet (ugh).

This is the fix for the regression, though the following patch turns on
constexpr support in cbindgen, which would've caught this at build-time,
and guarantees that we don't have extra static constructors.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1765746 - Update cbindgen again. r=dshin,#style,#layout-reviewers

This basically improves and turns on constexpr support:

https://github.com/eqrion/cbindgen/pull/756

Also incorporates:

https://github.com/eqrion/cbindgen/pull/754

Though that has no real impact for our builds.

Depends on D144316

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8dc81ce9b1f
Tweak contain bitflag definition order to avoid static constructors. r=dshin
https://hg.mozilla.org/integration/autoland/rev/b6fd2cff0114
Update cbindgen again. r=dshin

Comment 6

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/d8dc81ce9b1f
https://hg.mozilla.org/mozilla-central/rev/b6fd2cff0114

Comment 7

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/perfherder/graphs?timerange=1209600&series=autoland,1689977,1,2 shows num_static_constructors regression is fixed.

The cache_write_errors seem to be back to baseline numbers, I suspect they weren't caused by my patch in particular, probably bad infra.

Comment 8

[Andra Esanu (needinfo me)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

What does cache_write_errors measure?

We encourage you to ask on #build channel on element. There's not much insight we can tell about this metric.