452836 - Optimize calls to sqlite3_prepare_v2

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

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

Comment 1

[Shawn Wilsher :sdwilsh]

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.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 3

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/6c46bdf4a7d7

Comment 4

[mmc]

Since you changed nsPromiseFlatCString to PromiseFlatCString, shouldn't have you done the same also in the logging bit just below?

Comment 5

[Shawn Wilsher :sdwilsh]

That would be outside the scope of this bug.  Feel free to file a new bug for that.

Comment 6

[[e-mail]]

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.

Comment 7

[Shawn Wilsher :sdwilsh]

So PromiseFlatCString actually copies the string first?

Comment 8

[[e-mail]]

(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.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

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

Comment 10

[[e-mail]]

Comment on attachment 336283 [details] [diff] [review]
v2.0

That's a neat way to avoid the documentation/code mismatch :-)

Comment 11

[Shawn Wilsher :sdwilsh]

Passing -1 is actually a recommended practice, but I think there was some miscommunication between myself and drh...  Oh well!

Comment 12

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/eecb83d9acdc

Comment 13

[Alfred Kayser]

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...

Comment 14

[Shawn Wilsher :sdwilsh]

New bugs please