Closed
Bug 504422
Opened 15 years ago
Closed 15 years ago
Be smarter with our strings for AUTOCOMPLETE_MATCH SQL function
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla1.9.2b1
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
Details
Attachments
(1 file, 2 obsolete files)
5.70 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
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•15 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•15 years ago
|
||
Attachment #389023 -
Flags: review?(dwitte)
Attachment #389023 -
Flags: review?(dietrich)
Comment 3•15 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•15 years ago
|
||
Looks like I just forgot to qrefresh that logic error. Fixed locally.
Comment 5•15 years ago
|
||
Comment on attachment 389023 [details] [diff] [review] v1.0 r=me w/ dwitte's change.
Attachment #389023 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 6•15 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•15 years ago
|
||
Like I said - this doesn't work right.
Attachment #389023 -
Attachment is obsolete: true
Assignee | ||
Comment 8•15 years ago
|
||
All of the dependent string stuff is actually a bad idea - violates the SQLite API. Sadfaces.
Assignee | ||
Comment 9•15 years ago
|
||
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•15 years ago
|
Attachment #392326 -
Flags: review? → review?(dietrich)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Comment 10•15 years ago
|
||
Comment on attachment 392326 [details] [diff] [review] v3.0 r=me
Attachment #392326 -
Flags: review?(dietrich) → review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review dietrich] → [can land]
Assignee | ||
Comment 11•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/367678baeb7e
Status: ASSIGNED → RESOLVED
Closed: 15 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.
Description
•