421315 - Just re-use autocomplete richlistitems instead of re-adjusting for no purpose

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Ed Lee :Mardak]

So I was looking at how many times _setUpDescription gets called for bug 407946 and turns out there's a lot of calls to invalidate.

1) autocomplete controller gets search results for first chunk ProcessResult calls popup->Invalidate()
2) ProcessResult also calls PopupOpen() and the autocomplete binding calls invalidate on opening
3) next chunk arrives and ProcessResult calls Invalidate()
4) repeat step 3 several times depending on the number of chunks searched

(Step 4 could repeat hundreds of times if you have to search through your whole history)

Each call to invalidate causes each row in the autocomplete to be rebuilt.

Either we should not call invalidate so often from the autocomplete controller or be smarter in the binding to not actually rebuild the list every time invalidate is called.

1) Don't call invalidate if the number of results don't change

That doesn't quite help if we do keep getting new results..

2) Binding should just reuse the richlistitem if it's the same entry (same url, same type)

3) Don't call invalidate when popup is opened?? (saves on one invalidate)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

See also bug 393902.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> 2) Binding should just reuse the richlistitem if it's the same entry (same url,
> same type)

Don't we already do this? We don't ever remove richlistitems, and we don't set their value if it hasn't changed:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.120#1224

Comment 3

[Ed Lee :Mardak]

Attachment: v1

(In reply to comment #2)
> (In reply to comment #0)
> Don't we already do this? We don't ever remove richlistitems, and we don't set
> their value if it hasn't changed:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.120#1224
Yes, but that still goes through the whole motion of adjusting the descriptions and ellipsis and more. That could get expensive if we match all search terms which has a variable number of children text nodes.

We can do even better by skipping all that and just showing what we already have.

Not sure if adding extra parameters to an interface is kosher... But it saves on an extra invalidate. (And makes sure items are built with the correct width instead of building them when the popup width isn't set.)

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

A gavin-friendly patch -u32p that patches -p0!

Also, makes things more kosher with a _invalidate.

Comment 5

[Ed Lee :Mardak]

Attachment: v1.2

This patch is on top of bug 419656 (and bug 421412, but that doesn't affect this patch).

Comment 6

[Ed Lee :Mardak]

This would be good to have before bug 407946 lands as this will prevent *A LOT* of unnecessary adjusts that will be more expensive when emphasizing multiple matches.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 307890 [details] [diff] [review]
v1.2

The amount of context in this patch is insane - are you trying to make a point? :)

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>+              // Completely re-use the existing richlistitem if it's the same
>+              if (item.getAttribute("text") == trimmedSearchString &&
>+                  item.getAttribute("url") == url) {
>+                item.collapsed = false;
>+                this._currentIndex++;
>+                continue;
>+              }

Is it possible for title/image/type to change without text/url changing? Probably not...

Comment 8

[Ed Lee :Mardak]

(In reply to comment #7)
> (From update of attachment 307890 [details] [diff] [review])
> The amount of context in this patch is insane - are you trying to make a point?
> :)
Not really. ;) I just did the huge context because this patch is on top of bug 419656, so comparing against the file in the repo wouldn't have helped.

> >diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml
> >+              // Completely re-use the existing richlistitem if it's the same
> >+              if (item.getAttribute("text") == trimmedSearchString &&
> >+                  item.getAttribute("url") == url) {
> Is it possible for title/image/type to change without text/url changing?
> Probably not...
Nah. Not for a given search string. You could get a url that has a bookmark type in one case but not another.. e.g., keyword bookmarks that happen to match a url. But you would have to type "bug 421315" for the keyword or "re-use autocomplete" for non-bookmark.

Comment 9

[Ed Lee :Mardak]

Attachment: v1.3

Same patch. Less context. Same perf wins. ;)

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 308996 [details] [diff] [review]
v1.3

a=mconnor on behalf of 1.9 drivers

Comment 11

[Ed Lee :Mardak]

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.124; previous revision: 1.123
done