402486 - deleting lots of items from the history sidebar is slow, locks up browser until finished

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

deleting lots of items from the history sidebar is slow, locks up browser until finished

steps to reproduce:

1) I've got a relatively new profile, with less than 5 days of history.
2) I opened up the history sidebar, searched on foo.com and got back about 100 results
3) selected all the items, hit delete

the browser was unresponsive for quite some time (10 - 30 seconds?), and I could hear lots of disk activity.

I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110311 Minefield/3.0a9pre

this seems like a common thing for users to do, so asking for blocking.

looking at _removeRowsFromHistory(), we don't use a transaction, so that will help.  (good news is that _removeRowsFromBookmarks() does use transactions.)

Comment 1

[(not reading, please use [e-mail] instead)]

actually, I'm not 100% sure about _removeRowsFromBookmarks(), as it uses the transaction manager so that we can undo the transaction, but we might still not be batching up the deletes, using a db transaction.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 287348 [details] [diff] [review]
patch

not seeing the perf gains I had hoped, but I think that might be due to in nsNavHistory::RemovePage(), we already have a transaction, and _removeRowsFromHistory() will call removePage() for each selected uri.

Comment 4

[(not reading, please use [e-mail] instead)]

in addition to the transactions in nsNavHistory::RemovePage(), we currently notify every history observer on every delete, which in turn can result in database modifications.

Comment 5

[Marco Bonardo [:mak]]

taking a look:

- on controller.js we call removePage but we do not check if there are duplicates, we should not try to delete the same url twice (in some sidebar view  duplicates are not collapsed)

- on nsnavhistory removePage we call GetUrlIdFor(aURI, &placeId, PR_TRUE); with the third argument we are requesting to create an entry in moz_places if not present, why?

if we try to delete twice an url i think that on the second (and third, and so on...) deletes the entry will be created and deleted at once... 

should be also better not calling observers if nothing has been deleted, a select count before should 

i will do some more testing on this asap

Comment 7

[Simon Bünzli]

