405765 - Opening folder in the right pane of the Places Organizer is slow

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P2)

Description

[Ryuichi KUBUKI]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112705 Minefield/3.0b2pre ID:2007112705

Opening folder in the right pane of the Places Organizer is slow (around 5 seconds for a relatively large folder).

It's OK in the left pane, you can open it immediately as it was in the old Places.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112810 Minefield/3.0b2pre

Slower than the Bookmarks-menu but less than 1 second here.

Comment 2

[Ryuichi KUBUKI]

Forgot to add the size of places.sqlite, it's 23MB. While opening a folder in the right pane all windows freeze.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122308 Minefield/3.0b3pre ID:2007122308

Comment 3

[Marco Bonardo [:mak]]

what folder? a self created one or one of the default folders?
how many items are in that folder?

what are your system spec? is still 5 seconds in the latest nightly?

Comment 4

[Ryuichi KUBUKI]

(In reply to comment #3)
> what folder? a self created one or one of the default folders?
> how many items are in that folder?
> 
> what are your system spec? is still 5 seconds in the latest nightly?
> 

Basically every folder.
It takes time regardless of the number of items in it, right now it takes 5 sec for a folder with 5 items on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122715 Minefield/3.0b3pre ID:2007122715

The CPU is Athlon 64 X2 4800+.

Comment 5

[Ryuichi KUBUKI]

Sorry, it seems the default folders such as Bookmarks Toolbar and the top level of Bookmarks Menu seem responsive enough. However, self-created ones in Bookmarks Menu and the top level of Unified Bookmarks are slow.

Comment 6

[Mike Schroepfer]

Dietrich can you find the right owner?  Or maybe Ondrej wants to take this?

Comment 7

[Ondrej Brablc]

I'm waiting for review of 385245 and have time for this.

Comment 8

[Ondrej Brablc]

I can see again the very same problems with annotations which are repeatedly read for all items and even for their parents.

For instance:

  nodeIsReadOnly: function PU_nodeIsReadOnly(aNode) {
    NS_ASSERT(aNode, "null node");

    if (this.nodeIsFolder(aNode))
      return this.bookmarks.getFolderReadonly(asQuery(aNode).folderItemId);
    if (this.nodeIsQuery(aNode))
      return asQuery(aNode).childrenReadOnly;
    return false;
  },

This always does lookup in the database - result is huge number of database queries. Similar to 399566, but now the annotation is READ_ONLY_ANNO and it is requested from the parent (not the child).

Comment 9

[Ryuichi KUBUKI]

A bit quicker now on this build but still have to wait for 2-3 seconds.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041017 Minefield/3.0pre ID:2008041017

Comment 10

[Ondrej Brablc]

Attachment: Optimize livemark checking

This patch solves part of the problem. The remaining part seems to be checking whether a folder is not read only and may be something more. I will check whether this could be optimized too. With my current configuration I can see a 1 second delay when clicking on the item on the right side.

Comment 11

[Ondrej Brablc]

When I click on a folder in the left pane it does following number of annotation queries:

00002=>3
00091=>9
00109=>7

PlacesOrganizer/OrganizerQuery=>7
placesInternal/READ_ONLY=>12

That is already a lot, you can see, that we ask for two annotation on three nodes. And we do 19! queries instead of 6.

And now when we click on it in the right pane:

00002=>6
00091=>121
00109=>150

PlacesOrganizer/OrganizerQuery=>114
bookmarkProperties/description=>1
bookmarks/staticTitle=>1
placesInternal/READ_ONLY=>161

We should need up to 3x4 = 12 queries and we perform 277 queries. That is 14 times as much as if we clicked on the item on the left side. 

I will analyze this deeper, but it looks like we should be caching the result of annotation queries. However, it should be investigated why we need so many queries when clicking on the item on the right side.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #11)
> I will analyze this deeper, but it looks like we should be caching the result
> of annotation queries. However, it should be investigated why we need so many
> queries when clicking on the item on the right side.

how many bookmarks/folders are there in the right pane root (not only visible, full tree hierarchy)? left side is an excludeItems list, items are excluded in the backend... right side is a flatList, IIRC items are excluded in the frontend, so we could still ask annotation also for hidden items?

Comment 13

[Ondrej Brablc]

(In reply to comment #12)
> (In reply to comment #11)
> > I will analyze this deeper, but it looks like we should be caching the result
> > of annotation queries. However, it should be investigated why we need so many
> > queries when clicking on the item on the right side.
> 
> how many bookmarks/folders are there in the right pane root (not only visible,
> full tree hierarchy)? left side is an excludeItems list, items are excluded in
> the backend... right side is a flatList, IIRC items are excluded in the
> frontend, so we could still ask annotation also for hidden items?

I do not think this is related to the problem. The list of nodes that annotations are read for is the same if I click on the folder in the left pane or in the right pane. 

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 315858 [details] [diff] [review]
Optimize livemark checking


>
>   isLivemark: function LS_isLivemark(aFolderId) {
>-    return this._ans.itemHasAnnotation(aFolderId, LMANNO_FEEDURI);
>+    if (aFolderId == -1)
>+      return false;
>+    for (var i=0; i < this._livemarks.length; ++i) {
>+      if (this._livemarks[i].folderId == aFolderId)
>+        return true;
>+    }
>+    return false;
>   },

you could use _getLivemarkIndex() here, instead of duplicating that loop in this method.

>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v

can you confirm that these changed methods are not called during startup? if they are, maybe you should add back in the _livemarksInitialized approach in your first patch to bug 420078, in order to not regress bug 398300.

Comment 15

[Ondrej Brablc]

Attachment: Reduce number of repeated SQL statements (wip)

This patch reduced number of requests for read only and organizer queries. So maybe about 200 queries less, but it still does 32 queries which seem to be slow. So further optimization is needed.

(In reply to comment #14)
> (From update of attachment 315858 [details] [diff] [review])
> ..
> you could use _getLivemarkIndex() here, instead of duplicating that loop in
> this method.

This was an optimization on my side, I wanted to avoid exception throwing and catching, what would happen in 99% of cases.
 
> ..
> can you confirm that these changed methods are not called during startup? if
> they are, maybe you should add back in the _livemarksInitialized approach in
> your first patch to bug 420078, in order to not regress bug 398300.

I think the second patch in bug 420078 solved this on the necessary place and I have now fixed it in this patch and hopefully a little more elegantly.

Comment 16

[Asaf Romano (gone)]

The reason for this slowness is actually the endless rebuilds caused by leftPane.selectPlaceURI, a rebuilt is caused by each container-opening.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Could we employ a chunking strategy like what is used in the Download Manager to get instant response here, even if opening the full list takes a long time?

Or an expedient fix that shows some form of progress indication so that the user knows that something's happening.

Comment 18

[Dietrich Ayala (:dietrich)]

(In reply to comment #17)
> Could we employ a chunking strategy like what is used in the Download Manager
> to get instant response here, even if opening the full list takes a long time?

that's high on the list for 3.next

> 
> Or an expedient fix that shows some form of progress indication so that the
> user knows that something's happening.
> 

this would likely not be expedient, and would require UI, string changes, etc.

mano: do you have a strategy for resolving comment #16? also, is there a reason why we don't want to take at least some of the changes from Ondrej's patch? i haven't read through the read-only caching bit, but the livemark checking optimizations look good.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

We'll take a patch here, but we're not hearing enough complaints for this to block, which makes it a good candidate for stability and optimization in a branch release.

Marking wanted, not-blocking, and adding [3.0.1] to the whiteboard.

Comment 20

[jigar shah]

Don't know if the same issue. but even clicking folder inside Places Organizer is also very slow.  And i dont have much data there still (~11MB).

Comment 21

[Dietrich Ayala (:dietrich)]

not duping this against bug 490664 because we should check the log attached to this bug to look for additional optimization opportunities.

Comment 22

[Asaf Romano (gone)]

How bad is this on trunk?

Comment 23

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 24

[Marco Bonardo [:mak]]

I don't think this is still slow, mostly thanks to bug 472343, but also other improvements. The data here is outdated so that a new bug would be needed even we'd still see some slowdown (reasons could be completely different).