488148 - Share the mutex used by AsyncExecuteStatements on Connection

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

in mozStorageEvents.cpp, we have a class, AsyncExecuteStatements, that has one member PRLock.  If someone ends up calling mozIStorageStatement::executeAsync or mozIStorageConnection::executeAsync (both of which end up creating an AsyncExecuteStatements instance) many times, they'll be creating many locks.

The lock is used to mange the cancellation state of executing statement(s).  The calling thread can call Cancel() on the mozIStoragePendingStatement, which returns true if it was able to cancel, or false if it was not because execution was already completed, even though the calling thread was not notified about it.  The lock is also used to create cancellation points in the following methods:
1) ExecuteAndProcessStatement
2) BuildAndNotifyResults

The lock is first obtained in the Run method, and (should be earlier; see bug 488146) released there as well.  The two methods are called inside there giving the cancellation points.

Comment 1

[Shawn Wilsher :sdwilsh]

My original comment wasn't 100% correct.  The lock is actually acquired in ExecuteAndProcessStatement.

Comment 2

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

Presumably a per-connection cancellation lock would suffice?  Only the asynchronous execution thread would normally need to touch the lock, so I would think it would be at least as efficient as the current N locks approach (as long as it is on its own cache line).

Comment 3

[Shawn Wilsher :sdwilsh]

Yeah, we could do that.  That would be many less locks, and since all the AsyncExecuteStatements for a db connection are serialized on a thread, it's OK to do.  It doesn't add any additional contention for resources, and we'll have less allocations for locks.

Comment 4

[Benjamin Smedberg]

Note that I wasn't particularly concerned about *allocations* for locks: I was more concerned that there might be an OS-level limit on the total number of locks that a process could create. For example, I see some articles saying that Windows has a limit of 64k handles per process, although that might be GDI handles and not apply to mutex handles.

Comment 5

[Shawn Wilsher :sdwilsh]

I'll tackle this once I get bug 489257 landed.  I'd like to bring a bit more sanity to this code before I start hacking on it more.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This creates a Mutex on Connection that will be shared across AsyncExecuteStatements instances per connection.  MutexAutoLock doesn't have an unlock method like nsAutoLock has, so I had to switch to some manual locking.  I'm told by bent that we really don't want to add those methods, so I'm not going to bother filing a bug on them.

I also added a bunch of asserts for lock ownership (even if Mutex::AssertNotCurrentThreadOwnsLock is not currently implemented) as per @pre comments in the header file.

Comment 7

[Shawn Wilsher :sdwilsh]

Alternatively, we could use MutexAutoUnlock instead of manually locking and unlocking.  Downside is that we'll lock, and then unlock before returning.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

