1870077 - 0.32% installer size (OSX) regression on Mon December 11 2023

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P5)

Description

[Alex Finder]

Perfherder has detected a build_metrics performance regression from push e84648aa607a32b9e4bbd2378f5a7caf9e31264a. 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.32%	installer size	osx-cross		89,188,521.08 -> 89,472,119.50

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

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

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

:jstutte, since you are the author of the regressor, bug 1839051, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Jens Stutte [:jstutte]]

It is kind of expected and wanted to have more inlining happening thanks to the use of std::sort. It surprises me a bit that this should be such significant.

If I read the data correctly, on Linux we have a 0.22% regression and on Windows only 0.13%.

I do not really see why the value for Mac OS should be higher, maybe there are different compiler flags for more aggressive inlining? But it might just be the price we need to pay here (and we will remove the NS_QuickSort code soon, FWIW).

Olli, do you have any thought here?

Comment 3

[Jens Stutte [:jstutte]]

We discussed this a bit in the xpcom channel on matrix.
It seems, that MacOS installers contain the binaries twice, thus binary increases count more wrt the other files there. There might be a need to adjust the thresholds accordingly?

For the rest, it is expected that the patches cause significantly more template inlining than before, and there are other good reasons to use std::sort everywhere, so it might just be the price to pay.

Comment 4

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

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

Comment 5

[Donal Meehan [:dmeehan]]

:jstutte any update since Comment 3?
Could this be triaged for Severity, are you considering it a wontfix for Fx122?

Comment 6

[Jens Stutte [:jstutte]]

I consider this a wontfix for 122, yes. And probably in general, for now nobody had any suggestion to do anything, as returning to the old NS_QuickSort is not really an option.

It seems, that MacOS installers contain the binaries twice, thus binary increases count more wrt the other files there. There might be a need to adjust the thresholds accordingly?

could be the only action, IMHO. IIUC the threshold is an absolute value for all OS (only different on Android). The current threshold is 6 years old, coming from bug 1434587. :jmaher, you reviewed that patch back then, any opinion?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

It has been many years since I have done perf stuff (which this falls into that bucket). In general, we do need to revisit static numbers if they exist; This looks like a 0.32% increase in installer size, if this is explainable, then it seems logical to wontfix. For fixing installer size a focused effort would be effective to look at everything and see what we can trim down, I am not sure if Mozilla is in a spot where we need to reduce our installer size. It is just good to document what changes occur along the way.