422177 - Awesome bar interrupts typing, so continue from where it left off

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Ed Lee :Mardak]

At least for the backend when the user types in something which causes the autocomplete controller to poke nsNavHistory..

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp&rev=1.46&mark=265,275#265

line 275: nsNavHistory::StartSearch
line 265: nsNavHistory::DoneSearching

To get some more, you can start at nsAutoCompleteController

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp&mark=973#973

nsAutoCompleteController::StartSearch

Comment 1

[Ed Lee :Mardak]

You can reuse nsNavHistory::DoneSearching with nsAutoCompleteController::StartSearch

Alternatively, just use

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp&mark=1267#1267

line 1267: nsAutoCompleteController::PostSearchCleanup

Comment 2

[Ed Lee :Mardak]

If it's easier from js/xul, you can do something similar to what dolske used to show the location bar throbber:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rev=1.444&mark=321-322#321

- onsearchbegin="LocationBarHelpers._searchBegin();"
- onsearchcomplete="LocationBarHelpers._searchComplete();"
+ onsearchbegin="connectShark(); startShark(); LocationBarHelpers._searchBegin();"
+ onsearchcomplete="stopShark(); disconnectShark(); LocationBarHelpers._searchComplete();"

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm not sure I understand the previous comments, but I understand the summary well enough to know that this will be one of those "human performance" issues that makes the browser feel slower.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

When I type in a new URL, I sometimes see 100% CPU spikes for 1-2 seconds, which I didn't notice before.  (This is a case where it's already decided there are no hits, and typing more should be a no-op for the awesomebar, because narrowing empty always gives empty, right?)

I can try to get some cycles to shark early next week; activity monitor's CPU widget lights up like Trinity, so it shouldn't be hard to spot.

Comment 5

[Ed Lee :Mardak]

Bug 414257 should have fixed the issue of typing in new urls... assuming the throbber stops and shows no results so that it knows it can't find anything when you type even more.

I suppose if you're still typing when the throbber is still spinning, it would keep trying to start a new search.

I'll think about how we could take a currently-searching result and continue from where it is instead of starting from the beginning each time... hrmm....

Comment 6

[Ed Lee :Mardak]

Attachment: v1

This builds on top of bug 422491 which adds support of reusing previous search results if the search stopped.

This patch would make it so that it keeps track of how many pages it's searched through so far (which chunk it's on), so when it gets a new query, start with the urls that it has already found from earlier chunks and continue from the chunk it left off at.

Comment 7

[Ed Lee :Mardak]

Can people try this build?

https://build.mozilla.org/tryserver-builds/2008-03-21_02:[e-mail]/

Maybe also up this pref timeout to something bigger like..

browser.urlbar.search.timeout 200

Or if you really want to see it restarting from its current position, set it to something big like 1000 and look at how long it takes to find "mozilla" then add more like "mozillablahlbhlahalbh" on trunk then compare it to this build.

This also makes it so results from later chunks don't keep disappearing/reappearing when you type more because it no longer starts from the very beginning of history.

Comment 8

[Ed Lee :Mardak]

Attachment: screenshot of trunk activity

Comment 9

[Ed Lee :Mardak]

Attachment: screenshot of v1 activity

For both these screenshots (this and attachment 311008 [details]), I slowly typed out "planet mozillablah" one letter at a time after ~5 bars appears in the activity cpu monitor.

Here's a summary of the results for various things typed..
p: Stop early because it found a ton of p results
pl-plan: Finds several results..
plane-planet mozilla: Around 7 results
planet mozillab-: No results

Currently on trunk, it's searching through the whole history on each letter unless it finds maxRichResults and quits early.

With v1, the search will restart from where it left off. Additionally with bug 422491, it won't even bother searching through history once it has reached the end of history and found all the pages it could find.

Comment 10

[Ed Lee :Mardak]

Attachment: screenshot of v1 activity, timeout=200

I increased the search timeout from 50 to 200, so there's more pause between each chunk. We kept it low before because a bigger number means a longer time to go through all of history, but that's not as bad if we can restart from where we left off.

Comment 11

[Ed Lee :Mardak]

Attachment: screenshot of v1 activity, timeout=200, chunkSize=500

Same as previous screenshot with timeout 50->200 but also chunkSize 1000->500.

So with half the size, it's doing ~half the work and hitting the search timeout twice as much. It's not really half the work because there's overheads of starting new chunks. The peaks aren't that much lower and now they're twice as fat.

So probably just increase the search timeout time?

Comment 12

[Ed Lee :Mardak]

FYI: For attachment 311008 [details]:

The first several sets of "bars" are low because of bug 414257 where we stop early when we have enough results.

The last sets of bars (at the very end) is very low because of bug 422490 because we don't bother searching if the previous results had nothing.

Comment 13

[Ed Lee :Mardak]

Attachment: v1.1

I've moved this patch to 2nd in my stack after bug 422491, so no special sauce bug 395161 and keyword search bug 392143 to mess up the context.

Also, updated the comments for review. Additionally, got some more optimization by not requiring prevMatchCount > 0 so that we can continue from where the previous search left off even if we haven't found any results yet.