(From bug 412194 comment #0)
> Might be related to bug 373256 (places.sqlite doesn't seem to be cached). Might
> also be due to the fact that SQLite somehow doesn't allow the OS to keep
> places.sqlite in its own memory cache.

Comment 8

[Dietrich Ayala (:dietrich)]

un-assigning seth's places/autocomplete bugs.

Comment 9

[Marco Bonardo [:mak]]

temporary taking for testing

Comment 10

[Marco Bonardo [:mak]]

I've tried the following:

- added a new RemovePages function to nsNavHistory that takes an array of urls and deletes them in bunch
- modified controller to call RemovePages with chunks of max 100 urls (to avoid passing a too big array)

this gets slightly better, but the real problem are observers, i tried to make RemovePages not call Observers at all, so the idea is "if you want classic delete use RemovePage, if you want fast batch delete use RemovePages but it will not call observers". 

This is really faster, but this way the treeview does not rebuild after delete, and i need to find a way to force a rebuild on that (how can i force a treeview refresh?)

notice that RemovePage deletes all visits for an URL, and should be the default when grouped by most visited or by host, while other group-by should do delete by date (Bug 363621)

Comment 11

[Marco Bonardo [:mak]]

Attachment: wip

- this does not rebuild treeview after a delete
- removePage is using almost the same code as RemovePages, we could call RemovePages(created array,1) but we would call geturlidfor twice (one in removepages, and one in removepage for calling observers) 

dietrich could you take a look at this to see if it could be an applicable change?

Comment 12

[Marco Bonardo [:mak]]

Attachment: wip

this manually calls removedItem in the treeview, so it is correctly updated after deletes. Should be testable now.

the only remaining issue is code duplication in removepage, i could add a third argument to removepages, a boolean callObservers, so we could make removePage do a call to removePages with count = 1 and callObservers = true

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 297335 [details] [diff] [review]
wip

i have a better patch that makes RemovePage use RemovePages, and i use UpdateBatch to refresh the tree if we are deleting more than 10 urls, while it uses the old method if deleting less than 10 urls (to avoid rebuilding the tree completely). i'll post after some testing since i need some perf number comparison first.

Comment 14

[Marco Bonardo [:mak]]

Attachment: wip

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch

ok, this is the first version valid for review

This will not make sidebar lightning fast, but reduces a lot disk usage and timings on large deletes, i've been able to clear full history from the sidebar in less than 1 minute, with 18.000 places and 150.000 visits with less disk chunking. 

I have choosen 2 magic numbers in controller.js that could be tweaked:
- use the old way of deleting and removing one by one for the first 10 items, that is because with 20 items the timing is similar but the disk is chunking a lot more, an actual case is when the user wants to delete only a few items in the sidebar, for privacy reasons
- use a chunkLen of 300, with 200 in my full history removal test i got the "slow script" dialog after about 1 minute, with a 300 chunkLen i finished the test in less than 1 minute. the user however should never delete full history from the sidebar, but i considered 18000 places as a max cap to allow that somehow.

Comment 16

[Marco Bonardo [:mak]]

a little auto-review / reminder

+NS_IMETHODIMP
+nsNavHistory::RemovePage(nsIURI *aURI)
+{
+  nsIURI** URIs = &aURI;
+  RemovePages(URIs, 1, PR_FALSE);
   return NS_OK;

should be 

   return RemovePages(URIs, 1, PR_FALSE);

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 298695 [details] [diff] [review]
patch

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.193
>diff -u -8 -p -r1.193 controller.js
>--- browser/components/places/content/controller.js	11 Jan 2008 22:04:01 -0000	1.193
>+++ browser/components/places/content/controller.js	23 Jan 2008 10:44:18 -0000
>@@ -909,29 +909,56 @@ PlacesController.prototype = {
>       this._removeRange(ranges[i], transactions);
>     if (transactions.length > 0) {
>       var txn = PlacesUtils.ptm.aggregateTransactions(txnName, transactions);
>       PlacesUtils.ptm.doTransaction(txn);
>     }
>   },
> 
>   /**
>-   * 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 URINode = PlacesUtils._uri(node.uri);
>+        // avoid trying to delete the same url twice
>+        if (URIs.indexOf(URINode) < 0) {
>+          URIs.push(URINode);

nit: s/URINode/uri/, or something without "node", as it's an nsIURI not a result node.

>+        }
>+      }
>+    }
>+
>+    // 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 > 10) {

nit: move this value into a const at the top of the file, with comment

>+      // do removal in chunks to avoid passing a too big array to removePages
>+      var chunkLen = 300;

nit: move this value into a const at the top of the file, with comment

>+      for (var i = 0; i < URIs.length; i += chunkLen) {
>+        var URIslice = URIs.slice(i, Math.max(i + chunkLen, URIs.length));
>+        // call Observers only on the last chunk
>+        bhist.removePages(URIslice, URIslice.length,
>+                          (i + 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]);
>+      }

nit: remove unnecessary brackets to conform w/ local style

> NS_IMETHODIMP
>-nsNavHistory::RemovePage(nsIURI *aURI)
>+nsNavHistory::RemovePages(nsIURI **aURIs, PRUint32 aLength, PRBool aRefreshResults)
> {
>-  PRInt64 placeId;
>-  nsresult rv = GetUrlIdFor(aURI, &placeId, PR_TRUE);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  // 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())

nit: i think we're trying to eradicate the space after !

>+      deletePlaceIdsQueryString.AppendLiteral(",");
>+    deletePlaceIdsQueryString.AppendInt(placeId);
>+    }

nit: indent

> 
>   // now that visits have been removed, run annotation expiration.
>-  // this will remove all expire-able annotations for this URI.
>+  // this will remove all expire-able annotations for these URIs.
>   (void)mExpire.OnDeleteURI();

fodder for a followup: i wonder if we really need to do all this expiration work here. maybe we should instead move to a model where we disconnect the places records, and have cleanup of disconnnected records done at idle/shutdown. thoughts?

>-  // 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.
>+  // force a full refresh calling onEndUpdateBatch (will call Refresh())
>+  if (aRefreshResults) {
>+    UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to obsrvrs.

spelling nit: observers

>+  }
>+  // if we don't want to refesh all results and we are deleting only

spelling nit: refresh

>+  // one entry, we call onDeleteUri on that entry
>+  // This is required for RemovePage
>+  else if(aLength == 1) {
>+      ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnDeleteURI(aURIs[0]))
>+    }

hrm, i don't like this. it adds a special-case where none seems necessary, and you say in several places that this doesn't notify, which isn't really true. i'd prefer maybe just remove |if(aLength == 1)|, and change aRefreshResults to aDoBatchNotify or something like that.


>+// 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
>+nsNavHistory::RemovePage(nsIURI *aURI)
>+{
>+  nsIURI** URIs = &aURI;
>+  RemovePages(URIs, 1, PR_FALSE);

please check result value here

Comment 18

[Marco Bonardo [:mak]]

> > 
> >   // now that visits have been removed, run annotation expiration.
> >-  // this will remove all expire-able annotations for this URI.
> >+  // this will remove all expire-able annotations for these URIs.
> >   (void)mExpire.OnDeleteURI();
> 
> fodder for a followup: i wonder if we really need to do all this expiration
> work here. maybe we should instead move to a model where we disconnect the
> places records, and have cleanup of disconnnected records done at
> idle/shutdown. thoughts?

i was wondering about the same thing, but then i thought of the case when the user deletes a lot of urls from the sidebar, then he closes the browser and goes looking at the db to check if they're gone, it's a similar case to clearhistory, we could only delete visits and then let expiration remove unreferenced items but then users will complain that their choice to delete has not been fulfilled. also after this we are removing items without annotation, so we need to remove annotations before if we want to do a good cleanup. However with a large history the most of the time is spent in rebuilding the tree, those queries are a little part of the full time

> >+  // one entry, we call onDeleteUri on that entry
> >+  // This is required for RemovePage
> >+  else if(aLength == 1) {
> >+      ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnDeleteURI(aURIs[0]))
> >+    }
> 
> hrm, i don't like this. it adds a special-case where none seems necessary, and
> you say in several places that this doesn't notify, which isn't really true.
> i'd prefer maybe just remove |if(aLength == 1)|, and change aRefreshResults to
> aDoBatchNotify or something like that.

i could move onDeleteURI observer call to RemovePage directly, so there is no doubt that removepages could call observers when i think that it will not do that.

Comment 19

[Marco Bonardo [:mak]]

i have an hang here in refreshVisibleSection after Mano's patch for getConcreteItemId, investigating...

Comment 20

[Marco Bonardo [:mak]]

filled Bug 414549 about the hang, marking as dependancy since that code is causing an infinite loop on tree refresh

Comment 21

[Marco Bonardo [:mak]]

Attachment: updated patch

- addressed review comments
- changed aRefreshResult to aDoBatchNotify and fixed comments
- moved onDeleteURI observer call to removePage
- Before testing this you'll need the patch for Bug 414549 (hang on treeView.js)

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 300019 [details] [diff] [review]
updated patch

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.194
>diff -u -8 -p -r1.194 controller.js
>--- browser/components/places/content/controller.js	25 Jan 2008 19:01:18 -0000	1.194
>+++ browser/components/places/content/controller.js	29 Jan 2008 14:44:43 -0000
>@@ -48,16 +48,27 @@ const RELOAD_ACTION_NOTHING = 0;
> // Inserting items new to the view, select the inserted rows
> const RELOAD_ACTION_INSERT = 1;
> // Removing items from the view, select the first item after the last selected
> const RELOAD_ACTION_REMOVE = 2;
> // Moving items within a view, don't treat the dropped items as additional 
> // 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. 
>  * @param   aItemId
>  *          The identifier of the parent container
>  * @param   aIndex
>  *          The index within the container where we should insert
>  * @param   aOrientation
>@@ -910,30 +921,54 @@ PlacesController.prototype = {
>       this._removeRange(ranges[i], transactions);
>     if (transactions.length > 0) {
>       var txn = PlacesUtils.ptm.aggregateTransactions(txnName, transactions);
>       PlacesUtils.ptm.doTransaction(txn);
>     }
>   },
> 
>   /**
>-   * 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]);

nit: for multiline blocks please add brackets

> 
>-    // 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);
> 

why did you remove this?

please add the xpcshell test for removePages API, fix the above comments, and then this patch should be done.

Comment 23

[Marco Bonardo [:mak]]

>-    // 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);
> 

why did you remove this?

i haven't, it was probably a problem with not-in-sync checkout!

Comment 24

[Marco Bonardo [:mak]]

Attachment: updated patch

- fixed bracket
- fixed some typo in comments
- re-added back frecencies changes (doh!)
- added an XPCshell test for removePages API

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 300361 [details] [diff] [review]
updated patch

the change looks good. however, can you add these factors to the test?

- bookmark a uri, delete the uri via the new api, and confirm the bookmark still exists

- annotate a uri w/ unexpirable annotation, delete the uri via the new api, confirm the annotation still exists

Comment 26

[Marco Bonardo [:mak]]

Attachment: updated patch

- added additional test on bookmarked and annotated items

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 300628 [details] [diff] [review]
updated patch

great, thanks for updating the tests.

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 300628 [details] [diff] [review]
updated patch

drivers: this is a pretty low-risk perf/hang patch for deleting history from the sidebar. it's got test coverage, and pretty low footprint, would be nice to make it into b3.

Comment 29

[Mike Schroepfer]

Comment on attachment 300628 [details] [diff] [review]
updated patch

Go for it - esp if we can get it in tonight to test out in tomorrow's nightlies.

Comment 30

[Reed Loden [:reed]]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.198; previous revision: 1.197
done
Checking in toolkit/components/places/public/nsIBrowserHistory.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIBrowserHistory.idl,v  <--  nsIBrowserHistory.idl
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.240; previous revision: 1.239
done
Checking in toolkit/components/places/tests/unit/test_browserhistory.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v  <--  test_browserhistory.js
new revision: 1.4; previous revision: 1.3
done

Comment 31

[Hasham]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008060302 Minefield/3.1a1pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008060306 Minefield/3.0pre

Checked it out on RC2 and nighlty on 10.5 and XP. Checks out on both. Marking as verified.

Comment 32

[Gervase Markham [:gerv]]

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