Investigate removal of #includes from browser.js

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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
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.
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
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 :)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
ah right, I didn't recall that browser.js itself was included.
(Assignee)

Comment 6

3 years ago
Created attachment 8693052 [details] [diff] [review]
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
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 8

3 years ago
Created attachment 8693397 [details]
MozReview Request: Bug 1228627 - Remove #includes from browser.js. r?Gijs

Bug 1228627 - Remove #includes from browser.js. r?Gijs
Attachment #8693397 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 9

3 years ago
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 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. ;-)
Attachment #8693397 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/830a2218d209
https://hg.mozilla.org/mozilla-central/rev/fb7bdd4da8d9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Assignee)

Updated

3 years ago
Blocks: 1150859
Blocks: 1229856
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.
(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?
Flags: needinfo?(dolske)
(Assignee)

Comment 16

3 years ago
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.
If <script> isn't actually causing a regression, let's stick with that.
Flags: needinfo?(dolske)
You need to log in before you can comment on or make changes to this bug.