1777275 - 23.64 - 2.02% compiler_metrics num_static_constructors / compiler_metrics num_static_constructors + 11 more (Linux, Windows) regression on Tue June 28 2022

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Andra Esanu (needinfo me)]

Perfherder has detected a build_metrics performance regression from push 03f913ac3bae811ec702287cb1bef7bd6aa87908. 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)
24%	compiler_metrics num_static_constructors	windows2012-aarch64	aarch64-no-eme	110.00 -> 136.00
23%	compiler_metrics num_static_constructors	windows2012-64-shippable		112.00 -> 138.00
23%	compiler_metrics num_static_constructors	windows2012-aarch64	aarch64	114.00 -> 140.00
23%	compiler_metrics num_static_constructors	windows2012-64		115.00 -> 141.00
22%	compiler_metrics num_static_constructors	windows2012-32-shippable		119.00 -> 145.00
21%	compiler_metrics num_static_constructors	windows2012-32		123.50 -> 150.00
19%	compiler_metrics num_static_constructors	windows2012-64-shippable	instrumented	137.00 -> 163.00
18%	compiler_metrics num_static_constructors	windows2012-32-shippable	instrumented	146.92 -> 172.67
16%	compiler_metrics num_static_constructors	windows2012-aarch64	aarch64	164.00 -> 191.00
16%	compiler_metrics num_static_constructors	windows2012-32		167.50 -> 195.00
16%	compiler_metrics num_static_constructors	windows2012-64		165.00 -> 192.00
16%	compiler_metrics num_static_constructors	windows2012-64	fuzzing	167.00 -> 194.00
2%	compiler_metrics num_static_constructors	linux64	base-toolchains	321.92 -> 328.42

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

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

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

Comment 2

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

This was regressed by a third-party library update. While I'd like to leave this bug open for now to see what can be done about the reported regression, we shouldn't backout in the mean time either.

Comment 3

[Daniel Holbert [:dholbert]]

grepping through https://hg.mozilla.org/integration/autoland/rev/71c27941e870eaf684830af8101296a43ee245fd ("Bug 1773604 - Update bundled protobuf to version 21.2"), for static, it looks like the new static variables are all of the form:

static const ClassData _class_data_;

...whose instances seem to be defined/constructed like so:

const ::PROTOBUF_NAMESPACE_ID::Message::ClassData Api::_class_data_ = {
    ::PROTOBUF_NAMESPACE_ID::Message::CopyWithSourceCheck,
    Api::MergeImpl
};

https://hg.mozilla.org/integration/autoland/rev/71c27941e870eaf684830af8101296a43ee245fd#l9.797

The struct itself is defined here:
https://searchfox.org/mozilla-central/rev/2946e9b450cb35afaf8dad927a8d187975dcd74d/toolkit/components/protobuf/src/google/protobuf/message.h#370
https://hg.mozilla.org/integration/autoland/rev/71c27941e870eaf684830af8101296a43ee245fd#l91.252

So if we take action or suggest-upstream-fixes on this, it would probably be something associated with that type and its instances.

Having said that, I also don't know what the real-world impact of these new variables is. (They seem to be POD, i.e. structs that just contain two pointers [fucntion-pointers]. Each static constructor here would just set those two pointers and be done.)

Comment 4

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

I reached out to the upstream maintainers and they're saying that the ClassData variables should be getting constant-initialized so that there's no runtime cost. Given that and Daniel's analysis in comment 3, let's call this a wontfix.