Closed Bug 1355178 Opened 4 years ago Closed 4 years ago

MessageChannel::ProcessPendingRequests does a bunch of unnecessary refcounting


(Core :: IPC, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: froydnj, Assigned: billm)



(2 files)

This profile:

is anomalous for the thousands of PRemoteSpellcheckEngine::Msg_Check messages.  But having all those messages helps highlight another performance problem:

We're spending half of our time inside ProcessPendingRequests doing completely unnecessary refcounting on messages:

        // (1) (unnecessary strong reference)
        for (RefPtr<MessageTask> p = mPending.getFirst(); p; ) {
            Message &msg = p->Msg();

                               "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) {
                IPC_LOG("ShouldDeferMessage(seqno=%d) = %d", msg.seqno(), defer);

            if (!defer) {
                if (!toProcess.append(Move(msg)))

                // More unnecessary strong references.
                p = p->removeAndGetNext();
            // ...and here.
            p = p->getNext();

At (1), we shouldn't be taking a strong reference, we should just be getting a reference to the element, from which we can extract the `Message` we need to examine.  Further assignments to `p` should also take ordinary references, and not increase the refcount.

I remember talking some about this when we added the RefPtr<> support to LinkedList; I think we came out with a badly-performing choice. :(
Flags: needinfo?(wmccloskey)
Actually, we're spending more like three-quarters of our time refcounting, because the first expensive stack doesn't include the cost of the Release() as well.
I think this is just as simple as not taking strong references with `p`, but maybe I'm missing something subtle here.
I'll work on this tomorrow. Besides the refcounting thing, I think we can avoid calling ProcessPendingRequests at all in most cases.
This patch removes some refcounting. It also changes the return type of removeAndGetNext/Previous. I think originally I thought it would be safer for removeAndGetPrevious to return ClientType, but now I don't see any reason why it should.
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
Attachment #8858086 - Flags: review?(nfroyd)
This patch tries to avoid calling ProcessPendingRequests if we don't need to. It keeps track of how many messages in mPending might not be deferred (i.e., need to be processed by ProcessPendingRequests). It skips the call to ProcessPendingRequests if this number is 0. When we're not using CPOWs, it should almost always be 0.
Attachment #8858092 - Flags: review?(dvander)
Comment on attachment 8858086 [details] [diff] [review]
remove refcounting

Review of attachment 8858086 [details] [diff] [review]:

Thank you!
Attachment #8858086 - Flags: review?(nfroyd) → review+
Comment on attachment 8858092 [details] [diff] [review]
don't ProcessPendingRequests if not needed

Review of attachment 8858092 [details] [diff] [review]:

David is pretty busy. Can you look at this Kan-Ru? I think you don't need to know too much about CPOW stuff: just that ProcessPendingRequests doesn't do anything if ShouldDeferMessage returns true for all the messages in the queue.
Attachment #8858092 - Flags: review?(dvander) → review?(kchen)
Comment on attachment 8858092 [details] [diff] [review]
don't ProcessPendingRequests if not needed

Review of attachment 8858092 [details] [diff] [review]:

Looks good to me!
Attachment #8858092 - Flags: review?(kchen) → review+
Pushed by
Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj)
Don't call MessageChannel::ProcessPendingRequests if we don't expect it to do anything (r=kanru)
You need to log in before you can comment on or make changes to this bug.