Closed Bug 488148 Opened 11 years ago Closed 11 years ago

Share the mutex used by AsyncExecuteStatements on Connection

Categories

(Toolkit :: Storage, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
My original comment wasn't 100% correct.  The lock is actually acquired in ExecuteAndProcessStatement.
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).
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.
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2a1
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.
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.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Depends on: 489257, 58904
Summary: Investigate the need for a lock per instance on AsyncExecuteStatements → Share the mutex used by AsyncExecuteStatements on Connection
Attached patch v1.0 (obsolete) — Splinter Review
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.
Attachment #373876 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
Alternatively, we could use MutexAutoUnlock instead of manually locking and unlocking.  Downside is that we'll lock, and then unlock before returning.
Attached patch v1.1 (obsolete) — Splinter Review
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.
Attachment #373876 - Attachment is obsolete: true
Attachment #374087 - Flags: review?(bugmail)
Attachment #373876 - Flags: review?(bugmail)
Depends on: 491977
Comment on attachment 374087 [details] [diff] [review]
v1.1

Trying to free up asuth's review queue just a bit.
Attachment #374087 - Flags: review?(bugmail) → review?(bent.mozilla)
Whiteboard: [needs review asuth] → [needs review bent]
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.
Attachment #374087 - Flags: review?(bent.mozilla) → review-
Attached patch v1.2 (obsolete) — Splinter Review
We can just use a nsRefPtr here to hold onto the connection.
Attachment #374087 - Attachment is obsolete: true
Attachment #376783 - Flags: review?(bent.mozilla)
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.
Attachment #376783 - Flags: review?(bent.mozilla) → review-
(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.
Whiteboard: [needs review bent] → [needs new patch]
Attached patch v1.3Splinter Review
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).
Attachment #376783 - Attachment is obsolete: true
Attachment #377025 - Flags: review?(bugmail)
Attachment #377025 - Flags: review?(bent.mozilla)
Whiteboard: [needs new patch] → [needs reivew bent][needs review asuth]
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...)
Attachment #377025 - Flags: review?(bugmail) → review+
Whiteboard: [needs reivew bent][needs review asuth] → [needs reivew bent]
Comment on attachment 377025 [details] [diff] [review]
v1.3

This looks fine.
Attachment #377025 - Flags: review?(bent.mozilla) → review+
Whiteboard: [needs reivew bent] → [can land]
http://hg.mozilla.org/mozilla-central/rev/0997bcc75daf
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Depends on: 499271
This patch broke thunderbird on mozilla-central (rightfully so!).  Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bustage fix v1.0Splinter Review
I'll fix the tests once I apply the patch in bug 490085 (I don't want to bitrot that)
Attachment #384730 - Flags: review?(bugmail)
I should have noted that this was a diff ignoring whitespace.
Attachment #384756 - Flags: review?(bugmail)
Whiteboard: [needs review asuth]
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.
Attachment #384730 - Flags: review?(bugmail) → review+
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.
Attachment #384756 - Flags: review?(bugmail) → review+
(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.
Whiteboard: [needs review asuth] → [can land]
http://hg.mozilla.org/mozilla-central/rev/2a103d1f03e2
http://hg.mozilla.org/mozilla-central/rev/42e818e6da7f
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
You need to log in before you can comment on or make changes to this bug.