Loading bookmarks into the bookmark list is slow

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
x86
Mac OS X

Details

Attachments

(3 attachments)

Loading 50 bookmarks is taking around 20 seconds (on n810)
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.
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.
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
Created attachment 366073 [details]
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.
Drat, the first profile row got stuck to the header row in the profile
Created attachment 367768 [details] [diff] [review]
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.
Assignee: nobody → mark.finkle
Attachment #367768 - Flags: review?(gavin.sharp)
Created attachment 367769 [details]
profile of loading 10 bookmarks with the speedup patch

Here is a profile of loading bookmarks with the patch applied.
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?
Attachment #367768 - Flags: review?(gavin.sharp) → review+
(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
http://hg.mozilla.org/mobile-browser/rev/89326eb6a992
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.