Closed Bug 481986 Opened 12 years ago Closed 12 years ago

Loading bookmarks into the bookmark list is slow

Categories

(Firefox for Android Graveyard :: Bookmarks, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(3 files)

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
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
Attached patch patchSplinter Review
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)
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
Closed: 12 years ago
Resolution: --- → FIXED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.