Last Comment Bug 681420 - Improve responsiveness of history removals
: Improve responsiveness of history removals
Status: RESOLVED FIXED
[Tsnap]
: addon-compat, dev-doc-complete, regression
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
: 681231 714366 (view as bug list)
Depends on: 734640
Blocks: 432706 466421 479666 630240 889527
  Show dependency treegraph
 
Reported: 2011-08-23 11:57 PDT by Marco Bonardo [::mak]
Modified: 2013-11-26 12:21 PST (History)
12 users (show)
mak77: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (24.48 KB, patch)
2011-08-23 14:16 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
patch v1.1 (33.43 KB, patch)
2011-08-24 02:29 PDT, Marco Bonardo [::mak]
robert.strong.bugs: superreview+
Details | Diff | Splinter Review
patch v1.2 (24.79 KB, patch)
2011-08-31 04:45 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-08-23 11:57:23 PDT
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] 2011-08-23 11:59:31 PDT
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] 2011-08-23 14:16:35 PDT
Created attachment 555201 [details] [diff] [review]
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] 2011-08-23 14:22:55 PDT
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) 2011-08-23 18:35:35 PDT
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 5 Alice0775 White 2011-08-23 19:29:09 PDT
*** Bug 681231 has been marked as a duplicate of this bug. ***
Comment 6 Marco Bonardo [::mak] 2011-08-24 02:29:18 PDT
Created attachment 555348 [details] [diff] [review]
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 [:rstrong] (use needinfo to contact me) 2011-08-30 22:45:43 PDT
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] 2011-08-31 02:43:01 PDT
(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] 2011-08-31 04:45:39 PDT
Created attachment 557117 [details] [diff] [review]
patch v1.2
Comment 11 Marco Bonardo [::mak] 2011-08-31 04:54:05 PDT
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) 2011-08-31 07:59:32 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/7a8b51e5be41
Comment 13 Ed Morley [:emorley] 2011-09-01 08:42:46 PDT
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] 2011-11-04 12:35:22 PDT
Documentation updated:

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

And mentioned on Firefox 9 for developers.
Comment 15 Marco Bonardo [::mak] 2012-01-03 02:24:55 PST
*** Bug 714366 has been marked as a duplicate of this bug. ***
Comment 16 Marco Bonardo [::mak] 2012-03-15 13:42:20 PDT
*** Bug 734643 has been marked as a duplicate of this bug. ***
Comment 17 Goce Mitevski 2013-11-26 12:21:54 PST
> 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

Note You need to log in before you can comment on or make changes to this bug.