Closed
Bug 1228627
Opened 9 years ago
Closed 9 years ago
Investigate removal of #includes from browser.js
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: Felipe, Assigned: Felipe)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
Gijs
:
review+
|
Details |
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•9 years ago
|
||
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•9 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•9 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•9 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
Comment 5•9 years ago
|
||
ah right, I didn't recall that browser.js itself was included.
Assignee | ||
Comment 6•9 years ago
|
||
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•9 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•9 years ago
|
||
Bug 1228627 - Remove #includes from browser.js. r?Gijs
Attachment #8693397 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 9•9 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 10•9 years ago
|
||
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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/830a2218d209
https://hg.mozilla.org/mozilla-central/rev/fb7bdd4da8d9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 14•9 years ago
|
||
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•9 years ago
|
||
(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•9 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.
Comment 17•9 years ago
|
||
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.
Description
•