Closed Bug 414285 Opened 14 years ago Closed 14 years ago

Refactor AutoCompleteTagsSearch token splitting code and persist tokens

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
Yay unit tests.

I'm aware that mCurrentSearchTokens.Count() will be non-zero if we have any search term.. see followup.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #299649 - Flags: review?(seth)
Blocks: 414288
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 on attachment 299649 [details] [diff] [review]
v1

clearing review request, based on my previous comment.

can you double check?
Attachment #299649 - Flags: review?(seth)
(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.
> 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 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.
Attachment #299649 - Flags: review?(seth)
Blocks: 401869
Comment on attachment 299649 [details] [diff] [review]
v1

no longer applies
Attached patch v1.1 (obsolete) — Splinter Review
Unbitrot.
Attachment #299649 - Attachment is obsolete: true
Attachment #300274 - Flags: review?(dietrich)
Attachment #299649 - Flags: review?(seth)
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. !
Attachment #300274 - Flags: review?(seth)
Comment on attachment 300274 [details] [diff] [review]
v1.1

this patch was noticeably laggy against big history/bookmarks set.
Attachment #300274 - Flags: review?(seth)
Attachment #300274 - Flags: review?(dietrich)
This is a build with this patch and the multi-word searching:

https://build.mozilla.org/tryserver-builds/2008-01-29_23:19-edward.lee@engineering.uiuc.edu-multiword.stopsearch.nonenglish/

It seems to be just as fast as before. Perhaps my 11776 pages isn't enough to notice?
Blocks: 413784
(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:58-edward.lee@engineering.uiuc.edu-multi.0stop.escap.adapt/
No longer blocks: 413784
Attached patch v1.2 (obsolete) — Splinter Review
Now with more code reuse!
Attachment #300274 - Attachment is obsolete: true
Attachment #301076 - Flags: review?
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.
Attachment #301076 - Flags: review? → review?(dietrich)
Comment on attachment 301076 [details] [diff] [review]
v1.2

looks good, r=me. thanks for doing the perf testing.
Attachment #301076 - Flags: review?(dietrich) → review+
Attached patch v1.3Splinter Review
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.
Attachment #301076 - Attachment is obsolete: true
Attachment #301359 - Flags: review?(dietrich)
Comment on attachment 301359 [details] [diff] [review]
v1.3

r=me
Attachment #301359 - Flags: review?(dietrich) → review+
Comment on attachment 301359 [details] [diff] [review]
v1.3

For after unfreeze.
Attachment #301359 - Flags: approval1.9?
Comment on attachment 301359 [details] [diff] [review]
v1.3

a=beltzner
Attachment #301359 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
You need to log in before you can comment on or make changes to this bug.