Closed Bug 1605110 Opened 4 years ago Closed 4 years ago

0.47% installer size (osx-shippable) regression on push 65cf656ecce94b8c8bc4933cf57eb760a3b8d10f (Wed December 18 2019)

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox-esr68 unaffected, firefox71 unaffected, firefox72 unaffected, firefox73 fixed)

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- unaffected
firefox73 --- fixed

People

(Reporter: marauder, Assigned: chmanchester)

References

(Regression)

Details

(Keywords: perf-alert, regression)

Attachments

(1 file)

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=65cf656ecce94b8c8bc4933cf57eb760a3b8d10f

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

0.47% installer size osx-shippable opt nightly 80,834,640.75 -> 81,211,369.58

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=24523

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

The regression is pretty small, maybe this is expected.
Chris what do you think?
Thanks!

Blocks: 1600879
Component: Performance → General
Flags: needinfo?(cmanchester)
Product: Testing → Firefox Build System
Regressed by: 1604578
Target Milestone: --- → mozilla73
Version: Version 3 → unspecified
Has Regression Range: --- → yes

Presumably due to additional inlining. erahm, do you think this is worth investigating?

Flags: needinfo?(cmanchester) → needinfo?(erahm)

Eric pointed out on slack that XUL is smaller according to our perfherder data. From looking at things locally it's significantly larger, although other things are smaller. We're reporting it as smaller because rust-size doesn't include a big increase in __LINKEDIT size. Still investigating.

Flags: needinfo?(erahm)

There are a large number of symbols in the symbol table of the "after" build that weren't there before. Could this just be "different" inlining decisions?

Michael, does this sound like something we'd expect from turning on pgo for Rust? Do you have any tips for how I might investigate this further? Thanks!

Flags: needinfo?(mwoerister)

(In reply to Chris Manchester (:chmanchester) from comment #4)

There are a large number of symbols in the symbol table of the "after" build that weren't there before. Could this just be "different" inlining decisions?

"large number" is about 300k.

A change in inlining decisions and thus larger or smaller output binaries is an expected result of PGO, yes. 300k additional symbols sounds like quite a bit though. Where exactly do those show up? __LINKEDIT sounds like something related to symbol export. That should not be affected by inlining, I think. Are the new symbols all Rust symbols? What does an example look like?

Flags: needinfo?(mwoerister)

I was a little off, it's more like 65k symbols. I noticed https://searchfox.org/mozilla-central/rev/ac43d7e69a435ede789827f20ab309da6f04c09b/build/moz.configure/lto-pgo.configure#247 along the way, and adjusting that down to 10 to match other platforms fixes the size regression (although not in __LINKEDIT, interestingly).

Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0beaf81bb02c
Adjust `import-instr-limit` on macOS to match other platforms now that we have pgo. r=froydnj
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Hey Chris,
There is a new alert that seems to be related to your last changes:

https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&selected=2056485,617355,305.84001305312376,172.69154526187737,1008391325&series=autoland,2056485,1,1&series=mozilla-inbound,2131893,1,1&timerange=1209600&zoom=1576877223515,1576880321624,794.1666118140134,942.3239685629317

== Change summary for alert #24579 (as of Mon, 23 Dec 2019 10:04:12 GMT) ==

Regressions:

7% perf_reftest_singletons id-getter-2.html macosx1014-64-shippable opt e10s stylo 818.91 -> 876.96
6% perf_reftest_singletons id-getter-1.html macosx1014-64-shippable opt e10s stylo 488.50 -> 518.27
6% perf_reftest_singletons id-getter-2.html macosx1014-64-shippable opt e10s stylo 824.20 -> 874.43

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24579

Flags: needinfo?(cmanchester)

There is also an improvement:

== Change summary for alert #24574 (as of Sat, 21 Dec 2019 18:40:02 GMT) ==

Improvements:

16% build times osx-shippable opt nightly taskcluster-c5d.4xlarge 3,386.36 -> 2,860.27

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24574

These two benchmarks show a small regression, but they're still at an improvement from before pgo landed. This patch fixed the installer size regression as well, so I don't think we should consider a backout.

Flags: needinfo?(cmanchester)

Yes, perfherder created an improvement alert for installer size:

== Change summary for alert #24584 (as of Mon, 23 Dec 2019 14:19:52 GMT) ==

Improvements:

3% installer size osx-shippable opt nightly 81,214,321.00 -> 79,141,694.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24584

I closed the regression alert and marked it as fixed.

Thank you Chris!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: