Improve responsiveness of history removals

RESOLVED FIXED in mozilla9

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Depends on: 1 bug, {addon-compat, dev-doc-complete, regression})

Trunk
mozilla9
addon-compat, dev-doc-complete, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Tsnap])

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
Blocks: 630240
Keywords: regression
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.
Attachment #555201 - Flags: review?(dietrich)
Summary: Improve responsiveness of history deletion → Improve responsiveness of history removals
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.
Flags: in-testsuite-
Whiteboard: [Tsnap]
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?
Attachment #555201 - Flags: review?(dietrich) → review+

Updated

6 years ago
Duplicate of this bug: 681231
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 :(
Attachment #555201 - Attachment is obsolete: true
Attachment #555348 - Flags: superreview?(robert.bugzilla)
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.
Attachment #555348 - Flags: superreview?(robert.bugzilla) → superreview+
(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.
Created attachment 557117 [details] [diff] [review]
patch v1.2
Attachment #555348 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/7a8b51e5be41
Keywords: addon-compat
Whiteboard: [Tsnap] → [Tsnap][inbound]
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.
Keywords: dev-doc-needed
Blocks: 479666
Blocks: 466421
Pushed:
http://hg.mozilla.org/mozilla-central/rev/7a8b51e5be41
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Tsnap][inbound] → [Tsnap]
Target Milestone: --- → mozilla9
Impressive job Marco! Removing 1500 items from library->history used to take 30-50 mins for me, now takes <10s :-D

Updated

6 years ago
Blocks: 432706
Documentation updated:

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

And mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Duplicate of this bug: 714366

Updated

6 years ago
Depends on: 734640
Duplicate of this bug: 734643
Blocks: 889527
Depends on: 920421
No longer depends on: 920421

Comment 17

4 years ago
> 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
You need to log in before you can comment on or make changes to this bug.