Share the mutex used by AsyncExecuteStatements on Connection

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Storage
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

14.32 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
asuth
: review+
Details | Diff | Splinter Review
1.95 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.26 KB, patch
Details | Diff | Splinter Review
2.19 KB, patch
asuth
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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).
(Assignee)

Comment 3

9 years ago
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

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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
(Assignee)

Updated

9 years ago
Summary: Investigate the need for a lock per instance on AsyncExecuteStatements → Share the mutex used by AsyncExecuteStatements on Connection
(Assignee)

Comment 6

9 years ago
Created attachment 373876 [details] [diff] [review]
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.
Attachment #373876 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review asuth]
(Assignee)

Comment 7

9 years ago
Alternatively, we could use MutexAutoUnlock instead of manually locking and unlocking.  Downside is that we'll lock, and then unlock before returning.
(Assignee)

Comment 8

9 years ago
Created attachment 374087 [details] [diff] [review]
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.
Attachment #373876 - Attachment is obsolete: true
Attachment #374087 - Flags: review?(bugmail)
Attachment #373876 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Depends on: 491977
(Assignee)

Comment 9

9 years ago
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)
(Assignee)

Updated

9 years ago
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-
(Assignee)

Comment 11

9 years ago
Created attachment 376783 [details] [diff] [review]
v1.2

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

Comment 13

9 years ago
(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]
(Assignee)

Comment 14

9 years ago
Created attachment 377025 [details] [diff] [review]
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).
Attachment #376783 - Attachment is obsolete: true
Attachment #377025 - Flags: review?(bugmail)
Attachment #377025 - Flags: review?(bent.mozilla)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Updated

9 years ago
Whiteboard: [needs reivew bent] → [can land]
(Assignee)

Comment 17

9 years ago
http://hg.mozilla.org/mozilla-central/rev/0997bcc75daf
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Depends on: 499271
(Assignee)

Comment 18

9 years ago
This patch broke thunderbird on mozilla-central (rightfully so!).  Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

9 years ago
Created attachment 384730 [details] [diff] [review]
bustage fix v1.0

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

Comment 20

9 years ago
I should have noted that this was a diff ignoring whitespace.
(Assignee)

Comment 21

9 years ago
Created attachment 384732 [details] [diff] [review]
bustage fix v1.0 with whitespace changes
(Assignee)

Comment 22

9 years ago
Created attachment 384756 [details] [diff] [review]
bustage fix test v1.0
Attachment #384756 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 25

9 years ago
(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.
(Assignee)

Updated

9 years ago
Whiteboard: [needs review asuth] → [can land]
(Assignee)

Comment 26

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2a103d1f03e2
http://hg.mozilla.org/mozilla-central/rev/42e818e6da7f
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
You need to log in before you can comment on or make changes to this bug.