Closed Bug 504422 Opened 11 years ago Closed 11 years ago

Be smarter with our strings for AUTOCOMPLETE_MATCH SQL function

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2b1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 2 obsolete files)

fixupURISpec returns an nsString, but this results in us allocating said string three times (once in function, once by generating a temporary for returning, and once for the return variable).  Ouch.  We should just make it an out param.

Also, all the calls to GetString end up copying.  We really want to just call GetSharedString and store it in the dependent string ourselves.

Less string copying will make this a wee bit faster.
In fact, profiling shows that calling GetString (because it calls Assign) accounts for ~1% of the time in the function!  Yikes!
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #389023 - Flags: review?(dwitte)
Attachment #389023 - Flags: review?(dietrich)
Comment on attachment 389023 [details] [diff] [review]
v1.0

>-    nsString fixedSpec;
>-    if (IsUTF8(unescapedSpec))
>-      CopyUTF8toUTF16(unescapedSpec, fixedSpec);
>+    NS_ASSERTION(_fixedSpec.IsEmpty(),
>+                 "Passing a non-empty string as an out parameter!");
>+    if (!IsUTF8(unescapedSpec))
>+      CopyUTF8toUTF16(unescapedSpec, _fixedSpec);

way to invert your logic, shawn :)

r=dwitte with that fixed.
Attachment #389023 - Flags: review?(dwitte) → review+
Looks like I just forgot to qrefresh that logic error.  Fixed locally.
Comment on attachment 389023 [details] [diff] [review]
v1.0

r=me w/ dwitte's change.
Attachment #389023 - Flags: review?(dietrich) → review+
This landed, but was backed out once I built in a debug build and hit assertions.  The trivial fix passed tests (I'll attach that shortly), so this is going to require more work.  I'll do that /after/ I fix all the blockers for bug 455555.
Attached patch v2.0 (obsolete) — Splinter Review
Like I said - this doesn't work right.
Attachment #389023 - Attachment is obsolete: true
All of the dependent string stuff is actually a bad idea - violates the SQLite API.  Sadfaces.
Attached patch v3.0Splinter Review
This passes tests and uses the string API to reduce some coping and malloc's when possible.
Attachment #390061 - Attachment is obsolete: true
Attachment #392326 - Flags: review?
Attachment #392326 - Flags: review? → review?(dietrich)
Whiteboard: [needs review dietrich]
Comment on attachment 392326 [details] [diff] [review]
v3.0

r=me
Attachment #392326 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich] → [can land]
http://hg.mozilla.org/mozilla-central/rev/367678baeb7e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla1.9.2b1
You need to log in before you can comment on or make changes to this bug.