506022 - Avoid obtaining the database mutex at all costs in Connection::ExecuteAsync

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

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

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

forgot to qrefresh...

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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?

Comment 4

[Shawn Wilsher :sdwilsh]

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

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

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 6

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 8

[Shawn Wilsher :sdwilsh]

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.

Comment 9

[Shawn Wilsher :sdwilsh]

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

Comment 10

[Shawn Wilsher :sdwilsh]

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.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

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.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v2.0->v2.1 interdiff

interdiff for ease of reviewing.

Comment 13

[Shawn Wilsher :sdwilsh]

This latest patch passed beautifully on the try server.

Comment 14

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places//rev/8aa74784d1af

Comment 15

[Shawn Wilsher :sdwilsh]

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

Comment 16

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/cae7b01ca38f

Comment 17

[Owen Marshall (Not reading bugmail)]

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

This checkin caused bug 544496.