1944663 - 0.18% installer size (OSX) regression on Tue January 21 2025

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a build_metrics performance regression from push c7fcb496868d1770308103b680948f07afabdeec. 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)
0.18%	installer size	osx-aarch64-shippable	aarch64 nightly	98,431,821.25 -> 98,609,777.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 patch(es) may be backed out in accordance with our regression policy.

You can run all of these tests on try with ./mach try perf --alert 43436

The following documentation link provides more information about this command.

For more information on performance sheriffing please see our FAQ.

If you have any questions, please do not hesitate to reach out to [e-mail].

Comment 1

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

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

Comment 2

[Florian Quèze [:florian]]

Here's a binary profile diff of before/after for opt (non-pgo) Mac builds: https://share.firefox.dev/40XGGJv

Warning: The profile takes a long time to load, and because size profiling is very experimental, things might be confusing in the UI, or even completely incorrect.

Comment 3

[Florian Quèze [:florian]]

Attachment: Screenshot of the relevant changes

Because loading the profile is very slow, I'm attaching a screenshot of the relevant information.

The functions that became much larger are generated functions that contain many metric ids. This means the regression is very likely caused by https://phabricator.services.mozilla.com/D234741 I suppose before we used hashes the compiler could figure out that the maximum value was small, and use fewer bytes to store the metric ids. Or maybe do code optimizations because the ids were consecutive.

Comment 4

[Jan-Erik Rediger [:janerik]]

So probably what we could cover in bug 1932617?

Comment 5

[Florian Quèze [:florian]]

(In reply to Jan-Erik Rediger [:janerik] from comment #4)

So probably what we could cover in bug 1932617?

Bug 1932617 comment 4 sounds like you have ideas of things to do, but no immediate plan to do it. Given this bug is a regression, I wonder if we should try to find a quick solution to revert the binary size increase. The point of the hashing is to enable quick rebuilds when adding a metric, I think that's only useful in a developer context and is pointless for CI builds. How about disabling the hashing algorithm and going back to consecutive numeric integers for release builds?

Comment 6

[Jan-Erik Rediger [:janerik]]

If the hashing indeed is the problem then we could do that.
I didn't get around to work on bug 1932617, but I can see if I can prioritize that in the coming weeks.

Comment 7

[Florian Quèze [:florian]]

Attachment: Bug 1944663 - Only use a hashing algorithm to generate metric ids for local builds, r=chutten,sergesanspaille.

Comment 8

[Florian Quèze [:florian]]

Here's a binary size profile diff of try runs before/after the patch I just attached: https://share.firefox.dev/3WRBX9J The patch reduces the Mac opt libxul size by 112,900 bytes. I'm surprised this number is much lower than the number I was seeing in comment 2 (583,936 bytes), but I have doubts about the numbers generated by the size profiler, so I think it confirming we are definitely reducing size of things that had grown is already good.

Comment 9

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #8)

Here's a binary size profile diff of try runs before/after the patch I just attached: https://share.firefox.dev/3WRBX9J The patch reduces the Mac opt libxul size by 112,900 bytes. I'm surprised this number is much lower than the number I was seeing in comment 2 (583,936 bytes), but I have doubts about the numbers generated by the size profiler, so I think it confirming we are definitely reducing size of things that had grown is already good.

Markus, do you think that difference between losing (adding) 583,936 bytes with the regression and winning (removing) 112,900 bytes with the patch to fix it can be explained by the issue in the code at https://github.com/jrmuizel/size-profiler/blob/630edc9828fbdeea8a534bd612f88dc5c8a7fab9/src/main.rs#L107 ?

Comment 10

[Markus Stange [:mstange]]

Not sure. I'll take a stab at making the fix I have in mind, and then we'll see whether that makes a difference.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac0f933a5cce
Only use a hashing algorithm to generate metric ids for local builds, r=chutten,sergesanspaille.

Comment 12

[Sandor Molnar[:smolnar]]

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

Comment 13

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

The patch landed in nightly and beta is affected.
:florian, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox136 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Florian Quèze [:florian]]

Attachment: image.png

(In reply to Sandor Molnar[:smolnar] from comment #12)

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

Looking at the installer size data, I think there's a decrease on that changeset, so the patch seems to have the expected effect.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 9464025 [details]
Bug 1944663 - Only use a hashing algorithm to generate metric ids for local builds, r=chutten,sergesanspaille.

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: larger installer size
• 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): Reverts to the previous behavior of the script that generates glean metric ids; should not change any actual behavior of the code.
• String changes made/needed:
• Is Android affected?: Yes

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9464025 [details]
Bug 1944663 - Only use a hashing algorithm to generate metric ids for local builds, r=chutten,sergesanspaille.

Approved for 136.0b5

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4821f54ee682