Comment 14

[Ed Lee :Mardak]

Here's a build with v1.1 which has the extra optimization for "current results = none, not done searching". So if you start typing "blahblah" then type "blahblahblah", it'll find nothing faster and stop interrupting even earlier.

https://build.mozilla.org/tryserver-builds/2008-03-21_17:[e-mail]/

Bug 422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults
Bug 422177 - Awesome bar interrupts typing, so continue from where it left off
Bug 424454 - Typed urls (like domain roots) need more frecency bonus
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 418257 - Show what part of which tags match for urlbar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 249468 - Add all bookmark keywords to location bar autocomplete drop-down list
Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Bug 420437 - Search and emphasize quoted strings with spaces
Bug 422698 - different levels of URL decoding for address bar and autocomplete suggestions
Bug 423942 - "Clear List" in download manager should be "Clear Current List" during the search
Bug 423718 - Use native platform colors for URLs in the location bar autocomplete, make the line between rows lighter
Bug 414326 - Use DownloadUtils for software update downloads
Bug 424251 - current page's favicon displayed when letting an address starting with a protocol auto-fill

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 311029 [details] [diff] [review]
v1.1


>     } else {
>-      // We must have more than 0 but fewer than the max results, so create an
>-      // optimized query that only looks at these urls
>+      // We either have a previous in-progress search or a finished search that
>+      // has more than 0 results. We can continue from where the previous
>+      // search left off, but first we want to create an optimized query that
>+      // only searches through the urls that were previously found

now that we're handling the in-progress case, should you LIMIT mDBPreviousQuery by chunk size, since the result set could be significantly larger?

Comment 16

[Ed Lee :Mardak]

(In reply to comment #15)
> >-      // We must have more than 0 but fewer than the max results, so create an
> >-      // optimized query that only looks at these urls
> >+      // We either have a previous in-progress search or a finished search that
> >+      // has more than 0 results. We can continue from where the previous
> >+      // search left off, but first we want to create an optimized query that
> >+      // only searches through the urls that were previously found
> now that we're handling the in-progress case, should you LIMIT mDBPreviousQuery
> by chunk size, since the result set could be significantly larger?
mDBPreviousQuery will only find at most maxResults number of items because we're building a list of urls that come from the currently found URLs.

On a separate note, are we trying to get this in for beta5 or after?

Comment 17

[Ed Lee :Mardak]

Oh, should we also increase the search timeout with this bug as well?

Comment 18

[Ed Lee :Mardak]

(In reply to comment #16)
> (In reply to comment #15)
> > >-      // We must have more than 0 but fewer than the max results, so create an
> > >-      // optimized query that only looks at these urls
> > now that we're handling the in-progress case, should you LIMIT mDBPreviousQuery
> > by chunk size, since the result set could be significantly larger?
> mDBPreviousQuery will only find at most maxResults number of items because
> we're building a list of urls that come from the currently found URLs.
Ah, I see why you quoted that part now. :p mDBPreviousQuery will find from 0 to maxResults urls inclusive whereas before, it would be 0 to maxResults not including maxResults.

It's safe to do that because..
Pretend the previous in-progress search found 5 results in chunk0, 5 in chunk1, 2 in chunk2, but didn't completely finish processing chunk2 because it stopped early after finding 12 results. If the user types something, it'll start with the urls found from chunk0-2 and start searching again from chunk2. So we do a "previous query" with the 12 results and start from the same chunk that the previous one stopped at. So we quickly get the 12 urls from chunk0-2 and *reprocess* some stuff from chunk2 finding the same last 2 urls found previously, but that's okay because we filter out duplicates. And we continue to chunk3 if necessary.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 311029 [details] [diff] [review]
v1.1

ok, looks fine. r=me. about the timeout, 200 seems high. if this makes b5, then maybe just up it to 100. if it doesn't then we have more time to get feedback from nightly testers and can push it.

yeah i realized that about LIMIT right after i asked :P

Comment 20

[Ed Lee :Mardak]

Comment on attachment 311029 [details] [diff] [review]
v1.1

"that is some compelling graphage"

Comment 21

[Ed Lee :Mardak]

Attachment: v1.2

Just additionally switching timeout from 50ms to 100ms so that more cycles can be spent doing non-search stuff.

Comment 22

[Mike Connor [:mconnor]]

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

a=mconnor on behalf of 1.9 drivers

Comment 23

[Ed Lee :Mardak]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.318; previous revision: 1.317
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.279; previous revision: 1.278
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.152; previous revision: 1.151
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.51; previous revision: 1.50
done

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311009 [details]
screenshot of v1 activity

I have reviewed this screenshot, and I approve of it.

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050806 Minefield/3.0pre

Prior to this fix (and/or perhaps the slew of others in this area), I too saw the CPU-spiking behavior shaver references in comment 4, among other result-list-building maladies; post-fix, those are gone, and the inline results are narrowed without the jarring rebuilding of the list.

(Hard to conclusively qualify that it's fixed, but it is, by Jove!)