492805 - GetHasChildren executes too many queries

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

is really the tag check. use the tagging service instead.

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 377216 [details] [diff] [review]
v1

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -2176,37 +2176,39 @@ nsNavHistoryQueryResultNode::GetHasChild
>   if (!CanExpand()) {
>     *aHasChildren = PR_FALSE;
>     return NS_OK;
>   }
> 
>   PRUint16 resultType = mOptions->ResultType();
>   // For tag containers query we must check if we have any tag
>   if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) {
>-    nsNavHistory* history = nsNavHistory::GetHistoryService();
>-    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
>-    mozIStorageConnection *dbConn = history->GetStorageConnection();
>-
>-    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>-    NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>-    PRInt64 tagsFolderId;
>-    nsresult rv = bookmarks->GetTagsFolder(&tagsFolderId);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    nsCOMPtr<mozIStorageStatement> hasTagsStatement;
>-    rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
>-        "SELECT id FROM moz_bookmarks WHERE parent = ?1 LIMIT 1"),
>-      getter_AddRefs(hasTagsStatement));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    rv = hasTagsStatement->BindInt64Parameter(0, tagsFolderId);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    rv = hasTagsStatement->ExecuteStep(aHasChildren);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>+
>+    nsresult rv;
>+    nsCOMPtr<nsITaggingService> taggingService =
>+      do_GetService(TAGGING_SERVICE_CID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsIVariant *tagList;
>+    rv = taggingService->GetAllTags(&tagList);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRUint16 dataType;
>+    tagList->GetDataType(&dataType);
>+    NS_ENSURE_TRUE(dataType == nsIDataType::VTYPE_ARRAY, NS_ERROR_ILLEGAL_VALUE);

when this could happen? if it's just for debugging/sanity could be an assertion?

we should measure cost of this xpcom/nsIVariant traverse, does still make sense to cache this in the resultNode? Maybe we should still file a bug about "investigate if we can cache hasChildren for query nodes", since that also hits history nodes.

Comment 2

[Dietrich Ayala (:dietrich)]

this patch is slower than the sql queries, by a significant amount.

let's use this bug to cover the cache approach, no need to open a new one.

Comment 3

[Dietrich Ayala (:dietrich)]

another approach could cache the number of tag folders on a per-result basis, and use a bookmark observer to decrement the number.

it's a bit more complex, but better than fetching these values each and every time a node is evaluated in each and every tree in the browser.

Comment 4

[Marco Bonardo [:mak]]

oops I accidentally fixed this with bug 707946.