420078 - Optimize checking whether item is not a livemark

Status: RESOLVED DUPLICATE of bug 492796

(Firefox :: Bookmarks & History, defect)

Description

[Ondrej Brablc]

At the moment nsILivemarkService::isLivemark is very expensive, because it does request to the database. However, it already keep a list of all livemark items. We should use this list instead and use this function everywhere instead of reading annotation from database.

The patch should be based on Mano's attachment 305609 [details] [diff] [review] in bug 399566. It will improve performance for bug 399566, bug 405765, bug 417042, bug 417729 and bug 416988.

The solution should pay attention not to introduce a regression (see bug 398300).

Comment 1

[Marco Bonardo [:mak]]

true, we should not rely on livemark service until it is started, and that is in delayed startup (after 5s), in that time we should use the anno

Comment 2

[Ondrej Brablc]

Attachment: Utilize livemarks internal cache

This patch reduces number of database request when asking whether a node or item id is a livemark. On all places where direct reading from database has been used, _delayedIsLivemark function is used, which checks whether to use livemark service or database. This means that we do not delay anything on startup, but once we initialize the livemark service, every detection will use it.

I have tested it with import and number of SQL statements decreased from 102000 to 93000.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 306299 [details] [diff] [review]
Utilize livemarks internal cache

>@@ -401,40 +403,60 @@ var PlacesUtils = {
>       return true;
>     return false;
>   },
> 
>  /**
>   * Determines whether a result node is a remote container registered by the
>   * livemark service.
>   * @param aNode
>   *        A result Node
>   * @returns true if the node is a livemark container item
>   */
>   nodeIsLivemarkContainer: function PU_nodeIsLivemarkContainer(aNode) {
>-    // Use the annotations service directly to avoid instantiating
>-    // the Livemark service on startup. (bug 398300)
>-    return this.nodeIsFolder(aNode) &&
>-           this.annotations.itemHasAnnotation(aNode.itemId, LMANNO_FEEDURI);
>+    return this.nodeIsFolder(aNode) && this._delayedIsLivemark(aNode.itemId);
>+  },
>+
>+  _delayedIsLivemark: function PU__delayedIsLivemark(aItemId) {
>+    // Avoid service instantiating on startup (bug 398300) but
>+    // use if already exists.
>+    return (this._livemarksInitialized
>+            ?this.livemarks.isLivemark(aItemId)
>+            :this.annotations.itemHasAnnotation(aItemId, LMANNO_FEEDURI));

there's no need to worry about starting up the service early. the udpate timer (which is what we don't want to start up early) won't start until livemarks.start() is called.

you could remove this method, and instead just use livemarks.isLivemark() directly.

>+
>+ /**
>+  * Determines whether a result node is a tag container
>+  * @param aNode
>+  *        A result node
>+  * @returns true if the node is a tag container
>+  */
>+  nodeIsTagContainer: function PU_nodeIsTagContainer(aNode) {
>+    if (aNode.parent && this.nodeIsFolder(aNode.parent))
>+      return (aNode.parent.itemId == this.tagsFolderId);
>+    else
>+      return this.bookmarks.getFolderIdForItem(aNode.itemId) == 
>+             this.tagsFolderId;
>+  },

please spin this off into a new bug

also:

>+    if (aNode.parent && this.nodeIsFolder(aNode.parent))
>+      return (aNode.parent.itemId == this.tagsFolderId);

this should all be in a single if() block, since a consumer could presumable query directly for a tag node via it's id, and the node would therefore have no parent, but still be a tag container node. in that instance we'd want to check the bookmarks service directly (fall through to the |else|).

