A connection that doesn't run anything on mainthread would be useful for a lot of reasons: - would allow dropping mutexes (no contention) - would be safer for addons (no risk of doing mainthread IO) while we fix consumers in the codebase, we should replace their connections with a pure async one.

< mak> well, the idea is someone makes this new pure-async API, once a consumer has no more synchronous API it can use it

I think I could work on this bug with some help. I've emailed Shawn to get more informations.

Hi Marco, did Shawn respond? Any update on this?

(In reply to Dietrich Ayala (:dietrich) from comment #3) > Hi Marco, did Shawn respond? Any update on this? No reply so far. Now I've started contributing to the js engine, but I'm still interested.

Sorry; sketching out an API here has taken longer than I thought it would. I should have responded to the e-mail, but I kept telling myself "oh, I'll have it done real soon..."

(In reply to Shawn Wilsher :sdwilsh from comment #5) > Sorry; sketching out an API here has taken longer than I thought it would. > I should have responded to the e-mail, but I kept telling myself "oh, I'll > have it done real soon..." Don't worry! Let me know when you've done the design of the API, and I'll see if I can work on it.

A few notes from an exchange with mak. At the moment, sqlite.jsm does the following: - read preferences from the main thread [async MTIO]; - open files from the main thread [MTIO]; - for each connection, fork an I/O thread; - close files on the main thread [MTIO]. The objective of this work is to change this to the following: - read preferences from the main thread [async MTIO]; - fork a shared thread and use it for opening/closing files; - for each connection, fork an I/O thread. We can't get rid of main thread preference reading easily, but doing this at a later stage should not cause any breaking API changes. Behind the scenes, this means: - implement mozIStorageAsyncConnection (bug 702559); - extend mozIStorageService to form a shared thread for opening/closing files; - extend mozIStorageService to open instances of mozIStorageAsyncConnection using said shared thread; - port Sqlite.jsm to use this new API.

(In reply to David Rajchenbach Teller [:Yoric] from comment #7) > - extend mozIStorageService to form a shared thread for opening/closing > files; I wonder if we couldn't instead just create the connection async thread earlier and use that (each mozIStorageConnection has an async thread that is lazily created at the first async call iirc). I suppose it's more expensive than an existing shared thread, but regardless we'd have to spawn that thread if the connection is going to be used with the async API. The additional cost would be for an off-main-thread consumer that does never need the async thread, but it's a cost we'd pay off-main-thread already.

Is this the kind of thing you had in mind, mak?

Comment on attachment 726651 [details] [diff] [review] Simple version of mozIStorageAsyncConnection Review of attachment 726651 [details] [diff] [review]: ----------------------------------------------------------------- The separated parts look fine so far, I will verify better the callers at the next iteration. As a rule of thumb, being able to use the new connection type in SQLite.jsm is the first goal, so any api used there should exist. ::: storage/public/moz.build @@ +24,5 @@ > 'mozIStorageStatementRow.idl', > 'mozIStorageVacuumParticipant.idl', > 'mozIStorageValueArray.idl', > + 'mozIStorageAsyncConnection.idl', > + 'mozIStorageCompletionCallback2.idl' did you forget to hg add these files?

This now passes the tests of Sqlite.jsm.

I have rewritten the QueryInterface using canonical macro. That's certainly less risky.

Try: https://tbpl.mozilla.org/?tree=Try&rev=413d4adf9319

Same code, just split across several patches.

Comment on attachment 729008 [details] [diff] [review] Adapting sqlite.jsm Review of attachment 729008 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/Sqlite.jsm @@ +282,1 @@ > }, We should consider changing these from properties to function. Also, does this actually work? a) I thought you needed to denote parameters with ":param" or "?" b) I don't think parameterized statements work with PRAGMA.

Comment on attachment 729007 [details] [diff] [review] Simple version of mozIStorageAsyncConnection, v4 unfortunately I won't be able to go through this until next week, I wonder if Andrew may have any time for a first pass in the meanwhile

Comment on attachment 729007 [details] [diff] [review] Simple version of mozIStorageAsyncConnection, v4 - connectionReady should be removed. I think it's reasonable to add something like 'connectionCloseRequested' in its stead that makes it clear that asyncClose has been requested and that all subsequent executeAsync calls will throw. - lastError, and lastErrorString should be removed and don't need alternatives. - lastInsertRowID, affectedRows should be removed. If code needs this, it seems like we should call sqlite3_last_insert_rowid and sqlite3_changes calls inside the held mutex on the async thread and return the data that way. - createFunction, createAggregateFunction, setProgressHandler, and removeProgressHandler can't allow a JS implementation of their functions. The comments should clearly call this out, and problema-avoidance options are: - Just mark them all [noscript]; C++ funcs being registered by JS need to also take a mozIStorageConnection so they can register themselves. Rogue C++ could still try and put a JS impl on them though. - Create async variants of mozIStorageFunction and mozIStorageProgressHandler that are not marked scriptable. - Don't change the IDL but check if we get an XPConnect JS-wrapped implementation. I assume a future revision of the patch will do the actual async opening, or is there going to be a follow-on bug?

