681420 - Improve responsiveness of history removals

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

A common reported issue is that removing a bunch of pages from history hangs the UI for a long time, there are a couple small improvements we can do that should improve that without being extremely time consuming:
- RemovePages method is not using batch correctly, being in a sub scope it ends before work is done. We should just always batch.
- the controller does chunking, but it's pretty much useless since there is no redispatch. we should give some breath to the main-thread by dispatching chunk removals to it
- the resultnodes usually try to do simple updates one by one, this is usually faster for a small number of changes but much slower for large changes. We can detect number of changes in a batch and change the update mode on the way. This should preserve advantages of both approaches.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

notice, the third point above is actually a partial regression from bug 630240, while that largely improved small updates, it has slowed down large updates.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

requires SR, the change is only the useless third argument to RemovePages, but since this idl was so bad, I updated some comment and removed a couple old deprecated methods.
Actually, I think we should put this whole interface in the trashcan.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So briefly this patch reintroduces refreshes that I removed in bug 630240 (still preserving all the goodness from there), but it enables them only after a certain number of changes during a batch.
So for the first 5 changes we use the one-by-one method, then we switch to a full refresh() and ignore further notifications till the end of the batch.
Controller chunks removals and dispatches them to the main-thread, allowing the ui to refresh in the middle, so this is not a long hang asking user to terminate the process.

While this is not a miracle nor perfect, it is a pretty cheap patch that makes "infinite hang of hours" become "small distributed hangs in a minute". I could remove about 9000 entries in less than a minute.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 555201 [details] [diff] [review]
patch v1.0

Review of attachment 555201 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, r=me w/ these addressed. thanks!

::: toolkit/components/places/nsNavHistory.cpp
@@ +4302,5 @@
> +  }
> +  nsCAutoString deletePlaceIdsQueryString;
> +  deletePlaceIdsQueryString.AppendInt(placeId);
> +
> +  rv = RemovePagesInternal(deletePlaceIdsQueryString);

The plural of deletePlaceIdsQueryString is confusing. Make singular?

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +92,4 @@
>      return NS_OK; \
>    } else
>  
> +// Number of changes to handle separately in a batch.  If more than

unfinished comment?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Robert, the relevant changes in the idl are:
- removed the third argument from RemovePages
- removed 2 deprecated methods

the remaining changes are updates to the idl documentation, some was really outdated :(

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 555348 [details] [diff] [review]
patch v1.1

Sorry for taking so long on this... I got a tad lost looking at the updates to the idl comments which is MUCH improved but could use a little more love. I'm r+'ing the api changes and would appreciate it if someone could spend a little more time on the idl comments as time permits.

>diff --git a/toolkit/components/places/nsIBrowserHistory.idl b/toolkit/components/places/nsIBrowserHistory.idl
>--- a/toolkit/components/places/nsIBrowserHistory.idl
>+++ b/toolkit/components/places/nsIBrowserHistory.idl
>@@ -37,151 +37,145 @@
> 
> /*
>  * browser-specific interface to global history
>  */
> 
> #include "nsISupports.idl"
> #include "nsIGlobalHistory2.idl"
> 
>-[scriptable, uuid(540aca25-1e01-467f-b24c-df89cbe40f8d)]
>+[scriptable, uuid(212371ab-d8b9-4835-b867-d0eb78c0cb18)]
> interface nsIBrowserHistory : nsIGlobalHistory2
> {
>     /**
>-     * addPageWithDetails
>+     * Adds a page to history with specific timestamp information. This is used
>+     * by the History migrator.
nit: "specific timestamp information" could be clearer regarding what it is used for here. Is the timestamp only needed by the migrator? If so, it might be good to include that in the same sentence as well and maybe just say last visit time instead of specific timestamp information.

>      *
>-     * Adds a page to history with specific time stamp information. This is used in
>-     * the History migrator. 
>+     * @param aURI
>+     *        URI of the page to be added.
>+     * @param aTitle
>+     *        Title of the page.
>+     * @param aLastvisited
>+     *        Microseconds from epoch representing the last visit time.
>      */
>-    void addPageWithDetails(in nsIURI aURI, in wstring aTitle, in long long aLastVisited);
>+    void addPageWithDetails(in nsIURI aURI,
>+                            in wstring aTitle,
>+                            in long long aLastVisited);
> 
>     /**
>-     * lastPageVisited
>-     *
>      * The last page that was visited in a top-level window.
>      */
>     readonly attribute AUTF8String lastPageVisited;
> 
>     /**
>-     * count
>+     * Indicates if there are entries in global history.
>      *
>-     * Indicate if there are entries in global history
>-     * For performance reasons this does not return the real number of entries
>+     * @note For performance reasons this does not return the real number of
>+     *       entries.
>      */
Though it states in the note that this doesn't return the real number of entries it should also state what it is returning. It would also be nice for this attribute wasn't named count... is there a bug to fix this so it is the count or to rename it?

>     readonly attribute PRUint32 count;
> 
>     /**
>-     * removePage
>-     *
>-     * Remove a page from history
>+     * Removes a page from global history.
"global history" and "history" are used in the comments without it being clear what the difference is. For example, what is the difference between history when used in addPageWithDetails and this one?

It isn't clear from the documentation why removePage exists when removePages could be used by just passing a single url page though it can be found in the code. Would be good to add why to the idl comments.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> >     /**
> >-     * count
> >+     * Indicates if there are entries in global history.
> >      *
> >-     * Indicate if there are entries in global history
> >-     * For performance reasons this does not return the real number of entries
> >+     * @note For performance reasons this does not return the real number of
> >+     *       entries.
> >      */
> Though it states in the note that this doesn't return the real number of
> entries it should also state what it is returning. It would also be nice for
> this attribute wasn't named count... is there a bug to fix this so it is the
> count or to rename it?

Yes, it's absolutely confusing, was not renamed to preserve compatibility with js consumers, btw a bug already exists.
bug 416832 - BrowserHistory count property is quite confusing and should be renamed 

will address other concerns further improving comments.
I think there is no difference between global history and history today, I suppose global word was used in the past to differentiate from session history.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a8b51e5be41

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

for dev-doc-needed:
idl comments have been improved, that may help fixing docs to those APIs
for addon-compat and dev-doc-needed:
third param to RemovePages has been removed, obsolete registerOpenPage and unregisterOpenPage have been removed, should use the same methods from mozIPlacesAutoComplete instead.

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/7a8b51e5be41

Comment 13

[Ed Morley [:emorley]]

Impressive job Marco! Removing 1500 items from library->history used to take 30-50 mins for me, now takes <10s :-D

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIBrowserHistory

And mentioned on Firefox 9 for developers.

Comment 17

[Goce Mitevski]

> A common reported issue is that removing a bunch of pages from history hangs the UI for a long time...

This is still an issue on Windows 7 x64, even in Firefox 25.x. I tried deleting a large list of history items from a single month and Firefox went '(Not Responding)' until the deletion was complete - after 10 minutes or maybe even more.

Regards,
Goce