406259 - update 5 richlistitems at a time to improve url bar autocomplete performance

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

reduce perception of "clunky" of urlbar autocomplete results

in d.a.f, Benjamin writes:  "It's large... it feels large and clunky for some reason".

colin and sayre expressed the same feedback, especially on the mac.

a few of thoughts and things that might help:

1) only show 6 or 7 results instead of 10 (see bug #406257)

2)  instead of uncollapsing and setting row height once to the min(match count, max row count) and then filling in the values, we could adjust the row height and uncollapse as we go.

2) we can still update rows on a timeout (necessary to not block typing in the url bar), but we could try updating the visible ones in one chunk, and the rest, one at a time.  (note, in my previous tests, one at a time was necessary to not block the ui when typing fast.)

I think a combination of #1 and #2 would help things here.

Comment 1

[Tony Mechelynck [:tonymec]]

(In reply to comment #0)
[...]
> 1) only show 6 or 7 results instead of 10 (see bug #406257)
[...]

but if and when results grow to three lines each (bug 406241) this would mean using 3 or 4 results instead -- which seems to me to be too few to be useful. What about keeping the same number of results, but maybe in a smaller font, or maybe in a box with a given maxheight, but with a scrollbox if results overflow? (The latter would, I suppose, have the advantage of being CSS-friendly)

Comment 2

[(not reading, please use [e-mail] instead)]

> e could try updating the visible ones in one chunk

vlad and I did some work today on the "faabar", and chunking (even 5 at a time) is going to be a noticable perf win, something simple and worth doing for m10

> note, in my previous tests, one at a time was necessary to not block the ui 
> when typing fast.

With fewer max results (currently 25) this is less of an issue.

I'm going to make this bug cover updating 5 rows at a time, and spin the other issues in this bug off to new bugs.

Comment 3

[(not reading, please use [e-mail] instead)]

not a p1, because we could ship with it as is, but it would be nice for b2 to have the "faabar" be faster.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: by default, update 5 richlistitems at a time before yielding

this is controlled by the browser.urlbar.richResultsChunkSize pref, as if you bumped up the browser.urlbar.maxRichResults pref you would want to lower this one.

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 291833 [details]
by default, update 5 richlistitems at a time before yielding

when am I going to learn to stop using browser prefs in toolkit?

thanks dietrich, fixing.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 7

[Benjamin Smedberg]

FWIW, I think that clunky feeling is primarily because the title and URL have different font sizes. It looks very busy that way.

Comment 8

[Robert Sayre]

(In reply to comment #3)
> we could ship with it as is

I wouldn't.

(In reply to comment #7)
> FWIW, I think that clunky feeling is primarily because the title and URL have
> different font sizes. It looks very busy that way.

Yeah, it's not particularly good-looking either, so it's not clear why it's like that.

Comment 9

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

(In reply to comment #7)
> FWIW, I think that clunky feeling is primarily because the title and URL have
> different font sizes. It looks very busy that way.

I agree. Maybe this is just me not being quite used to making use of the awesomebar's awesome, but I still find myself looking for URLs at least half of the time, so having them be smaller than the title bothers me.

Comment 10

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

Comment on attachment 291838 [details] [diff] [review]
patch

Why are we making this a pref? I think we should just choose a reasonable default, unless you expect need for easy experimentation or something.

Comment 11

[(not reading, please use [e-mail] instead)]

> Why are we making this a pref?

for both experimentation (vlad and I were measuring the difference between 1,2,5,10) and because if you were to set browser.urlbar.maxRichResults to something large (100), you'd want this new pref to be set to something very small (1).

There are other performance changes (I'll cc you) that might make us be able to set it to an even larger value, possibly the same as browser.urlbar.maxRichResult (which in effect removes the setTimeout())

Comment 12

[(not reading, please use [e-mail] instead)]

benjamin / gavin:  I'll log a spin off bug about title / url not being the same font size (and allow faaborg / beltzner to comment on the design.)  I didn't realize that is what you meant by "clunky".

robert: "it's not particularly good-looking either", can you be more specific, or does one of the existing bugs hanging off of the meta bug (#405745) cover your concerns?

>> we could ship with it as is
>I wouldn't.

I was thinking b2 / m10, not Fx3 final.

Comment 13

[Robert Sayre]

> 
> robert: "it's not particularly good-looking either", can you be more specific,
> or does one of the existing bugs hanging off of the meta bug (#405745) cover
> your concerns?

I just meant the large font isn't particularly good looking--wondering why we have it. But I have been paying close attention to it today and noticed some more things: 

The vertical placement is odd, so the URL is closer to the border than the title text. This makes it look like the results are sitting low in the rows.

When you open a new window, the green triangle flashes before being replaced by the star. 

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #11)
> > Why are we making this a pref?
> 
> for both experimentation (vlad and I were measuring the difference between
> 1,2,5,10) and because if you were to set browser.urlbar.maxRichResults to
> something large (100), you'd want this new pref to be set to something very
> small (1).

You don't really ever want to lower the chunk value -- the total number of items shouldn't matter, since all you're really controlling is the number of items you can update at one time and still maintain interactivity.  It shouldn't take any longer per batch of items to update 10 batches or 5; though it depends on how extensive the reflow that's taking place is.

I wouldn't stress over this; I'd even avoid making it a pref at this point just to keep the code simple, as when some of the other stuff is completed post-b2 this is going to become even simpler still and might just go away entirely.

Comment 15

[(not reading, please use [e-mail] instead)]

based on vlad's comments, I'll remove the pref and simplify the patch and seek re-review from gavin.

Comment 16

[(not reading, please use [e-mail] instead)]

morphing bug to cover the perf issue. 

as for the font issue, see bug #407204

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: simplified patch

Comment 18

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

Comment on attachment 291944 [details] [diff] [review]
simplified patch

>Index: toolkit/content/widgets/autocomplete.xml

>           // determine the height dynamically.  (see bug #401939)
>           if (!this._rowHeight && this.richlistbox.childNodes.length)
>             this._rowHeight = this.richlistbox.childNodes[0].boxObject.height;
> 
>           var height = this._rowHeight * rows;
>           if (this._rowHeight && this.richlistbox.height != height)
>             this.richlistbox.height = height;

Why are we running the code above each time, anyways? Seems like we could split it out into it's own method that's called from invalidate() before calling _appendCurrentResult, or just move it into invalidate.

>-          if (this._currentIndex < this._matchCount) {
>+          for (var i = 0; i < 5; i++) {

Add a comment explaining why 5 is used here? Or put it in a <field> with a descriptive name, like the previous patch? Or both! :)

Comment 19

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

I think we should try to get this patch in for b2.

Comment 20

[(not reading, please use [e-mail] instead)]

Attachment: test case (1 at a time takes ~8 seconds, 5 at a time takes ~3 seconds)

Comment 21

[(not reading, please use [e-mail] instead)]

that test case is something vlad and I were using to verify that batching works (and for other performance improvements).

change numPerChildIter from 1 to 5 to see the improvements.

Comment 22

[Mike Schroepfer]

If this is ready we should definitely do it for b2.  Have we tested on a slow machine to make sure it doesn't lock up the ui?

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 24

[(not reading, please use [e-mail] instead)]

> Why are we running the code above each time, anyways? Seems like we could split
> it out into it's own method that's called from invalidate() before calling
> _appendCurrentResult, or just move it into invalidate.

I'll move that to a separate bug, but I remember there being a reason (such as this._rowHeight being 0)

> Add a comment explaining why 5 is used here? Or put it in a <field> with a
> descriptive name, like the previous patch? Or both! :)

fixed, thanks.

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 292011 [details] [diff] [review]
updated patch

carrying over gavin's review

Comment 26

[(not reading, please use [e-mail] instead)]

fixed.

Checking in autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.98; previous revision: 1.97
done

Comment 27

[(not reading, please use [e-mail] instead)]

> Why are we running the code above each time, anyways? Seems like we could split
> it out into it's own method that's called from invalidate() before calling
> _appendCurrentResult, or just move it into invalidate.

spun off to bug #407776

Comment 28

[(not reading, please use [e-mail] instead)]

benjamin wrote:

> FWIW, I think that clunky feeling is primarily because the title and URL have
>different font sizes. It looks very busy that way.

robert wrote:

> I just meant the large font isn't particularly good looking--wondering why we
> have it.

This issue covered by bug #407204

> The vertical placement is odd, so the URL is closer to the border than the
> title text. This makes it look like the results are sitting low in the rows.

I've started bug #407804 on this.

> When you open a new window, the green triangle flashes before being replaced
> by the star. 

Thanks for pointing that out!  I've started bug #407805 on this.

Comment 29

[(not reading, please use [e-mail] instead)]

Attachment: test case for testing other changes to autocomplete.xml (does 5 at a time, sets the height of the richlistbox to be 10 rows.)