1228627 - Investigate removal of #includes from browser.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

An attempt to remove the #includes pre-processing from browser.js.

I sent to tryserver a patch that moves all includes into script tags to global-scripts.inc.  Let's see if it breaks anything and what talos looks like.

Perhaps some of these could go to browser.xul instead of global-scripts.inc. This could be even a win for Mac because that would make macBrowserOverlay.xul smaller.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1e44969f5f8

Comment 1

[Marco Bonardo [:mak]]

I think we should add as few as possible to global-scripts.inc since IIRC it's all duped content on Mac (not sure why chatWindow includes it...). I don't think it's a great idea to move stuff to it.

I suspect most of the things we include in browser.js could be factored out to modules, some could be instanced per window, some just get window/document as argument. That's a big chunk of work though.
For sure we should be more critic about what we #include, there's almost always a better alternative.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Hm should have probably built locally first before sending to try..  Obviously these files don't exist in jar.mn so it's not just moving them to a <script> tag

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to Marco Bonardo [::mak] from comment #1)
> I think we should add as few as possible to global-scripts.inc since IIRC
> it's all duped content on Mac (not sure why chatWindow includes it...). I
> don't think it's a great idea to move stuff to it.

Yeah, I intend to include as little as possible in global-scripts.inc. But right now the intention is just to keep things the way they are, but remove the preprocessing in browser.js (so that we can run eslint there).  In theory this change shouldn't break anything because browser.js itself is included in global-scripts.inc and all of these files where included in it. But this will open the opportunity to optimze things further.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Gonna assign it to me for the moment as I'm playing with this, but I make no promise at the moment to go with this through the end if it takes longer than I can spare :)

Comment 5

[Marco Bonardo [:mak]]

ah right, I didn't recall that browser.js itself was included.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: remove-includes-browserjs

Alright, this seems to be working locally with a quick sanity check, + 5min of mochitest-browser running. Now on to tryserver to do the full run.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ad52320bf80

Comment 7

[:Felipe Gomes (needinfo for replies!)]

New push

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e7c3aacf97
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=76e7c3aacf97

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1228627 - Remove #includes from browser.js. r?Gijs

Bug 1228627 - Remove #includes from browser.js. r?Gijs

Comment 9

[:Felipe Gomes (needinfo for replies!)]

A number of these files still need to go through the preprocessor because they have #ifdefs in them. I'm removing all of that in a separate bug, bug 1228655.

One minor question here is that some, but not all of these files, have emacs indent configs in the first line. I decided not to change any, but I wondered if the same settings should be added to the files that don't have it.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8693397 [details]
MozReview Request: Bug 1228627 - Remove #includes from browser.js. r?Gijs

https://reviewboard.mozilla.org/r/26445/#review23885

::: browser/base/content/browser-tabview.js:1
(Diff revision 1)
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Mumble mumble why you bitrottin' my patches. ;-)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/830a2218d209

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fb7bdd4da8d9

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/830a2218d209
https://hg.mozilla.org/mozilla-central/rev/fb7bdd4da8d9

Comment 14

[Justin Dolske [:Dolske]]

Wow, nice!

I must admit I'm a little surprised this didn't cause a perf regression. I thought we've run into this before, where adding more code in a <script> caused a startup regression but #including it in browser.js didn't.

Comment 15

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #14)
> Wow, nice!
> 
> I must admit I'm a little surprised this didn't cause a perf regression. I
> thought we've run into this before, where adding more code in a <script>
> caused a startup regression but #including it in browser.js didn't.

I wouldn't be surprised if the regression my panorama removal code caused might have hidden this. If so though, it would have been a small regression (< 2%).

I think it would be worth checking that and/or if we can add a build system hack that concats all the scripts in global-scripts.inc and uses that instead of browser.xul, and see if that gets us a noticeable perf win. Should be easy to hack up, the hard part will be ensuring we can figure out debug information and so on. Might be worth a followup bug? Justin, what do you think?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

I was surprised at first too but I triggered 4x talos runs for all platforms and used compare-talos against m-c before the Panorama removal and didn't spot any problems. Could be worth to double check.

Comment 17

[Justin Dolske [:Dolske]]

If <script> isn't actually causing a regression, let's stick with that.