Closed
Bug 681420
Opened 13 years ago
Closed 13 years ago
Improve responsiveness of history removals
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Summary: Improve responsiveness of history deletion → Improve responsiveness of history removals
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite-
Whiteboard: [Tsnap]
Comment 4•13 years ago
|
||
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+
Assignee | ||
Comment 6•13 years ago
|
||
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 7•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #555348 -
Attachment is obsolete: true
Assignee | ||
Comment 10•13 years ago
|
||
Keywords: addon-compat
Whiteboard: [Tsnap] → [Tsnap][inbound]
Assignee | ||
Comment 11•13 years ago
|
||
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
Comment 12•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [Tsnap][inbound] → [Tsnap]
Target Milestone: --- → mozilla9
Comment 13•13 years ago
|
||
Impressive job Marco! Removing 1500 items from library->history used to take 30-50 mins for me, now takes <10s :-D
Comment 14•13 years ago
|
||
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
Comment 17•11 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.
Description
•