1903713 - 3.48 - 1.52% compiler_metrics num_static_constructors / compiler_metrics num_static_constructors + 3 more (Linux, OSX, Windows) regression on Mon June 10 2024

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P1)

Description

[Florin Bilt]

Hello! I linked the four bugs that all together generated the following mozilla-beta alert. These bugs have not generated alerts on the autoland because they were right under the threshold. You can find here a graph comparison that will help you with the investigation. You can uncheck either the autoland or mozilla-beta data to see only one of them. Thank you!

Perfherder has detected a build_metrics performance regression from push 967812d4cdc454261b887c7b76ef67ed6fb5a677. 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)
3%	compiler_metrics num_static_constructors	osx-cross	fuzzing	115.00 -> 119.00
3%	compiler_metrics num_static_constructors	osx-cross-aarch64	aarch64-fuzzing	116.00 -> 120.00
2%	compiler_metrics num_static_constructors	windows2012-aarch64	aarch64	159.00 -> 162.00
2%	compiler_metrics num_static_constructors	linux64	base-toolchains-clang	166.00 -> 169.00
2%	compiler_metrics num_static_constructors	linux64	base-toolchains	198.00 -> 201.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 patch(es) may be backed out in accordance with our regression policy.

You can run these tests on try with ./mach try perf --alert 718

For more information on performance sheriffing please see our FAQ.

Comment 1

[Paul Zühlcke [:pbz]]

Looking! The only thing relevant I see is https://phabricator.services.mozilla.com/D211578 which changed some includes. I'll see if I can optimize those.

Comment 2

[Paul Zühlcke [:pbz]]

Thanks!

I found some unused includes I'm removing in Bug 1903825 which could have impact on compile time. Otherwise I can't see any performance issues with the work in Bug 1890580. Can you advise on how to further investigate here? Is there documentation on the metric num_static_constructors?

Are folks looking into the perf regressions from Bug 1897473 and Bug 1842682 and what is the share of regression respectively?

Comment 3

[Paul Zühlcke [:pbz]]

I couldn't find any good documentation but looking at similar bugs filed it looks like this might be the issue: https://searchfox.org/mozilla-central/rev/74518d4f6979b088e28405ba7e6238f4707639cf/toolkit/components/antitracking/bouncetrackingprotection/ClearDataCallback.cpp#36

Instead one should use StaticAutoPtr. I'll make the change and test this on try.

Comment 4

[Paul Zühlcke [:pbz]]

Attachment: Bug 1903713 - Use StaticAutoPtr for static member in ClearDataCallback to avoid static constructor. r=#anti-tracking!

Comment 5

[Paul Zühlcke [:pbz]]

static constructor -1: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=d10cb44c5b8f764c50508e9ee55d791f08cc1e24&newProject=try&newRevision=256087ef6ed88f89421a49e7f2f0d3057c66e6d9&page=2&framework=2#tableLink-header-1540502897

Comment 6

[Nathan LaPré]

It looks like I'm one of the authors of a patch implicated here. In this commit I added a static nsTHashSet and static bool. I'm guessing that the nsTHashSet is the only thing we care about here? I'm happy to put it behind a StaticAutoPtr, or maybe a function that defines the static nsTHashSet internally and returns it. Any guidance appreciated, thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2807d2cfa4dc
Use StaticAutoPtr for static member in ClearDataCallback to avoid static constructor. r=anti-tracking-reviewers,timhuang

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5695d20218eb
Backed out 2 changesets (bug 1903713, bug 1903825) for causing non unified build bustages on BounceTrackingProtection.h. CLOSED TREE

Comment 9

[Nathan LaPré]

Attachment: Bug 1903713: Avoid static nsTHashSet constructor via instance method, r?Jamie,fbilt

This revision avoids static construction of the nsTHashSet by hiding the set
behind an instance method.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36bf06ad0d87
Use StaticAutoPtr for static member in ClearDataCallback to avoid static constructor. r=anti-tracking-reviewers,timhuang

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/36bf06ad0d87

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0cd0d29eaa4
Avoid static nsTHashSet constructor via instance method, r=Jamie

Comment 13

[pstanciu]

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