Optimize AwesomeBar if it finished searching and found fewer than maxResults

VERIFIED FIXED in Firefox 3 beta5

Status

()

Firefox
Address Bar
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

({perf})

Trunk
Firefox 3 beta5
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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?
(Assignee)

Comment 1

10 years ago
This patch depends on bug 422490.
Depends on: 422490
(Assignee)

Comment 2

10 years ago
Created attachment 308943 [details] [diff] [review]
v1

Definitely won't be able to apply this without bug 422490.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #308943 - Flags: review?(dietrich)
(Assignee)

Comment 3

10 years ago
FYI, this even works nicely with special searches from bug 395161. Adaptive results still do their thing and keyword results keep updating.
(Assignee)

Comment 4

10 years ago
Created attachment 309227 [details] [diff] [review]
v1.1

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)
(Assignee)

Comment 5

10 years ago
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-
(Assignee)

Updated

10 years ago
Blocks: 422177
(Assignee)

Comment 7

10 years ago
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]
(Assignee)

Comment 8

10 years ago
Created attachment 311021 [details] [diff] [review]
v1.2

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+
(Assignee)

Comment 10

10 years ago
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+
(Assignee)

Comment 12

10 years ago
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
Last Resolved: 10 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [has patch][needed for 422177]
Target Milestone: --- → Firefox 3 beta5
(Assignee)

Comment 13

10 years ago
Created attachment 311225 [details] [diff] [review]
leak bustage fix

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
(Assignee)

Comment 14

10 years ago
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.