Closed Bug 626457 Opened 11 years ago Closed 11 years ago

Move some code out of Browser.startup();

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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; ...
Attachment #504514 - Flags: review?(mark.finkle)
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
Attachment #504514 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.2 (obsolete) — Splinter Review
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
Attachment #504514 - Attachment is obsolete: true
Attachment #504771 - Flags: review?(mark.finkle)
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 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.
Attachment #504771 - Flags: review?(mark.finkle) → review+
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!
Attachment #504771 - Flags: review+ → review-
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.
Attached patch Patch v0.3 (obsolete) — Splinter Review
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.
Attachment #504771 - Attachment is obsolete: true
Attachment #505083 - Flags: review?(mark.finkle)
Comment on attachment 505083 [details] [diff] [review]
Patch v0.3

I'm seeing an error again..
Attachment #505083 - Flags: review?(mark.finkle)
Attached patch Patch v0.3 (obsolete) — Splinter Review
I've forgot to Cu.import Geometry.jsm in bindings/browser.js! Corrected now (and also removed loadFrameScript(forms.js) from FormUI.init
Assignee: nobody → 21
Attachment #505083 - Attachment is obsolete: true
Attachment #505088 - Flags: review?(mark.finkle)
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.
Severity: normal → enhancement
Priority: -- → P1
Severity: enhancement → normal
Attached patch PatchSplinter Review
The patch addressed comments and remove all the changes done in xul that seems to slow down the browser.
Attachment #505088 - Attachment is obsolete: true
Attachment #506451 - Flags: review?(mark.finkle)
Attachment #505088 - Flags: review?(mark.finkle)
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.
Attachment #506451 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/494b9a91cd90
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Can somebody verify this 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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.