Bug 1355178 - Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj)
authorBill McCloskey <billm@mozilla.com>
Mon, 10 Apr 2017 20:40:00 -0700
changeset 353853 af3fe1a5be65a7b11b17ea4429a34a95d2418124
parent 353852 5c706be68a0742078cb76c03f58a8e9d01de2dec
child 353854 13895acd7c9de2e4539f96dc37219570ce2093a7
push id89356
push userwmccloskey@mozilla.com
push dateWed, 19 Apr 2017 18:51:29 +0000
treeherdermozilla-inbound@13895acd7c9d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1355178
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 1355178 - Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj) MozReview-Commit-ID: DWEF1ObNmMa
ipc/glue/MessageChannel.cpp
mfbt/LinkedList.h
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -679,17 +679,17 @@ MessageChannel::Clear()
     mOnChannelConnectedTask->Cancel();
 
     if (mChannelErrorTask) {
         mChannelErrorTask->Cancel();
         mChannelErrorTask = nullptr;
     }
 
     // Free up any memory used by pending messages.
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         task->Clear();
     }
     mPending.clear();
 
     mOutOfTurnReplies.clear();
     while (!mDeferred.empty()) {
         mDeferred.pop();
     }
@@ -1036,17 +1036,17 @@ MessageChannel::OnMessageReceivedFromLin
             // Replace it with the newer message.
             MOZ_RELEASE_ASSERT(mPending.getLast()->Msg().compress_type() ==
                                IPC::Message::COMPRESSION_ENABLED);
             mPending.getLast()->Msg() = Move(aMsg);
 
             reuseTask = true;
         }
     } else if (aMsg.compress_type() == IPC::Message::COMPRESSION_ALL && !mPending.isEmpty()) {
-        for (RefPtr<MessageTask> p = mPending.getLast(); p; p = p->getPrevious()) {
+        for (MessageTask* p = mPending.getLast(); p; p = p->getPrevious()) {
             if (p->Msg().type() == aMsg.type() &&
                 p->Msg().routing_id() == aMsg.routing_id())
             {
                 // This message type has compression enabled, and the queue
                 // holds a message with the same message type and routed to the
                 // same destination. Erase it. Note that, since we always
                 // compress these redundancies, There Can Be Only One.
                 MOZ_RELEASE_ASSERT(p->Msg().compress_type() == IPC::Message::COMPRESSION_ALL);
@@ -1113,17 +1113,17 @@ MessageChannel::OnMessageReceivedFromLin
 }
 
 void
 MessageChannel::PeekMessages(std::function<bool(const Message& aMsg)> aInvoke)
 {
     // FIXME: We shouldn't be holding the lock for aInvoke!
     MonitorAutoLock lock(*mMonitor);
 
-    for (RefPtr<MessageTask> it : mPending) {
+    for (MessageTask* it : mPending) {
         const Message &msg = it->Msg();
         if (!aInvoke(msg)) {
             break;
         }
     }
 }
 
 void
@@ -1142,17 +1142,17 @@ MessageChannel::ProcessPendingRequests(A
         // cause even NOT_NESTED sync messages to be processed (but not
         // NOT_NESTED async messages), which would break message ordering.
         if (aTransaction.IsCanceled()) {
             return;
         }
 
         mozilla::Vector<Message> toProcess;
 
-        for (RefPtr<MessageTask> p = mPending.getFirst(); p; ) {
+        for (MessageTask* p = mPending.getFirst(); p; ) {
             Message &msg = p->Msg();
 
             MOZ_RELEASE_ASSERT(!aTransaction.IsCanceled(),
                                "Calling ShouldDeferMessage when cancelled");
             bool defer = ShouldDeferMessage(msg);
 
             // Only log the interesting messages.
             if (msg.is_sync() || msg.nested_level() == IPC::Message::NESTED_INSIDE_CPOW) {
@@ -1674,17 +1674,17 @@ MessageChannel::RunMessage(MessageTask& 
 
     if (!Connected()) {
         ReportConnectionError("RunMessage");
         return;
     }
 
     // Check that we're going to run the first message that's valid to run.
 #ifdef DEBUG
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         if (task == &aTask) {
             break;
         }
 
         MOZ_ASSERT(!ShouldRunMessage(task->Msg()) ||
                    aTask.Msg().priority() != task->Msg().priority());
 
     }
@@ -2593,17 +2593,17 @@ MessageChannel::EndTimeout()
 
     RepostAllMessages();
 }
 
 void
 MessageChannel::RepostAllMessages()
 {
     bool needRepost = false;
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         if (!task->IsScheduled()) {
             needRepost = true;
         }
     }
     if (!needRepost) {
         // If everything is already scheduled to run, do nothing.
         return;
     }
@@ -2655,17 +2655,17 @@ MessageChannel::CancelTransaction(int tr
             mTransactionStack->Cancel();
         }
     } else {
         MOZ_RELEASE_ASSERT(mTransactionStack->TransactionID() == transaction);
         mTransactionStack->Cancel();
     }
 
     bool foundSync = false;
-    for (RefPtr<MessageTask> p = mPending.getFirst(); p; ) {
+    for (MessageTask* p = mPending.getFirst(); p; ) {
         Message &msg = p->Msg();
 
         // If there was a race between the parent and the child, then we may
         // have a queued sync message. We want to drop this message from the
         // queue since if will get cancelled along with the transaction being
         // cancelled. This happens if the message in the queue is NESTED_INSIDE_SYNC.
         if (msg.is_sync() && msg.nested_level() != IPC::Message::NOT_NESTED) {
             MOZ_RELEASE_ASSERT(!foundSync);
--- a/mfbt/LinkedList.h
+++ b/mfbt/LinkedList.h
@@ -248,33 +248,35 @@ public:
     mPrev = this;
 
     Traits::exitList(this);
   }
 
   /*
    * Remove this element from the list containing it.  Returns a pointer to the
    * element that follows this element (before it was removed).  This method
-   * asserts if the element does not belong to a list.
+   * asserts if the element does not belong to a list. Note: In a refcounted list,
+   * |this| may be destroyed.
    */
-  ClientType removeAndGetNext()
+  RawType removeAndGetNext()
   {
-    ClientType r = getNext();
+    RawType r = getNext();
     remove();
     return r;
   }
 
   /*
    * Remove this element from the list containing it.  Returns a pointer to the
    * previous element in the containing list (before the removal).  This method
-   * asserts if the element does not belong to a list.
+   * asserts if the element does not belong to a list. Note: In a refcounted list,
+   * |this| may be destroyed.
    */
-  ClientType removeAndGetPrevious()
+  RawType removeAndGetPrevious()
   {
-    ClientType r = getPrevious();
+    RawType r = getPrevious();
     remove();
     return r;
   }
 
   /*
    * Identical to remove(), but also asserts in debug builds that this element
    * is in aList.
    */