MessageChannel::ProcessPendingRequests does a bunch of unnecessary refcounting

RESOLVED FIXED in Firefox 55



11 months ago
11 months ago


(Reporter: froydnj, Assigned: billm)



Firefox Tracking Flags

(firefox55 fixed)



(2 attachments)



11 months ago
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)

Comment 1

11 months ago
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.

Comment 2

11 months ago
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.
Created attachment 8858086 [details] [diff] [review]
remove refcounting

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)
Created attachment 8858092 [details] [diff] [review]
don't ProcessPendingRequests if not needed

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 6

11 months ago
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+

Comment 9

11 months ago
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)

Comment 10

11 months ago
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.