481986 - Loading bookmarks into the bookmark list is slow

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

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

Loading 50 bookmarks is taking around 20 seconds (on n810)

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I have noticed similar results with 10 bookmarks.  I have seen it a few times where the first three bookmarks load and the rest load later (10-30 seconds).  I think it is always when I have multiple tabs open and the other tab is loading or refreshing.

Comment 2

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

Some background: The places calls themselves are a little slow, but not slow enough for the delays we are seeing. Adding each "bookmark" item (creating the XBL element, loading its values, appending to the richlistbox) is probably the main cause.

We could look at adding items as a single entity using DocumentFragment. We should also look for just dumb coding too.

Comment 3

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

I just added 11 bookmarks to b1rc2. They display in less than 3 secs. Sometimes the first 4-5 appear first, but all appear in less than 3 secs.

I'm not saying we have no speed problems, just that I don't see major speed issues. I'm sure 50 bookmarks cause more problems.

Profiles coming up

Comment 4

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

Attachment: profile of loading 10 bookmarks

This is a simple JS profile of loading 10 bookmarks into the bookmark display list. Some observations (I'm looking mostly at "Own Time"):

* Total time for the action is 1678 ms
* appendItem is the slowest method at 591 ms (35%). Called 10 times.
* Setting the bookmark/folder name into the readonly textbox is a close second at 520 ms.
* Pulling some data from the Places API for each bookmar/folder is third.

Comment 5

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

Drat, the first profile row got stuck to the header row in the profile

Comment 6

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

Attachment: patch

This patch makes some improvements to the load speed.

* Reduce the need to use the Places API to retrieve data we already have.
* Use xbl:inherit when possible.
* Small cleanups
* Small style tweaks

The first 2 changes seem to give about 38% faster loading. I did try using a DocumentFragment to make appending the placeitems faster, but didn't see any measurable improvements.

Comment 7

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

Attachment: profile of loading 10 bookmarks with the speedup patch

Here is a profile of loading bookmarks with the patch applied.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 367768 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+            if (this.hasAttribute("uri")) {
>+              let ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);

Probably would help to cache this service somehow? Happens elsewhere in this file as well (nsIFaviconService, nsIStringBundleService).

>       <method name="addFolder">

>+              children.insertBefore(child, children.lastChild)

why insertBefore lastChild?

>+              items.appendChild(this.createItem(node, aLevel));

>+              let child = this.createItem(node, aLevel);
>+              items.appendChild(child);

No need for the child temporary, right?

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

> .bookmark-folder textbox[readonly="true"], .bookmark-item textbox[readonly="true"] {

>-  padding: 0px 9px !important;
>-  margin: 0px !important;
>+  /*padding: 0px 9px !important;*/
>+  /*margin: 0px !important;*/

What's this about?

Comment 9

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

(In reply to comment #8)
> (From update of attachment 367768 [details] [diff] [review])
> >+            if (this.hasAttribute("uri")) {
> >+              let ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
> 
> Probably would help to cache this service somehow? Happens elsewhere in this
> file as well (nsIFaviconService, nsIStringBundleService).

Done.

> >+              children.insertBefore(child, children.lastChild)
> 
> why insertBefore lastChild?

We have an "Add new folder" row/button at the very bottom

> >+              items.appendChild(this.createItem(node, aLevel));
> 
> >+              let child = this.createItem(node, aLevel);
> >+              items.appendChild(child);
> 
> No need for the child temporary, right?

Right. Removed.

> > .bookmark-folder textbox[readonly="true"], .bookmark-item textbox[readonly="true"] {
> 
> >-  padding: 0px 9px !important;
> >-  margin: 0px !important;
> >+  /*padding: 0px 9px !important;*/
> >+  /*margin: 0px !important;*/
> 
> What's this about?

Left-over testing. We didn't need it so I removed it from hildon and wince

Comment 10

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

http://hg.mozilla.org/mobile-browser/rev/89326eb6a992