Closed Bug 506022 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 506023
Blocks: 506027
Attached patch v1.0 (obsolete) — Splinter Review
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).
Attached patch v1.0 (obsolete) — Splinter Review
forgot to qrefresh...
Attachment #390298 - Attachment is obsolete: true
Attachment #390299 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
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?
Attachment #390299 - Flags: review?(bugmail) → review+
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).
Whiteboard: [needs review asuth]
Attached patch v2.0 (obsolete) — Splinter Review
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).
Attachment #390299 - Attachment is obsolete: true
Attachment #390830 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
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.
Attachment #390830 - Flags: review?(bugmail) → review+
(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.)
Whiteboard: [needs review asuth]
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.
Attached patch v2.1Splinter Review
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.
Attachment #390830 - Attachment is obsolete: true
Attachment #390923 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
interdiff for ease of reviewing.
No longer blocks: 506023
Blocks: 503701
This latest patch passed beautifully on the try server.
Attachment #390923 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth]
http://hg.mozilla.org/projects/places//rev/8aa74784d1af
Whiteboard: [fixed-in-places]
Target Milestone: --- → mozilla1.9.2a1
This has tests for behavior changes, and lock checking will be added in bug 506027.
Flags: in-testsuite+
http://hg.mozilla.org/mozilla-central/rev/cae7b01ca38f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-places]
Depends on: 544496
You need to log in before you can comment on or make changes to this bug.