From 71364148c0bc57fb50d9ee302de1c4729d4bfefc Mon Sep 17 00:00:00 2001 From: "reed@reedloden.com" Date: Thu, 31 Jan 2008 19:03:14 -0800 Subject: [PATCH] Bug 402486 - "deleting lots of items from the history sidebar is slow, locks up browser until finished" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=dietrich a1.9b3=schrep] --- .../components/places/content/controller.js | 44 ++++- .../places/public/nsIBrowserHistory.idl | 28 ++- .../components/places/src/nsNavHistory.cpp | 175 ++++++++++-------- .../places/tests/unit/test_browserhistory.js | 61 ++++++ 4 files changed, 217 insertions(+), 91 deletions(-) diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js index 6a69e3c612e..0a08ddfb640 100755 --- a/browser/components/places/content/controller.js +++ b/browser/components/places/content/controller.js @@ -51,6 +51,17 @@ const RELOAD_ACTION_REMOVE = 2; // rows. const RELOAD_ACTION_MOVE = 3; +// when removing a bunch of pages we split them in chunks to avoid passing +// a too big array to RemovePages +// 300 is the best choice with an history of about 150000 visits +// smaller chunks could cause a Slow Script warning with a huge history +const REMOVE_PAGES_CHUNKLEN = 300; +// if we are removing less than this pages we will remove them one by one +// since it will be reflected faster on the UI +// 10 is a good compromise, since allows the user to delete a little amount of +// urls for privacy reasons, but does not cause heavy disk access +const REMOVE_PAGES_MAX_SINGLEREMOVES = 10; + /** * Represents an insertion point within a container where we can insert * items. @@ -912,19 +923,44 @@ PlacesController.prototype = { }, /** - * Removes the set of selected ranges from history. + * Removes the set of selected ranges from history. */ _removeRowsFromHistory: function PC__removeRowsFromHistory() { // Other containers are history queries, just delete from history // history deletes are not undoable. var nodes = this._view.getSelectionNodes(); + var URIs = []; + var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory); + var resultView = this._view.getResultView(); for (var i = 0; i < nodes.length; ++i) { var node = nodes[i]; - var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory); if (PlacesUtils.nodeIsHost(node)) bhist.removePagesFromHost(node.title, true); - else if (PlacesUtils.nodeIsURI(node)) - bhist.removePage(PlacesUtils._uri(node.uri)); + else if (PlacesUtils.nodeIsURI(node)) { + var uri = PlacesUtils._uri(node.uri); + // avoid trying to delete the same url twice + if (URIs.indexOf(uri) < 0) { + URIs.push(uri); + } + } + } + + // if we have to delete a lot of urls RemovePage will be slow, it's better + // to delete them in bunch and rebuild the full treeView + if (URIs.length > REMOVE_PAGES_MAX_SINGLEREMOVES) { + // do removal in chunks to avoid passing a too big array to removePages + for (var i = 0; i < URIs.length; i += REMOVE_PAGES_CHUNKLEN) { + var URIslice = URIs.slice(i, Math.max(i + REMOVE_PAGES_CHUNKLEN, URIs.length)); + // set DoBatchNotify only on the last chunk + bhist.removePages(URIslice, URIslice.length, + (i + REMOVE_PAGES_CHUNKLEN) >= URIs.length); + } + } + else { + // if we have to delete fewer urls, removepage will allow us to avoid + // rebuilding the full treeView + for (var i = 0; i < URIs.length; ++i) + bhist.removePage(URIs[i]); } }, diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl index 7414140b375..70559f9d8b6 100644 --- a/toolkit/components/places/public/nsIBrowserHistory.idl +++ b/toolkit/components/places/public/nsIBrowserHistory.idl @@ -42,11 +42,12 @@ #include "nsISupports.idl" #include "nsIGlobalHistory2.idl" -[scriptable, uuid(c43079c3-3d8d-4b7c-af14-0e30ab46865f)] +[scriptable, uuid(ef807ae6-4ae9-4d1f-8ade-3ee75e24d18c)] interface nsIBrowserHistory : nsIGlobalHistory2 { - /** + /** * addPageWithDetails + * * Adds a page to history with specific time stamp information. This is used in * the History migrator. */ @@ -54,23 +55,38 @@ interface nsIBrowserHistory : nsIGlobalHistory2 /** * lastPageVisited + * * The last page that was visited in a top-level window. */ readonly attribute AUTF8String lastPageVisited; /** * count + * * The number of entries in global history */ readonly attribute PRUint32 count; /** - * remove a page from history + * removePage + * + * Remove a page from history */ void removePage(in nsIURI aURI); + /** + * removePages + * + * Remove a bunch of pages from history + * Notice that this does not call observers for performance reasons, + * instead setting aDoBatchNotify true will send Begin/EndUpdateBatch + */ + void removePages([array, size_is(aLength)] in nsIURI aURIs, + in unsigned long aLength, in boolean aDoBatchNotify); + /** * removePagesFromHost + * * Remove all pages from the given host. * If aEntireDomain is true, will assume aHost is a domain, * and remove all pages from the entire domain. @@ -79,22 +95,24 @@ interface nsIBrowserHistory : nsIGlobalHistory2 /** * removeAllPages + * * Remove all pages from global history */ void removeAllPages(); /** * hidePage + * * Hide the specified URL from being enumerated (and thus * displayed in the UI) - * - * if the page hasn't been visited yet, then it will be added + * If the page hasn't been visited yet, then it will be added * as if it was visited, and then marked as hidden */ void hidePage(in nsIURI aURI); /** * markPageAsTyped + * * Designate the url as having been explicitly typed in by * the user, so it's okay to be an autocomplete result. */ diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp index 571c73bbea9..ef680946302 100644 --- a/toolkit/components/places/src/nsNavHistory.cpp +++ b/toolkit/components/places/src/nsNavHistory.cpp @@ -3089,96 +3089,107 @@ nsNavHistory::GetCount(PRUint32 *aCount) } +// nsNavHistory::RemovePages +// +// Removes a bunch of uris from history. +// Has better performance than RemovePage when deleting a lot of history. +// Notice that this function does not call the onDeleteURI observers, +// instead, if aDoBatchNotify is true, we call OnBegin/EndUpdateBatch. +// We don't do duplicates removal, URIs array should be cleaned-up before. + +NS_IMETHODIMP +nsNavHistory::RemovePages(nsIURI **aURIs, PRUint32 aLength, PRBool aDoBatchNotify) +{ + // build a list of place ids to delete + nsCString deletePlaceIdsQueryString; + nsresult rv; + for (PRInt32 i = 0; i < aLength; i++) { + PRInt64 placeId; + rv = GetUrlIdFor(aURIs[i], &placeId, PR_FALSE); + NS_ENSURE_SUCCESS(rv, rv); + if (placeId != 0) { + if (!deletePlaceIdsQueryString.IsEmpty()) + deletePlaceIdsQueryString.AppendLiteral(","); + deletePlaceIdsQueryString.AppendInt(placeId); + } + } + + // early return if there is nothing to delete + if (deletePlaceIdsQueryString.IsEmpty()) + return NS_OK; + + mozStorageTransaction transaction(mDBConn, PR_FALSE); + nsCOMPtr statement; + + // delete all visits + rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "DELETE FROM moz_historyvisits WHERE place_id IN (") + + deletePlaceIdsQueryString + + NS_LITERAL_CSTRING(")")); + NS_ENSURE_SUCCESS(rv, rv); + + // now that visits have been removed, run annotation expiration. + // this will remove all expire-able annotations for these URIs. + (void)mExpire.OnDeleteURI(); + + // if there are no more annotations, and the entry is not bookmarked + // then we can remove the moz_places entry. + // Note that we do NOT delete favicons. Any unreferenced favicons will be + // deleted next time the browser is shut down. + rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "DELETE FROM moz_places WHERE id IN (" + "SELECT h.id FROM moz_places h WHERE h.id IN (") + + deletePlaceIdsQueryString + + NS_LITERAL_CSTRING(") AND " + "NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) AND " + "NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.place_id = h.id))")); + NS_ENSURE_SUCCESS(rv, rv); + + // if a moz_place was annotated or was a bookmark, + // we didn't delete it, but we did delete the moz_visits + // so we need to reset the frecency. Note, we don't + // reset the visit_count, as we use that in our "on idle" + // query to figure out which places to recalculate frecency first. + rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "UPDATE moz_places SET frecency = -1 WHERE id IN(") + + deletePlaceIdsQueryString + + NS_LITERAL_CSTRING(")")); + NS_ENSURE_SUCCESS(rv, rv); + + // placeId could have a livemark item, so setting the frecency to -1 + // would cause it to show up in the url bar autocomplete + // call FixInvalidFrecenciesForExcludedPlaces() to handle that scenario + // XXX this might be dog slow, further degrading delete perf. + rv = FixInvalidFrecenciesForExcludedPlaces(); + NS_ENSURE_SUCCESS(rv, rv); + + // XXX todo + // forcibly call the "on idle" timer here to do a little work + // but the rest will happen on idle. + + rv = transaction.Commit(); + NS_ENSURE_SUCCESS(rv, rv); + + // force a full refresh calling onEndUpdateBatch (will call Refresh()) + if (aDoBatchNotify) + UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers + + return NS_OK; +} + + // nsNavHistory::RemovePage // // Removes all visits and the main history entry for the given URI. // Silently fails if we have no knowledge of the page. -NS_IMETHODIMP + NS_IMETHODIMP nsNavHistory::RemovePage(nsIURI *aURI) { - PRInt64 placeId; - nsresult rv = GetUrlIdFor(aURI, &placeId, PR_TRUE); + nsIURI** URIs = &aURI; + nsresult rv = RemovePages(URIs, 1, PR_FALSE); NS_ENSURE_SUCCESS(rv, rv); - - mozStorageTransaction transaction(mDBConn, PR_FALSE); - nsCOMPtr statement; - - // delete all visits for this page - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "DELETE FROM moz_historyvisits WHERE place_id = ?1"), - getter_AddRefs(statement)); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->BindInt64Parameter(0, placeId); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - - // now that visits have been removed, run annotation expiration. - // this will remove all expire-able annotations for this URI. - (void)mExpire.OnDeleteURI(); - - // does the uri have un-expirable annotations? - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_STATE(annosvc); - nsTArray annoNames; - rv = annosvc->GetAnnotationNamesTArray(placeId, &annoNames, PR_FALSE); - NS_ENSURE_SUCCESS(rv, rv); - - // is the uri bookmarked? - nsNavBookmarks* bookmarksService = nsNavBookmarks::GetBookmarksService(); - NS_ENSURE_STATE(bookmarksService); - PRBool bookmarked = PR_FALSE; - rv = bookmarksService->IsBookmarked(aURI, &bookmarked); - NS_ENSURE_SUCCESS(rv, rv); - - // if there are no more annotations, and the entry is not bookmarked - // then we can remove the moz_places entry. - if (annoNames.Length() == 0 && !bookmarked) { - // Note that we do NOT delete favicons. Any unreferenced favicons will be - // deleted next time the browser is shut down. - - // delete main history entries - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "DELETE FROM moz_places WHERE id = ?1"), - getter_AddRefs(statement)); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->BindInt64Parameter(0, placeId); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - } - else { - // because the moz_place was annotated (or it was a bookmark), - // we didn't delete it, but we did delete the moz_visits - // so we need to reset the frecency. Note, we don't - // reset the visit_count, as we use that in our "on idle" - // query to figure out which places to recalcuate frecency first. - rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( - "UPDATE moz_places SET frecency = -1 WHERE id = ?1"), - getter_AddRefs(statement)); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->BindInt64Parameter(0, placeId); - NS_ENSURE_SUCCESS(rv, rv); - rv = statement->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - - // placeId could have a livemark item, so setting the frecency to -1 - // would cause it to show up in the url bar autocomplete - // call FixInvalidFrecenciesForExcludedPlaces() to handle that scenario - // XXX this might be dog slow, further degrading delete perf. - rv = FixInvalidFrecenciesForExcludedPlaces(); - NS_ENSURE_SUCCESS(rv, rv); - - // XXX todo - // forcibly call the "on idle" timer here to do a little work - // but the rest will happen on idle. - } - - // Observers: Be sure to finish transaction before calling observers. Note also - // that we always call the observers even though we aren't sure something - // actually got deleted. - transaction.Commit(); + // call observers here for the removed url ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnDeleteURI(aURI)) return NS_OK; } diff --git a/toolkit/components/places/tests/unit/test_browserhistory.js b/toolkit/components/places/tests/unit/test_browserhistory.js index ee6d5d2e142..039bdfbd171 100644 --- a/toolkit/components/places/tests/unit/test_browserhistory.js +++ b/toolkit/components/places/tests/unit/test_browserhistory.js @@ -23,6 +23,7 @@ * Darin Fisher * Dietrich Ayala * Dan Mills + * Marco Bonardo * * 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 @@ -45,6 +46,20 @@ try { do_throw("Could not get history service\n"); } +// Get annotation service +try { + var annosvc= Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService); +} catch(ex) { + do_throw("Could not get annotation service\n"); +} + +// Get bookmark service +try { + var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService); +} catch(ex) { + do_throw("Could not get nav-bookmarks-service\n"); +} + // main function run_test() { var testURI = uri("http://mozilla.com"); @@ -83,6 +98,52 @@ function run_test() { do_check_eq(0, bhist.count); do_check_eq("", bhist.lastPageVisited); + /** + * remove a bunch of pages from history + */ + var deletedPages = []; + deletedPages.push(uri("http://mirror1.mozilla.com")); + deletedPages.push(uri("http://mirror2.mozilla.com")); + deletedPages.push(uri("http://mirror3.mozilla.com")); + deletedPages.push(uri("http://mirror4.mozilla.com")); + deletedPages.push(uri("http://mirror5.mozilla.com")); + deletedPages.push(uri("http://mirror6.mozilla.com")); + deletedPages.push(uri("http://mirror7.mozilla.com")); + deletedPages.push(uri("http://mirror8.mozilla.com")); + + try { + for (var i = 0; i < deletedPages.length ; ++i) + bhist.addPageWithDetails(deletedPages[i], "testURI" + (i+1), Date.now() * 1000); + } catch(ex) { + do_throw("addPageWithDetails failed"); + } + + // annotated and bookmarked items should not be removed from moz_places + var annoIndex = 1; + var annoName = "testAnno"; + var annoValue = "foo"; + var bookmarkIndex = 2; + var bookmarkName = "bar"; + annosvc.setPageAnnotation(deletedPages[annoIndex], annoName, annoValue, 0, + Ci.nsIAnnotationService.EXPIRE_NEVER); + var bookmark = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, + deletedPages[bookmarkIndex], bmsvc.DEFAULT_INDEX, bookmarkName); + + try { + bhist.removePages(deletedPages, deletedPages.length, false); + } catch(ex) { + do_throw("removePages failed"); + } + do_check_eq(0, bhist.count); + do_check_eq("", bhist.lastPageVisited); + // check that bookmark and annotation still exist + do_check_eq(bmsvc.getBookmarkURI(bookmark).spec, deletedPages[bookmarkIndex].spec); + do_check_eq(annosvc.getPageAnnotation(deletedPages[annoIndex], annoName), annoValue); + // remove annotation and bookmark + annosvc.removePageAnnotation(deletedPages[annoIndex], annoName); + bmsvc.removeItem(bookmark); + bhist.removeAllPages(); + /** * removePagesFromHost * Remove all pages from the given host.