414285 - Refactor AutoCompleteTagsSearch token splitting code and persist tokens

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Ed Lee :Mardak]

It would be useful to save the tokens around if another search needed it other than just the TagsSearch. E.g., full history search if it wanted to search on multiple words.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Yay unit tests.

I'm aware that mCurrentSearchTokens.Count() will be non-zero if we have any search term.. see followup.

Comment 2

[Seth Spitzer]

Comment on attachment 299649 [details] [diff] [review]
v1

first, I like the idea here.

second, I'm trying to convince my self (from just reading the patch) that this is correct.

from a quick read, it doesn't look right because

+  if (!mCurrentSearchTokens.Count()) {}

for clarity, consider using mCurrentSearchTokens[0] instead of mCurrentSearchString in that case.

the reason this is probably working is the else block can handle 1 token.

but, we want the first block in the case of one token, as it is precompiled.

Comment 3

[Seth Spitzer]

Comment on attachment 299649 [details] [diff] [review]
v1

clearing review request, based on my previous comment.

can you double check?

Comment 4

[Ed Lee :Mardak]

(In reply to comment #2)
> +  if (!mCurrentSearchTokens.Count()) {}
> 
> for clarity, consider using mCurrentSearchTokens[0] instead of
> mCurrentSearchString in that case.
> 
> the reason this is probably working is the else block can handle 1 token.
> 
> but, we want the first block in the case of one token, as it is precompiled.

Can you take a look at bug 414288 comment 0? Using the precompiled vs dynamically generating the query for *one* token shouldn't be much of an issue.

Comment 5

[Seth Spitzer]

> Using the precompiled vs dynamically generating the query for *one* token 
> shouldn't be much of an issue.

but don't we issue these queries on every key stroke in the url bar?

this saves us from doing that work until you hit your first space.

if we do decide to take your approach, we should remove the precomiled query and simplifying the code.

can you measure how much using the precomiled query saves us?  

you might be right, and I've prematurely optimized.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 299649 [details] [diff] [review]
v1

(In reply to comment #5)
> if we do decide to take your approach, we should remove the precomiled query
> and simplifying the code.
That'll be done in bug 414288 which will then allow for optimizations like bug 414426.

> but don't we issue these queries on every key stroke in the url bar?
> this saves us from doing that work until you hit your first space.
This is what the existing code is doing for each character stroke:
>-  nsString::const_iterator strStart, strEnd;
>-  mCurrentSearchString.BeginReading(strStart);
>-  mCurrentSearchString.EndReading(strEnd);
>-  nsString::const_iterator start = strStart, end = strEnd;
>-
>-  nsStringArray tagTokens;
>-
>-  // check if we have any delimiters
>-  while (FindInReadable(NS_LITERAL_STRING(" "), start, end,
>-                        nsDefaultStringComparator())) {
>-    nsAutoString currentMatch(Substring(strStart, start));
>-    currentMatch.Trim("\r\n\t\b");
>-    if (!currentMatch.IsEmpty())
>-      tagTokens.AppendString(currentMatch);
>-    strStart = start = end;
>-    end = strEnd;
>-  }
It also needs to look for the space character. However, this does save on iterating over the 1 search term to create the query, but bug 414426 will probably be more useful for perf.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 299649 [details] [diff] [review]
v1

no longer applies

Comment 8

[Ed Lee :Mardak]

Attachment: v1.1

Unbitrot.

Comment 9

[Ed Lee :Mardak]

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

Seth, this patch is needed for bug 401869 which I believe will increase functionality by a lot. We can make it for beta 3. !

Comment 10

[Dietrich Ayala (:dietrich)]

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

this patch was noticeably laggy against big history/bookmarks set.

Comment 11

[Ed Lee :Mardak]

This is a build with this patch and the multi-word searching:

https://build.mozilla.org/tryserver-builds/2008-01-29_23:[e-mail]/

It seems to be just as fast as before. Perhaps my 11776 pages isn't enough to notice?

Comment 12

[Ed Lee :Mardak]

(In reply to comment #10)
> (From update of attachment 300274 [details] [diff] [review])
> this patch was noticeably laggy against big history/bookmarks set.
Are you talking about this and bug 401869 together? The number of pages shouldn't affect things unless many results have frecency -1. Because we're chunking and selecting on the frecency index, it should be fairly independent of the total number of pages.

Perhaps this the lag is related to trying to do tags searches at the same time. See bug 414426.

Personally things are very fast on my very slow machine using this build:
https://build.mozilla.org/tryserver-builds/2008-01-30_01:[e-mail]/

Comment 13

[Ed Lee :Mardak]

Attachment: v1.2

Now with more code reuse!

Comment 14

[Ed Lee :Mardak]

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

Rerequesting review. This will be needed for bug 401869.

See bug 401869 comment 24 about perf measurements.

Comment 15

[Dietrich Ayala (:dietrich)]

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

looks good, r=me. thanks for doing the perf testing.

Comment 16

[Ed Lee :Mardak]

Attachment: v1.3

Generate lowercase tokens by changing the original search to be lower case as well. We only do case-insensitive matches anyway.

This will help reduce the size of the query for bug 414426 as well.

Comment 17

[Dietrich Ayala (:dietrich)]

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

r=me

Comment 18

[Ed Lee :Mardak]

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

For after unfreeze.

Comment 19

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

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

a=beltzner

Comment 20

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.130; previous revision: 1.129
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.38; previous revision: 1.37
done