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

RESOLVED FIXED in mozilla1.9beta5

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

({perf})

Trunk
mozilla1.9beta5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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)
Flags: blocking-firefox3?
See also bug 393902.
Component: Location Bar and Autocomplete → Autocomplete
Flags: blocking-firefox3?
Product: Firefox → Toolkit
QA Contact: location.bar → autocomplete
(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
(Assignee)

Updated

11 years ago
Summary: Autocomplete gets invalidated too often (each chunk, popup open) → Just re-use autocomplete richlistitems instead of re-adjusting for no purpose
(Assignee)

Comment 3

11 years ago
Created attachment 307753 [details] [diff] [review]
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.)
Assignee: nobody → edilee
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #307753 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

11 years ago
Created attachment 307757 [details] [diff] [review]
v1.1

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

Also, makes things more kosher with a _invalidate.
Attachment #307753 - Attachment is obsolete: true
Attachment #307757 - Flags: review?(gavin.sharp)
Attachment #307753 - Flags: review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Attachment #307757 - Attachment is patch: true
Attachment #307757 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 5

11 years ago
Created attachment 307890 [details] [diff] [review]
v1.2

This patch is on top of bug 419656 (and bug 421412, but that doesn't affect this patch).
Attachment #307757 - Attachment is obsolete: true
Attachment #307890 - Flags: review?(gavin.sharp)
Attachment #307757 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

11 years ago
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.
Blocks: 407946
Flags: blocking1.9?
(Assignee)

Updated

11 years ago
Depends on: 419656
Whiteboard: [has patch][need review gavin][need bug 419656]
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...
Attachment #307890 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 8

11 years ago
(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.
Whiteboard: [has patch][need review gavin][need bug 419656] → [has patch][need bug 419656]
(Assignee)

Comment 9

11 years ago
Created attachment 308996 [details] [diff] [review]
v1.3

Same patch. Less context. Same perf wins. ;)
Attachment #307890 - Attachment is obsolete: true
Attachment #308996 - Flags: approval1.9?
Comment on attachment 308996 [details] [diff] [review]
v1.3

a=mconnor on behalf of 1.9 drivers
Attachment #308996 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 11

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite-
Flags: in-litmus-
Flags: blocking1.9?
Resolution: --- → FIXED
Whiteboard: [has patch][need bug 419656]
Target Milestone: --- → mozilla1.9beta5
You need to log in before you can comment on or make changes to this bug.