I changed my mind.  I'll use MutexAutoUnlock in the error cases, since I don't really care if we are fast there (they aren't common).  I'll still manually control the lock in Run, but that's such a small code block, I don't think it matters.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 374087 [details] [diff] [review]
v1.1

Trying to free up asuth's review queue just a bit.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 374087 [details] [diff] [review]
v1.1

>-, mLock(nsAutoLock::NewLock("AsyncExecuteStatements::mLock"))
>+, mMutex(aConnection->sharedAsyncExecutionMutex)

Nothing holds the connection alive and so I'm not comfortable with having this be a simple reference to an object that could die.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

We can just use a nsRefPtr here to hold onto the connection.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 376783 [details] [diff] [review]
v1.2

>   NS_IF_ADDREF(mCallback);

Totally unrelated to this patch but why are you using manual refcounting here?

> AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement,
> ...
>+  mMutex.AssertNotCurrentThreadOwns();
> ...
>+  MutexAutoLock lockedScope(mMutex);

Reentering a lock should abort so I think this added assertion is unnecessary.

>       // Drop our mutex - notifyError doesn't want it held
>-      mutex.unlock();
>+      MutexAutoUnlock unlockedScope(mMutex);

I don't think this is a good idea. If you don't want to rework the semi-complicated logic in this function then I'd suggest not using the scoped lock. Given the choice I think I'd try to clean the function up a little.

>+  mMutex.Lock();
>+  if (mCancelRequested) {
>+    mState = CANCELED;
>+    mMutex.Unlock();
>+    return notifyComplete();
>   }
>+  mMutex.Unlock();

I prefer this kind of construct instead of manual lock/unlock:

  PRBool cancelRequested;
  {
    MutexAutoLock lock(mMutex);
    cancelRequested = mCancelRequested;
    if (cancelRequested)
      mState = CANCELED;
  }
  if (cancelRequested)
    return notifyComplete();

>@@ -521,6 +510,7 @@ AsyncExecuteStatements::Run()
>     PRBool finished = (i == (mStatements.Length() - 1));
>     if (!executeAndProcessStatement(mStatements[i], finished))
>       break;
>+    mMutex.AssertNotCurrentThreadOwns();

I can understand why you added this here but it indicates to me again that executeAndProcessStatement is too complicated.

>-  mozIStorageConnection *mConnection;
>+  nsRefPtr<Connection> mConnection;

You sure this doesn't cause leaks?

>diff --git a/storage/src/mozStorageConnection.h b/storage/src/mozStorageConnection.h
> ...
>+struct PRLock;

Unnecessary, right?

>+  Mutex sharedAsyncExecutionMutex;

All other members have a leading 'm', mSharedAsyncExecutionMutex.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Totally unrelated to this patch but why are you using manual refcounting here?
So we addref and release on the calling thread (thread safety issues)

> > AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement,
> > ...
> >+  mMutex.AssertNotCurrentThreadOwns();
> > ...
> >+  MutexAutoLock lockedScope(mMutex);
> 
> Reentering a lock should abort so I think this added assertion is unnecessary.
It may be unnecessary, but I figured it really enforces in the code that no lock should be held.  Since the precondition about it not being held is in the .h file, it's easier to miss.  Your call on this though - I figure it doesn't hurt.

> >       // Drop our mutex - notifyError doesn't want it held
> >-      mutex.unlock();
> >+      MutexAutoUnlock unlockedScope(mMutex);
> I don't think this is a good idea. If you don't want to rework the
> semi-complicated logic in this function then I'd suggest not using the scoped
> lock. Given the choice I think I'd try to clean the function up a little.
I talked to cjones about this a bit and realized that this is really an edge case (error condition), so we shouldn't care so much about the performance.  That's assuming that performance is the concern still.

> I can understand why you added this here but it indicates to me again that
> executeAndProcessStatement is too complicated.
I know we talked about this in the past, but I can't quite recall what the solution was.  Can you elaborate please?

> You sure this doesn't cause leaks?
Yes - there will be no cycles created here.  I didn't do it before because I wanted to save an AddRef and Release (but probably should have used a refptr).

> >+struct PRLock;
> Unnecessary, right?
No, since I don't include anything that defines PRLock in that file, and we still use PRLocks (I had the newly added mutex use the new API).

> >+  Mutex sharedAsyncExecutionMutex;
> All other members have a leading 'm', mSharedAsyncExecutionMutex.
I debated on this for a while.  I thought since it's meant to be publicly accessible, I should name it more conventional.  I'm going to defer to asuth on this one.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

I've gone and refactored executeAndProcessStatement.  It now calls a function called executeStatement which handles the error conditions, and returns true if results were obtained, and false if not.  Both of these functions are much smaller than the original now, and the logic is easier to follow.

This addresses all other review comments as far as I can tell (baring the decision that I want feedback from asuth on).

Comment 15

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

Comment on attachment 377025 [details] [diff] [review]
v1.3

I like sharedExecutionMutex without the 'm'.  It is never accessed via an implicit 'this', so the lack of an 'm' is unlikely to mislead.  And when it is accessed, it looks less like a hack when there is no 'm'.  (And it's not a hack...)

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 377025 [details] [diff] [review]
v1.3

This looks fine.

Comment 17

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/0997bcc75daf

Comment 18

[Shawn Wilsher :sdwilsh]

This patch broke thunderbird on mozilla-central (rightfully so!).  Backed out.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: bustage fix v1.0

I'll fix the tests once I apply the patch in bug 490085 (I don't want to bitrot that)

Comment 20

[Shawn Wilsher :sdwilsh]

I should have noted that this was a diff ignoring whitespace.

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: bustage fix v1.0 with whitespace changes

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: bustage fix test v1.0

Comment 23

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

Comment on attachment 384730 [details] [diff] [review]
bustage fix v1.0

This looks right by inspection.  Unfortunately, I can't build trunk right now because of a JS/compiler issue.

Comment 24

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

Comment on attachment 384756 [details] [diff] [review]
bustage fix test v1.0

>diff --git a/storage/test/unit/test_statement_executeAsync.js b/storage/test/unit/test_statement_executeAsync.js
>+function test_multiple_results()
>+{
>+  // First, we need to know how many rows we are expecting.
>+  let stmt = createStatement("SELECT COUNT(1) FROM test");
>+  try {
>+    do_check_true(stmt.executeStep());
>+    var expectedResults = stmt.getInt32(0);
>+  }
>+  finally {
>+    stmt.finalize();
>+  }
>+
>+  // Sanity check - we should have more than one result, but let's be sure.
>+  do_check_true(expectedResults > 1);

nit: could you declare var expectedResults outside the try block?  Although 'var' is declaring the variable with function scope, it still looks like an error to declare something in a block and then access it outside the block.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> nit: could you declare var expectedResults outside the try block?  Although
> 'var' is declaring the variable with function scope, it still looks like an
> error to declare something in a block and then access it outside the block.
but that's why I used var :P

I'll fix that at checkin.

Comment 26

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/2a103d1f03e2
http://hg.mozilla.org/mozilla-central/rev/42e818e6da7f