From 1404e8bc7eb94c75369b889f2c0c6d5fb38bf6ea Mon Sep 17 00:00:00 2001 From: Mayank Srivastav Date: Tue, 30 May 2017 19:14:39 +0530 Subject: [PATCH] Bug 1354032 - Remove Places analyze call and use PRAGMA optimize(0x02) for optimizations. r=mak MozReview-Commit-ID: AOkh3vOKD4E --HG-- extra : rebase_source : 90f13425b7d5aec779787612781c2a49dd005037 --- toolkit/components/places/Bookmarks.jsm | 4 +- toolkit/components/places/Database.cpp | 63 ++-------- .../components/places/nsPlacesExpiration.js | 58 ++------- .../tests/expiration/test_analyze_runs.js | 111 ------------------ .../tests/expiration/test_outdated_analyze.js | 72 ------------ .../places/tests/expiration/xpcshell.ini | 2 - .../places/tests/unit/test_analyze.js | 28 ----- .../components/places/tests/unit/xpcshell.ini | 1 - 8 files changed, 23 insertions(+), 316 deletions(-) delete mode 100644 toolkit/components/places/tests/expiration/test_analyze_runs.js delete mode 100644 toolkit/components/places/tests/expiration/test_outdated_analyze.js delete mode 100644 toolkit/components/places/tests/unit/test_analyze.js diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 27c2c0b35fb0..bad634e20299 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -2186,7 +2186,9 @@ async function(db, folderGuids, options) { p.parent AS _grandParentId, NULL AS _childCount, b.syncStatus AS _syncStatus FROM descendants - JOIN moz_bookmarks b ON did = b.id + /* The usage of CROSS JOIN is not random, it tells the optimizer + to retain the original rows order, so the hierarchy is respected */ + CROSS JOIN moz_bookmarks b ON did = b.id JOIN moz_bookmarks p ON p.id = b.parent LEFT JOIN moz_places h ON b.fk = h.id`, { folderGuid }); diff --git a/toolkit/components/places/Database.cpp b/toolkit/components/places/Database.cpp index 76edeedd6e66..da79eda65619 100644 --- a/toolkit/components/places/Database.cpp +++ b/toolkit/components/places/Database.cpp @@ -157,52 +157,6 @@ hasRecentCorruptDB() return false; } -/** - * Updates sqlite_stat1 table through ANALYZE. - * Since also nsPlacesExpiration.js executes ANALYZE, the analyzed tables - * must be the same in both components. So ensure they are in sync. - * - * @param aDBConn - * The database connection. - */ -nsresult -updateSQLiteStatistics(mozIStorageConnection* aDBConn) -{ - MOZ_ASSERT(NS_IsMainThread()); - nsCOMPtr analyzePlacesStmt; - aDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING( - "ANALYZE moz_places" - ), getter_AddRefs(analyzePlacesStmt)); - NS_ENSURE_STATE(analyzePlacesStmt); - nsCOMPtr analyzeBookmarksStmt; - aDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING( - "ANALYZE moz_bookmarks" - ), getter_AddRefs(analyzeBookmarksStmt)); - NS_ENSURE_STATE(analyzeBookmarksStmt); - nsCOMPtr analyzeVisitsStmt; - aDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING( - "ANALYZE moz_historyvisits" - ), getter_AddRefs(analyzeVisitsStmt)); - NS_ENSURE_STATE(analyzeVisitsStmt); - nsCOMPtr analyzeInputStmt; - aDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING( - "ANALYZE moz_inputhistory" - ), getter_AddRefs(analyzeInputStmt)); - NS_ENSURE_STATE(analyzeInputStmt); - - mozIStorageBaseStatement *stmts[] = { - analyzePlacesStmt, - analyzeBookmarksStmt, - analyzeVisitsStmt, - analyzeInputStmt - }; - - nsCOMPtr ps; - (void)aDBConn->ExecuteAsync(stmts, ArrayLength(stmts), nullptr, - getter_AddRefs(ps)); - return NS_OK; -} - /** * Sets the connection journal mode to one of the JOURNAL_* types. * @@ -644,11 +598,6 @@ Database::EnsureConnection() mDatabaseStatus = nsINavHistoryService::DATABASE_STATUS_UPGRADED; } - if (mDatabaseStatus == nsINavHistoryService::DATABASE_STATUS_UPGRADED || - mDatabaseStatus == nsINavHistoryService::DATABASE_STATUS_CREATE) { - MOZ_ALWAYS_SUCCEEDS(updateSQLiteStatistics(mMainConn)); - } - // Initialize here all the items that are not part of the on-disk database, // like views, temp triggers or temp tables. The database should not be // considered corrupt if any of the following fails. @@ -2626,6 +2575,18 @@ Database::Shutdown() mClosed = true; + // Execute PRAGMA optimized as last step, this will ensure proper database + // performance across restarts. + nsCOMPtr optimizeStmt; + nsresult rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( + "PRAGMA optimize(0x02)" + ), getter_AddRefs(optimizeStmt)); + MOZ_ASSERT(NS_SUCCEEDED(rv)); + if (NS_SUCCEEDED(rv)) { + nsCOMPtr ps; + MOZ_ALWAYS_SUCCEEDS(optimizeStmt->ExecuteAsync(nullptr, getter_AddRefs(ps))); + } + (void)mMainConn->AsyncClose(connectionShutdown); mMainConn = nullptr; } diff --git a/toolkit/components/places/nsPlacesExpiration.js b/toolkit/components/places/nsPlacesExpiration.js index 731559d0325b..40235d95b879 100644 --- a/toolkit/components/places/nsPlacesExpiration.js +++ b/toolkit/components/places/nsPlacesExpiration.js @@ -114,10 +114,6 @@ const IDLE_TIMEOUT_SECONDS = 5 * 60; // clearHistory to decide to skip expiration at shutdown. const SHUTDOWN_WITH_RECENT_CLEARHISTORY_TIMEOUT_SECONDS = 10; -// If the pages delta from the last ANALYZE is over this threashold, the tables -// should be analyzed again. -const ANALYZE_PAGES_THRESHOLD = 100; - // If the number of pages over history limit is greater than this threshold, // expiration will be more aggressive, to bring back history to a saner size. const OVERLIMIT_PAGES_THRESHOLD = 1000; @@ -158,12 +154,11 @@ const STATUS = { const ACTION = { TIMED: 1 << 0, // happens every this._interval TIMED_OVERLIMIT: 1 << 1, // like TIMED but only when history is over limits - TIMED_ANALYZE: 1 << 2, // happens when ANALYZE statistics should be updated - CLEAR_HISTORY: 1 << 3, // happens when history is cleared - SHUTDOWN_DIRTY: 1 << 4, // happens at shutdown for DIRTY state - IDLE_DIRTY: 1 << 5, // happens on idle for DIRTY state - IDLE_DAILY: 1 << 6, // happens once a day on idle - DEBUG: 1 << 7, // happens on TOPIC_DEBUG_START_EXPIRATION + CLEAR_HISTORY: 1 << 2, // happens when history is cleared + SHUTDOWN_DIRTY: 1 << 3, // happens at shutdown for DIRTY state + IDLE_DIRTY: 1 << 4, // happens on idle for DIRTY state + IDLE_DAILY: 1 << 5, // happens once a day on idle + DEBUG: 1 << 6, // happens on TOPIC_DEBUG_START_EXPIRATION }; // The queries we use to expire. @@ -414,32 +409,6 @@ const EXPIRATION_QUERIES = { actions: ACTION.TIMED | ACTION.TIMED_OVERLIMIT | ACTION.SHUTDOWN_DIRTY | ACTION.IDLE_DIRTY | ACTION.IDLE_DAILY | ACTION.DEBUG }, - - // The following queries are used to adjust the sqlite_stat1 table to help the - // query planner create better queries. These should always be run LAST, and - // are therefore at the end of the object. - // Since also nsNavHistory.cpp executes ANALYZE, the analyzed tables - // must be the same in both components. So ensure they are in sync. - - QUERY_ANALYZE_MOZ_PLACES: { - sql: "ANALYZE moz_places", - actions: ACTION.TIMED_OVERLIMIT | ACTION.TIMED_ANALYZE | - ACTION.CLEAR_HISTORY | ACTION.IDLE_DAILY | ACTION.DEBUG - }, - QUERY_ANALYZE_MOZ_BOOKMARKS: { - sql: "ANALYZE moz_bookmarks", - actions: ACTION.TIMED_ANALYZE | ACTION.IDLE_DAILY | ACTION.DEBUG - }, - QUERY_ANALYZE_MOZ_HISTORYVISITS: { - sql: "ANALYZE moz_historyvisits", - actions: ACTION.TIMED_OVERLIMIT | ACTION.TIMED_ANALYZE | - ACTION.CLEAR_HISTORY | ACTION.IDLE_DAILY | ACTION.DEBUG - }, - QUERY_ANALYZE_MOZ_INPUTHISTORY: { - sql: "ANALYZE moz_inputhistory", - actions: ACTION.TIMED | ACTION.TIMED_OVERLIMIT | ACTION.TIMED_ANALYZE | - ACTION.CLEAR_HISTORY | ACTION.IDLE_DAILY | ACTION.DEBUG - }, }; /** @@ -626,17 +595,10 @@ nsPlacesExpiration.prototype = { notify: function PEX_timerCallback() { // Check if we are over history capacity, if so visits must be expired. - this._getPagesStats((aPagesCount, aStatsCount) => { + this._getPagesStats((aPagesCount) => { let overLimitPages = aPagesCount - this._urisLimit; this._overLimit = overLimitPages > 0; - let action = this._overLimit ? ACTION.TIMED_OVERLIMIT : ACTION.TIMED; - // If the number of pages changed significantly from the last ANALYZE - // update SQLite statistics. - if (Math.abs(aPagesCount - aStatsCount) >= ANALYZE_PAGES_THRESHOLD) { - action = action | ACTION.TIMED_ANALYZE; - } - // Adapt expiration aggressivity to the number of pages over the limit. let limit = overLimitPages > OVERLIMIT_PAGES_THRESHOLD ? LIMIT.LARGE : LIMIT.SMALL; @@ -890,22 +852,18 @@ nsPlacesExpiration.prototype = { _getPagesStats: function PEX__getPagesStats(aCallback) { if (!this._cachedStatements["LIMIT_COUNT"]) { this._cachedStatements["LIMIT_COUNT"] = this._db.createAsyncStatement( - `SELECT (SELECT COUNT(*) FROM moz_places), - (SELECT SUBSTR(stat,1,LENGTH(stat)-2) FROM sqlite_stat1 - WHERE idx = 'moz_places_url_uniqueindex')` + `SELECT COUNT(*) FROM moz_places` ); } this._cachedStatements["LIMIT_COUNT"].executeAsync({ _pagesCount: 0, - _statsCount: 0, handleResult(aResults) { let row = aResults.getNextRow(); this._pagesCount = row.getResultByIndex(0); - this._statsCount = row.getResultByIndex(1); }, handleCompletion(aReason) { if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) { - aCallback(this._pagesCount, this._statsCount); + aCallback(this._pagesCount); } }, handleError(aError) { diff --git a/toolkit/components/places/tests/expiration/test_analyze_runs.js b/toolkit/components/places/tests/expiration/test_analyze_runs.js deleted file mode 100644 index 6f6ee9c22720..000000000000 --- a/toolkit/components/places/tests/expiration/test_analyze_runs.js +++ /dev/null @@ -1,111 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -// Constants - -const TOPIC_AUTOCOMPLETE_FEEDBACK_INCOMING = "autocomplete-will-enter-text"; - -// Helpers - -/** - * Ensures that we have no data in the tables created by ANALYZE. - */ -function clearAnalyzeData() { - let db = DBConn(); - if (!db.tableExists("sqlite_stat1")) { - return; - } - db.executeSimpleSQL("DELETE FROM sqlite_stat1"); -} - -/** - * Checks that we ran ANALYZE on the specified table. - * - * @param aTableName - * The table to check if ANALYZE was ran. - * @param aRan - * True if it was expected to run, false otherwise - */ -function do_check_analyze_ran(aTableName, aRan) { - let db = DBConn(); - do_check_true(db.tableExists("sqlite_stat1")); - let stmt = db.createStatement("SELECT idx FROM sqlite_stat1 WHERE tbl = :table"); - stmt.params.table = aTableName; - try { - if (aRan) { - do_check_true(stmt.executeStep()); - do_check_neq(stmt.row.idx, null); - } else { - do_check_false(stmt.executeStep()); - } - } finally { - stmt.finalize(); - } -} - -// Tests - -add_task(async function init_tests() { - const TEST_URI = NetUtil.newURI("http://mozilla.org/"); - const TEST_TITLE = "This is a test"; - - await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.unfiledGuid, - title: TEST_TITLE, - url: TEST_URI - }); - await PlacesTestUtils.addVisits(TEST_URI); - let thing = { - QueryInterface: XPCOMUtils.generateQI([Ci.nsIAutoCompleteInput, - Ci.nsIAutoCompletePopup, - Ci.nsIAutoCompleteController]), - get popup() { return thing; }, - get controller() { return thing; }, - popupOpen: true, - selectedIndex: 0, - getValueAt() { return TEST_URI.spec; }, - searchString: TEST_TITLE, - }; - Services.obs.notifyObservers(thing, TOPIC_AUTOCOMPLETE_FEEDBACK_INCOMING); -}); - -add_task(async function test_timed() { - clearAnalyzeData(); - - // Set a low interval and wait for the timed expiration to start. - let promise = promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED); - setInterval(3); - await promise; - setInterval(3600); - - do_check_analyze_ran("moz_places", false); - do_check_analyze_ran("moz_bookmarks", false); - do_check_analyze_ran("moz_historyvisits", false); - do_check_analyze_ran("moz_inputhistory", true); -}); - -add_task(async function test_debug() { - clearAnalyzeData(); - - await promiseForceExpirationStep(1); - - do_check_analyze_ran("moz_places", true); - do_check_analyze_ran("moz_bookmarks", true); - do_check_analyze_ran("moz_historyvisits", true); - do_check_analyze_ran("moz_inputhistory", true); -}); - -add_task(async function test_clear_history() { - clearAnalyzeData(); - - let promise = promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED); - let listener = Cc["@mozilla.org/places/expiration;1"] - .getService(Ci.nsINavHistoryObserver); - listener.onClearHistory(); - await promise; - - do_check_analyze_ran("moz_places", true); - do_check_analyze_ran("moz_bookmarks", false); - do_check_analyze_ran("moz_historyvisits", true); - do_check_analyze_ran("moz_inputhistory", true); -}); diff --git a/toolkit/components/places/tests/expiration/test_outdated_analyze.js b/toolkit/components/places/tests/expiration/test_outdated_analyze.js deleted file mode 100644 index 8d0940cc8a7a..000000000000 --- a/toolkit/components/places/tests/expiration/test_outdated_analyze.js +++ /dev/null @@ -1,72 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -// Test that expiration executes ANALYZE when statistics are outdated. - -const TEST_URL = "http://www.mozilla.org/"; - -XPCOMUtils.defineLazyServiceGetter(this, "gHistory", - "@mozilla.org/browser/history;1", - "mozIAsyncHistory"); - -/** - * Object that represents a mozIVisitInfo object. - * - * @param [optional] aTransitionType - * The transition type of the visit. Defaults to TRANSITION_LINK if not - * provided. - * @param [optional] aVisitTime - * The time of the visit. Defaults to now if not provided. - */ -function VisitInfo(aTransitionType, aVisitTime) { - this.transitionType = - aTransitionType === undefined ? TRANSITION_LINK : aTransitionType; - this.visitDate = aVisitTime || Date.now() * 1000; -} - -function run_test() { - do_test_pending(); - - // Init expiration before "importing". - force_expiration_start(); - - // Add a bunch of pages (at laast IMPORT_PAGES_THRESHOLD pages). - let places = []; - for (let i = 0; i < 100; i++) { - places.push({ - uri: NetUtil.newURI(TEST_URL + i), - title: "Title" + i, - visits: [new VisitInfo] - }); - } - gHistory.updatePlaces(places); - - // Set interval to a small value to expire on it. - setInterval(1); // 1s - - Services.obs.addObserver(function observeExpiration(aSubject, aTopic, aData) { - Services.obs.removeObserver(observeExpiration, - PlacesUtils.TOPIC_EXPIRATION_FINISHED); - - // Check that statistica are up-to-date. - let stmt = DBConn().createAsyncStatement( - "SELECT (SELECT COUNT(*) FROM moz_places) - " - + "(SELECT SUBSTR(stat,1,LENGTH(stat)-2) FROM sqlite_stat1 " - + "WHERE idx = 'moz_places_url_hashindex')" - ); - stmt.executeAsync({ - handleResult(aResultSet) { - let row = aResultSet.getNextRow(); - this._difference = row.getResultByIndex(0); - }, - handleError(aError) { - do_throw("Unexpected error (" + aError.result + "): " + aError.message); - }, - handleCompletion(aReason) { - do_check_true(this._difference === 0); - do_test_finished(); - } - }); - stmt.finalize(); - }, PlacesUtils.TOPIC_EXPIRATION_FINISHED); -} diff --git a/toolkit/components/places/tests/expiration/xpcshell.ini b/toolkit/components/places/tests/expiration/xpcshell.ini index b261612302f2..d3e35d657aad 100644 --- a/toolkit/components/places/tests/expiration/xpcshell.ini +++ b/toolkit/components/places/tests/expiration/xpcshell.ini @@ -2,7 +2,6 @@ head = head_expiration.js skip-if = toolkit == 'android' -[test_analyze_runs.js] [test_annos_expire_history.js] [test_annos_expire_never.js] [test_annos_expire_policy.js] @@ -13,6 +12,5 @@ skip-if = toolkit == 'android' [test_notifications.js] [test_notifications_onDeleteURI.js] [test_notifications_onDeleteVisits.js] -[test_outdated_analyze.js] [test_pref_interval.js] [test_pref_maxpages.js] diff --git a/toolkit/components/places/tests/unit/test_analyze.js b/toolkit/components/places/tests/unit/test_analyze.js deleted file mode 100644 index 207cb8279a6a..000000000000 --- a/toolkit/components/places/tests/unit/test_analyze.js +++ /dev/null @@ -1,28 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -// Tests sqlite_sta1 table exists, it should be created by analyze. -// Since the bookmark roots are created when the DB is created (bug 704855), -// the table will contain data. - -function run_test() { - do_test_pending(); - - let stmt = DBConn().createAsyncStatement( - "SELECT ROWID FROM sqlite_stat1" - ); - stmt.executeAsync({ - _gotResult: false, - handleResult(aResultSet) { - this._gotResult = true; - }, - handleError(aError) { - do_throw("Unexpected error (" + aError.result + "): " + aError.message); - }, - handleCompletion(aReason) { - do_check_true(this._gotResult); - do_test_finished(); - } - }); - stmt.finalize(); -} diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index 8613a2ef6ad6..4565ef09c149 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -56,7 +56,6 @@ skip-if = os == "linux" [test_1105208.js] [test_1105866.js] [test_adaptive_bug527311.js] -[test_analyze.js] [test_annotations.js] [test_asyncExecuteLegacyQueries.js] [test_async_in_batchmode.js]