Closed Bug 421315 Opened 17 years ago Closed 17 years ago

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

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

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
Summary: Autocomplete gets invalidated too often (each chunk, popup open) → Just re-use autocomplete richlistitems instead of re-adjusting for no purpose
Attached patch v1 (obsolete) (deleted) — Splinter Review
(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
Attachment #307753 - Flags: review?(gavin.sharp)
Attached patch v1.1 (obsolete) (deleted) — Splinter Review
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)
Attachment #307757 - Attachment is patch: true
Attachment #307757 - Attachment mime type: application/octet-stream → text/plain
Attached patch v1.2 (obsolete) (deleted) — Splinter Review
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)
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?
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+
(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]
Attached patch v1.3 (deleted) — Splinter Review
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+
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
Closed: 17 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.

Attachment

General

Creator:
Created:
Updated:
Size: