Closed Bug 422491 Opened 14 years ago Closed 14 years ago

Optimize AwesomeBar if it finished searching and found fewer than maxResults

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

We can avoid bug 412730 but still do some optimization by only using *finished* results with no possibility of there being more results. This means we haven't hit maxResults and the search finished.

We only need to look through the urls from the previous search while making sure not to kill keyword results and stuff.
Flags: blocking-firefox3?
This patch depends on bug 422490.
Depends on: 422490
Attached patch v1 (obsolete) — Splinter Review
Definitely won't be able to apply this without bug 422490.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #308943 - Flags: review?(dietrich)
FYI, this even works nicely with special searches from bug 395161. Adaptive results still do their thing and keyword results keep updating.
Attached patch v1.1 (obsolete) — Splinter Review
Switched to a nsStringArray to keep things sane ;)

Too bad.. no more triple pointer, dereference, postincrement, dereference, assign address magic! :p


>+  // The void * argument is a pointer to a nsString * array. We post-increment
>+  // the contents of the pointer so that it points to the next nsString *
>+  // element for the next enumeration. For the current nsString *, assign it
>+  // the address of the current hashtable entry's string.
>+  *(*(const nsAString ***)aArg)++ = &aKey;
Attachment #308943 - Attachment is obsolete: true
Attachment #309227 - Flags: review?
Attachment #308943 - Flags: review?(dietrich)
Oh, forgot the main reason for saneness. Apparently compiling on windows didn't support C99 ability for variable length stack arrays.
This will not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Blocks: 422177
Bug 422490 was a step to get this patch in, which is another step towards bug 422177 which is a P2 blocker.

I'll unbitrot this patch from moving it to the top of my stack above the special sauce bug 395161 and keyword magic bug 392143.
Flags: blocking-firefox3- → blocking-firefox3?
Priority: -- → P2
Whiteboard: [has patch][need review dietrich][needed for 422177]
Attached patch v1.2Splinter Review
Moved to the top of my stack and pulled in some changes from bug 395161 because we need to use that query fragment in multiple places.
Attachment #309227 - Attachment is obsolete: true
Attachment #311021 - Flags: review?(dietrich)
Attachment #309227 - Flags: review?
Comment on attachment 311021 [details] [diff] [review]
v1.2

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

Making things go faster and needed for bug 422177 which makes things go really fast.
Attachment #311021 - Flags: approval1.9b5?
Attachment #311021 - Flags: approval1.9?
Whiteboard: [has patch][need review dietrich][needed for 422177] → [has patch][needed for 422177]
Comment on attachment 311021 [details] [diff] [review]
v1.2

a=mconnor on behalf of 1.9 drivers
Attachment #311021 - Flags: approval1.9b5?
Attachment #311021 - Flags: approval1.9b5+
Attachment #311021 - Flags: approval1.9?
Attachment #311021 - Flags: approval1.9+
To see this working, you need to type in some search terms that result in ~5 results. Have the search throbber finish spinning, then type a letter that matches in fewer than 5 of those results. It should show the results right away instead of showing nothing and an activity throbber.

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.151; previous revision: 1.150
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.50; previous revision: 1.49
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [has patch][needed for 422177]
Target Milestone: --- → Firefox 3 beta5
Attached patch leak bustage fixSplinter Review
Apparently bad stuff happens with a global const nsCString reference. Converted it to a #define.

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.52; previous revision: 1.51
done

We were leaking 8 bytes..
Error: Leak Test Failed: Number of leaks 8 is greater than LeakFailureThreshold 0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1206254100.1206254570.32461.gz
FYI, the leak was reported here..

|<----------------Class--------------->
                        nsStringBuffer

|<-----Bytes------>
 Per-Inst   Leaked
        8        8

|<----------------Objects---------------->
    Total      Rem      Mean       StdDev
    32460        1 ( 4330.68 +/-  2367.57)

|<--------------References-------------->|
     Total      Rem      Mean       StdDev
    59418        1 ( 4792.61 +/-  2814.15)
This is pretty obfuscated code here, especially the #define above the former const ref.  It's also I think doing scary thing with temporaries for the strings, and could break depending on how the final #define is being used.

This really just wants to be a nsPrintfCString for the entire thing in some Init function.. maybe with multiple nsPrintfCStrings for the fragments that are now being done via string pasting in the #define.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 beta5 → Firefox 3
Target Milestone: Firefox 3 → Firefox 3 beta5
added this expectation to test case https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=8955
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.