Closed
Bug 452836
Opened 16 years ago
Closed 16 years ago
Optimize calls to sqlite3_prepare_v2
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: sdwilsh, Assigned: sdwilsh)
Details
Attachments
(2 files)
1.33 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.55 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
If you pass a length, sqlite will copy the whole string. If you don't pass a length, sqlite assumes it's a null-terminated string and will use the string without copying it. While we are at it, we should use PromiseFlatCString and not nsPromiseFlatCString
Assignee | ||
Comment 1•16 years ago
|
||
Although, I'm told that sqlite checks if the length given is '\0', and if it is, does not copy the string. We can save a call to strlen then by passing Length() + 1.
Assignee | ||
Updated•16 years ago
|
Summary: Pass -1 for length argument in sqlite3_open_v2 → Pass -1 for length argument in sqlite3_prepare_v2
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #336090 -
Flags: review?(bholley)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review bholley]
Assignee | ||
Updated•16 years ago
|
Summary: Pass -1 for length argument in sqlite3_prepare_v2 → Optimize calls to sqlite3_prepare_v2
Updated•16 years ago
|
Attachment #336090 -
Flags: review?(bholley) → review+
Assignee | ||
Comment 3•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/6c46bdf4a7d7
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bholley]
Target Milestone: --- → mozilla1.9.1b1
Since you changed nsPromiseFlatCString to PromiseFlatCString, shouldn't have you done the same also in the logging bit just below?
Assignee | ||
Comment 5•16 years ago
|
||
That would be outside the scope of this bug. Feel free to file a new bug for that.
Comment 6•16 years ago
|
||
The documentation at http://www.sqlite.org/capi3ref.html doesn't seem to match relevant line of source code: if( nBytes>=0 && zSql[nBytes]!=0 ){ This check assumes nByte doesn't include the null-terminator byte. In fact we the calls to sqlite3_prepare_v2 in mozStorageConnection all pass the string's length excluding the null-terminator byte. Initialize goes as far as to copy a literal string first! As I read the code, you should pass aSQLStatement.BeginReading() and aSQLStatement.Length() and sqlite will only copy if it needs to.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•16 years ago
|
||
So PromiseFlatCString actually copies the string first?
Comment 8•16 years ago
|
||
(In reply to comment #7) > So PromiseFlatCString actually copies the string first? I'm not sure why you're asking this. PromiseFlatCString and sqlite_prepare_v2 both contain code to null-terminate an unterminated string by copying it.
Assignee | ||
Comment 9•16 years ago
|
||
Maybe I should just look at the code next time instead of hearing how it works from drh... Just pass -1 since that is the most efficient code path
Attachment #336283 -
Flags: review?(neil)
Updated•16 years ago
|
Attachment #336283 -
Flags: review?(neil) → review+
Comment 10•16 years ago
|
||
Comment on attachment 336283 [details] [diff] [review] v2.0 That's a neat way to avoid the documentation/code mismatch :-)
Assignee | ||
Comment 11•16 years ago
|
||
Passing -1 is actually a recommended practice, but I think there was some miscommunication between myself and drh... Oh well!
Whiteboard: [has patch][has review][can land]
Assignee | ||
Comment 12•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/eecb83d9acdc
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
Comment 13•16 years ago
|
||
I noticed some local nsCString's here. Should these not be replaced by nsCAutoString (because generally these queries are less than 64 characters, thus saving a heap malloc)? Also TableExists nd IndexExists are roughly the same (but with subtle and probably unintended differences in error handling), so these should be factored out...
Assignee | ||
Comment 14•16 years ago
|
||
New bugs please
You need to log in
before you can comment on or make changes to this bug.
Description
•