Closed Bug 622423 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Keywords: perf)

Attachments

(2 files, 6 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
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.
Keywords: perf
Attached patch WIP 2 (obsolete) — Splinter Review
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...
Assignee: nobody → mark.finkle
Attachment #500693 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
* 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.
Attachment #500707 - Attachment is obsolete: true
Attached patch WIP 4 (obsolete) — Splinter Review
* 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
Attachment #500746 - Attachment is obsolete: true
Forgot to mention this patch applies on the patch in bug 518111
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
(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.
tracking-fennec: --- → ?
OS: Linux → All
Hardware: x86_64 → All
Attachment #500747 - Flags: feedback?(21)
Attachment #500747 - Flags: feedback?(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.
Attachment #500747 - Flags: feedback?(mbrubeck) → feedback+
(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 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.
Attachment #500747 - Flags: feedback?(21) → feedback-
Summary: Load more JS via CU.import and less via <script> includes → More delay loaded JS scripts and less via <script> includes
Attached patch WIP 5 (different approach) (obsolete) — Splinter Review
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.
Attachment #500747 - Attachment is obsolete: true
Attachment #501024 - Flags: feedback?(21)
Comment on attachment 501024 [details] [diff] [review]
WIP 5 (different approach)

I prefer it like that much more.
Attachment #501024 - Flags: feedback?(21) → feedback+
Attached patch patch 1Splinter Review
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.
Attachment #501024 - Attachment is obsolete: true
Attachment #501035 - Flags: review?(mbrubeck)
Attachment #501035 - Flags: review?(21)
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.
Attachment #501035 - Flags: review?(mbrubeck) → review+
pushed with looped-getter for browser chrome scripts

http://hg.mozilla.org/mobile-browser/rev/4ac6be99537d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #501324 - Flags: review?(21)
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 :)
Attached patch patch 2 v2Splinter Review
Now with common-ui.js
Attachment #501324 - Attachment is obsolete: true
Attachment #501327 - Flags: review?(21)
Attachment #501324 - Flags: review?(21)
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
Attachment #501327 - Flags: review?(21) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/7d47dd39deaf

I'll file a new bug for any other work.
Blocks: 622908
So this can be marked as verified fixed, right?
Verified by inspecting the source code.
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.