Closed Bug 1439839 Opened 2 years ago Closed Last year

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

Categories

(DevTools :: Debugger, defect)

Unspecified
Windows
defect
Not set

Tracking

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: igoldan, Assigned: jlast)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
: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?
Flags: needinfo?(jlaster)
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(jlaster)
(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.
Flags: needinfo?(bobbyholley) → needinfo?(jlaster)
Thanks :bholley, i'll work on making the two files smaller. Sorry for the slow reply
Flags: needinfo?(jlaster)
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.
Nice - thanks for jumping on this Jason!
Attached patch rel-19-1.patchSplinter Review
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.
Attachment #8955308 - Flags: review?(jdescottes)
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.
Flags: needinfo?(jcristau)
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.
Attachment #8955308 - Flags: review?(jdescottes) → review+
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.
Thanks for letting me know!
Flags: needinfo?(jcristau)
Keywords: checkin-needed
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
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2307a921159
Remove references to babel-types. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c2307a921159
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee: nobody → jlaster
Re-opening so we can land another patch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Sounds good, i'll do that for the next patch.
Depends on: 1443193
Is there more to do still to address this regression?
Flags: needinfo?(jlaster)
I believe this is resolved.
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Flags: needinfo?(jlaster)
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.