Bug 1334097 - Avoid busy waiting caused by MaybeUndeferIncall (r=aklotz)
authorBill McCloskey <billm@mozilla.com>
Fri, 21 Apr 2017 12:43:08 -0700
changeset 354958 78480dd41c9dca7dd127a8d3ab59a1c16e982924
parent 354957 de88e4dc9d99ea7cef2d571bf5bebf2bdc5d99f6
child 354959 5501d934a1ee72ca0d2dc78ee0b9a52e08d839bd
push id31717
push usercbook@mozilla.com
push dateWed, 26 Apr 2017 06:41:51 +0000
treeherdermozilla-central@0f5ba06c4c59 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1334097
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1334097 - Avoid busy waiting caused by MaybeUndeferIncall (r=aklotz) In order to avoid a busy wait where we defer and then immediately un-defer a message, we need to ensure that we only un-defer a message if it's actually ready to be processed. This patch uses the same condition in MaybeUndeferIncall as we use in DispatchInterruptMessage. MozReview-Commit-ID: L2xZfSO0Yrk
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -2024,66 +2024,28 @@ MessageChannel::DispatchAsyncMessage(con
 void
 MessageChannel::DispatchInterruptMessage(Message&& aMsg, size_t stackDepth)
 {
     AssertWorkerThread();
     mMonitor->AssertNotCurrentThreadOwns();
 
     IPC_ASSERT(aMsg.is_interrupt() && !aMsg.is_reply(), "wrong message type");
 
-    // Race detection: see the long comment near mRemoteStackDepthGuess in
-    // MessageChannel.h. "Remote" stack depth means our side, and "local" means
-    // the other side.
-    if (aMsg.interrupt_remote_stack_depth_guess() != RemoteViewOfStackDepth(stackDepth)) {
-        // Interrupt in-calls have raced. The winner, if there is one, gets to defer
-        // processing of the other side's in-call.
-        bool defer;
-        const char* winner;
-        const MessageInfo parentMsgInfo =
-          (mSide == ChildSide) ? MessageInfo(aMsg) : mInterruptStack.top();
-        const MessageInfo childMsgInfo =
-          (mSide == ChildSide) ? mInterruptStack.top() : MessageInfo(aMsg);
-        switch (mListener->MediateInterruptRace(parentMsgInfo, childMsgInfo))
-        {
-          case RIPChildWins:
-            winner = "child";
-            defer = (mSide == ChildSide);
-            break;
-          case RIPParentWins:
-            winner = "parent";
-            defer = (mSide != ChildSide);
-            break;
-          case RIPError:
-            MOZ_CRASH("NYI: 'Error' Interrupt race policy");
-            return;
-          default:
-            MOZ_CRASH("not reached");
-            return;
-        }
-
-        if (LoggingEnabled()) {
-            printf_stderr("  (%s: %s won, so we're%sdeferring)\n",
-                          (mSide == ChildSide) ? "child" : "parent",
-                          winner,
-                          defer ? " " : " not ");
-        }
-
-        if (defer) {
-            // We now know the other side's stack has one more frame
-            // than we thought.
-            ++mRemoteStackDepthGuess; // decremented in MaybeProcessDeferred()
-            mDeferred.push(Move(aMsg));
-            return;
-        }
-
-        // We "lost" and need to process the other side's in-call. Don't need
-        // to fix up the mRemoteStackDepthGuess here, because we're just about
-        // to increment it in DispatchCall(), which will make it correct again.
+    if (ShouldDeferInterruptMessage(aMsg, stackDepth)) {
+        // We now know the other side's stack has one more frame
+        // than we thought.
+        ++mRemoteStackDepthGuess; // decremented in MaybeProcessDeferred()
+        mDeferred.push(Move(aMsg));
+        return;
     }
 
+    // If we "lost" a race and need to process the other side's in-call, we
+    // don't need to fix up the mRemoteStackDepthGuess here, because we're just
+    // about to increment it, which will make it correct again.
+
 #ifdef OS_WIN
     SyncStackFrame frame(this, true);
 #endif
 
     nsAutoPtr<Message> reply;
 
     ++mRemoteStackDepthGuess;
     Result rv = mListener->OnCallReceived(aMsg, *getter_Transfers(reply));
@@ -2098,33 +2060,87 @@ MessageChannel::DispatchInterruptMessage
     reply->set_seqno(aMsg.seqno());
 
     MonitorAutoLock lock(*mMonitor);
     if (ChannelConnected == mChannelState) {
         mLink->SendMessage(reply.forget());
     }
 }
 
+bool
+MessageChannel::ShouldDeferInterruptMessage(const Message& aMsg, size_t aStackDepth)
+{
+    AssertWorkerThread();
+
+    // We may or may not own the lock in this function, so don't access any
+    // channel state.
+
+    IPC_ASSERT(aMsg.is_interrupt() && !aMsg.is_reply(), "wrong message type");
+
+    // Race detection: see the long comment near mRemoteStackDepthGuess in
+    // MessageChannel.h. "Remote" stack depth means our side, and "local" means
+    // the other side.
+    if (aMsg.interrupt_remote_stack_depth_guess() == RemoteViewOfStackDepth(aStackDepth)) {
+        return false;
+    }
+
+    // Interrupt in-calls have raced. The winner, if there is one, gets to defer
+    // processing of the other side's in-call.
+    bool defer;
+    const char* winner;
+    const MessageInfo parentMsgInfo =
+        (mSide == ChildSide) ? MessageInfo(aMsg) : mInterruptStack.top();
+    const MessageInfo childMsgInfo =
+        (mSide == ChildSide) ? mInterruptStack.top() : MessageInfo(aMsg);
+    switch (mListener->MediateInterruptRace(parentMsgInfo, childMsgInfo))
+    {
+        case RIPChildWins:
+            winner = "child";
+            defer = (mSide == ChildSide);
+            break;
+        case RIPParentWins:
+            winner = "parent";
+            defer = (mSide != ChildSide);
+            break;
+        case RIPError:
+            MOZ_CRASH("NYI: 'Error' Interrupt race policy");
+        default:
+            MOZ_CRASH("not reached");
+    }
+
+    IPC_LOG("race in %s: %s won",
+            (mSide == ChildSide) ? "child" : "parent",
+            winner);
+
+    return defer;
+}
+
 void
 MessageChannel::MaybeUndeferIncall()
 {
     AssertWorkerThread();
     mMonitor->AssertCurrentThreadOwns();
 
     if (mDeferred.empty())
         return;
 
     size_t stackDepth = InterruptStackDepth();
 
+    Message& deferred = mDeferred.top();
+
     // the other side can only *under*-estimate our actual stack depth
-    IPC_ASSERT(mDeferred.top().interrupt_remote_stack_depth_guess() <= stackDepth,
+    IPC_ASSERT(deferred.interrupt_remote_stack_depth_guess() <= stackDepth,
                "fatal logic error");
 
+    if (ShouldDeferInterruptMessage(deferred, stackDepth)) {
+        return;
+    }
+
     // maybe time to process this message
-    Message call(Move(mDeferred.top()));
+    Message call(Move(deferred));
     mDeferred.pop();
 
     // fix up fudge factor we added to account for race
     IPC_ASSERT(0 < mRemoteStackDepthGuess, "fatal logic error");
     --mRemoteStackDepthGuess;
 
     MOZ_RELEASE_ASSERT(call.nested_level() == IPC::Message::NOT_NESTED);
     RefPtr<MessageTask> task = new MessageTask(this, Move(call));
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -471,16 +471,17 @@ class MessageChannel : HasResultCodes, M
 
     // Returns true if ShouldDeferMessage(aMsg) is guaranteed to return true.
     // Otherwise, the result of ShouldDeferMessage(aMsg) may be true or false,
     // depending on context.
     static bool IsAlwaysDeferred(const Message& aMsg);
 
     bool WasTransactionCanceled(int transaction);
     bool ShouldDeferMessage(const Message& aMsg);
+    bool ShouldDeferInterruptMessage(const Message& aMsg, size_t aStackDepth);
     void OnMessageReceivedFromLink(Message&& aMsg);
     void OnChannelErrorFromLink();
 
   private:
     // Run on the not current thread.
     void NotifyChannelClosed();
     void NotifyMaybeChannelError();