From 11be4ff865921b69d86b2ac96cffba3d543ec881 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Mon, 9 Aug 2010 17:59:43 +0200 Subject: [PATCH] Bug 572030 - Use a memory cache for keywords associations. r=dietrich a=sdwilsh --- .../components/places/src/nsNavBookmarks.cpp | 264 ++++++++++-------- .../components/places/src/nsNavBookmarks.h | 18 +- .../places/tests/bookmarks/test_keywords.js | 125 +++++++++ 3 files changed, 293 insertions(+), 114 deletions(-) create mode 100644 toolkit/components/places/tests/bookmarks/test_keywords.js diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp index b70fa08d2b5..c1cbaa7dabf 100644 --- a/toolkit/components/places/src/nsNavBookmarks.cpp +++ b/toolkit/components/places/src/nsNavBookmarks.cpp @@ -61,6 +61,8 @@ #include "mozilla/FunctionTimer.h" +#define BOOKMARKS_TO_KEYWORDS_INITIAL_CACHE_SIZE 64 + const PRInt32 nsNavBookmarks::kFindBookmarksIndex_ID = 0; const PRInt32 nsNavBookmarks::kFindBookmarksIndex_Type = 1; const PRInt32 nsNavBookmarks::kFindBookmarksIndex_PlaceID = 2; @@ -94,6 +96,31 @@ PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsNavBookmarks, gBookmarksService) #define GUID_ANNO NS_LITERAL_CSTRING("placesInternal/GUID") #define READ_ONLY_ANNO NS_LITERAL_CSTRING("placesInternal/READ_ONLY") + +namespace { + +struct keywordSearchData +{ + PRInt64 itemId; + nsString keyword; +}; + +PLDHashOperator +SearchBookmarkForKeyword(nsTrimInt64HashKey::KeyType aKey, + const nsString aValue, + void* aUserArg) +{ + keywordSearchData* data = reinterpret_cast(aUserArg); + if (data->keyword.Equals(aValue)) { + data->itemId = aKey; + return PL_DHASH_STOP; + } + return PL_DHASH_NEXT; +} + +} // Anonymous namespace. + + nsNavBookmarks::nsNavBookmarks() : mItemCount(0) , mRoot(0) , mBookmarksRoot(0) @@ -336,12 +363,6 @@ nsNavBookmarks::GetStatement(const nsCOMPtr& aStmt) RETURN_IF_STMT(mDBSetItemIndex, NS_LITERAL_CSTRING( "UPDATE moz_bookmarks SET position = :item_index WHERE id = :item_id")); - // Get keyword text for bookmark id. - RETURN_IF_STMT(mDBGetKeywordForBookmark, NS_LITERAL_CSTRING( - "SELECT k.keyword FROM moz_bookmarks b " - "JOIN moz_keywords k ON k.id = b.keyword_id " - "WHERE b.id = :item_id")); - // Get keyword text for bookmarked URI. RETURN_IF_STMT(mDBGetKeywordForURI, NS_LITERAL_CSTRING( "SELECT k.keyword " @@ -356,19 +377,6 @@ nsNavBookmarks::GetStatement(const nsCOMPtr& aStmt) "JOIN moz_bookmarks b ON b.fk = h.id " "JOIN moz_keywords k ON k.id = b.keyword_id")); - // Get URI for keyword. - RETURN_IF_STMT(mDBGetURIForKeyword, NS_LITERAL_CSTRING( - "SELECT url FROM moz_keywords k " - "JOIN moz_bookmarks b ON b.keyword_id = k.id " - "JOIN moz_places_temp h ON b.fk = h.id " - "WHERE k.keyword = :keyword " - "UNION ALL " - "SELECT url FROM moz_keywords k " - "JOIN moz_bookmarks b ON b.keyword_id = k.id " - "JOIN moz_places h ON b.fk = h.id " - "WHERE k.keyword = :keyword " - "LIMIT 1")); - RETURN_IF_STMT(mDBAdjustPosition, NS_LITERAL_CSTRING( "UPDATE moz_bookmarks SET position = position + :delta " "WHERE parent = :parent " @@ -495,8 +503,6 @@ nsNavBookmarks::FinalizeStatements() { mDBSetItemLastModified, mDBSetItemIndex, mDBGetKeywordForURI, - mDBGetKeywordForBookmark, - mDBGetURIForKeyword, mDBAdjustPosition, mDBRemoveItem, mDBGetLastChildId, @@ -1120,6 +1126,9 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId) NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); rv = history->UpdateFrecency(placeId, IsRealBookmark(placeId)); NS_ENSURE_SUCCESS(rv, rv); + + rv = UpdateKeywordsHashForRemovedBookmark(aItemId); + NS_ENSURE_SUCCESS(rv, rv); } NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, @@ -1688,6 +1697,9 @@ nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId) NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); rv = history->UpdateFrecency(placeId, IsRealBookmark(placeId)); NS_ENSURE_SUCCESS(rv, rv); + + rv = UpdateKeywordsHashForRemovedBookmark(child.itemId); + NS_ENSURE_SUCCESS(rv, rv); } } @@ -2746,96 +2758,95 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex) } +nsresult +nsNavBookmarks::UpdateKeywordsHashForRemovedBookmark(PRInt64 aItemId) +{ + nsAutoString kw; + if (NS_SUCCEEDED(GetKeywordForBookmark(aItemId, kw)) && !kw.IsEmpty()) { + nsresult rv = EnsureKeywordsHash(); + NS_ENSURE_SUCCESS(rv, rv); + mBookmarkToKeywordHash.Remove(aItemId); + } + return NS_OK; +} + + NS_IMETHODIMP nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId, const nsAString& aKeyword) { NS_ENSURE_ARG_MIN(aBookmarkId, 1); - mozStorageTransaction transaction(mDBConn, PR_FALSE); - nsresult rv; - PRInt64 keywordId = 0; - if (!aKeyword.IsEmpty()) { - // Shortcuts are always lowercased internally. - nsAutoString kwd(aKeyword); - ToLowerCase(kwd); - - // Attempt to find a pre-existing keyword record. - nsCOMPtr getKeywordStmnt; - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "SELECT id from moz_keywords WHERE keyword = :keyword"), - getter_AddRefs(getKeywordStmnt)); - NS_ENSURE_SUCCESS(rv, rv); - rv = getKeywordStmnt->BindStringByName(NS_LITERAL_CSTRING("keyword"), kwd); - NS_ENSURE_SUCCESS(rv, rv); - PRBool hasResult; - rv = getKeywordStmnt->ExecuteStep(&hasResult); - NS_ENSURE_SUCCESS(rv, rv); - - if (hasResult) { - rv = getKeywordStmnt->GetInt64(0, &keywordId); - NS_ENSURE_SUCCESS(rv, rv); - } - else { - // Create a new keyword record. - nsCOMPtr addKeywordStmnt; - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "INSERT INTO moz_keywords (keyword) VALUES (:keyword)"), - getter_AddRefs(addKeywordStmnt)); - NS_ENSURE_SUCCESS(rv, rv); - rv = addKeywordStmnt->BindStringByName(NS_LITERAL_CSTRING("keyword"), kwd); - NS_ENSURE_SUCCESS(rv, rv); - rv = addKeywordStmnt->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - - nsCOMPtr idStmt; - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "SELECT id " - "FROM moz_keywords " - "ORDER BY ROWID DESC " - "LIMIT 1"), - getter_AddRefs(idStmt)); - NS_ENSURE_SUCCESS(rv, rv); - - rv = idStmt->ExecuteStep(&hasResult); - NS_ENSURE_SUCCESS(rv, rv); - NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?"); - rv = idStmt->GetInt64(0, &keywordId); - NS_ENSURE_SUCCESS(rv, rv); - } - } - - // Update bookmark record w/ the keyword's id or null. - nsCOMPtr updateKeywordStmnt; - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "UPDATE moz_bookmarks SET keyword_id = :keyword_id, lastModified = :date " - "WHERE id = :item_id"), - getter_AddRefs(updateKeywordStmnt)); + nsresult rv = EnsureKeywordsHash(); NS_ENSURE_SUCCESS(rv, rv); - if (keywordId > 0) { - rv = updateKeywordStmnt->BindInt64ByName(NS_LITERAL_CSTRING("keyword_id"), - keywordId); + + // Shortcuts are always lowercased internally. + nsAutoString keyword(aKeyword); + ToLowerCase(keyword); + + // Check if bookmark was already associated to a keyword. + nsAutoString oldKeyword; + rv = GetKeywordForBookmark(aBookmarkId, oldKeyword); + NS_ENSURE_SUCCESS(rv, rv); + + // Trying to set the same value or to remove a nonexistent keyword is a no-op. + if (keyword.Equals(oldKeyword) || (keyword.IsEmpty() && oldKeyword.IsEmpty())) + return NS_OK; + + mozStorageTransaction transaction(mDBConn, PR_FALSE); + + nsCOMPtr updateBookmarkStmt; + rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "UPDATE moz_bookmarks " + "SET keyword_id = (SELECT id FROM moz_keywords WHERE keyword = :keyword), " + "lastModified = :date " + "WHERE id = :item_id " + ), getter_AddRefs(updateBookmarkStmt)); + NS_ENSURE_SUCCESS(rv, rv); + + if (keyword.IsEmpty()) { + // Remove keyword association from the hash. + mBookmarkToKeywordHash.Remove(aBookmarkId); + rv = updateBookmarkStmt->BindNullByName(NS_LITERAL_CSTRING("keyword")); } - else { - rv = updateKeywordStmnt->BindNullByName(NS_LITERAL_CSTRING("keyword_id")); + else { + // We are associating bookmark to a new keyword. Create a new keyword + // record if needed. + nsCOMPtr newKeywordStmt; + rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "INSERT OR IGNORE INTO moz_keywords (keyword) VALUES (:keyword)" + ), getter_AddRefs(newKeywordStmt)); + NS_ENSURE_SUCCESS(rv, rv); + rv = newKeywordStmt->BindStringByName(NS_LITERAL_CSTRING("keyword"), + keyword); + NS_ENSURE_SUCCESS(rv, rv); + rv = newKeywordStmt->Execute(); + NS_ENSURE_SUCCESS(rv, rv); + + // Add new keyword association to the hash, removing the old one if needed. + if (!oldKeyword.IsEmpty()) + mBookmarkToKeywordHash.Remove(aBookmarkId); + mBookmarkToKeywordHash.Put(aBookmarkId, keyword); + rv = updateBookmarkStmt->BindStringByName(NS_LITERAL_CSTRING("keyword"), keyword); } NS_ENSURE_SUCCESS(rv, rv); PRTime lastModified = PR_Now(); - rv = updateKeywordStmnt->BindInt64ByName(NS_LITERAL_CSTRING("date"), lastModified); + rv = updateBookmarkStmt->BindInt64ByName(NS_LITERAL_CSTRING("date"), + lastModified); NS_ENSURE_SUCCESS(rv, rv); - rv = updateKeywordStmnt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aBookmarkId); + rv = updateBookmarkStmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), + aBookmarkId); NS_ENSURE_SUCCESS(rv, rv); - rv = updateKeywordStmnt->Execute(); + rv = updateBookmarkStmt->Execute(); NS_ENSURE_SUCCESS(rv, rv); rv = transaction.Commit(); NS_ENSURE_SUCCESS(rv, rv); - // Pass the new keyword to OnItemChanged. NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver, OnItemChanged(aBookmarkId, NS_LITERAL_CSTRING("keyword"), - PR_FALSE, NS_ConvertUTF16toUTF8(aKeyword), + PR_FALSE, NS_ConvertUTF16toUTF8(keyword), lastModified, TYPE_BOOKMARK)); return NS_OK; @@ -2872,21 +2883,17 @@ nsNavBookmarks::GetKeywordForBookmark(PRInt64 aBookmarkId, nsAString& aKeyword) NS_ENSURE_ARG_MIN(aBookmarkId, 1); aKeyword.Truncate(0); - DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBGetKeywordForBookmark); - nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), - aBookmarkId); + nsresult rv = EnsureKeywordsHash(); NS_ENSURE_SUCCESS(rv, rv); - PRBool hasMore = PR_FALSE; - rv = stmt->ExecuteStep(&hasMore); - if (NS_FAILED(rv) || ! hasMore) { + nsAutoString keyword; + if (!mBookmarkToKeywordHash.Get(aBookmarkId, &keyword)) { aKeyword.SetIsVoid(PR_TRUE); - return NS_OK; // not found: return void keyword string + } + else { + aKeyword.Assign(keyword); } - // found, get the keyword - rv = stmt->GetString(0, aKeyword); - NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } @@ -2899,24 +2906,57 @@ nsNavBookmarks::GetURIForKeyword(const nsAString& aKeyword, nsIURI** aURI) *aURI = nsnull; // Shortcuts are always lowercased internally. - nsAutoString kwd(aKeyword); - ToLowerCase(kwd); + nsAutoString keyword(aKeyword); + ToLowerCase(keyword); - DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBGetURIForKeyword); - nsresult rv = stmt->BindStringByName(NS_LITERAL_CSTRING("keyword"), kwd); + nsresult rv = EnsureKeywordsHash(); NS_ENSURE_SUCCESS(rv, rv); - PRBool hasMore = PR_FALSE; - rv = stmt->ExecuteStep(&hasMore); - if (NS_FAILED(rv) || ! hasMore) - return NS_OK; // not found: leave URI null + keywordSearchData searchData; + searchData.keyword.Assign(aKeyword); + searchData.itemId = -1; + mBookmarkToKeywordHash.EnumerateRead(SearchBookmarkForKeyword, &searchData); - // found, get the URI - nsCAutoString spec; - rv = stmt->GetUTF8String(0, spec); + if (searchData.itemId == -1) { + // Not found. + return NS_OK; + } + + rv = GetBookmarkURI(searchData.itemId, aURI); NS_ENSURE_SUCCESS(rv, rv); - rv = NS_NewURI(aURI, spec); + + return NS_OK; +} + + +nsresult +nsNavBookmarks::EnsureKeywordsHash() { + if (mBookmarkToKeywordHash.IsInitialized()) + return NS_OK; + + mBookmarkToKeywordHash.Init(BOOKMARKS_TO_KEYWORDS_INITIAL_CACHE_SIZE); + + nsCOMPtr stmt; + nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT b.id, k.keyword " + "FROM moz_bookmarks b " + "JOIN moz_keywords k ON k.id = b.keyword_id " + ), getter_AddRefs(stmt)); NS_ENSURE_SUCCESS(rv, rv); + + PRBool hasMore; + while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) { + PRInt64 itemId; + rv = stmt->GetInt64(0, &itemId); + NS_ENSURE_SUCCESS(rv, rv); + nsAutoString keyword; + rv = stmt->GetString(1, keyword); + NS_ENSURE_SUCCESS(rv, rv); + + rv = mBookmarkToKeywordHash.Put(itemId, keyword); + NS_ENSURE_SUCCESS(rv, rv); + } + return NS_OK; } diff --git a/toolkit/components/places/src/nsNavBookmarks.h b/toolkit/components/places/src/nsNavBookmarks.h index f5238751f63..b0b4b284306 100644 --- a/toolkit/components/places/src/nsNavBookmarks.h +++ b/toolkit/components/places/src/nsNavBookmarks.h @@ -381,8 +381,7 @@ private: nsCOMPtr mDBSetItemLastModified; nsCOMPtr mDBSetItemIndex; nsCOMPtr mDBGetKeywordForURI; - nsCOMPtr mDBGetKeywordForBookmark; - nsCOMPtr mDBGetURIForKeyword; + nsCOMPtr mDBGetBookmarksToKeywords; nsCOMPtr mDBAdjustPosition; nsCOMPtr mDBRemoveItem; nsCOMPtr mDBGetLastChildId; @@ -449,6 +448,21 @@ private: nsCategoryCache mCacheObservers; bool mShuttingDown; + + /** + * Always call EnsureKeywordsHash() and check it for errors before actually + * using the hash. Internal keyword methods are already doing that. + */ + nsresult EnsureKeywordsHash(); + nsDataHashtable mBookmarkToKeywordHash; + + /** + * This function must be called every time a bookmark is removed. + * + * @param aURI + * Uri to test. + */ + nsresult UpdateKeywordsHashForRemovedBookmark(PRInt64 aItemId); }; struct nsBookmarksUpdateBatcher diff --git a/toolkit/components/places/tests/bookmarks/test_keywords.js b/toolkit/components/places/tests/bookmarks/test_keywords.js new file mode 100644 index 00000000000..162fe1817bd --- /dev/null +++ b/toolkit/components/places/tests/bookmarks/test_keywords.js @@ -0,0 +1,125 @@ +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim:set ts=2 sw=2 sts=2 et: */ +/* ***** BEGIN LICENSE BLOCK ***** + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 + * + * The contents of this file are subject to the Mozilla Public License Version + * 1.1 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * http://www.mozilla.org/MPL/ + * + * Software distributed under the License is distributed on an "AS IS" basis, + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License + * for the specific language governing rights and limitations under the + * License. + * + * The Original Code is Places Test Code. + * + * The Initial Developer of the Original Code is the Mozilla Foundation. + * Portions created by the Initial Developer are Copyright (C) 2010 + * the Initial Developer. All Rights Reserved. + * + * Contributor(s): + * Marco Bonardo (Original Author) + * + * Alternatively, the contents of this file may be used under the terms of + * either the GNU General Public License Version 2 or later (the "GPL"), or + * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), + * in which case the provisions of the GPL or the LGPL are applicable instead + * of those above. If you wish to allow use of your version of this file only + * under the terms of either the GPL or the LGPL, and not to allow others to + * use your version of this file under the terms of the MPL, indicate your + * decision by deleting the provisions above and replace them with the notice + * and other provisions required by the GPL or the LGPL. If you do not delete + * the provisions above, a recipient may use your version of this file under + * the terms of any one of the MPL, the GPL or the LGPL. + * + * ***** END LICENSE BLOCK ***** */ + +let bs = PlacesUtils.bookmarks; +let db = DBConn(); + +function check_keyword(aItemId, aExpectedBookmarkKeyword, aExpectedURIKeyword) { + if (aItemId) { + print("Check keyword for bookmark"); + do_check_eq(bs.getKeywordForBookmark(aItemId), aExpectedBookmarkKeyword); + + print("Check keyword for uri"); + let uri = bs.getBookmarkURI(aItemId); + do_check_eq(bs.getKeywordForURI(uri), aExpectedURIKeyword); + + print("Check uri for keyword"); + // This API can't tell which uri the user wants, so it returns a random one. + if (aExpectedURIKeyword) + do_check_true(/http:\/\/test[0-9]\.mozilla\.org/.test(bs.getURIForKeyword(aExpectedURIKeyword).spec)); + } + else { + stmt = db.createStatement( + "SELECT id FROM moz_keywords WHERE keyword = :keyword" + ); + stmt.params.keyword = aExpectedBookmarkKeyword; + try { + do_check_false(stmt.executeStep()); + } finally { + stmt.finalize(); + } + } + + print("Check there are no orphan database entries"); + let stmt = db.createStatement( + "SELECT b.id FROM moz_bookmarks b " + + "LEFT JOIN moz_keywords k ON b.keyword_id = k.id " + + "WHERE keyword_id NOTNULL AND k.id ISNULL" + ); + try { + do_check_false(stmt.executeStep()); + } finally { + stmt.finalize(); + } +} + +function run_test() { + print("Check that leyword does not exist"); + do_check_eq(bs.getURIForKeyword("keyword"), null); + + let folderId = bs.createFolder(PlacesUtils.unfiledBookmarksFolderId, + "folder", bs.DEFAULT_INDEX); + + print("Add a bookmark with a keyword"); + let itemId1 = bs.insertBookmark(folderId, + uri("http://test1.mozilla.org/"), + bs.DEFAULT_INDEX, + "test1"); + check_keyword(itemId1, null, null); + bs.setKeywordForBookmark(itemId1, "keyword"); + check_keyword(itemId1, "keyword", "keyword"); + + print("Add another bookmark with the same uri, should not inherit keyword."); + let itemId1_bis = bs.insertBookmark(folderId, + uri("http://test1.mozilla.org/"), + bs.DEFAULT_INDEX, + "test1_bis"); + + check_keyword(itemId1_bis, null, "keyword"); + + print("Set same keyword on another bookmark with a different uri."); + let itemId2 = bs.insertBookmark(folderId, + uri("http://test2.mozilla.org/"), + bs.DEFAULT_INDEX, + "test2"); + check_keyword(itemId2, null, null); + bs.setKeywordForBookmark(itemId2, "keyword"); + check_keyword(itemId1, "keyword", "keyword"); + check_keyword(itemId1_bis, null, "keyword"); + check_keyword(itemId2, "keyword", "keyword"); + + print("Remove a bookmark with a keyword, it should not be removed from others"); + bs.removeItem(itemId2); + check_keyword(itemId1, "keyword", "keyword"); + + print("Remove a folder containing bookmarks with keywords"); + // Keyword should be removed as well. + bs.removeItem(folderId); + check_keyword(null, "keyword"); +} +