(In reply to Andrew Sutherland (:asuth) from comment #19) > Comment on attachment 729007 [details] [diff] [review] > Simple version of mozIStorageAsyncConnection, v4 > > - connectionReady should be removed. I think it's reasonable to add > something like 'connectionCloseRequested' in its stead that makes it clear > that asyncClose has been requested and that all subsequent executeAsync > calls will throw. Ok. This will need some porting for Sqlite.jsm and FHR. > - Create async variants of mozIStorageFunction and > mozIStorageProgressHandler that are not marked scriptable. If you don't mind, I'll do this in a followup bug. > - Don't change the IDL but check if we get an XPConnect JS-wrapped > implementation. > > I assume a future revision of the patch will do the actual async opening, > or is there going to be a follow-on bug? I was planning to do this in a followup bug, but I can be convinced to do it in a further version of the patch, if necessary.

(In reply to David Rajchenbach Teller [:Yoric] from comment #20) > (In reply to Andrew Sutherland (:asuth) from comment #19) > > Comment on attachment 729007 [details] [diff] [review] > > Simple version of mozIStorageAsyncConnection, v4 > > > > - connectionReady should be removed. I think it's reasonable to add > > something like 'connectionCloseRequested' in its stead that makes it clear > > that asyncClose has been requested and that all subsequent executeAsync > > calls will throw. > > Ok. This will need some porting for Sqlite.jsm and FHR. Actually, we also need an indicator that the connection has been opened properly.

(In reply to David Rajchenbach Teller [:Yoric] from comment #20) > > - Create async variants of mozIStorageFunction and > > mozIStorageProgressHandler that are not marked scriptable. > > If you don't mind, I'll do this in a followup bug. I think it's safest to comment out those methods on the async idl until they're safe, then. > > I assume a future revision of the patch will do the actual async opening, > > or is there going to be a follow-on bug? > > I was planning to do this in a followup bug, but I can be convinced to do it > in a further version of the patch, if necessary. I think it should be done in this bug and land at the same time; could be a different patch. Given the very low turnover in the mozStorage code-base there's low chance of painful bit-rotting. So it doesn't seem like there's any advantage to landing an interface that claims to make things async but doesn't actually do that. My specific concern is that for B2G we're seeing that we have to do a lot of crazy uplifts, and I could see an unfortunate situation where a patch that deeply needs mozIStorageAsyncConnection to work gets uplifted onto a branch where it's a lie but no one realized this because all the tests still passed. (In reply to David Rajchenbach Teller [:Yoric] from comment #21) > Actually, we also need an indicator that the connection has been opened > properly. Ah right, openAsyncDatabase should really take a callback that gets notified when the connection has been successfully opened or instead failed to open. It seems reasonable for users of the connection to assume the opening will be successful. In the unlikely path where the opening fails, all of those queued operations should just fail. If that does't sound right, then openAsyncDatabase should probably be a void return and the callback is where the async connection would be returned. I worry that have a variable one could check about whether the connection has actually been opened might result in consumers polling or something equally ill-advised.

Applied Andrew's feedback. - The opening methods now call a callback instead of returning a connection. The callback is |null| if the connection could not be opened. - Moved connectionReady back to the sync version. I have not replaced it for the moment, as Sqlite.jsm does not seem to need it. - Moved last* back to the sync version. I have not replaced them for the moment, which means that I had to remove them from Sqlite.jsm, too. If need arises, I can try and reimplement them. - Marked Function-related methods as [noscript]. « - Don't change the IDL but check if we get an XPConnect JS-wrapped implementation. » I actually did not understand that item. Check if we get this where?

Taken into account gps' remarks and the fallback from changes to mozIStorageAsyncConnection. I had to remove getters for |last*| properties. Please tell me if you believe that I should reimplement them in this bug. This is certainly feasible, but it might take some time.

Trivial propagation of changes to FHR.

Comment on attachment 730084 [details] [diff] [review] Adapting sqlite.jsm Review of attachment 730084 [details] [diff] [review]: ----------------------------------------------------------------- I guess we didn't have test coverage for schemas. Boo. We should add that. Maybe not in this bug, but sometime. ::: toolkit/modules/Sqlite.jsm @@ +109,2 @@ > log.warn("Connection is not ready."); > + deferred.reject(new Error("Connection is not ready.")); Do we not get an error on why the open failed? If we don't, that's a fault in the interface: the caller needs to be able to determine why an open failed and be able to take an appropriate course of action. Read: we need an error code to denote failure conditions. @@ +233,3 @@ > */ > + get closeRequested() { > + return !this._connection || this._connection.closeRequested; Do we use this anywhere? If not, nuke it. Else, add a test for it. @@ +255,4 @@ > }, > > + setSchemaVersion: function(value) { > + if (typeof value != "number" || isNaN(value) || Math.floor(value) != value) { Number.isInteger()? ::: toolkit/modules/tests/xpcshell/test_sqlite.js @@ +111,3 @@ > do_check_eq(c.lastInsertRowID, 1); > do_check_eq(c.affectedRows, 1); > +*/ Just remove dead code. That being said, this test would is now not actually testing anything. We should check the statement counter and verify the data was inserted into the DB.

Comment on attachment 730085 [details] [diff] [review] Adapting FHR, v2 Review of attachment 730085 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/metrics/storage.jsm @@ +1114,5 @@ > for (let k of self._SCHEMA_STATEMENTS) { > yield self._connection.execute(SQL[k]); > } > > + self._connection.setSchemaVersion(1); We need a yield here, no?

(In reply to Gregory Szorc [:gps] from comment #26) > Comment on attachment 730084 [details] [diff] [review] > Adapting sqlite.jsm > > Review of attachment 730084 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess we didn't have test coverage for schemas. Boo. We should add that. > Maybe not in this bug, but sometime. > > ::: toolkit/modules/Sqlite.jsm > @@ +109,2 @@ > > log.warn("Connection is not ready."); > > + deferred.reject(new Error("Connection is not ready.")); > > Do we not get an error on why the open failed? If we don't, that's a fault > in the interface: the caller needs to be able to determine why an open > failed and be able to take an appropriate course of action. Read: we need an > error code to denote failure conditions. Good point. I'm not sure how to best do this, though. > @@ +233,3 @@ > > */ > > + get closeRequested() { > > + return !this._connection || this._connection.closeRequested; > > Do we use this anywhere? If not, nuke it. Else, add a test for it. nuked > @@ +255,4 @@ > > }, > > > > + setSchemaVersion: function(value) { > > + if (typeof value != "number" || isNaN(value) || Math.floor(value) != value) { > > Number.isInteger()? Oops :) > > ::: toolkit/modules/tests/xpcshell/test_sqlite.js > @@ +111,3 @@ > > do_check_eq(c.lastInsertRowID, 1); > > do_check_eq(c.affectedRows, 1); > > +*/ > > Just remove dead code. > > That being said, this test would is now not actually testing anything. We > should check the statement counter and verify the data was inserted into the > DB. What is the best way of checking the statement counter with Sqlite.jsm?

Applied gps' feedback (except for the statement counter, which I don't know how to access).

(In reply to Gregory Szorc [:gps] from comment #27) > Comment on attachment 730085 [details] [diff] [review] > Adapting FHR, v2 > > Review of attachment 730085 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: services/metrics/storage.jsm > @@ +1114,5 @@ > > for (let k of self._SCHEMA_STATEMENTS) { > > yield self._connection.execute(SQL[k]); > > } > > > > + self._connection.setSchemaVersion(1); > > We need a yield here, no? Barring any mistake, all requests are serialized, so it should not be necessary to explicitly wait for completion of this request.

Just access connection._statementCounter from the tests. It's an integer.

planning to review this tomorrow from the morning, there's quite some stuff to check here, thus I prefer starting with fresh mind.

Comment on attachment 730082 [details] [diff] [review] Simple version of mozIStorageAsyncConnection, v5 Review of attachment 730082 [details] [diff] [review]: ----------------------------------------------------------------- I'm starting thinking we may do open differently. Currently you added 4 methods, mimicking the sync API: openSpecialAsyncDatabase openAsyncDatabase openUnsharedAsyncDatabase openAsyncDatabaseWithFileURL We could provide a single openAsyncDatabase method with options. It may get an nsIVariant to distinguish nsIFile, nsIFileURL or string (for special database), or we could at least merge some of those, though I don't think we need all of the 4 methods. It may get an optional "in unsigned long aOptions" and have bool flags for shared_cache and read_only (with possibility to add more in future). Would even be nicer if we could add options with value, like growthIncrement (so we can kill the setGrowthIcrement API), but I can't think of a way to make optional options nice for cpp consumers. For JS consumers would be trivial using a jsval, but for both cpp and js the only thing I may think of is a property bag. Not that it's so bad, both should just create a writable property bag and add specific properties to it, but it's not as nice as a common bitfield of options... What do you think? I'd like if all of us would reach an agreement on a plausible better open API here. ::: storage/public/mozIStorageAsyncConnection.idl @@ +34,5 @@ > + * databases. > + * This value must stay in sync with the SQLITE_DEFAULT_PAGE_SIZE define in > + * /db/sqlite3/src/Makefile.in > + */ > + const long DEFAULT_PAGE_SIZE = 32768; fyi this is changing in bug 857376, will bitrot @@ +76,5 @@ > + * Defaults to false. > + * @return the cloned database connection. > + */ > + void asyncClone(in boolean aReadOnly, > + in mozIStorageCompletionCallback2 aCallback); indentation @@ +104,5 @@ > + */ > + mozIStorageAsyncStatement createAsyncStatement(in AUTF8String aSQLStatement); > + > + > + /** too many newlines here @@ +200,5 @@ > + * Remove a progress handler. > + * > + * @return previous registered handler. > + */ > + mozIStorageProgressHandler removeProgressHandler(); should be [noscript] as well @@ +216,5 @@ > + * See http://sqlite.org/c3ref/file_control.html for more details. > + * @throws NS_ERROR_FILE_TOO_BIG > + * If the system is short on storage space. > + */ > + void setGrowthIncrement(in int32_t aIncrement, in AUTF8String aDatabaseName); This invokes synchronously xFileControl on main-thread, should not do anything bad if I got it correctly, but would be nice if we could somehow merge it with open and do the call on the async thread... My suggestion to provide opening options may help, though I'm not totally sold on it. Thoughts are welcome. ::: storage/public/mozIStorageCompletionCallback2.idl @@ +15,5 @@ > + * completed successfully. > + * @param value If the operation produces a result, the result. Otherwise, > + * |null|. > + */ > + void complete(in nsresult status, [optional] in nsISupports value); If I look at the current uses of this, all of them return a value, that may be null, but it's not optional. Imo, either we call this mozIStorageCompletionCallbackWithValue and make the value non optional, or we change mozIStorageCompletionCallback like this, forcing it to get a status and we avoid adding another completion callback. Instead this looks like an half-way between the 2 solutions. I think would make sense to just change mozIStorageCompletionCallback, it has only one use in asyncClose, js consumers won't notice a difference (they can just avoid defining arguments), existing cpp consumers are easy to fix just updating the signature. ::: storage/src/mozStorageConnection.cpp @@ +419,5 @@ > } > > NS_IMPL_THREADSAFE_ADDREF(Connection) > + > +NS_IMPL_QUERY_HEAD(Connection) add comment that this implements a NS_IMPL_THREADSAFE_QUERY_INTERFACE2 conditioned on mSupportsSynchronousOperations It's too much time from when I played with custom NS_IMPL, any reason to directly use NS_IMPL_QUERY instead of NS_INTERFACE_MAP_BEGIN and friends? NS_IMPL_QUERY_HEAD looks more for internal use @@ +430,3 @@ > > // This is identical to what NS_IMPL_THREADSAFE_RELEASE provides, but with the > +// extra |1 == count| case.// why this "//" change? @@ +991,5 @@ > NS_IMETHODIMP > +Connection::AsyncClone(bool aReadOnly, > + mozIStorageCompletionCallback2 *aCallback) > +{ > + // For this first version, AsyncClone is actually implemented synchronously As Andrew said, before landing this we need a separate part where asyncClone and asyncOpen are really async, to avoid dangerously having different storage versions exposing differently behaving APIs. @@ +994,5 @@ > +{ > + // For this first version, AsyncClone is actually implemented synchronously > + nsCOMPtr<mozIStorageConnection> clone; > + nsresult rv = this->Clone(aReadOnly, getter_AddRefs(clone)); > + /*ignore error*/ aCallback->Complete(rv, clone); usually, when we want to ignore errors explicitly we cast to (void)

Comment on attachment 731090 [details] [diff] [review] Adapting sqlite.jsm, v2 Review of attachment 731090 [details] [diff] [review]: ----------------------------------------------------------------- you want a test for get/setSchemaVersion :) Fwiw, the schema changes and removal of affectedRows/lastInsertRowID/connectionReady could be moved to a separate bug and be landed earlier. That would make lot of sense indeed. ::: toolkit/modules/Sqlite.jsm @@ +236,3 @@ > */ > + getSchemaVersion: function() { > + let self = this; unused? @@ +247,4 @@ > }, > > + setSchemaVersion: function(value) { > + if (Number.isInteger(value)) { should not this be the opposite? the schema version should be an integer!

(In reply to Marco Bonardo [:mak] from comment #33) > Comment on attachment 730082 [details] [diff] [review] > Simple version of mozIStorageAsyncConnection, v5 > > Review of attachment 730082 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm starting thinking we may do open differently. > > Currently you added 4 methods, mimicking the sync API: > openSpecialAsyncDatabase > openAsyncDatabase > openUnsharedAsyncDatabase > openAsyncDatabaseWithFileURL > > We could provide a single openAsyncDatabase method with options. > It may get an nsIVariant to distinguish nsIFile, nsIFileURL or string (for > special database), or we could at least merge some of those, though I don't > think we need all of the 4 methods. > It may get an optional "in unsigned long aOptions" and have bool flags for > shared_cache and read_only (with possibility to add more in future). > Would even be nicer if we could add options with value, like growthIncrement > (so we can kill the setGrowthIcrement API), but I can't think of a way to > make optional options nice for cpp consumers. For JS consumers would be > trivial using a jsval, but for both cpp and js the only thing I may think of > is a property bag. Not that it's so bad, both should just create a writable > property bag and add specific properties to it, but it's not as nice as a > common bitfield of options... > > What do you think? Well, we could have a JS-oriented version that takes a jsval and a C++-oriented version that takes both a bitfield and an optional property bag. Eventually, the JS-oriented version could call the C++-oriented version. For testing purposes, it would be easier to first write the JS-oriented version, and then add a minor refactoring to move the body to the C++-oriented version. Does this plan sound acceptable to you? > > I'd like if all of us would reach an agreement on a plausible better open > API here. > > ::: storage/public/mozIStorageAsyncConnection.idl > @@ +34,5 @@ > > + * databases. > > + * This value must stay in sync with the SQLITE_DEFAULT_PAGE_SIZE define in > > + * /db/sqlite3/src/Makefile.in > > + */ > > + const long DEFAULT_PAGE_SIZE = 32768; > > fyi this is changing in bug 857376, will bitrot What do you suggest? > @@ +216,5 @@ > > + * See http://sqlite.org/c3ref/file_control.html for more details. > > + * @throws NS_ERROR_FILE_TOO_BIG > > + * If the system is short on storage space. > > + */ > > + void setGrowthIncrement(in int32_t aIncrement, in AUTF8String aDatabaseName); > > This invokes synchronously xFileControl on main-thread, should not do > anything bad if I got it correctly, but would be nice if we could somehow > merge it with open and do the call on the async thread... My suggestion to > provide opening options may help, though I'm not totally sold on it. > Thoughts are welcome. I'll try and move this to |open| once we have somewhat agreed upon what |open| should do. > ::: storage/public/mozIStorageCompletionCallback2.idl > @@ +15,5 @@ > > + * completed successfully. > > + * @param value If the operation produces a result, the result. Otherwise, > > + * |null|. > > + */ > > + void complete(in nsresult status, [optional] in nsISupports value); > > If I look at the current uses of this, all of them return a value, that may > be null, but it's not optional. > Imo, either we call this mozIStorageCompletionCallbackWithValue and make the > value non optional, or we change mozIStorageCompletionCallback like this, > forcing it to get a status and we avoid adding another completion callback. > Instead this looks like an half-way between the 2 solutions. I think would > make sense to just change mozIStorageCompletionCallback, it has only one use > in asyncClose, js consumers won't notice a difference (they can just avoid > defining arguments), existing cpp consumers are easy to fix just updating > the signature. I can do that. It's just going to increase the size of the patch, though. > > ::: storage/src/mozStorageConnection.cpp > @@ +419,5 @@ > > } > > > > NS_IMPL_THREADSAFE_ADDREF(Connection) > > + > > +NS_IMPL_QUERY_HEAD(Connection) > > add comment that this implements a NS_IMPL_THREADSAFE_QUERY_INTERFACE2 > conditioned on mSupportsSynchronousOperations > It's too much time from when I played with custom NS_IMPL, any reason to > directly use NS_IMPL_QUERY instead of NS_INTERFACE_MAP_BEGIN and friends? > NS_IMPL_QUERY_HEAD looks more for internal use Ok, I'll do that. > > @@ +430,3 @@ > > > > // This is identical to what NS_IMPL_THREADSAFE_RELEASE provides, but with the > > +// extra |1 == count| case.// > > why this "//" change? > > @@ +991,5 @@ > > NS_IMETHODIMP > > +Connection::AsyncClone(bool aReadOnly, > > + mozIStorageCompletionCallback2 *aCallback) > > +{ > > + // For this first version, AsyncClone is actually implemented synchronously > > As Andrew said, before landing this we need a separate part where asyncClone > and asyncOpen are really async, to avoid dangerously having different > storage versions exposing differently behaving APIs. Will do. > > @@ +994,5 @@ > > +{ > > + // For this first version, AsyncClone is actually implemented synchronously > > + nsCOMPtr<mozIStorageConnection> clone; > > + nsresult rv = this->Clone(aReadOnly, getter_AddRefs(clone)); > > + /*ignore error*/ aCallback->Complete(rv, clone); > > usually, when we want to ignore errors explicitly we cast to (void) ok

(In reply to David Rajchenbach Teller [:Yoric] from comment #35) > Well, we could have a JS-oriented version that takes a jsval and a > C++-oriented version that takes both a bitfield and an optional property bag. > > Eventually, the JS-oriented version could call the C++-oriented version. For > testing purposes, it would be easier to first write the JS-oriented version, > and then add a minor refactoring to move the body to the C++-oriented > version. I'd rather say to start putting down a suggested js and C API we can evaluate, if we reach consensus proceed with tests. The js version should just be a converter from a jsval to a propertybag basically, should be simple. Otherwise there's risk you start writing tests, then we figure something doesn't work in the js/C world and that work must be redone. > > ::: storage/public/mozIStorageAsyncConnection.idl > > fyi this is changing in bug 857376, will bitrot > > What do you suggest? just to unbitrot :)

Comment on attachment 737143 [details] [diff] [review] Possible API for mozIStorageService Review of attachment 737143 [details] [diff] [review]: ----------------------------------------------------------------- What do you think of this API?

Comment on attachment 737143 [details] [diff] [review] Possible API for mozIStorageService Review of attachment 737143 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/public/mozIStorageService.idl @@ +42,5 @@ > + * see |mozIStorageConnection::setGrowthIncrement(..., NULL)| > + * > + * - int tmpGrowthIncrement (defaults to sqlite default value) > + * -- Set the growth increment for the temporary database. > + * see |mozIStorageConnection::setGrowthIncrement(..., "TEMP")| why the temporary database? I don't see anything similar in the existing API, actually there is also no consumer of the second argument of setGrowthIncrement, so we may avoid it @@ +48,5 @@ > + * @param aCallback > + * > + * @throws NS_ERROR_INVALID_ARG if |aKey| is invalid. > + */ > + void openAsyncDatabase(in string aKey, I'm not sure taking a string in any case is fine, nsIFile and nsIFileURL have stricter checks, esp for cross-platform behavior. May this rather be an nsIVariant, and we could distinguish string (special), nsIFile, nsIFileURL? Actually from a nsIFileURL is very easy to extract the nsIFile (there's a .file property) so I don't think it was a great idea to add openDatabaseWithFileURL to the API, that patch didn't even get a superreview afaict... I'd say we could just limit to 2 inputs, string (and then it's a special db) or nsIFile (and then it's common db file)

(In reply to Marco Bonardo [:mak] from comment #39) > Comment on attachment 737143 [details] [diff] [review] > Possible API for mozIStorageService > > Review of attachment 737143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: storage/public/mozIStorageService.idl > @@ +42,5 @@ > > + * see |mozIStorageConnection::setGrowthIncrement(..., NULL)| > > + * > > + * - int tmpGrowthIncrement (defaults to sqlite default value) > > + * -- Set the growth increment for the temporary database. > > + * see |mozIStorageConnection::setGrowthIncrement(..., "TEMP")| > > why the temporary database? I don't see anything similar in the existing > API, actually there is also no consumer of the second argument of > setGrowthIncrement, so we may avoid it Well, the current API doc kindly informs users to look at sqlite's documentation. But if we don't need this, let's just get rid of it. > > @@ +48,5 @@ > > + * @param aCallback > > + * > > + * @throws NS_ERROR_INVALID_ARG if |aKey| is invalid. > > + */ > > + void openAsyncDatabase(in string aKey, > > I'm not sure taking a string in any case is fine, nsIFile and nsIFileURL > have stricter checks, esp for cross-platform behavior. > May this rather be an nsIVariant, and we could distinguish string (special), > nsIFile, nsIFileURL? > Actually from a nsIFileURL is very easy to extract the nsIFile (there's a > .file property) so I don't think it was a great idea to add > openDatabaseWithFileURL to the API, that patch didn't even get a superreview > afaict... > > I'd say we could just limit to 2 inputs, string (and then it's a special db) > or nsIFile (and then it's common db file) Sounds good to me.

Ok, this should do the trick (Try in progress). Unless I'm missing something, implementing AsyncClone OMT doesn't look too hard. I am not quite sure how to proceed to implement OpenAsyncDatabase OMT. Would it be sufficient for me to create a nsIThread in OpenAsyncDatabase?

As I said, I think the simplest thing would be to create mAsyncExecutionThread earlier and throw runnables at it

Ok, I'll see what gives by creating that thread before the connection.

Comment on attachment 738692 [details] [diff] [review] Simple version of mozIStorageAsyncConnection, v6 Review of attachment 738692 [details] [diff] [review]: ----------------------------------------------------------------- It looks good overall, proceeding healthy ::: storage/public/mozIStorageAsyncConnection.idl @@ +20,5 @@ > + * mozIStorageAsyncConnection represents an asynchronous database > + * connection attached to a specific file or to the in-memory data > + * storage. It is the primary interface for interacting with a > + * database, including creating prepared statements, executing SQL, > + * and examining database errors. "for interacting with a database from the main thread." since I don't think it's very useful to use async API off mainthread, even if it's feasible. @@ +23,5 @@ > + * database, including creating prepared statements, executing SQL, > + * and examining database errors. > + * > + * In the future, default implementations will be considerably faster > + * than the default implementation of mozIStorageConnection. I don't think this comment is needed, future internal improvements don't need to be t part of an interface definition. Rather, do we already have a bug to try dropping some mutexes when working on this new kind of connection? @@ +35,5 @@ > + * @param aCallback [optional] > + * A callback that will be notified when the close is completed. > + * > + * @throws NS_ERROR_UNEXPECTED > + * If is called on a thread other than the one that opened it. In future we may actually evaluate to simplify this, by making the async API main-thread only. @@ +63,5 @@ > + * If this connection is a memory database. > + * > + * @param aReadOnly > + * If true, the returned database should be put into read-only mode. > + * Defaults to false. the callback param is not documented nit: move the @note and @throws after the @param and @return @@ +101,5 @@ > + * transaction. These statements can be reused immediately, and reset does > + * not need to be called. > + * > + * Note: If you have any custom defined functions, they must be re-entrant > + * since they can be called on multiple threads. move to a @note ::: storage/public/mozIStorageCompletionCallback.idl @@ +13,5 @@ > + * > + * @param status The status of the call. Generally NS_OK if the operation > + * completed successfully. > + * @param value If the operation produces a result, the result. Otherwise, > + * |null|. I think you should add a @see the calling method for expected values. ::: storage/public/mozIStorageConnection.idl @@ +23,5 @@ > * primary interface for interacting with a database, including > * creating prepared statements, executing SQL, and examining database > * errors. > * > + * Note: From the main thread, you should rather use mozIStorageAsyncConnection. @note ::: storage/public/storage.h @@ +25,5 @@ > #include "mozIStorageBindingParamsArray.h" > #include "mozIStorageBindingParams.h" > #include "mozIStorageVacuumParticipant.h" > #include "mozIStorageCompletionCallback.h" > +#include "mozIStorageCompletionCallback2.h" should not be anymore Callback2 ::: storage/src/mozStorageConnection.h @@ +62,5 @@ > * connection. > * @param aFlags > * The flags to pass to sqlite3_open_v2. > */ > + Connection(Service *aService, int aFlags, bool supportsSync); considered we have a product called Sync this is a bit ambiguous If we are looking for a short name to use acrsso the code, we may evaluate inverting the condition and using asyncOnly, regardless, would be nice to keep a coherent name across all of the changes, either asyncOnly and mAsyncOnly, or supportsSynchronousOperations and mSupportsSynchronousOperations... and so on... ::: storage/src/mozStorageService.cpp @@ +707,5 @@ > + > + if (keyString == "memory") { > + // just fall through with NULL storageFile, this will cause the storage > + // connection to use a memory DB. > + } else if (keyString == "profile") { you know what, I just verified and nobody is using this "profile" database feature. Makes sense too, cause it's unlikely people is willing to share the same db with others and end up in a large db with risk to hit coherence and performance issues. It's also so easy to have your own db... So, we won't support it here, we'll only support "memory". Please file a bug to also remove this feature (and all of the defines) completely from OpenSpecialDatabase, there is only one known add-on using it, having 58 users... @@ +722,5 @@ > + > + int32_t growthIncrement = -1; > + if (aOptions) { > + rv = aOptions->GetPropertyAsInt32(NS_LITERAL_STRING("growthIncrement"), &growthIncrement); > + if (!NS_SUCCEEDED(rv)) { !NS_SUCCEEDED => NS_FAILED @@ +733,5 @@ > + nsRefPtr<Connection> msc = new Connection(this, flags, true); > + > + rv = storageFile ? msc->initialize(storageFile) : msc->initialize(); > + if (NS_FAILED(rv)) { > + aCallback->Complete(rv, nullptr); (void) cast calls where you explicitly ignore the result, per code convention in Storage @@ +738,5 @@ > + return NS_OK; > + } > + > + if (growthIncrement >= 0) { > + rv = msc->SetGrowthIncrement(growthIncrement, EmptyCString()); this is useless for memory special database @@ +740,5 @@ > + > + if (growthIncrement >= 0) { > + rv = msc->SetGrowthIncrement(growthIncrement, EmptyCString()); > + if (NS_FAILED(rv)) { > + aCallback->Complete(rv, nullptr); I think in this case we may want to proceed regardless, the connection is usable even if the groth increment didn't pass. Sure it's unfortunate we can't tell the caller, but should not happen ideally. this may just be a MOZ_ASSERT

mak, would it make sense to also have an option for |PRAGMA cache_size|? It seems to show up quite a lot on slowsql telemetry, and it looks rather easy to place here.

I don't think we should add options for things that are very easy to do through queries. PRAGMA cache_size is visible cause it is usually executed on main-thread at connection open and it may happen while the disk is busy. With the async query it would happen on the async thread connection open, and users can (but they should not) eventually change that through a simple async query.

I meant "with the async connection it would happen..."

Quick update: I have an experimental version that performs opening OMT. Superficially, it seems to work, but it actually segfaults during garbage-collection or breaks the thread assertion at Connection::Release if I activate it.

mak, here is the current version of the patch. In this version, it raises assertion failure in |Connection::Release()|. If I do not activate the assertion in that method, it segfaults in |AsyncStatement::~AsyncStatement()|, apparently because |mDBConnection| is a dangling pointer. Artificially incrementing the refcnt of the |Connection| when we create it solves the segfault, but of course creates a leak.

Looks like you are creating the connection object off main-thread, and it is released on main-thread, I think there is a lot of complication due to releasing all members here if you go that way. In general it's not safe to create an xpcom object on a thread and destroy it on another thread (that's why we have things like NS_ProxyRelease, for example). I think you could simplify a bit the approach, you are trying to do too much off main thread. I suppose a simpler approach would still to create the Connection object on main thread, like OpenDatabase, but then instead of invoking ->initialize() on it you may invoke an ->asyncInitialize (with proper options) that would invoke getAsyncExecutionTarget, create and get the async thread, and at that point send on it a runnable that does the initialization. This should partially solve the problem since the Connection will be on the expected thread. Then in the async runner you should pay attention, since I'm not sure everything in initializeInternal is thread-safe (for example I'm not sure ::PR_NewLogModule is), we can evaluate on a case basis. I hope this helps, otherwise feel free to nag me!

Yes, I had tried that approach but it didn't succeed. Well, let's see if I can change that. Thanks.

(In reply to Marco Bonardo [:mak] from comment #44) > I don't think this comment is needed, future internal improvements don't > need to be t part of an interface definition. > Rather, do we already have a bug to try dropping some mutexes when working > on this new kind of connection? Bug 865181. > @@ +35,5 @@ > > + * @param aCallback [optional] > > + * A callback that will be notified when the close is completed. > > + * > > + * @throws NS_ERROR_UNEXPECTED > > + * If is called on a thread other than the one that opened it. > > In future we may actually evaluate to simplify this, by making the async API > main-thread only. Done. > ::: storage/src/mozStorageService.cpp > @@ +707,5 @@ > > + > > + if (keyString == "memory") { > > + // just fall through with NULL storageFile, this will cause the storage > > + // connection to use a memory DB. > > + } else if (keyString == "profile") { > > you know what, I just verified and nobody is using this "profile" database > feature. > Makes sense too, cause it's unlikely people is willing to share the same db > with others and end up in a large db with risk to hit coherence and > performance issues. It's also so easy to have your own db... > So, we won't support it here, we'll only support "memory". > > Please file a bug to also remove this feature (and all of the defines) > completely from OpenSpecialDatabase, there is only one known add-on using > it, having 58 users... Filed: bug 865188.

Andrew, as it turns out, createFunction et al. are already used from JS by the test suite. For the moment, I'm inclided to remove the [noscript]. Any recommendation?

(In reply to David Rajchenbach Teller [:Yoric] from comment #55) > Andrew, as it turns out, createFunction et al. are already used from JS by > the test suite. For the moment, I'm inclided to remove the [noscript]. Any > recommendation? I see some synchronous uses of createFunction, but no async uses. To be clear, I was only suggesting setting [noscript] for the async connection interface you are defining. createFunction is 'fine' to use for a synchronous-only DB connection for JS (in that it's safe, but a horrible idea to be doing things on the main-thread). It's a foot-gun for a traditional connection which is then used asynchronously, but XPConnect's assertions should back-stop us there when present (and crashes when not...). If you are trying to run the existing test suite for the async connection that's run for synchronous, just skip those tests.

Thanks to your help, mak, I managed to get rid of most segfaults. Unfortunately, I still encounter a few that baffle me. At the moment, calling asyncClone() on a database, regardless of how the database was opened, yields a connection that still causes segfaults at a later stage, during garbage-collection, with a stack that looks unrelated (i.e. it segfaults during NS_ProcessNextEvent). mak, if you have any further idea on the topic, I would be interested.

(In reply to Andrew Sutherland (:asuth) from comment #56) > (In reply to David Rajchenbach Teller [:Yoric] from comment #55) > > Andrew, as it turns out, createFunction et al. are already used from JS by > > the test suite. For the moment, I'm inclided to remove the [noscript]. Any > > recommendation? > > I see some synchronous uses of createFunction, but no async uses. > > To be clear, I was only suggesting setting [noscript] for the async > connection interface you are defining. Well, at the moment, the general (sync + async) interface is a subinterface of the async interface, so I cannot really mark the method as [noscript] in one and not the other. What I can do, otoh, is - put a [noscript] asyncCreateFunction et al. in the async interface; - move the existing createFunction et al. to the sync subinterface. Would that do for you?

Sorry, hadn't been keeping up with the interface changes. (B2G) Your approach sounds great. That also provides an evolutionary path for a unique type signature that is exposed-to-script but precludes JS-implemented implementations of the functions.

Comment on attachment 741835 [details] [diff] [review] mozIStorageAsyncConnection, WIP Review of attachment 741835 [details] [diff] [review]: ----------------------------------------------------------------- I may have found the issue. Test in progress.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3d9b7175f556

I still have assertion failures that need some ironing out. However, in the current state, I would already be interested in your feedback.

Hum. For some reason, adding a stupid assertion breaks the code in DEBUG mode. Go figure. So, same code, minus the dumb assertion and up for review. Try: https://tbpl.mozilla.org/?tree=Try&rev=047f12f156d8

(works better when I upload the right patch)

- Fixed user_version setter. - Added test for user_version get/set. - connectionCounters is now a Map.

Same one, minus a typo.

Comment on attachment 742936 [details] [diff] [review] mozIStorageAsyncConnection, v2 Review of attachment 742936 [details] [diff] [review]: ----------------------------------------------------------------- Could you please sum-up the current state of the patch, or better, what's broken yet? Regardless this starts looking great. I have some comments about threading, they may help making the code less fragile, though I'm not sure if I can be clear enough, in case please drop me a mail. I'd like to have a better understanding of your choice about the transactable interface. ::: storage/public/mozIStorageService.idl @@ +43,5 @@ > + * > + * @param aOptions A set of options (may be null). Options may contain: > + * - bool shared (defaults to |true|). If set to |false|, open the > + * database without sharing a sqlite cache. This is much less > + * efficient than opening with a shared cache, but necessary if should be "less memory efficient", cause later we explain that working without a cache decreases probability of contention, since there is less locking of write transactions. For this reason I actually think we should make the default false, shared cache mode was originally intended to have many connections while preserving memory efficiency, but sacrificing some perf, that is basically embedded servers case. Our use cases in the product are more likely to have few (a couple in most cases) concurrent connections for perf reasons, the memory size is a secondary issue considered we limit the cache size of each connection to a maximum of 2MB. @@ +45,5 @@ > + * - bool shared (defaults to |true|). If set to |false|, open the > + * database without sharing a sqlite cache. This is much less > + * efficient than opening with a shared cache, but necessary if > + * you use a feature of SQlite that is incompatible with a > + * shared cache, such as virtual tables or full text indexing this is no more true, the limitation has been fixed at SQLite 3.6.something, now you can use shared cache with virtual tables and that means it will also work with full test indexing, since the limitation was the same. So please while here remove this wrong comment from here and from the other place where it was copied from. @@ +48,5 @@ > + * you use a feature of SQlite that is incompatible with a > + * shared cache, such as virtual tables or full text indexing > + * support. In specific cases ,if cache contention is expected, > + * for instance when operating on a database from multiple > + * threads, using unshared connections may be a performance win. comma after "specific cases" is wrongly positioned @@ +50,5 @@ > + * support. In specific cases ,if cache contention is expected, > + * for instance when operating on a database from multiple > + * threads, using unshared connections may be a performance win. > + * > + * - int growthIncrement (defaults to sqlite default value). I don't think there is a default, if not defined there is just no growth increment enabled at all. ::: storage/public/mozIStorageTransactableConnection.idl @@ +9,5 @@ > +/** > + * A connection supporting transactions. > + */ > +[scriptable, uuid(64d3790c-317a-4ebe-93df-602d9fe7903c)] > +interface mozIStorageTransactableConnection: mozIStorageAsyncConnection { please, explain reasoning for having this new kind of connection between sync and async if it's just for internal need there should be an explicit comment stating consumers should not use it, and likely should be mozPIStorage... (P as private) ::: storage/src/mozStorageConnection.cpp @@ +391,5 @@ > + * An event used to initialize the clone of a connection. > + * > + * Must be executed on the clone's async execution thread. > + */ > +class AsyncInitializeClone: public nsRunnable you probably want to annotate the class with MOZ_FINAL @@ +400,5 @@ > + * @param aClone The clone. > + * @param aReadOnly If |true|, the clone is read only. > + * @param aCallbackEvent An event to trigger once initialization > + * is complete. This event will be triggered on > + * aClone->threadOpenedOn please indent like @param name something something else @@ +409,5 @@ > + CallbackComplete* aCallbackEvent) > + : mConnection(aConnection) > + , mClone(aClone) > + , mReadOnly(aReadOnly) > + , mCallbackEvent(aCallbackEvent) so, why don't you just pass the real callback object and rather create the callback runnable when you actually need it, instead of creating the runnable outside and passing it here? I suppose that both aConnection and aCallback may be const? Also, you can't tell if this runnable will be destroyed on the right thread, so you want a destructor that checks which thread it's running and if off-main-thread NS_ProxyReleases mConnection, mClone and mCallback to the mainthread. @@ +414,5 @@ > + {} > + > + nsresult Dispatch(nsresult aResult, nsISupports* aValue) { > + mCallbackEvent->mStatus = aResult; > + mCallbackEvent->mValue = aValue; I think you may create mCallbackEvent here, but with some attention. Since here we are off the expected thread for both value and the callback, it should swap() their ownership and its destructor should NS_ProxyRelease them on main-thread. Alternatively, in run() you may dispatch again the same runnable to the main thread, and inside run() you distinguish the 2 cases: off-main-thread do the initialization, on-main-thread do the dispatching. Actually, it's possible you don't even need CallbackComplete class at all once you are on the proper thread, that may simplify thread handling (less objects to properly release). @@ +426,5 @@ > + // reference to this object held by that event loop), we need to explicitly > + // null out our pointers here. It is possible this object will be destroyed > + // on the asynchronous thread and if the references are still alive we will > + // release them on that thread. We definitely do not want that for > + // mConnection or mClone and it's nice to avoid for mCallbackEvent too. and then likely you may avoid this manual handling. I'm also not sure this is correct, since afaict Dispatch here is invoked off main thread, so you are likely increasing the change to destroy these objects on the wrong thread, they should be destroyed on the thread that created them, ideally. @@ +438,5 @@ > + MOZ_ASSERT (NS_GetCurrentThread() == mClone->getAsyncExecutionTarget()); > + > + if (!mClone) { > + return Dispatch(NS_ERROR_OUT_OF_MEMORY, nullptr); > + } may this ever happen? the clone is created through the new operator that is infallible in our code, I'd expect we'd have aborted far earlier. @@ +440,5 @@ > + if (!mClone) { > + return Dispatch(NS_ERROR_OUT_OF_MEMORY, nullptr); > + } > + > + nsresult rv = initializeClone(mConnection, mClone, mReadOnly); why the need for this global function instead of directly invoking the connection's method? @@ +444,5 @@ > + nsresult rv = initializeClone(mConnection, mClone, mReadOnly); > + if (NS_FAILED(rv)) { > + return Dispatch(rv, nullptr); > + } > + return Dispatch(rv, at this point rv can just be NS_OK, so better expliciting that. @@ +517,5 @@ > } else if (0 == count) { > mRefCnt = 1; /* stabilize */ > +#if 1 /* enable this to find non-threadsafe destructors: */ > + NS_ASSERT_OWNINGTHREAD(Connection); > +#endif why changing this, in both cases you have to manually edit the source, so it looks like a not very useful change. @@ +1070,3 @@ > { > PROFILER_LABEL("storage", "Connection::Clone"); > + MOZ_ASSERT(NS_IsMainThread()); this won't do anything in release, maybe you want to throw an actual error, we should probably do the same from all async APIs, since they are not expected to be used off main-thread (even if it's possible today). @@ +1181,5 @@ > + return rv; > + } > + > + NS_IF_ADDREF(*_connection = clone); > + return rv; NS_OK ::: storage/src/mozStorageConnection.h @@ +67,5 @@ > * The flags to pass to sqlite3_open_v2. > + * @param aAsyncOnly > + * If |true|, the result behaves as a |mozIStorageAsyncConnection| > + * and |mozIStorageTransactableConnection|. If |false|, the result also > + * behaves as a |mozIStorageConnection|. the first part is... confusing. Likely cause it's missing a reasoning about the transactable interface @@ +282,5 @@ > nsRefPtr<Service> mStorageService; > + > + /** > + * If |false|, this instance supports synchronous operations: > + * - it can be casted to mozIStorageConnection; casted is an old form, afaik modern english uses cast @@ +301,5 @@ > + nsISupports* aValue, > + mozIStorageCompletionCallback* aCallback): > + mStatus(aStatus), > + mValue(aValue), > + mCallback(aCallback) both of these could possibly use swap() and the destructor could proxyrelease to the appropriate thread @@ +306,5 @@ > + {} > + NS_IMETHOD Run() { > + nsresult rv = mCallback->Complete(mStatus, mValue); > + mValue = nullptr; > + mCallback = nullptr; this looks fragile, since those have been addrefed by the constructor on thread A and now you are releasing them on thread B ::: storage/src/mozStorageService.cpp @@ +667,5 @@ > + CallbackComplete* aCallbackEvent): > + mConnection(aConnection), > + mStorageFile(aStorageFile), > + mGrowthIncrement(aGrowthIncrement), > + mCallbackEvent(aCallbackEvent) similar comments as for the other runnable Fwiw, once you have mConnection, don't you already have the file pointer through it? @@ +686,5 @@ > + } > + > + NS_IMETHOD Run() > + { > + MOZ_ASSERT(!NS_IsMainThread()); I liked the fact in the other case you verified exactly the thread and not a generic check for !mainthread @@ +710,5 @@ > + > +private: > + nsRefPtr<Connection> mConnection; > + nsCOMPtr<nsIThread> mOwnerThread; > + nsCOMPtr<nsIThread> mExecutionThread; these threads pointers don't look used anywhere @@ +796,5 @@ > + > + nsRefPtr<CallbackComplete> completeEvent = > + new CallbackComplete(NS_OK, > + nullptr, > + aCallback); same comment

Comment on attachment 742287 [details] [diff] [review] Adapting sqlite.jsm, v3 Review of attachment 742287 [details] [diff] [review]: ----------------------------------------------------------------- this looks good ::: toolkit/modules/Sqlite.jsm @@ +94,5 @@ > + let number; > + if (!connectionCounters.has(basename)) { > + number = 1; > + } else { > + number = connectionCounters.get(basename); let number = connectionCounters.get(basename) || 0?

(In reply to Marco Bonardo [:mak] from comment #68) > Comment on attachment 742936 [details] [diff] [review] > mozIStorageAsyncConnection, v2 > > Review of attachment 742936 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could you please sum-up the current state of the patch, or better, what's > broken yet? > Regardless this starts looking great. Last time I checked, with the exception of the optional thread-safety test (bug 868313), it seemed to pass all tests. > I have some comments about threading, they may help making the code less > fragile, though I'm not sure if I can be clear enough, in case please drop > me a mail. > > I'd like to have a better understanding of your choice about the > transactable interface. > > ::: storage/public/mozIStorageService.idl > @@ +43,5 @@ > > + * > > + * @param aOptions A set of options (may be null). Options may contain: > > + * - bool shared (defaults to |true|). If set to |false|, open the > > + * database without sharing a sqlite cache. This is much less > > + * efficient than opening with a shared cache, but necessary if > > should be "less memory efficient", cause later we explain that working > without a cache decreases probability of contention, since there is less > locking of write transactions. > > For this reason I actually think we should make the default false, shared > cache mode was originally intended to have many connections while preserving > memory efficiency, but sacrificing some perf, that is basically embedded > servers case. Our use cases in the product are more likely to have few (a > couple in most cases) concurrent connections for perf reasons, the memory > size is a secondary issue considered we limit the cache size of each > connection to a maximum of 2MB. I'm not sure I understand the explanations, but we can certainly default to false. [...] > ::: storage/public/mozIStorageTransactableConnection.idl > @@ +9,5 @@ > > +/** > > + * A connection supporting transactions. > > + */ > > +[scriptable, uuid(64d3790c-317a-4ebe-93df-602d9fe7903c)] > > +interface mozIStorageTransactableConnection: mozIStorageAsyncConnection { > > please, explain reasoning for having this new kind of connection between > sync and async > > if it's just for internal need there should be an explicit comment stating > consumers should not use it, and likely should be mozPIStorage... (P as > private) iirc, I needed to split mozIStorageTransactableConnection from mozIStorageConnection to get the funky binary tests to compile. I expect users to use mozIStorageTransactableConnection as part of mozIStorageConnection, so I believe that we should not mark it private. > @@ +409,5 @@ > > + CallbackComplete* aCallbackEvent) > > + : mConnection(aConnection) > > + , mClone(aClone) > > + , mReadOnly(aReadOnly) > > + , mCallbackEvent(aCallbackEvent) > > so, why don't you just pass the real callback object and rather create the > callback runnable when you actually need it, instead of creating the > runnable outside and passing it here? > I suppose that both aConnection and aCallback may be const? > > Also, you can't tell if this runnable will be destroyed on the right thread, > so you want a destructor that checks which thread it's running and if > off-main-thread NS_ProxyReleases mConnection, mClone and mCallback to the > mainthread. I assumed that the runnable is destroyed on the thread to which it is dispatched, but let's add NS_ProxyRelease instead of nullifying everything. As for creating mCallbackEvent here, I have the impression that this complicates the code a lot, without any gain that I can think of. [...] > Alternatively, in run() you may dispatch again the same runnable to the main > thread, and inside run() you distinguish the 2 cases: off-main-thread do the > initialization, on-main-thread do the dispatching. Unless you insist, I would prefer not using this approach. In my experience, it pretty much guarantees that the code will be impossible to read. > Actually, it's possible you don't even need CallbackComplete class at all > once you are on the proper thread, that may simplify thread handling (less > objects to properly release). > > @@ +426,5 @@ > > + // reference to this object held by that event loop), we need to explicitly > > + // null out our pointers here. It is possible this object will be destroyed > > + // on the asynchronous thread and if the references are still alive we will > > + // release them on that thread. We definitely do not want that for > > + // mConnection or mClone and it's nice to avoid for mCallbackEvent too. > > and then likely you may avoid this manual handling. > I'm also not sure this is correct, since afaict Dispatch here is invoked off > main thread, so you are likely increasing the change to destroy these > objects on the wrong thread, they should be destroyed on the thread that > created them, ideally. Ok, replacing this by forget() + NS_ProxyRelease. > @@ +440,5 @@ > > + if (!mClone) { > > + return Dispatch(NS_ERROR_OUT_OF_MEMORY, nullptr); > > + } > > + > > + nsresult rv = initializeClone(mConnection, mClone, mReadOnly); > > why the need for this global function instead of directly invoking the > connection's method? Because method |initializeClone| is private. I can only call it from a |friend| and I cannot declare |AsyncInitializeClone| (defined later, and in an anonymous namespace) to be a |friend|. Can you think of a better solution? > @@ +517,5 @@ > > } else if (0 == count) { > > mRefCnt = 1; /* stabilize */ > > +#if 1 /* enable this to find non-threadsafe destructors: */ > > + NS_ASSERT_OWNINGTHREAD(Connection); > > +#endif > > why changing this, in both cases you have to manually edit the source, so it > looks like a not very useful change. Actually, that's debugging code. However, it was my understanding that commenting out code is not considered an acceptable source control method. Currently attempting > @@ +301,5 @@ > > + nsISupports* aValue, > > + mozIStorageCompletionCallback* aCallback): > > + mStatus(aStatus), > > + mValue(aValue), > > + mCallback(aCallback) > > both of these could possibly use swap() and the destructor could > proxyrelease to the appropriate thread > > @@ +306,5 @@ > > + {} > > + NS_IMETHOD Run() { > > + nsresult rv = mCallback->Complete(mStatus, mValue); > > + mValue = nullptr; > > + mCallback = nullptr; > > this looks fragile, since those have been addrefed by the constructor on > thread A and now you are releasing them on thread B > > ::: storage/src/mozStorageService.cpp > @@ +667,5 @@ > > + CallbackComplete* aCallbackEvent): > > + mConnection(aConnection), > > + mStorageFile(aStorageFile), > > + mGrowthIncrement(aGrowthIncrement), > > + mCallbackEvent(aCallbackEvent) > > similar comments as for the other runnable > Fwiw, once you have mConnection, don't you already have the file pointer > through it? > > @@ +686,5 @@ > > + } > > + > > + NS_IMETHOD Run() > > + { > > + MOZ_ASSERT(!NS_IsMainThread()); > > I liked the fact in the other case you verified exactly the thread and not a > generic check for !mainthread > > @@ +710,5 @@ > > + > > +private: > > + nsRefPtr<Connection> mConnection; > > + nsCOMPtr<nsIThread> mOwnerThread; > > + nsCOMPtr<nsIThread> mExecutionThread; > > these threads pointers don't look used anywhere > > @@ +796,5 @@ > > + > > + nsRefPtr<CallbackComplete> completeEvent = > > + new CallbackComplete(NS_OK, > > + nullptr, > > + aCallback); > > same comment

(In reply to Marco Bonardo [:mak] from comment #68) > Comment on attachment 742936 [details] [diff] [review] > mozIStorageAsyncConnection, v2 > > Review of attachment 742936 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could you please sum-up the current state of the patch, or better, what's > broken yet? > Regardless this starts looking great. Last time I checked, with the exception of the optional thread-safety test (bug 868313), it seemed to pass all tests. > I have some comments about threading, they may help making the code less > fragile, though I'm not sure if I can be clear enough, in case please drop > me a mail. > > I'd like to have a better understanding of your choice about the > transactable interface. > > ::: storage/public/mozIStorageService.idl > @@ +43,5 @@ > > + * > > + * @param aOptions A set of options (may be null). Options may contain: > > + * - bool shared (defaults to |true|). If set to |false|, open the > > + * database without sharing a sqlite cache. This is much less > > + * efficient than opening with a shared cache, but necessary if > > should be "less memory efficient", cause later we explain that working > without a cache decreases probability of contention, since there is less > locking of write transactions. > > For this reason I actually think we should make the default false, shared > cache mode was originally intended to have many connections while preserving > memory efficiency, but sacrificing some perf, that is basically embedded > servers case. Our use cases in the product are more likely to have few (a > couple in most cases) concurrent connections for perf reasons, the memory > size is a secondary issue considered we limit the cache size of each > connection to a maximum of 2MB. I'm not sure I understand the explanations, but we can certainly default to false. [...] > ::: storage/public/mozIStorageTransactableConnection.idl > @@ +9,5 @@ > > +/** > > + * A connection supporting transactions. > > + */ > > +[scriptable, uuid(64d3790c-317a-4ebe-93df-602d9fe7903c)] > > +interface mozIStorageTransactableConnection: mozIStorageAsyncConnection { > > please, explain reasoning for having this new kind of connection between > sync and async > > if it's just for internal need there should be an explicit comment stating > consumers should not use it, and likely should be mozPIStorage... (P as > private) iirc, I needed to split mozIStorageTransactableConnection from mozIStorageConnection to get the funky binary tests to compile. I expect users to use mozIStorageTransactableConnection as part of mozIStorageConnection, so I believe that we should not mark it private. > @@ +409,5 @@ > > + CallbackComplete* aCallbackEvent) > > + : mConnection(aConnection) > > + , mClone(aClone) > > + , mReadOnly(aReadOnly) > > + , mCallbackEvent(aCallbackEvent) > > so, why don't you just pass the real callback object and rather create the > callback runnable when you actually need it, instead of creating the > runnable outside and passing it here? > I suppose that both aConnection and aCallback may be const? > > Also, you can't tell if this runnable will be destroyed on the right thread, > so you want a destructor that checks which thread it's running and if > off-main-thread NS_ProxyReleases mConnection, mClone and mCallback to the > mainthread. I assumed that the runnable is destroyed on the thread to which it is dispatched, but let's add NS_ProxyRelease instead of nullifying everything. As for creating mCallbackEvent here, I have the impression that this complicates the code a lot, without any gain that I can think of. [...] > Alternatively, in run() you may dispatch again the same runnable to the main > thread, and inside run() you distinguish the 2 cases: off-main-thread do the > initialization, on-main-thread do the dispatching. Unless you insist, I would prefer not using this approach. In my experience, it pretty much guarantees that the code will be impossible to read. > Actually, it's possible you don't even need CallbackComplete class at all > once you are on the proper thread, that may simplify thread handling (less > objects to properly release). > > @@ +426,5 @@ > > + // reference to this object held by that event loop), we need to explicitly > > + // null out our pointers here. It is possible this object will be destroyed > > + // on the asynchronous thread and if the references are still alive we will > > + // release them on that thread. We definitely do not want that for > > + // mConnection or mClone and it's nice to avoid for mCallbackEvent too. > > and then likely you may avoid this manual handling. > I'm also not sure this is correct, since afaict Dispatch here is invoked off > main thread, so you are likely increasing the change to destroy these > objects on the wrong thread, they should be destroyed on the thread that > created them, ideally. Ok, replacing this by forget() + NS_ProxyRelease. > @@ +440,5 @@ > > + if (!mClone) { > > + return Dispatch(NS_ERROR_OUT_OF_MEMORY, nullptr); > > + } > > + > > + nsresult rv = initializeClone(mConnection, mClone, mReadOnly); > > why the need for this global function instead of directly invoking the > connection's method? Because method |initializeClone| is private. I can only call it from a |friend| and I cannot declare |AsyncInitializeClone| (defined later, and in an anonymous namespace) to be a |friend|. Can you think of a better solution? > @@ +517,5 @@ > > } else if (0 == count) { > > mRefCnt = 1; /* stabilize */ > > +#if 1 /* enable this to find non-threadsafe destructors: */ > > + NS_ASSERT_OWNINGTHREAD(Connection); > > +#endif > > why changing this, in both cases you have to manually edit the source, so it > looks like a not very useful change. Actually, that's debugging code. However, it was my understanding that commenting out code is not considered an acceptable source control method. I'll try and apply your NS_ProxyRelease(), swap(), etc. suggestions. For the moment, this causes releasing errors, so I guess I'm not a master at cross-thread refcounting yet.

(In reply to David Rajchenbach Teller [:Yoric] from comment #71) > I'm not sure I understand the explanations, but we can certainly default to > false. In shared cache mode the entire process uses a shared cache for all of the connections to that database, instead of giving each connection a separate cache. This means memory usage is much lower, but connections have to fight for the cache resource, that in the end is a perf hit. This is good when you have an embedded server, cause it will likely have hundreds of connections, you want the memory win and you don't care much about locking. But in our case we usually have 2 or 3 connections, so the memory loss is ignorable, but the performance win is not. > iirc, I needed to split mozIStorageTransactableConnection from > mozIStorageConnection to get the funky binary tests to compile. I expect > users to use mozIStorageTransactableConnection as part of > mozIStorageConnection, so I believe that we should not mark it private. I'd like a better explanation than a generic "needed for tests", since we are adding complication for consumers due to that. What is failing, what is unreachable? It's possible there are other solutions that allow to avoid this. > I assumed that the runnable is destroyed on the thread to which it is > dispatched, but let's add NS_ProxyRelease instead of nullifying everything. > > As for creating mCallbackEvent here, I have the impression that this > complicates the code a lot, without any gain that I can think of. It will cause some more code, but should be easier to follow, since currently you are creating an object that will be needed much much later and then you have to pass it over until it's really needed. > [...] > > Alternatively, in run() you may dispatch again the same runnable to the main > > thread, and inside run() you distinguish the 2 cases: off-main-thread do the > > initialization, on-main-thread do the dispatching. > > Unless you insist, I would prefer not using this approach. In my experience, > it pretty much guarantees that the code will be impossible to read. I don't insist, was just a possibility, if the run() body is small and it just multiplexes to 2 methods it's usually easy to follow. > > @@ +440,5 @@ > > > + if (!mClone) { > > > + return Dispatch(NS_ERROR_OUT_OF_MEMORY, nullptr); > > > + } > > > + > > > + nsresult rv = initializeClone(mConnection, mClone, mReadOnly); > > > > why the need for this global function instead of directly invoking the > > connection's method? > > Because method |initializeClone| is private. I can only call it from a > |friend| and I cannot declare |AsyncInitializeClone| (defined later, and in > an anonymous namespace) to be a |friend|. Can you think of a better solution? Well, I suppose you may define AsyncInizializeClone in the mozilla::storage namespace and add a forward declaration? Or just make initializeClose public... initialize is public, internalClose is public, not sure why we should start being picky now.

I have applied most of your suggestions. I have not swap()ed, because it looked very much like this could create fragile situations in which our objects can be deallocated before the nsIRunnable is executed. However, this segfaults once again, which seems to indicate that I have not quite succeeded at my objectives - possibly because I do not understand properly how NS_ProxyRelease works. If you have some time to try and help me fix this, that would be great – at the moment, I am somewhat stumbling in the dark.

Comment on attachment 755908 [details] [diff] [review] mozIStorageAsyncConnection, v3 Review of attachment 755908 [details] [diff] [review]: ----------------------------------------------------------------- Ah, found my problem. Note to future self: using NS_ProxyRelease() and NS_ISUPPORTS_CAST() in the same expression? Bad idea.

Ok, solved my issues. I still have two problems: 1. AsyncClone() seems to leak a nsThread (see the next patch on tests); 2. Failing to AsyncClose() a database that has been openAsync()-ed causes an assertion failure during shutdown. For 1., I have been unable to trace the reason. All threads seem to be Shutdown() properly. For 2., The comments already present in Connection::Close() seem to sum up the situation nicely. It seems that I will need to fix that issue before proceeding. Any suggestion, mak?

(In reply to David Rajchenbach Teller [:Yoric] from comment #75) > For 1., I have been unable to trace the reason. All threads seem to be > Shutdown() properly. Uploading weird fix. Can you tell me what you think of that fix? > For 2., The comments already present in Connection::Close() seem to sum up > the situation nicely. It seems that I will need to fix that issue before > proceeding. Actually, looks like this would depend upon bug 874814. Do I have your blessing to postpone this to a followup bug?

I will apply your patch and investigate the issue tomorrow. I'm not sure regarding your question 2 in comment 75 though, when you say "failing to close" do you mean if the consumer forgets to asyncClose() the connection or if asyncClose() fails for other reasons (and if so, which ones?)

(In reply to Marco Bonardo [:mak] from comment #79) > I will apply your patch and investigate the issue tomorrow. Thanks. The fix is at mozStorageService.cpp line 815. > I'm not sure regarding your question 2 in comment 75 though, when you say > "failing to close" do you mean if the consumer forgets to asyncClose() the > connection or if asyncClose() fails for other reasons (and if so, which > ones?) I mean if the consumer forgets to call asyncClose(). For the moment, forgetting to call asyncClose() causes an assertion failure upon garbage-collection if an OMT operation has been called already. This patch introduces two new operations that cause OMT operations: openAsyncConnection() and asyncClone(). This increases the number of cases in which we encounter the assertion failure.

if the assertion is caused by consumer's mistake it's good for it to happen, nobody should forget to asyncClose() and we should do the best we can to notify the mistake.

to try to speed this up, I'm working on a diff on top of yours, so you could just check my changes and merge with yours when satisfied.

Sounds good. Also, I'll probably want to make the assertion message more precise at some point.

This applies on top of your v5 patch, it's a review where I fixed the comments for you, I may have some more comments on the final version since it's a bit hard to check everything without a merged version. What I'd like you to do with this: 1. merge mozIStorageAsyncConnection, v5 and Propagating API changes to clients, v1 (doesn't make sense to keep these separated, I didn't notice that patch and spent time fixing compiling bugs) 2. check if my changes make sense to you 3. merge my changes into the new patch 4. remove function test_open_async_do_not_close from the tests, it doesn't make sense Once this is done we should have our merged patch v6. The last remaining thing is that I still don't like the addition of the transactable interface, things are complicate enough with 2 interfaces, the third one makes them worse. I'd like that to be done in a separate diff for now though, we'll merge it later once we agree on the approach.

Comment on attachment 756706 [details] [diff] [review] mozIStorageAsyncConnection, v5 Review of attachment 756706 [details] [diff] [review]: ----------------------------------------------------------------- See previous comment!

Comment on attachment 759922 [details] [diff] [review] suggested changes Review of attachment 759922 [details] [diff] [review]: ----------------------------------------------------------------- Ok, your code is definitely smoother than mine. I do not understand how you avoid the leak for which I had to add a mysterious call to Release(), though. Do you know more? ::: storage/src/mozStorageService.cpp @@ +747,5 @@ > > rv = storageFile->Clone(getter_AddRefs(storageFile)); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > > + // Wnsure that SQLITE_OPEN_CREATE is passed in for compatibility reasons. Typo

I don't see any leak, steps to reproduce?

could be related to bug 802239

Let me rephrase: with my version of the patch (minus the ::Release()), testing test_storage_connection.js leaked an instance of nsThread. With ::Release(), it doesn't and version your version of the patch, it doesn't either. So all's good. I'm just trying to understand the magic.

Here we go. I can't seem to be able to reproduce the test errors that initially caused me to separate transaction stuff, so I'm fully reverting that part of the change.

Comment on attachment 761434 [details] [diff] [review] mozIStorageAsyncConnection, v6 Review of attachment 761434 [details] [diff] [review]: ----------------------------------------------------------------- I don't see any blocking issue, we should try to land and see if any issue arises from the wild. Very good, can't wait to see this in action. ::: storage/public/moz.build @@ +24,5 @@ > 'mozIStorageStatementCallback.idl', > 'mozIStorageStatementParams.idl', > 'mozIStorageStatementRow.idl', > 'mozIStorageVacuumParticipant.idl', > + 'mozIStorageValueArray.idl' I didn't actually mean to cause this change, while it may be fine, let's avoid polluting blame when it's not needed. ::: storage/public/mozIStorageAsyncConnection.idl @@ +17,5 @@ > +interface nsIFile; > + > +/** > + * mozIStorageAsyncConnection represents an asynchronous database > + * connection attached to a specific file or to the in-memory data s/to the/to an/ @@ +25,5 @@ > + */ > +[scriptable, uuid(0e661a1d-27ff-4e6b-ac5a-126314cef61a)] > +interface mozIStorageAsyncConnection : nsISupports { > + /** > + * Asynchronously closes a database connection, allowing all pending s/a database/the database/ Also, I think we can remove the "Asynchronously" specification now, it's already explicit in the interface name and the method name. The other methods in this idl don't use the third person, so just "Close" @@ +26,5 @@ > +[scriptable, uuid(0e661a1d-27ff-4e6b-ac5a-126314cef61a)] > +interface mozIStorageAsyncConnection : nsISupports { > + /** > + * Asynchronously closes a database connection, allowing all pending > + * asynchronous statements to complete first. even here "asynchronous" is redundant, synchronous statements can't be pending on main thread. @@ +29,5 @@ > + * Asynchronously closes a database connection, allowing all pending > + * asynchronous statements to complete first. > + * > + * @param aCallback [optional] > + * A callback that will be notified when the close is completed. would be good to specify which value of the arguments to expect in the callback. @@ +32,5 @@ > + * @param aCallback [optional] > + * A callback that will be notified when the close is completed. > + * > + * @throws NS_ERROR_UNEXPECTED > + * If is called on a thread other than the one that opened it. actually it now throws NS_ERROR_NOT_SAME_THREAD if not invoked from the main thread @@ +37,5 @@ > + */ > + void asyncClose([optional] in mozIStorageCompletionCallback aCallback); > + > + /** > + * Clones a database and makes the clone read only if needed. the other methods in this idl don't use the third person, so just "Clone" @@ +42,5 @@ > + * > + * @param aReadOnly > + * If true, the returned database should be put into read-only mode. > + * Defaults to false. > + * @return the cloned database connection. it doesn't @@ +44,5 @@ > + * If true, the returned database should be put into read-only mode. > + * Defaults to false. > + * @return the cloned database connection. > + * @throws NS_ERROR_UNEXPECTED > + * If this connection is a memory database. also NS_ERROR_NOT_SAME_THREAD, as above @@ +48,5 @@ > + * If this connection is a memory database. > + * > + * @note If your connection is already read-only, you will get a read-only > + * clone. > + * @note Due to a bug in SQLite, if you use the shared cache (openDatabase), specifying openDatabase is now not enough here since we also have another open method... I think you should just put "(see mozIStorageService)" @@ +63,5 @@ > + * - synchronous > + * - wal_autocheckpoint > + */ > + void asyncClone(in boolean aReadOnly, > + in mozIStorageCompletionCallback aCallback); aCallback is not documented, same as above for the expected arguments values @@ +75,5 @@ > + ////////////////////////////////////////////////////////////////////////////// > + //// Statement creation > + > + /** > + * Create an asynchronous statement (mozIStorageAsyncStatement) for the given specifying mozIStorageAsyncStatement is pointless, the idl does a great job at that by itself. @@ +76,5 @@ > + //// Statement creation > + > + /** > + * Create an asynchronous statement (mozIStorageAsyncStatement) for the given > + * SQL expression. An asynchronous statement can only be used to dispatch s/expression// (expressions are another thing in SQL) @@ +80,5 @@ > + * SQL expression. An asynchronous statement can only be used to dispatch > + * asynchronous requests to the asynchronous execution thread and cannot be > + * used to take any synchronous actions on the database. > + * > + * The expression may use ? to indicate sequential numbered arguments, s/expression/statement/ @@ +86,5 @@ > + * $var to indicate named arguments. > + * > + * @param aSQLStatement > + * The SQL statement to execute. > + * @return a new mozIStorageAsyncStatement I think it's worth to @note that the statement is prepared lazily on first execution, so this can't be used to synchronously check the validity of a statement. @@ +91,5 @@ > + */ > + mozIStorageAsyncStatement createAsyncStatement(in AUTF8String aSQLStatement); > + > + /** > + * Execute an array of queries created with this connection asynchronously s/queries/statements/ s/asynchronously// @@ +93,5 @@ > + > + /** > + * Execute an array of queries created with this connection asynchronously > + * using any currently bound parameters. The statements are ran wrapped in a > + * transaction. These statements can be reused immediately, and reset does "When multiple write statements exist, they are ran wrapped in a transaction." ::: storage/public/mozIStorageCompletionCallback.idl @@ +11,5 @@ > /** > * Indicates that the event this callback was passed in for has completed. > + * > + * @param status The status of the call. Generally NS_OK if the operation > + * completed successfully. please indent as in mozIStorageAsyncConnection.idl @@ +13,5 @@ > + * > + * @param status The status of the call. Generally NS_OK if the operation > + * completed successfully. > + * @param value If the operation produces a result, the result. Otherwise, > + * |null|. ditto ::: storage/public/mozIStorageConnection.idl @@ +43,5 @@ > */ > void close(); > > /** > + * Clones a database and makes the clone read only if needed. "a database connection", we don't clone the database :) ::: storage/public/mozIStorageService.idl @@ +35,5 @@ > + * SAME NAME for the file each time, including case. The sqlite code uses > + * a simple string compare to see if there is already a connection. Opening > + * a connection to "Foo.sqlite" and "foo.sqlite" will CORRUPT YOUR DATABASE. > + * > + * @param aDatabaseStore Either a nsIFile representing the file that contains s/a/an/ @@ +45,5 @@ > + * - bool shared (defaults to |false|). If |true|, opens the database > + * with a shared-cache. The shared-cache mode is more memory-efficient > + * when many connections to the same database are expected, though, > + * the connections will contend the cache resource. In any cases where > + * performances matter, working without a shared-cache will improve Probably "performance" is uncountable in software, thus we should use the singular here (my fault) @@ +50,5 @@ > + * concurrency. > + * @see openUnsharedDatabase > + * > + * - int growthIncrement (defaults to none). > + * -- Set the growth increment for the main database. This hints SQLite to you are using different styles for the documentation here, shared has description after definition, while here you have --... please make them coherent, one way or the other @@ +55,5 @@ > + * grow the database file by a given chunk size and may reduce > + * filesystem fragmentation on large databases. > + * @see mozIStorageConnection::setGrowthIncrement > + * > + * @param aCallback A callback that will receive the result of the operation. let's be a bit more specific and say "will receive the new connection as its value argument". ::: storage/src/mozStorageConnection.cpp @@ +402,5 @@ > + * @param aClone The clone. > + * @param aReadOnly If |true|, the clone is read only. > + * @param aCallbackEvent An event to trigger once initialization > + * is complete. This event will be triggered on > + * aClone->threadOpenedOn. no more, please fix the callback doc ::: storage/src/mozStorageConnection.h @@ +277,5 @@ > + > + /** > + * If |false|, this instance supports synchronous operations: > + * - it can be cast to |mozIStorageConnection|; > + * - most operations require a lock. the latter comment here doesn't have value as of now since nothing changed yet, we should wait to add it until we start removing some locks from async connections. ::: storage/src/mozStorageService.cpp @@ +855,1 @@ > NS_IMETHODIMP is this newline needed?

PS: you want a SR, wonder if Mossop wants to look at that.

Comment on attachment 761435 [details] [diff] [review] Async tests, v2 Review of attachment 761435 [details] [diff] [review]: ----------------------------------------------------------------- please add some test for other options, like for example asynchronously open memory database, shared/unshared. Thanks for the cleanup. ::: storage/test/unit/test_storage_connection.js @@ +6,5 @@ > > //////////////////////////////////////////////////////////////////////////////// > //// Test Functions > > +const Cu = Components.utils; move to head_storage.js @@ +8,5 @@ > //// Test Functions > > +const Cu = Components.utils; > + > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); please defineLazyModuleGetter this in head_storage.js since it could be useful for other tests in future. @@ +289,2 @@ > }); > + yield deferred.promise; why don't you use your asyncClose helper here?

(In reply to Marco Bonardo [:mak] from comment #92) > Comment on attachment 761434 [details] [diff] [review] > mozIStorageAsyncConnection, v6 [snip] > ::: storage/src/mozStorageConnection.cpp > @@ +402,5 @@ > > + * @param aClone The clone. > > + * @param aReadOnly If |true|, the clone is read only. > > + * @param aCallbackEvent An event to trigger once initialization > > + * is complete. This event will be triggered on > > + * aClone->threadOpenedOn. > > no more, please fix the callback doc What do you mean? I'm pretty sure it's triggered on that thread.

(In reply to David Rajchenbach Teller [:Yoric] from comment #95) > (In reply to Marco Bonardo [:mak] from comment #92) > > Comment on attachment 761434 [details] [diff] [review] > > mozIStorageAsyncConnection, v6 > > [snip] > > > ::: storage/src/mozStorageConnection.cpp > > @@ +402,5 @@ > > > + * @param aClone The clone. > > > + * @param aReadOnly If |true|, the clone is read only. > > > + * @param aCallbackEvent An event to trigger once initialization > > > + * is complete. This event will be triggered on > > > + * aClone->threadOpenedOn. > > > > no more, please fix the callback doc > > What do you mean? I'm pretty sure it's triggered on that thread. the method is no more taking aCallbackEvent, it's taking aCallback.

Same one, plus doctypo fix.

Ah, I had somehow removed nsISupports from the interface map. Fixed.

Added a few tests.

Comment on attachment 765362 [details] [diff] [review] Async tests, v3 It's not critical, but if you have time, mak, could you take a look at the additional tests?

Comment on attachment 765363 [details] [diff] [review] 3. Adapting sqlite.jsm, v5 Sorry, asking the review on the wrong patch.

Comment on attachment 765346 [details] [diff] [review] mozIStorageAsyncConnection, v9 Review of attachment 765346 [details] [diff] [review]: ----------------------------------------------------------------- Just to check, the only change to the mozIStorageConnection interface from a JS perspective is that the asyncClone method is added, all the other methods are just moving to the parent interface right? sr=me assuming that is the case. ::: storage/public/mozIStorageAsyncConnection.idl @@ +44,5 @@ > + * Clone a database and make the clone read only if needed. > + * > + * @param aReadOnly > + * If true, the returned database should be put into read-only mode. > + * Defaults to false. This makes it sound like the parameter is optional but it isn't. Should it be? ::: storage/public/mozIStorageConnection.idl @@ +29,3 @@ > * @threadsafe > */ > [scriptable, uuid(c8646e4b-3e2d-4df3-98a9-e2bbf74f279c)] Rev the IID ::: storage/public/mozIStorageService.idl @@ +21,5 @@ > * > * @note The first reference to mozIStorageService must be made on the main > * thread. > */ > [scriptable, uuid(12bfad34-cca3-40fb-8736-d8bf9db61a27)] Rev the IID

Comment on attachment 765364 [details] [diff] [review] Adapting FHR, v3 Review of attachment 765364 [details] [diff] [review]: ----------------------------------------------------------------- r+ conditional on adding yield. ::: services/metrics/storage.jsm @@ +1171,5 @@ > for (let k of self._SCHEMA_STATEMENTS) { > yield self._connection.execute(SQL[k]); > } > > + self._connection.setSchemaVersion(1); Missing yield. If we can't set the schema properly, we want an exception otherwise the database will be in an undefined state.

Ah, exceptions, good point.

(In reply to Dave Townsend (:Mossop) from comment #106) > Comment on attachment 765346 [details] [diff] [review] > mozIStorageAsyncConnection, v9 > > Review of attachment 765346 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just to check, the only change to the mozIStorageConnection interface from a > JS perspective is that the asyncClone method is added, all the other methods > are just moving to the parent interface right? That is correct. > ::: storage/public/mozIStorageAsyncConnection.idl > @@ +44,5 @@ > > + * Clone a database and make the clone read only if needed. > > + * > > + * @param aReadOnly > > + * If true, the returned database should be put into read-only mode. > > + * Defaults to false. > > This makes it sound like the parameter is optional but it isn't. Should it > be? Ah, right, docfixing.

Changed refcounting in |CallbackComplete| to ensure that we do never QI on the wrong thread. Also applied Mossop's remarks. Try: https://tbpl.mozilla.org/?tree=Try&rev=c468cb5e684e

I have different behaviors locally and on Try with a corner case test. Given that Try has the "correct behavior" (i.e. expected misbehavior), I believe that we can land as such.

Comment on attachment 766679 [details] [diff] [review] mozIStorageAsyncConnection, v10 Review of attachment 766679 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/src/mozStorageConnection.cpp @@ +439,5 @@ > + ~AsyncInitializeClone() { > + nsCOMPtr<nsIThread> thread; > + DebugOnly<nsresult> rv = NS_GetMainThread(getter_AddRefs(thread)); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + // Handle ambiguous nsISupports inheritance. I think you may keep the NS_ProxyRelease for mCallback here, if it's null it will just be a no-op. but until we forget() it's a valid ownership to lose. If the runnable is created but can't run(), maybe cause the thread is dying, it would leak. ::: storage/src/mozStorageConnection.h @@ +293,5 @@ > + /** > + * @param aValue The result to pass to the callback. It must > + * already be owned by the main thread. > + * @param aCallback The callback. It must already be owned by the > + * main thread. please indent as @param name description more description @@ +312,5 @@ > + > + // Ensure that we release on the main thread > + mValue = nullptr; > + mCallback = nullptr; > + mCallingThread = nullptr; I'm pretty sure mCallingThread uses threadsafe refcounting, so you should not care about releasing it manually. But actually, looks like you are not using it anymore, indeed we enforced the async APIs to be invoked on mainthread, so why not just removing it? ::: storage/src/mozStorageService.cpp @@ +704,5 @@ > + { > + nsCOMPtr<nsIThread> thread; > + DebugOnly<nsresult> rv = NS_GetMainThread(getter_AddRefs(thread)); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + (void)NS_ProxyRelease(thread, mStorageFile); as well as here, I'd retain the proxyrelease of mCallback

Applied feedback. Try: https://tbpl.mozilla.org/?tree=Try&rev=ebaf37203b88

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa98df7a3493 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1819ab302a https://hg.mozilla.org/integration/mozilla-inbound/rev/a01ced6de845 https://hg.mozilla.org/integration/mozilla-inbound/rev/562c3fe054e5

Backed out for xpcshell failures: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd729c9f1a4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/906f40e126fa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c77fefe498 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00cf2d01294f eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=24607896&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=24607738&tree=Mozilla-Inbound (Try run was opt-only)

Thanks for the catch.

Turns out that some previous tests were not closing their files. Not completely sure why this caused distinct behaviors between debug and opt, but this should fix the problem. Try: https://tbpl.mozilla.org/?tree=Try&rev=3404942b73bb

As it turns out, this benigne error was hiding a very real dynamic cast problem that causes assertion failures. Attaching an implementation of transaction that should be compatible with async connections. Try: https://tbpl.mozilla.org/?tree=Try&rev=f1a8aa0fd13b

Same patch, minus the erroneous assert and plus some minor cleanup. Try: https://tbpl.mozilla.org/?tree=Try&rev=9c5144b6f464

Comment on attachment 768088 [details] [diff] [review] Support for transactions on mozIStorageAsyncConnection, v2 Review of attachment 768088 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/public/mozStorageHelper.h @@ +134,5 @@ > + * An instance of the mozStorageTransaction<> family dedicated > + * to |mozIStorageConnection|. > + */ > +typedef mozStorageTransactionBase<mozIStorageConnection, > + nsCOMPtr<mozIStorageConnection> > nit: too much spacing after typedef ::: storage/src/mozStorageAsyncStatementExecution.cpp @@ +25,1 @@ > namespace mozilla { remove newline @@ +579,5 @@ > > if (statementsNeedTransaction()) { > + Connection* rawConnection = static_cast<Connection*>(mConnection.get()); > + mTransactionManager = new mozStorageAsyncTransaction(rawConnection, false, > + mozIStorageConnection::TRANSACTION_IMMEDIATE); please reindent as before (if it's crossing 80 chars by a few I don't care)

Applied feedback, folded the patch into its parent.

https://hg.mozilla.org/integration/mozilla-inbound/rev/90143e24ad66 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3aab5e3d15e https://hg.mozilla.org/integration/mozilla-inbound/rev/213f7a4fa7f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/712b834e7cbf

Oops, resolved too early.

https://hg.mozilla.org/mozilla-central/rev/90143e24ad66 https://hg.mozilla.org/mozilla-central/rev/e3aab5e3d15e https://hg.mozilla.org/mozilla-central/rev/213f7a4fa7f4 https://hg.mozilla.org/mozilla-central/rev/712b834e7cbf