Closed Bug 681420 Opened 13 years ago Closed 13 years ago

Improve responsiveness of history removals

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: mak, Assigned: mak)

References

(Depends on 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, regression, Whiteboard: [Tsnap])

Attachments

(1 file, 2 obsolete files)

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
Attached patch patch v1.0 (obsolete) (deleted) — Splinter Review
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+
Attached patch patch v1.1 (obsolete) (deleted) — Splinter Review
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.
Attached patch patch v1.2 (deleted) — Splinter Review
Attachment #555348 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 13 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
Blocks: 432706
Documentation updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIBrowserHistory And mentioned on Firefox 9 for developers.
Depends on: 734640
Blocks: 889527
No longer depends on: 920421
> 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.

Attachment

General

Created:
Updated:
Size: