626457 - Move some code out of Browser.startup();

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

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

Attachment: Patch

Using a simple Date.now(); method to calculate things, the patch turn Browser.startup to move from ~170ms with a remote page to ~50ms on my desktop machine. 
The debug refactoring is just because it was making me sad each time i see cont a = 65; ...

Comment 1

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

Comment on attachment 504514 [details] [diff] [review]
Patch


>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   _createBrowser: function _createBrowser(aURI, aInsertBefore) {

>+    let notification = null, browser = null;
>+    let isRemote = Services.prefs.getBoolPref("browser.tabs.remote") && !Util.isLocalScheme(aURI);
>+    let browsers = Elements.browsers;
>+    if (browsers.hasAttribute("status"))  {

Only care about the locale case (see below), so we could change this to:

      if (!isRemote && browsers.hasAttribute("status")) {

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+            <vbox class="tabs-list" anonid="tabs-children" flex="1">
>+              <documenttab selected="true"/>
>+            </vbox>
>+            <box class="tabs-list" anonid="tabs-undo"/>
>+          </vbox>

anonid -> id (I'm not sure we even need these IDs anymore)

>-                <deck id="browsers" flex="1"/>
>+                <deck id="browsers" flex="1" status="uninitialized">
>+                  <notificationbox class="inputHandler">
>+                    <browser type="content" class="viewable-width viewable-height" remote="false"/>
>+                    <browser type="content" class="viewable-width viewable-height" remote="true"/>
>+                  </notificationbox>
>+                </deck>

I don't think it's worth adding the <browser remote="true"/> case. The networking overhead involved with a remote page means it won't ever load really fast. I think we should optimize for the local start page instead. Also, I fear the <browser remote="true"/>, even though it might not be used, will still start the child process before we can remove it in Tab._createBrowser, which will slow things down for the local case.

I like this approach, but I don't like worrying about remote startup. Also, how much of a savings is this for local startpage?

r- for now, but could be r+ soon

Comment 2

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

Attachment: Patch v0.2

Addressed comments.

> I like this approach, but I don't like worrying about 
> remote startup. Also, how much of a savings is this
> for local startpage?

I've tried with the default Fennec Start page.

With the patch:
 * Browser.startup: 43/47/47/48/47
 * UIReadyDelayed event: 271/234/234/237/234

The first  UIReadyDelayed time (271) correspond to a cold start, so the overhead could be because the common-ui.js/SelectHelperUI.js/MenuListHelper.js files are parsed at this time.

Without the patch:
 * Browser.startup: 180/182/180/179/179
 * UIReadyDelayed event: 189/190/189/187/188

We could observe that around 50ms of Browser.startup is done during the delayed UIReady event. A few others ms are won because the TabOpen listener is added later so we don't loose time at startup to compute tabs size, while the rest is saved by adding the elements inside the XUL directly

Comment 3

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

By the way, I'm running a debug build so things could be completely different with an optimized build. I'm curious to see the effect on Ts

Comment 4

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

Comment on attachment 504771 [details] [diff] [review]
Patch v0.2


>diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js

>   init: function formHelperInit() {
>+    messageManager.loadFrameScript("chrome://browser/content/forms.js", true);
>+

I don't think we want to do this. First, we are still loading this in Browser.startup() and second, we want to make sure the script is in all browser contexts when they are loaded. The last param (allowDelayedLoad) might not load the script into an existing browser, only newly opened browsers.

r+, with the above removed

let's wait the nightly respin to complete before landing this.

Comment 5

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

Comment on attachment 504771 [details] [diff] [review]
Patch v0.2

Hold on! I see a few errors in the console:

Error: Util is not defined
Source File: chrome://browser/content/bindings/browser.js
Line: 359

Error: uncaught exception: Not Loading!

Comment 6

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

I think the bindings/browser.js is executing too quickly and "Util" is not loaded yet.

Let's copy the code out of Util anyway. bindings/browser.js should not depend on front-end code.

Comment 7

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

Attachment: Patch v0.3

I've removed the getScrollOffset method from Util and add it to ContentScroll instead and used it everywhere in the content process. (Util.getScrollOffset) was unused in the chrome process.

For the "Not Loading" error this is misleading because the browser was loading about:blank in fact. So I've added a this._loading = true check when we reused the existing browser.

Comment 8

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

Comment on attachment 505083 [details] [diff] [review]
Patch v0.3

I'm seeing an error again..

Comment 9

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

Attachment: Patch v0.3

I've forgot to Cu.import Geometry.jsm in bindings/browser.js! Corrected now (and also removed loadFrameScript(forms.js) from FormUI.init

Comment 10

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

I tested this patch on my N900 running Talos Ts tests. Average of 3 runs:
without: 6367ms
with:    6439ms

we could theorize that the <browser> is being initialized more than we think. When create a browser in JS code, we stop loading it as fast as possible. Maybe the uninitialized <browser> is doing too much work behind the scenes. I don't really know, but the results of Ts surprised me.

So of this patch is definitely worth landing:
* moving login mgr init and the TabOpen / TabSelect and the browsersearch pieces
* removing Util.xxx from bindings/browser.js
* memoizing the _formAssistant in content.js

but I'm not sure we want to make the tabs.xml, Tab._createBrowser and browser.xul changes until we better understand whats going on.

Comment 11

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

Attachment: Patch

The patch addressed comments and remove all the changes done in xul that seems to slow down the browser.

Comment 12

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

Comment on attachment 506451 [details] [diff] [review]
Patch


>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

>+
>+Cu.import("resource://gre/modules/Geometry.jsm");

Let's not do this. We Don't really need the Point class in this file. We can fake it, I think. See below:

>+  getScrollOffset: function(aWindow) {
>+    let cwu = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+    let scrollX = {}, scrollY = {};
>+    cwu.getScrollXY(false, scrollX, scrollY);
>+    return new Point(scrollX.value, scrollY.value);
>+  },

I think we can just use:
      return { x: scrollX.value, y: scrollY.value };

I don't think we explicitly need the Point. Try it and see.

Comment 13

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

http://hg.mozilla.org/mobile-browser/rev/494b9a91cd90

Comment 14

[Andreea Pod]

Can somebody verify this please?

Comment 15

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

You don't have any easy way to verify that except by looking at the source code.
But this has been done :) (Sorry I can't mark the bug as "Verified" myself)