622423 - More delay loaded JS scripts and less via <script> includes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

We load a lot of JS scripts in browser.xul via <script> tags. It might be beneficial for Ts to try to delay load any unnecessary scripts until needed, using lazy Cu.import loading.

Here is a WIP patch. It's a tricky web. I need to move Elements and Strings to JS modules too. I plan to make a single JS script that handles the lazy imports and include that into browser.xul via a <script> tag.

I also plan to move any non-essential JS from browser.js and browser-ui.js into modules too.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 2

This patch fixes a few issues:
* Adds a common-ui.js module that currently has the Elements and Strings helpers.
* The ExtensionsView and DownloadsView code used setTimeout / clearTimeout, which needed to use the | this._window | accessor now that a window is not present.
* The ExtensionsView and DownloadsView code had helper objects that assumed the views were singletons and accessed methods directly. We now need to pass the view to those helpers.

At this point, sanitizer.js, preferences.js, extensions.js, downloads.js and console.js are lazy loaded modules. common-ui.js also moved some code to a lazy module too.

I want to also move exceptions.js and AnimatedZoom.js to modules. Then I want to move some helper code out of browser-ui.js (currently 91KB), into modules. Code like FormHelperUI, FindHelperUI, ContextMenu, ContextCommandHelper, AppMenu and others...

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 3

* Adds the missing common-ui.js module
* Uses a sandbox for the Elements and Strings lazy getters. I found that when Strings was "loaded" it would also overwrite the Elements property too, since both were imported directly into the global.
* Adds exceptions.js to lazy load
* Mostly adds AnimatedZoom.js, but I need to handle the use of the Rect class.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 4

* AnimatedZoom.js is fully a module. Handling Rect was easy!.

We also use a callback for mozRequestAnimationFrame and stop using the MozBeforePaint event.

Time to start working on moving some JS objects out of browser-ui.js

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Forgot to mention this patch applies on the patch in bug 518111

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Did some talos Ts runs on my N900, just to see if this was having any affect:

Average Ts results over 30 runs:

Baseline:      6735 ms
w/bug 518111:  6687 ms
and this bug:  6585 ms

Maybe we can get below 6500 ms by removing some JS from browser-ui.js

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #5)
> Did some talos Ts runs on my N900, just to see if this was having any affect:
> 
> Average Ts results over 30 runs:
> 
> Baseline:      6735 ms
> w/bug 518111:  6687 ms
> and this bug:  6585 ms
> 
> Maybe we can get below 6500 ms by removing some JS from browser-ui.js

You can probably get    <script type="application/javascript" src="chrome://browser/content/input.js"/> out of the initial load too.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 500747 [details] [diff] [review]
WIP 4

Looks good to me.  It would be nice if we didn't need to pass "window" references into various modules, but cleaning that up properly is probably beyond the scope of this bug.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> Comment on attachment 500747 [details] [diff] [review]
> WIP 4
> 
> Looks good to me.  It would be nice if we didn't need to pass "window"
> references into various modules, but cleaning that up properly is probably
> beyond the scope of this bug.

Yeah, I hid that as best I could. Using this._window in the module is a pain, but I couldn't use a global on the chance more than on window could include the module.

We need to make the "window" be a per-instance reference.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 500747 [details] [diff] [review]
WIP 4

Some of these files are ok for me to be module, like AnimatedZoom but most of those I don't see the need to transform them as module, can't we simply use the mozIJSSubscriptLoader API ( http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/idl/mozIJSSubScriptLoader.idl) for them? 
It will preserve the need to change their actual code and to introduce some insanity like this._browserUI without preventing us to lazy load them.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 5 (different approach)

Uses mozIJSSubScriptLoader to avoid needing to re-map the window and document properties. Much less code changes.

Still more to come: I am cleaning up some additional script usage and will make a separate <script> include for the lazy defines.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 501024 [details] [diff] [review]
WIP 5 (different approach)

I prefer it like that much more.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 1

Core patch. Delay loads lots of script. We could do more in followup bugs. I just want to get the core landed first.

I wrapped some global helper scripts in a namespace. Makes exposing them simpler and easier to track where we use them.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 501035 [details] [diff] [review]
patch 1

>+XPCOMUtils.defineLazyGetter(this, "Sanitizer", function() {
>+  let sandbox = {};
>+  Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", sandbox);
>+  return sandbox.Sanitizer;
>+});
>+
>+XPCOMUtils.defineLazyGetter(this, "ExtensionsView", function() {
>+  let sandbox = {};
>+  Services.scriptloader.loadSubScript("chrome://browser/content/extensions.js", sandbox);
>+  return sandbox.ExtensionsView;
>+});

All these non-namespaced getters could be defined in a loop, like the Strings getters from bug 518111.  This would make the namespaced ones stand out more.  But if you prefer to keep it like this for consistency, go ahead.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with looped-getter for browser chrome scripts

http://hg.mozilla.org/mobile-browser/rev/4ac6be99537d

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

A quick followup with about 40KB of JS moved to a common JS file that is delay loaded. Since all the objects are in a single file, we currently pay a worse-case cost on the first object delay loaded. I cache the entire file so any other objects loaded do not cause the file to be loaded again.

We could look at making smaller files later. We still delay the JS load cost, it could just be better in the future.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 501324 [details] [diff] [review]
patch 2

># HG changeset patch
># Parent bd90a441956992cb830b08a74df7bf055c4df4f0
>diff --git a/chrome/content/browser-scripts.js b/chrome/content/browser-scripts.js
>--- a/chrome/content/browser-scripts.js
>+++ b/chrome/content/browser-scripts.js
>@@ -48,16 +48,50 @@ XPCOMUtils.defineLazyGetter(this, "Plura
> });
> 
> XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() {
>   Cu.import("resource://gre/modules/PlacesUtils.jsm");
>   return PlacesUtils;
> });
> 
> /**
>+ * Delay load some objects from a common script. We only load the script once,
>+ * into a namesapce, then access each object from the namespace.
>+ */

nit: namespace

I don't like common-ui.js name and I think this file is missing from your patch :)

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2 v2

Now with common-ui.js

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 501327 [details] [diff] [review]
patch 2 v2

I kind of happy to see such a bunch of code moving out of browser-ui.js

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/7d47dd39deaf

I'll file a new bug for any other work.

Comment 20

[Andreea Pod]

So this can be marked as verified fixed, right?

Comment 21

[Matt Brubeck (:mbrubeck)]

Verified by inspecting the source code.