Closed Bug 630240 Opened 9 years ago Closed 9 years ago

Avoid full refreshes in history results when incremental updates are easy

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 1 obsolete file)

Attached patch wip v1.0 (obsolete) — Splinter Review
refresh can be slow for some kind of queries, especially searchterm queries, plus causing useless complete invalidation of ui views.
Blocks: 365992
Flags: in-testsuite?
Summary: Avoid Refresh() in live-update results when incremental updates are easy → Avoid full refreshes in history results when incremental updates are easy
Attached patch patch v1.0Splinter Review
This involves a bunch of related changes, thus I think it's better to land early in central to have more baking time. I know it's scary to review but we also have some pretty large testbase (this already passed tryserver).

The basics:
- query for tags when we build the nodes, but don't sort them since that causes the tale scan queries to be incredibly slower.
- sort tags on request
- since now we have all of tags, titles and uris we can use the previous changes we did to filter if the node is pertinent to the result
- rather than refreshing use the new information to figure out if the node should be added/removed

The final scope is to reduce UI blocking when a large results view is visible, and it also fixes some longstanding bugs found thanks to the new paths.
Attachment #508438 - Attachment is obsolete: true
Attachment #525982 - Flags: review?(dietrich)
s/tale scan queries/table-scan queries/
Comment on attachment 525982 [details] [diff] [review]
patch v1.0

are there any changes in notification order, or notifications at all, that would affect Sync? i suppose those tests are run in a full tryserver run though...

> NS_IMETHODIMP
> nsNavBookmarks::OnDeleteURI(nsIURI* aURI)
> {
>-  // If the page is bookmarked, notify observers for each associated bookmark.
>-  ItemChangeData changeData;

why moving this code?


> nsresult // static
> nsNavHistory::AsciiHostNameFromHostString(const nsACString& aHostName,
>                                           nsACString& aAscii)
> {
>+  // To properly generate a uri we must provide a protocol.
>+  nsCAutoString fakeURL("http://");
>+  fakeURL.Append(aHostName);
>   nsCOMPtr<nsIURI> uri;
>-  nsresult rv = NS_NewURI(getter_AddRefs(uri), aHostName);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  return uri->GetAsciiHost(aAscii);
>+  nsresult rv = NS_NewURI(getter_AddRefs(uri), fakeURL);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = uri->GetAsciiHost(aAscii);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;
> }

looks unrelated to this patch. what's up?

>@@ -4237,52 +4297,45 @@ nsNavHistory::RemovePages(nsIURI **aURIs
>     NS_ENSURE_SUCCESS(rv, rv);
>     if (placeId != 0) {
>       if (!deletePlaceIdsQueryString.IsEmpty())
>         deletePlaceIdsQueryString.AppendLiteral(",");
>       deletePlaceIdsQueryString.AppendInt(placeId);
>     }
>   }
> 
>-  rv = RemovePagesInternal(deletePlaceIdsQueryString);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  // Clear the registered embed visits.
>-  clearEmbedVisits();
>-
>   // force a full refresh calling onEndUpdateBatch (will call Refresh())
>   if (aDoBatchNotify)
>     UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
> 
>+  rv = RemovePagesInternal(deletePlaceIdsQueryString);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Clear the registered embed visits.
>+  clearEmbedVisits();
>+

why the notification code move, here and the other removal method?

r=me with these answered.
Attachment #525982 - Flags: review?(dietrich) → review+
(In reply to comment #3)
> Comment on attachment 525982 [details] [diff] [review]
> patch v1.0
> 
> are there any changes in notification order, or notifications at all, that
> would affect Sync? i suppose those tests are run in a full tryserver run
> though...

Not in the order, but we'll now notify page and visits removals correctly, that is a huge win for Sync.

> > NS_IMETHODIMP
> > nsNavBookmarks::OnDeleteURI(nsIURI* aURI)
> > {
> >-  // If the page is bookmarked, notify observers for each associated bookmark.
> >-  ItemChangeData changeData;
> 
> why moving this code?

because it was wrong, onDeleteURI the page is removed from the db and thus from the result, then there is no reason to notify that we removed its visits, instead makes sense to do so if the page remains in the db (that is the purpose of onDeleteVisits

> > nsresult // static
> > nsNavHistory::AsciiHostNameFromHostString(const nsACString& aHostName,
> >                                           nsACString& aAscii)
> > {
> >+  // To properly generate a uri we must provide a protocol.
> >+  nsCAutoString fakeURL("http://");
> >+  fakeURL.Append(aHostName);
> >   nsCOMPtr<nsIURI> uri;
> >-  nsresult rv = NS_NewURI(getter_AddRefs(uri), aHostName);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-  return uri->GetAsciiHost(aAscii);
> >+  nsresult rv = NS_NewURI(getter_AddRefs(uri), fakeURL);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  rv = uri->GetAsciiHost(aAscii);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  return NS_OK;
> > }
> 
> looks unrelated to this patch. what's up?

To generate a new node for single insertion in certain history queries I need this method to work, and unfortunately it never worked.


> >-  rv = RemovePagesInternal(deletePlaceIdsQueryString);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-  // Clear the registered embed visits.
> >-  clearEmbedVisits();
> >-
> >   // force a full refresh calling onEndUpdateBatch (will call Refresh())
> >   if (aDoBatchNotify)
> >     UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
> > 
> >+  rv = RemovePagesInternal(deletePlaceIdsQueryString);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  // Clear the registered embed visits.
> >+  clearEmbedVisits();
> >+
> 
> why the notification code move, here and the other removal method?

As it was it was just firing an empty batch and we were skipping onDeleteURI and onDeleteVisits notifications, since now we notify them correctly make sense to correctly wrap them inside the batch notifications.
http://hg.mozilla.org/projects/places/rev/49420afe652c
Whiteboard: [fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/49420afe652c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Blocks: 530782
Blocks: 484221
Blocks: 501357
Depends on: 681420
Blocks: 556068
Depends on: 717510
You need to log in before you can comment on or make changes to this bug.