1439839 - 0.19 - 0.23% installer size (windows2012-32, windows2012-64) regression on push 189364f02193ddcad488215cfd8bd6a9b0733ce9 (Tue Feb 20 2018)

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Ionuț Goldan [:igoldan]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=189364f02193ddcad488215cfd8bd6a9b0733ce9

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

Regressions:

  0%  installer size windows2012-64 pgo      61,471,461.71 -> 61,614,866.92
  0%  installer size windows2012-32 pgo      57,443,807.42 -> 57,555,082.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=11658

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

[Ionuț Goldan [:igoldan]]

:jlast, :bholley It looks like bug 1438930 caused some increases in installer size (more than 100KBytes). Do you think we can reduce it, at least to some extent?

Comment 2

[Jason Laster [:jlast]]

Thanks :igoldan, can you share some statistics on where the increase is coming from? Also what is the level of importance? 

I think it is possible to reduce the sizes of the parser-worker.js and debugger.js files by 20% but it would require some work to do.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Jason Laster [:jlast] from comment #2)
> Thanks :igoldan, can you share some statistics on where the increase is
> coming from?

I'm not sure what you mean. The increase is coming from your patch, presumably because it further increases the size of a very large JS file from 1M to 1.7M, which is huge.

> Also what is the level of importance?

Installer size is correlated with download success rate, and is thus something product cares about. A 100k+ increase is considered large enough that we should at least investigate mitigations.
 
> I think it is possible to reduce the sizes of the parser-worker.js and
> debugger.js files by 20% but it would require some work to do.

Given how large the file is, I think that's probably worthwhile. It looks like there's also lots of external code and data tables, and it's not clear to me whether we need it all. Slimming it down may improve memory usage as well, because the JS engine needs to load all that stuff into memory.

Note also that the installer is compressed, so the final effects may be nonlinear. You can probably approximate the impact by compressing the files. Using regular zip, the old parser-worker.js compresses to 222k and the new one to 350k, which seems to map pretty well to the installer size regression we're seeing.

Comment 4

[Jason Laster [:jlast]]

Thanks :bholley, i'll work on making the two files smaller. Sorry for the slow reply

Comment 5

[Jason Laster [:jlast]]

here is a PR which is the works - https://github.com/devtools-html/debugger.html/pull/5557

It brings the bundle down from 1.7 to 1.0mb again. We'll have a follow up PR to bring it to 750kb afterwards. This should be fixed in the next couple of days.

Comment 6

[Bobby Holley (:bholley)]

Nice - thanks for jumping on this Jason!

Comment 7

[Jason Laster [:jlast]]

Attachment: rel-19-1.patch

This drops 200KB from the parser worker, bringing it from 1.7kb to 1.5kb. There are two more patches on the way that will bring it down to 1.3kb and later 800kb.

Comment 8

[Jason Laster [:jlast]]

Hey Julien, I just want to give you a heads up here. We hope to land 3 patches next week to central and eventually beta. 

The patches will bring the parser-worker.js size down from 1.7mb -> 1.5mb -> 1.2mb -> 800kb.

The first patch is up now. It looks big, but the original commit was one line. The reason why it is big, is that we no longer needs to bundle the babel-types package.

https://github.com/devtools-html/debugger.html/commit/aee1d39fba9726c56d8ffeb1be1998b6d79ae986

The other two changes will exclude the lodash and babel-traverse libraries. Both of these changes will have a big diff unfortunately, but should be safe as the original code change is safe.

Comment 9

[Jason Laster [:jlast]]

here's a try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be50b6f1277f1fe94af21688e812120ac2d2824

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8955308 [details] [diff] [review]
rel-19-1.patch

Review of attachment 8955308 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me. Thanks for pinging release management here, let's wait and see what they think about this before landing.

Comment 11

[Julian Descottes [:jdescottes]]

Some additional background as to why this comes in so late. On the original bug for release 17 we got a comment 2 days ago (Bug 1438930 comment 14) explaining that the patch would be backed out if the installer size was not reduced. While we considered this an important issue and something we had to fix, it wasn't really clear to us that this would lead to a backout of the patch.

We really want to avoid doing big patches doing the soft freeze period, so this is really unfortunate.

Comment 12

[Julien Cristau [:jcristau]]

Thanks for letting me know!

Comment 13

[Jason Laster [:jlast]]

I have a second patch, which I'll share after the first patch lands. The second patch includes two fixes that bring the bundle size down from 1.5mb to 800KB. The first fix, externalized lodash, which is a library we were bundling in. The second fix, removed @babel/traverse, which was a library we were using prior. 

Here is a try run for the 2nd patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc8fb661bd5b3656debd1dafd5db221d06c4cbb

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2307a921159
Remove references to babel-types. r=jdescottes

Comment 15

[Raul Gurzau (:RaulG)]

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

Comment 16

[Jason Laster [:jlast]]

Re-opening so we can land another patch

Comment 17

[Bobby Holley (:bholley)]

In general, it's best to land fixes for perf regression tracking bugs as separate dependent bugs, so that (a) multiple fixes can land without reopening and confusing relman, and (b) the perf sheriff can resolve the tracking bug appropriately without it also being associated with code changes that landed on m-c.

Not a big deal, but probably best to land any additional patches as separate bugs.

Comment 18

[Jason Laster [:jlast]]

Sounds good, i'll do that for the next patch.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is there more to do still to address this regression?

Comment 20

[Jason Laster [:jlast]]

I believe this is resolved.