Closed
Bug 1355178
Opened 7 years ago
Closed 7 years ago
MessageChannel::ProcessPendingRequests does a bunch of unnecessary refcounting
Categories
(Core :: IPC, enhancement)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: froydnj, Assigned: billm)
Details
Attachments
(2 files)
6.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
15.24 KB,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
This profile: https://perfht.ml/2nyBRoP is anomalous for the thousands of PRemoteSpellcheckEngine::Msg_Check messages. But having all those messages helps highlight another performance problem: https://perfht.ml/2nyh0C5 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(); 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) { IPC_LOG("ShouldDeferMessage(seqno=%d) = %d", msg.seqno(), defer); } if (!defer) { if (!toProcess.append(Move(msg))) MOZ_CRASH(); // More unnecessary strong references. p = p->removeAndGetNext(); continue; } // ...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)
Reporter | ||
Comment 1•7 years 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.
Reporter | ||
Comment 2•7 years ago
|
||
I think this is just as simple as not taking strong references with `p`, but maybe I'm missing something subtle here.
Assignee | ||
Comment 3•7 years ago
|
||
I'll work on this tomorrow. Besides the refcounting thing, I think we can avoid calling ProcessPendingRequests at all in most cases.
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Reporter | ||
Comment 6•7 years 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+
Assignee | ||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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 wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/af3fe1a5be65 Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/13895acd7c9d Don't call MessageChannel::ProcessPendingRequests if we don't expect it to do anything (r=kanru)
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af3fe1a5be65 https://hg.mozilla.org/mozilla-central/rev/13895acd7c9d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•