1467046 - 0.26 - 0.41% installer size (linux32, linux64, osx-cross, windows2012-32, windows2012-64) regression on push 22c2645c283cbff140a25290936e2d3ccb4698bf (Tue Jun 5 2018)

Status: RESOLVED WONTFIX

(Testing :: General, defect)

Description

[Ionuț Goldan [:igoldan]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=22c2645c283cbff140a25290936e2d3ccb4698bf

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

Regressions:

  >200KBytes  installer size linux32 pgo      63,076,933.67 -> 63,292,702.75
  >150KBytes  installer size linux64 pgo      62,243,331.04 -> 62,496,810.17
  >150KBytes  installer size windows2012-64 pgo 60,302,715.50 -> 60,530,208.50
  >150KBytes  installer size windows2012-32 pgo 55,958,805.54 -> 56,157,719.92
  >150KBytes  installer size osx-cross opt    65,161,330.08 -> 65,328,014.00


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

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

Comment 1

[Mark Banner (:standard8)]

This only touched a small bit of code, and even then it removed a function rather than added anything.

I suspect this is either a bad range, or there's some weird compiler optimization that's been hit. I certainly didn't add that much code...

Comment 2

[Ionuț Goldan [:igoldan]]

(In reply to Mark Banner (:standard8) from comment #1)
> This only touched a small bit of code, and even then it removed a function
> rather than added anything.
> 
> I suspect this is either a bad range, or there's some weird compiler
> optimization that's been hit. I certainly didn't add that much code...

I noticed the usage of templates in the code update. I know this influences installer size. Bad thing is all platforms are affected and point to bug 1444329. I have no expertise on this matter, so I ask :froydnj to check the validity of this regression.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #2)
> I noticed the usage of templates in the code update.

Do you mean the Span stuff? That's all super-simple that should should inline to nothingness before and after the change.

It makes no sense for the change around that point to have caused this kind of size increase across compilers. Misattribution of the regression range seems more believable.

Comment 4

[Marco Bonardo [:mak]]

Could it be we're measuring the effect of bug 1437942?
At least on the OSX graph, the increase seems to be effective at https://hg.mozilla.org/integration/autoland/rev/f4a2094a00c5

https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1514740,1,2&zoom=1528200465868.966,1528210734229.8643,65000000,65417388.708155856&selected=autoland,1514740,344715,482208736,2

And effectively that bug actually moved all the search plugins for all locales into Firefox installer, if I read it correctly.

Comment 5

[Marco Bonardo [:mak]]

Ah, that bug was just backed out, so it should be possible to check if the size increase disappears.

Comment 6

[Mike Kaply [:mkaply]]

Yes, it was intended that the installer would increase as a result of this. All of the searchplugins will be included with Firefox when this is finally landed.

I didn't realize we tracked installer size.

What is the right thing to do if we intend to increase the installer size?

Comment 7

[Marco Bonardo [:mak]]

I can finally confirm that the installer size was due to bug 1437942 and not bug 1444329
The other graphs already confirmed, this is Win PGO, 841300ddc8ec is the "backout" changeset.
https://treeherder.mozilla.org/perf.html#/graphs?timerange=172800&series=autoland,1457045,1,2

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mike Kaply [:mkaply] from comment #6)
> I didn't realize we tracked installer size.
> 
> What is the right thing to do if we intend to increase the installer size?

If you knew the size increase was coming (or even if you didn't), we generally ask that you think about whether the size increase is reasonable for the change that you made.  People sometimes make an innocent-looking change that winds up having a big increase on the installer size, and this alert is a good opportunity to enable people to reflect on their changes a bit. :)

200K+ on Windows sounds not so great, especially because that 200K increase is on a compressed artifact, so the actual size of the omni.ja increase is probably more.

So, does 200K seem reasonable, or are there opportunities to shrink those files somewhat?

Comment 9

[Mike Kaply [:mkaply]]

> 200K+ on Windows sounds not so great, especially because that 200K increase is on a compressed artifact, so the actual size of the omni.ja increase is probably more.

I'll do some checking on actual footprint.

> So, does 200K seem reasonable, or are there opportunities to shrink those files somewhat?

Unfortunately a lot of the files contain data: URLs for their icons and my wager is that those don't compress well. I'll do some investigation.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Mike Kaply [:mkaply] from comment #9)
> > 200K+ on Windows sounds not so great, especially because that 200K increase is on a compressed artifact, so the actual size of the omni.ja increase is probably more.
> 
> I'll do some checking on actual footprint.

Note that the test for installer size has subtests for libxul and the omni.ja sizes; you can see graphs for those tests on perfherder (you'll have to click the "Show subtests" box when adding tests to the graph to be able to select the subtests).

Comment 11

[Mike Kaply [:mkaply]]

I'm really confused by the subtests

Looking at:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=f8c98ae75bbf5043ab2f6d2f33b534cf58be7790&newProject=autoland&newRevision=22c2645c283cbff140a25290936e2d3ccb4698bf&originalSignature=6fe30befd371a869a2ea65b68ce54c22e6f129a6&newSignature=6fe30befd371a869a2ea65b68ce54c22e6f129a6&framework=2

IT shows a decrease to omni.ja and an increase to xul.dll?

Comment 12

[Mark Banner (:standard8)]

I think it might be helpful if someone who understands how to easily drive the perf tracking mechanism takes a look at:

- The effects of bug 1437942 on landing/backout
- The effects of bug 1444329 on landing

Presumably these are all based on clobber builds? i.e. there's no chance of left-overs from something else?

If there's a way to test things on try, then I guess we could do a comparison of bug 1444329 with installer size on its own.

Comment 13

[Marco Bonardo [:mak]]

My theory is that there has been an effective small increase to xul.dll due to bug 1444329, that was somehow re-absorbed soon, but 
bug 1437942 paved on that re-absorbption

xul.dll is pretty much in noise:
https://treeherder.mozilla.org/perf.html#/graphs?highlightedRevisions=f8c98ae75bbf&highlightedRevisions=22c2645c283c&series=%5B%22autoland%22,%22d8cfa9e1ef6b3d0c7a11d6608e5b702f21e41ce5%22,1,%222%22%5D&timerange=172800
while omni.js shows a clear increase
https://treeherder.mozilla.org/perf.html#/graphs?highlightedRevisions=f8c98ae75bbf&highlightedRevisions=22c2645c283c&series=%5B%22autoland%22,%22c0efb8eb578dea36e7d03ac6771fc80eee07529a%22,1,%222%22%5D&timerange=172800

Comment 14

[Marco Bonardo [:mak]]

basically, I suspect the system noticed a small increased followed by a larger one, and it coalesced the 2 assigning to the first increase.

Comment 15

[(Away)]

Attachment: symbolsort of autoland build

I ran SymbolSort on your autoland push and its parent. The interesting bits are at line 5355, "Increases in Total Size".

The DOM bindings functions blew up a lot. (When reading these files, you need to check out "Decreases in Total Size" to rule out false positives, in case there was just some mass rename or ICF or something. In this case I don't see any.)

I took a brief look at a random one, and `xul!js::shadow::Object::slotRef` and `xul!js::gc::AtomMarkingRuntime::markId` (and possibly others) got inlined, causing a lot of extra code.

If that is not related to your change, it might just be PGO craziness.

Comment 16

[Mike Kaply [:mkaply]]

One other comment based on discussion with mak.

We are only tracking omni.ja.

We should also track browser/omni.ja.

Comment 17

[Nathan Froyd [:froydnj]]

We track both omni.ja files:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=172800&series=autoland,1668151,1,2&series=autoland,1668150,1,2&highlightedRevisions=f8c98ae75bbf&highlightedRevisions=22c2645c283c&zoom=1528185319267.426,1528222607217.7832,15827090.659838043,37737203.01938861&selected=autoland,1668150,344713,482396901,2

Shows a 550K increase in browser/omni.ja for the searchplugins push.

Comment 18

[Nathan Froyd [:froydnj]]

(Also, our browser/omni.ja file is ~35MB?!)

Comment 19

[Mike Kaply [:mkaply]]

> (Also, our browser/omni.ja file is ~35MB?!)

Yes, and it's uncompressed. And yes, 550kb sounds about right.

Comment 20

[Marco Bonardo [:mak]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> (Also, our browser/omni.ja file is ~35MB?!)

Ok, this is a little bit OT here, but I just ran it through treesize to check what was taking so much space.
20.6MB out of those are actually chrome/devtools, 8MB of chrome/browser, 4MB of pdfjs and 3MB of modules.

Comment 21

[Mike Kaply [:mkaply]]

I guess that explains why devtools wants to move to an extension :).

Comment 22

[Mike Kaply [:mkaply]]

So we figured out what broke l10n and I'll be preparing to land this again.

Is there something I should do to let people know exactly what I'm doing here and why this will increase the size of Firefox desktop? (It actually reduces size on mobile.)

Comment 23

[Mike Kaply [:mkaply]]

Is there signoff needed here or something to that effect?

Comment 24

[Nathan Froyd [:froydnj]]

(In reply to Mike Kaply [:mkaply] from comment #23)
> Is there signoff needed here or something to that effect?

I think the discussion here has been signoff enough; bug 1467205 will win back some of the size increase, too.  Thanks!

Comment 25

[Ionuț Goldan [:igoldan]]

The relanding from https://bugzilla.mozilla.org/show_bug.cgi?id=1437942#c70 caused similar regressions:

== Change summary for alert #13725 (as of Fri, 08 Jun 2018 10:13:13 GMT) ==

Regressions:

  >150 KBytes  installer size windows2012-64 pgo      60,423,271.67 -> 60,574,945.92
  >150 KBytes  installer size osx-cross opt           65,236,923.50 -> 65,388,582.58

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

Comment 26

[Ionuț Goldan [:igoldan]]

Other regressions seem to have remained also. So are you considering accepting these regressions?

Comment 27

[Mike Kaply [:mkaply]]

Yes, we expect the size of the installer (and browser/omni.ja) to increase due to these changes.