misc perf/cleanup for history and bookmarks services

RESOLVED FIXED in Firefox 3 beta4

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

Trunk
Firefox 3 beta4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 300319 [details] [diff] [review]
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.
Attachment #300319 - Flags: review?(mano)
(Assignee)

Comment 1

11 years ago
Created attachment 300676 [details] [diff] [review]
v2
Assignee: nobody → dietrich
Attachment #300319 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #300676 - Flags: review?(mano)
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.
Attachment #300676 - Flags: review?(mano) → review+
(Assignee)

Comment 3

11 years ago
Created attachment 300698 [details] [diff] [review]
for checkin

drivers: very low impact perf and cleanup changes
Attachment #300676 - Attachment is obsolete: true
Attachment #300698 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Target Milestone: --- → Firefox 3 beta4
(Assignee)

Updated

11 years ago
Blocks: 384370

Updated

11 years ago
Attachment #300698 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 4

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It might not actually, but this is still out, please re-land this after the tree get its sanity back.
(Assignee)

Comment 7

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.