The moment we try to acquire the database mutex in ExecuteAsync is the moment we start to compete with resources in the background thread. This is a setup for failure, which we really really want to avoid.

This should make us not acquire the mutex at all if we use binding param arrays. I'd like to (in a follow-up bug) make all the binding methods then just store their bindings in an array internally, and bind at execution time. This will allow us to make async fast with normal usage, and shouldn't really hurt the sync API (especial when compared against time spent hitting the disk).

forgot to qrefresh...

Comment on attachment 390299 [details] [diff] [review] v1.0 http://reviews.visophyte.org/r/390299/ Looks good and passes the tests. getAsyncStatement handles the potential edge cases with a caller who inconsistently uses the binding support. on file: storage/src/mozStorageStatement.cpp line 249 > rc = sqlite3_transfer_bindings(mDBStatement, stmt); > if (rc != SQLITE_OK) > return rc; If it were possible for the call to sqlite3_transfer_bindings to fail (it's not), this would leak the cloned statement. Suggest adding a comment and changing the conditional return into an assertion or something equally loud. on file: storage/src/mozStorageStatementData.h line 112 > nsCOMPtr<nsISupports> mIsCachedStatement; I don't have super-strong feelings on this, but mCachedStatementOwner feels like a more accurate variable name. If you change it, change the temporary too. The gameplan for the next step sounds good too; it's unfortunate Thunderbird 3 won't be able to enjoy this on 1.9.1. My only concern is that there isn't an obviously easy way to prime the cached async statements. If you're a C++ caller and do not mind doing ugly things, you can call getAsynchronousStatementData on an explicitly cast Statement and then just throw away the StatementData, but pure JS code is out of luck. Well, they could make sure they use all of the statements before doing real work, but I like that idea even less. What are your plans on this front?

So, it turns out this patch isn't really good enough either. On the plane yesterday, I did a bunch of thinking/investigating. Binding parameters also acquires the database handle's mutex, so we will actually need to do the next step (what I said for after 1.9.2) now for this to actually be effective. I have a patch mostly done, but fails a few tests (I am so happy this code is well tested). Should have something up Monday. The patch makes a lot of this logic a lot simpler. I was also thinking about adding a new API call on mozIStorageConnection to create an asynchronous statement. That'd help JS and native code callers to not incur the cost of generating the second statement like they would now (although, that's not terribly expensive since it'd only happen on the first call).

This is smarter, and it passes our storage tests (some code fixes that might seem random are because it caused test failures and I determined that it's better to keep the old behavior).

Comment on attachment 390830 [details] [diff] [review] v2.0 The marginal changes required over the previous patch really speak to the good decisions made with the previous refactorings.

(In reply to comment #4) > I was also thinking about adding a new API call on mozIStorageConnection to > create an asynchronous statement. That'd help JS and native code callers to > not incur the cost of generating the second statement like they would now > (although, that's not terribly expensive since it'd only happen on the first > call). This would be good. I'm not sure the "not terribly expensive" rationalization totally works given that there's a good chance that the first call might also coincide with the OS's disk cache not yet being warmed up. (I forget if storage induces sqlite to prime its cache with a linear read, but the OS cache could still potentially be very cold if the file is large enough.)

This appears to have caused a bunch of places tests to fail on the places branch, however. Going to investigate and will probably need minor changes.

(In reply to comment #7) > This would be good. I'm not sure the "not terribly expensive" rationalization > totally works given that there's a good chance that the first call might also > coincide with the OS's disk cache not yet being warmed up. (I forget if > storage induces sqlite to prime its cache with a linear read, but the OS cache > could still potentially be very cold if the file is large enough.) We don't (intentionally) make SQLite warm up the database until the first statement is executed. My point was mostly that the cost of executeAsync is now amortized, which is way better than the previous situation without this patch. The new API should only be done if we see stuff showing up in profiles I think.

The places unit test failures aren't big - it just has to do with the fact that statements are not being finalized early enough, so sqlite3_close fails, which causes an assertion.

There was actually one other issue as well. Our use of nsCOMArray::InsertObjectAt was wrong. It would shift objects around, but our tests never caught it because they would bind in index order (0 first, then 1...). I switched one test to bind in reverse order, which causes the test to hang without this patch. I've also added a test to make sure we finalize the async cached statement. If I comment out the code that does the finalization of the async statement, this test fails.

interdiff for ease of reviewing.

This latest patch passed beautifully on the try server.

This has tests for behavior changes, and lock checking will be added in bug 506027.

(In reply to comment #16) > http://hg.mozilla.org/mozilla-central/rev/cae7b01ca38f This checkin caused bug 544496.