>-
>-  getSiteURI: function LS_getSiteURI(container) {
>-    this._ensureLivemark(container);
> 

nit: while you're there, please fix the the parameter to be more descriptive, eg: aContainerId or something like that (and for the others as well)

Comment 4

[Asaf Romano (gone)]

You should also optimize the RESULT_BY_TAG case.

Comment 5

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

This will not block the final release of Firefox 3.

Comment 6

[Ondrej Brablc]

Attachment: Use livemark cache, keep livemarks service delayed

So I have carefully checked what is going on and when and my patch contains only the necessary changes to use the livemark cache whenever possible without delaying start.

When the livemark service gets initialized it builds the cache, what does N+1 SQL statements where N is the number of livemarks. This happens when bookmark toolbar initialization from delayed startup calls nodeIsLivemarkContainer.

The check in this function delays the startup of the livemark service until the update timer fires. This is what bug 398300 requested. When the service is initialized, even this function will utilize the cache.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 306550 [details] [diff] [review]
Use livemark cache, keep livemarks service delayed


>@@ -401,28 +403,31 @@ var PlacesUtils = {
>       return true;
>     return false;
>   },
> 
>  /**
>   * Determines whether a result node is a remote container registered by the
>   * livemark service.
>   * @param aNode
>   *        A result Node
>   * @returns true if the node is a livemark container item
>   */
>   nodeIsLivemarkContainer: function PU_nodeIsLivemarkContainer(aNode) {
>-    // Use the annotations service directly to avoid instantiating
>-    // the Livemark service on startup. (bug 398300)
>-    return this.nodeIsFolder(aNode) &&
>-           this.annotations.itemHasAnnotation(aNode.itemId, LMANNO_FEEDURI);
>+    if (!this.nodeIsFolder(aNode))
>+      return false;
>+
>+    // Avoid service instantiating on startup (bug 398300)
>+    return (this._livemarksInitialized
>+            ?this.livemarks.isLivemark(aNode.itemId)
>+            :this.annotations.itemHasAnnotation(aNode.itemId, LMANNO_FEEDURI));

yep, you're certainly right about avoiding the full livemark population query on startup. sorry for my incorrect advice to do the opposite. given this, i think we should revert back to an optimized _itemIsLivemark(aItemId) internally, like you previously had implemented. that would be better than having methods like getMostRecentBookmarkForURI() talk to anno service or livemark service directly.

nit: always spaces before/after operators

>@@ -1595,25 +1600,25 @@ var PlacesUtils = {
>   /**
>    * Get all bookmarks for a URL, excluding items under tag or livemark
>    * containers.
>    */
>   getBookmarksForURI:
>   function PU_getBookmarksForURI(aURI) {
>     var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI, {});
> 
>     // filter the ids list
>     return bmkIds.filter(function(aID) {
>       var parent = this.bookmarks.getFolderIdForItem(aID);
>       // Livemark child
>-      if (this.annotations.itemHasAnnotation(parent, LMANNO_FEEDURI))
>+      if (this.livermarks.isLivemark(parent))

s/liver/live/

and elsewhere

> 
>-  isLivemark: function LS_isLivemark(folder) {
>-    return this._ans.itemHasAnnotation(folder, LMANNO_FEEDURI);
>+  isLivemark: function LS_isLivemark(aFolderId) {
>+    if (aFolderId == -1)
>+      return false;
>+    for (var i=0; i < this._livemarks.length; ++i) {
>+      if (this._livemarks[i].folderId == aFolderId)
>+        return true;
>+    }
>+    return false;
>   },
> 
>-  _ensureLivemark: function LS__ensureLivemark(container) {
>-    if (!this.isLivemark(container))
>+  getSiteURI: function LS_getSiteURI(aContainerId) {
>+    if (!this.isLivemark(aContainerId))
>       throw Cr.NS_ERROR_INVALID_ARG;
>-  },

bug 419832 ended up making these changes, so please remove from this patch.

>+  getFeedURI: function LS_getFeedURI(aContainerId) {
>+    try {
>+      var livemarkIndex = this._getLivemarkIndex(aContainerId);
>+      return this._livemarks[livemarkIndex].feedURI;
>+    }
>+    catch (e) {
>+    }
>     return null;
>   },
> 

see the discussion in bug 419832 about catching exceptions here

Comment 8

[Marco Bonardo [:mak]]

Michael, sorry for that, but Bug 419832 wil unbitrot this :(

Comment 9

[Marco Bonardo [:mak]]

Ops, bad c&p, s/Michael/Ondrej/

Comment 10

[Ondrej Brablc]

This bug blocks blocking bug 405765. We should have this fixed first and then check whether the problem is still there.

Comment 11

[Mike Connor [:mconnor]]

Not a blocker in itself.  If it fixes the other bug, great, we should take the patch there...

Comment 12

[Dietrich Ayala (:dietrich)]

bah. fixed in bug 492796.

Comment 13

[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