From 1e7f8b0da3dda1dca1372fa6964a5de2d1839f5e Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Wed, 1 Jun 2011 03:33:45 +0200 Subject: [PATCH] Bug 659740 - Frecency update causes SQL sort warning. r=sdwilsh --- toolkit/components/places/SQLFunctions.cpp | 48 ++++++++++-- toolkit/components/places/nsNavHistory.cpp | 90 ++++------------------ toolkit/components/places/nsNavHistory.h | 57 +++++++------- 3 files changed, 84 insertions(+), 111 deletions(-) diff --git a/toolkit/components/places/SQLFunctions.cpp b/toolkit/components/places/SQLFunctions.cpp index ebeedcc653f..2e2e63d643f 100644 --- a/toolkit/components/places/SQLFunctions.cpp +++ b/toolkit/components/places/SQLFunctions.cpp @@ -477,7 +477,23 @@ namespace places { // The page is already in the database, and we can fetch current // params from the database. nsCOMPtr getPageInfo = - history->GetStatementByStoragePool(DB_PAGE_INFO_FOR_FRECENCY); + history->GetStatementByStoragePool( + "SELECT typed, hidden, visit_count, " + "(SELECT count(*) FROM moz_historyvisits WHERE place_id = :page_id), " + "EXISTS ( " + "SELECT 1 FROM moz_bookmarks " + "WHERE fk = :page_id " + "AND NOT EXISTS( " + "SELECT 1 " + "FROM moz_items_annos a " + "JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id " + "WHERE n.name = :anno_name " + "AND a.item_id = parent " + ") " + "), " + "(url > 'place:' AND url < 'place;') " + "FROM moz_places " + "WHERE id = :page_id "); NS_ENSURE_STATE(getPageInfo); mozStorageStatementScoper infoScoper(getPageInfo); @@ -504,20 +520,40 @@ namespace places { rv = getPageInfo->GetInt32(5, &isQuery); NS_ENSURE_SUCCESS(rv, rv); + // NOTE: This is not limited to visits with "visit_type NOT IN (0,4,7,8)" + // because otherwise it would not return any visit for those transitions + // causing an incorrect frecency, see CalculateFrecencyInternal(). + // In case of a temporary or permanent redirect, calculate the frecency + // as if the original page was visited. + nsCAutoString visitsForFrecencySQL(NS_LITERAL_CSTRING( + "/* do not warn (bug 659740 - SQLite may ignore index if few visits exist) */" + "SELECT " + "ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400), " + "IFNULL(r.visit_type, v.visit_type), " + "v.visit_date " + "FROM moz_historyvisits v " + "LEFT JOIN moz_historyvisits r ON r.id = v.from_visit AND v.visit_type BETWEEN " + ) + nsPrintfCString("%d AND %d ", nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT, + nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY) + + NS_LITERAL_CSTRING("WHERE v.place_id = :page_id " + "ORDER BY v.visit_date DESC ") + ); + // Get a sample of the last visits to the page, to calculate its weight. nsCOMPtr getVisits = - history->GetStatementByStoragePool(DB_VISITS_FOR_FRECENCY); + history->GetStatementByStoragePool(visitsForFrecencySQL); NS_ENSURE_STATE(getVisits); mozStorageStatementScoper visitsScoper(getVisits); rv = getVisits->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), pageId); NS_ENSURE_SUCCESS(rv, rv); + // Fetch only a limited number of recent visits. PRInt32 numSampledVisits = 0; - // The visits query is already limited to the last N visits. - while (NS_SUCCEEDED(getVisits->ExecuteStep(&hasResult)) && hasResult) { - numSampledVisits++; - + for (PRInt32 maxVisits = history->GetNumVisitsForFrecency(); + numSampledVisits < maxVisits && + NS_SUCCEEDED(getVisits->ExecuteStep(&hasResult)) && hasResult; + numSampledVisits++) { PRInt32 visitType; rv = getVisits->GetInt32(1, &visitType); NS_ENSURE_SUCCESS(rv, rv); diff --git a/toolkit/components/places/nsNavHistory.cpp b/toolkit/components/places/nsNavHistory.cpp index c21d3e2a41f..7e72931f636 100644 --- a/toolkit/components/places/nsNavHistory.cpp +++ b/toolkit/components/places/nsNavHistory.cpp @@ -369,6 +369,8 @@ PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsNavHistory, gHistoryService) nsNavHistory::nsNavHistory() : mBatchLevel(0) , mBatchDBTransaction(nsnull) +, mAsyncThreadStatements(mDBConn) +, mStatements(mDBConn) , mDBPageSize(0) , mCurrentJournalMode(JOURNAL_DELETE) , mCachedNow(0) @@ -901,16 +903,6 @@ nsNavHistory::InitAdditionalDBItems() nsresult rv = InitTriggers(); NS_ENSURE_SUCCESS(rv, rv); - // These statements are used by frecency calculation. Since frecency runs in - // the storage async thread, it needs to access them through a const history - // object, for thread-safety. Due to const correctness it's not possible for - // the statements getter to lazily initialize them on first use, thus they - // are initialized here. - (void*)GetStatement(mDBPageInfoForFrecency); - (void*)GetStatement(mDBAsyncThreadPageInfoForFrecency); - (void*)GetStatement(mDBVisitsForFrecency); - (void*)GetStatement(mDBAsyncThreadVisitsForFrecency); - return NS_OK; } @@ -1282,34 +1274,6 @@ nsNavHistory::GetStatement(const nsCOMPtr& aStmt) "WHERE url = :page_url " )); - // NOTE: This is not limited to visits with "visit_type NOT IN (0,4,7,8)" - // because otherwise mDBVisitsForFrecency would return no visits - // for places with only embed (or undefined) visits. That would - // cause an incorrect frecency, see CalculateFrecencyInternal(). - // In such a case a place with only EMBED visits would result in a non-zero - // frecency. - // In case of a temporary or permanent redirect, calculate the frecency as if - // the original page was visited. - nsCAutoString visitsForFrecencySQL(NS_LITERAL_CSTRING( - "SELECT " - "ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400), " - "COALESCE( " - "(SELECT r.visit_type FROM moz_historyvisits r WHERE v.visit_type IN ") + - nsPrintfCString("(%d,%d) ", TRANSITION_REDIRECT_PERMANENT, - TRANSITION_REDIRECT_TEMPORARY) + - NS_LITERAL_CSTRING(" AND r.id = v.from_visit), " - "visit_type), " - "visit_date " - "FROM moz_historyvisits v " - "WHERE v.place_id = :page_id " - "ORDER BY visit_date DESC LIMIT ") + - nsPrintfCString("%d", mNumVisitsForFrecency) - ); - - RETURN_IF_STMT(mDBVisitsForFrecency, visitsForFrecencySQL); - - RETURN_IF_STMT(mDBAsyncThreadVisitsForFrecency, visitsForFrecencySQL); - RETURN_IF_STMT(mDBUpdateFrecency, NS_LITERAL_CSTRING( "UPDATE moz_places " "SET frecency = CALCULATE_FRECENCY(:page_id) " @@ -1322,34 +1286,6 @@ nsNavHistory::GetStatement(const nsCOMPtr& aStmt) "WHERE id = :page_id AND frecency <> 0" )); - RETURN_IF_STMT(mDBGetPlaceVisitStats, NS_LITERAL_CSTRING( - "SELECT typed, hidden, frecency " - "FROM moz_places " - "WHERE id = :page_id " - )); - - // When calculating frecency, we need special information for the page. - nsCAutoString pageInfoForFrecencySQL(NS_LITERAL_CSTRING( - "SELECT typed, hidden, visit_count, " - "(SELECT count(*) FROM moz_historyvisits WHERE place_id = :page_id), " - "(SELECT id FROM moz_bookmarks " - "WHERE fk = :page_id " - "AND parent NOT IN (" - "SELECT a.item_id " - "FROM moz_items_annos a " - "JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id " - "WHERE n.name = :anno_name" - ") " - "LIMIT 1), " - "(url > 'place:' AND url < 'place;') " - "FROM moz_places " - "WHERE id = :page_id " - )); - - RETURN_IF_STMT(mDBPageInfoForFrecency, pageInfoForFrecencySQL); - - RETURN_IF_STMT(mDBAsyncThreadPageInfoForFrecency, pageInfoForFrecencySQL); - #ifdef MOZ_XUL RETURN_IF_STMT(mDBFeedbackIncrease, NS_LITERAL_CSTRING( // Leverage the PRIMARY KEY (place_id, input) to insert/update entries. @@ -2504,16 +2440,14 @@ nsNavHistory::FixInvalidFrecenciesForExcludedPlaces() "SET frecency = 0 WHERE id IN (" "SELECT h.id FROM moz_places h " "WHERE h.url >= 'place:' AND h.url < 'place;' " - "UNION " + "UNION ALL " // Unvisited child of a livemark "SELECT b.fk FROM moz_bookmarks b " + "JOIN moz_places h ON b.fk = h.id AND visit_count = 0 AND frecency < 0 " "JOIN moz_bookmarks bp ON bp.id = b.parent " "JOIN moz_items_annos a ON a.item_id = bp.id " "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " "WHERE n.name = :anno_name " - "AND b.fk IN( " - "SELECT id FROM moz_places WHERE visit_count = 0 AND frecency < 0 " - ") " ")"), getter_AddRefs(dbUpdateStatement)); NS_ENSURE_SUCCESS(rv, rv); @@ -7259,13 +7193,8 @@ nsNavHistory::FinalizeStatements() { mDBVisitToVisitResult, mDBBookmarkToUrlResult, mDBUrlToUrlResult, - mDBVisitsForFrecency, mDBUpdateFrecency, mDBUpdateHiddenOnFrecency, - mDBGetPlaceVisitStats, - mDBPageInfoForFrecency, - mDBAsyncThreadPageInfoForFrecency, - mDBAsyncThreadVisitsForFrecency, }; for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(stmts); i++) { @@ -7273,6 +7202,17 @@ nsNavHistory::FinalizeStatements() { NS_ENSURE_SUCCESS(rv, rv); } + // Finalize the statementCaches on the correct threads. + mStatements.FinalizeStatements(); + + nsRefPtr > event = + new FinalizeStatementCacheProxy( + mAsyncThreadStatements, NS_ISUPPORTS_CAST(nsINavHistoryService*, this)); + nsCOMPtr target = do_GetInterface(mDBConn); + NS_ENSURE_TRUE(target, NS_ERROR_OUT_OF_MEMORY); + nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL); + NS_ENSURE_SUCCESS(rv, rv); + return NS_OK; } diff --git a/toolkit/components/places/nsNavHistory.h b/toolkit/components/places/nsNavHistory.h index 3205c05f443..a04624b2c6d 100644 --- a/toolkit/components/places/nsNavHistory.h +++ b/toolkit/components/places/nsNavHistory.h @@ -72,6 +72,7 @@ #include "nsNavHistoryQuery.h" #include "mozilla/storage.h" +#include "mozilla/storage/StatementCache.h" #define QUERYUPDATE_TIME 0 #define QUERYUPDATE_SIMPLE 1 @@ -137,8 +138,6 @@ namespace places { , DB_ADD_NEW_PAGE , DB_GET_URL_PAGE_INFO , DB_SET_PLACE_TITLE - , DB_PAGE_INFO_FOR_FRECENCY - , DB_VISITS_FOR_FRECENCY }; enum JournalMode { @@ -493,31 +492,32 @@ public: return GetStatement(mDBGetURLPageInfo); case DB_SET_PLACE_TITLE: return GetStatement(mDBSetPlaceTitle); - case DB_PAGE_INFO_FOR_FRECENCY: - return GetStatement(mDBPageInfoForFrecency); - case DB_VISITS_FOR_FRECENCY: - return GetStatement(mDBVisitsForFrecency); } return nsnull; } - mozIStorageStatement* GetStatementByStoragePool( - enum mozilla::places::HistoryStatementId aStatementId - ) const - { - using namespace mozilla::places; + /** + * This cache should be used only for background thread statements. + * + * @pre must be running on the background thread of mDBConn. + */ + mutable mozilla::storage::StatementCache mAsyncThreadStatements; + mutable mozilla::storage::StatementCache mStatements; - switch(aStatementId) { - case DB_PAGE_INFO_FOR_FRECENCY: - return NS_IsMainThread() ? mDBPageInfoForFrecency - : mDBAsyncThreadPageInfoForFrecency; - case DB_VISITS_FOR_FRECENCY: - return NS_IsMainThread() ? mDBVisitsForFrecency - : mDBAsyncThreadVisitsForFrecency; - default: - NS_NOTREACHED("Trying to handle an unknown statement"); - } - return nsnull; + template + already_AddRefed + GetStatementByStoragePool(const char (&aQuery)[N]) const + { + nsDependentCString query(aQuery, N - 1); + return GetStatementByStoragePool(query); + } + + already_AddRefed + GetStatementByStoragePool(const nsACString& aQuery) const + { + return NS_IsMainThread() + ? mStatements.GetCachedStatement(aQuery) + : mAsyncThreadStatements.GetCachedStatement(aQuery); } PRInt32 GetFrecencyAgedWeight(PRInt32 aAgeInDays) const @@ -580,6 +580,11 @@ public: } } + PRInt32 GetNumVisitsForFrecency() const + { + return mNumVisitsForFrecency; + } + PRInt64 GetNewSessionID(); /** @@ -647,14 +652,6 @@ protected: nsCOMPtr mDBUrlToUrlResult; // kGetInfoIndex_* results nsCOMPtr mDBUpdateFrecency; nsCOMPtr mDBUpdateHiddenOnFrecency; - nsCOMPtr mDBGetPlaceVisitStats; - // Cached statements used in frecency calculation. Since it could happen on - // both main thread or storage async thread, we keep two versions of them - // for thread-safety. - mutable nsCOMPtr mDBVisitsForFrecency; - mutable nsCOMPtr mDBPageInfoForFrecency; - mutable nsCOMPtr mDBAsyncThreadVisitsForFrecency; - mutable nsCOMPtr mDBAsyncThreadPageInfoForFrecency; #ifdef MOZ_XUL // AutoComplete stuff nsCOMPtr mDBFeedbackIncrease;