414802 - misc perf/cleanup for history and bookmarks services

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: fix

- fix straight folder queries to be static
- throw common case up front in QueryFolderChildren()
- remove unused vars
- don't create vars unless being used

no api changes. passes all unit tests.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 2

[Asaf Romano (gone)]

Comment on attachment 300676 [details] [diff] [review]
v2

>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v
>retrieving revision 1.148
>diff -u -8 -p -r1.148 nsNavBookmarks.cpp
>--- toolkit/components/places/src/nsNavBookmarks.cpp	25 Jan 2008 19:01:19 -0000	1.148
>+++ toolkit/components/places/src/nsNavBookmarks.cpp	31 Jan 2008 18:26:15 -0000
>@@ -149,32 +149,31 @@ nsNavBookmarks::Init()
>     getter_AddRefs(mDBFindURIBookmarks));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Construct a result where the first columns exactly match those returned by
>   // mDBGetURLPageInfo, and additionally contains columns for position,
>   // item_child, and folder_child from moz_bookmarks.
>   // Results are kGetInfoIndex_*
> 
>+  // mDBGetChildren: select all children of a given folder, sorted by position
>   nsCAutoString selectChildren(
>     NS_LITERAL_CSTRING("SELECT h.id, h.url, COALESCE(a.title, h.title), "
>       "h.rev_host, h.visit_count, "
>       SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
>       ", f.url, null, a.id, "
>       "a.dateAdded, a.lastModified, "
>       "a.position, a.type, a.fk "
>      "FROM moz_bookmarks a "
>      "LEFT JOIN moz_places h ON a.fk = h.id "
>      "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
>-     "WHERE a.parent = ?1 AND a.position >= ?2 AND a.position <= ?3"
>+     "WHERE a.parent = ?1 "
>      " ORDER BY a.position"));
> 
>-  // mDBGetChildren: select all children of a given folder, sorted by position
>-  rv = dbConn->CreateStatement(selectChildren,
>-                               getter_AddRefs(mDBGetChildren));
>+  rv = dbConn->CreateStatement(selectChildren, getter_AddRefs(mDBGetChildren));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // mDBFolderCount: count all of the children of a given folder
>   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("SELECT COUNT(*) FROM moz_bookmarks WHERE parent = ?1"),
>                                getter_AddRefs(mDBFolderCount));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("SELECT position FROM moz_bookmarks WHERE id = ?1"),
>@@ -1451,20 +1450,16 @@ nsNavBookmarks::RemoveFolderChildren(PRI
> 
>   nsTArray<PRInt64> folderChildren;
>   nsTArray<PRInt64> itemChildren; // bookmarks / separators
>   nsresult rv;
>   {
>     mozStorageStatementScoper scope(mDBGetChildren);
>     rv = mDBGetChildren->BindInt64Parameter(0, aFolder);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBGetChildren->BindInt32Parameter(1, 0);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBGetChildren->BindInt32Parameter(2, PR_INT32_MAX);
>-    NS_ENSURE_SUCCESS(rv, rv);
> 
>     PRBool hasMore;
>     while (NS_SUCCEEDED(mDBGetChildren->ExecuteStep(&hasMore)) && hasMore) {
>       PRInt32 type = mDBGetChildren->AsInt32(kGetChildrenIndex_Type);
>       if (type == TYPE_FOLDER) {
>         // folder
>         folderChildren.AppendElement(
>             mDBGetChildren->AsInt64(nsNavHistory::kGetInfoIndex_ItemId));
>@@ -1991,51 +1986,60 @@ nsNavBookmarks::QueryFolderChildren(PRIn
>                                     nsNavHistoryQueryOptions *aOptions,
>                                     nsCOMArray<nsNavHistoryResultNode> *aChildren)
> {
>   mozStorageStatementScoper scope(mDBGetChildren);
> 
>   nsresult rv = mDBGetChildren->BindInt64Parameter(0, aFolderId);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  rv = mDBGetChildren->BindInt32Parameter(1, 0);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  rv = mDBGetChildren->BindInt32Parameter(2, PR_INT32_MAX);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>   PRBool results;
> 
>   nsCOMPtr<nsNavHistoryQueryOptions> options = do_QueryInterface(aOptions, &rv);
>   PRInt32 index = -1;
>   while (NS_SUCCEEDED(mDBGetChildren->ExecuteStep(&results)) && results) {
> 
>     // The results will be in order of index. Even if we don't add a node
>     // because it was excluded, we need to count it's index, so do that
>     // before doing anything else. Index was initialized to -1 above, so
>     // it will start counting at 0 the first time through the loop.
>     index ++;
> 
>     PRInt32 itemType = mDBGetChildren->AsInt32(kGetChildrenIndex_Type);
>     PRInt64 id = mDBGetChildren->AsInt64(nsNavHistory::kGetInfoIndex_ItemId);
>     nsRefPtr<nsNavHistoryResultNode> node;
>-    if (itemType == TYPE_FOLDER || itemType == TYPE_DYNAMIC_CONTAINER) {
>+    if (itemType == TYPE_BOOKMARK) {
>+      rv = History()->RowToResult(mDBGetChildren, options,
>+                                  getter_AddRefs(node));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      PRUint32 nodeType;
>+      node->GetType(&nodeType);
>+      if ((nodeType == nsINavHistoryResultNode::RESULT_TYPE_QUERY &&
>+           aOptions->ExcludeQueries()) ||
>+          (nodeType != nsINavHistoryResultNode::RESULT_TYPE_QUERY &&
>+           nodeType != nsINavHistoryResultNode::RESULT_TYPE_FOLDER &&
>+           nodeType != nsINavHistoryResultNode::RESULT_TYPE_FOLDER_SHORTCUT &&
>+           aOptions->ExcludeItems())) {

just query or folder shortcut,.



>Index: toolkit/components/places/src/nsNavBookmarks.h
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v
>retrieving revision 1.50
>diff -u -8 -p -r1.50 nsNavBookmarks.h
>--- toolkit/components/places/src/nsNavBookmarks.h	9 Jan 2008 03:54:37 -0000	1.50
>+++ toolkit/components/places/src/nsNavBookmarks.h	31 Jan 2008 18:26:15 -0000
>@@ -162,16 +162,17 @@ private:
> 
>   nsresult GetParentAndIndexOfFolder(PRInt64 aFolder, PRInt64* aParent, 
>                                      PRInt32* aIndex);
> 
>   nsresult IsBookmarkedInDatabase(PRInt64 aBookmarkID, PRBool* aIsBookmarked);
> 
>   // kGetInfoIndex_* results + kGetChildrenIndex_* results
>   nsCOMPtr<mozIStorageStatement> mDBGetChildren;
>+  nsCOMPtr<mozIStorageStatement> mDBGetSomeChildren;

unused.

r=mano otherwise.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: for checkin

drivers: very low impact perf and cleanup changes

Comment 4

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.150; previous revision: 1.149
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.130; previous revision: 1.129
done

Comment 5

[Asaf Romano (gone)]

chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug413985-location.replace.js
	FAIL - Timed out - chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug413985-location.replace.js

	FAIL - Visit count is what we expect - Got 0, expected 1 - chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug413985-location.replace.js

	PASS - Load count is greater than 1

backing this out fixed that.

Comment 6

[Asaf Romano (gone)]

It might not actually, but this is still out, please re-land this after the tree get its sanity back.

Comment 7

[Dietrich Ayala (:dietrich)]

relanded:

Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.152; previous revision: 1.151
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.132; previous revision: 1.131
done

Comment 8

[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