630240 - Avoid full refreshes in history results when incremental updates are easy

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Attachment: wip v1.0

refresh can be slow for some kind of queries, especially searchterm queries, plus causing useless complete invalidation of ui views.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 2

[Marco Bonardo [:mak]]

s/tale scan queries/table-scan queries/

Comment 3

[Dietrich Ayala (:dietrich)]

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.

Comment 4

[Marco Bonardo [:mak]]

(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.

Comment 5

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/49420afe652c

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/49420afe652c