Be smarter with our strings for AUTOCOMPLETE_MATCH SQL function

RESOLVED FIXED in mozilla1.9.2b1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.2b1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

10 years ago
In fact, profiling shows that calling GetString (because it calls Assign) accounts for ~1% of the time in the function!  Yikes!
(Assignee)

Comment 2

10 years ago
Created attachment 389023 [details] [diff] [review]
v1.0
Attachment #389023 - Flags: review?(dwitte)
Attachment #389023 - Flags: review?(dietrich)

Comment 3

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

Comment 4

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

Comment 6

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

Comment 7

10 years ago
Created attachment 390061 [details] [diff] [review]
v2.0

Like I said - this doesn't work right.
Attachment #389023 - Attachment is obsolete: true
(Assignee)

Comment 8

10 years ago
All of the dependent string stuff is actually a bad idea - violates the SQLite API.  Sadfaces.
(Assignee)

Comment 9

10 years ago
Created attachment 392326 [details] [diff] [review]
v3.0

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

Updated

10 years ago
Attachment #392326 - Flags: review? → review?(dietrich)
(Assignee)

Updated

10 years ago
Whiteboard: [needs review dietrich]
Comment on attachment 392326 [details] [diff] [review]
v3.0

r=me
Attachment #392326 - Flags: review?(dietrich) → review+
(Assignee)

Updated

10 years ago
Whiteboard: [needs review dietrich] → [can land]
(Assignee)

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/367678baeb7e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.