Closed
Bug 1261099
Opened 8 years ago
Closed 8 years ago
Avoid two copies in MessageChannel::MaybeUndeferIncall()
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Whiteboard: [MemShrink][not e10s-specific])
Attachments
(1 file, 2 obsolete files)
1.18 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
Similar to bug 1261094, I think MessageChannel::DispatchInterruptMessage() ends up making a copy of the message. I think this actually does need the fully message, but as far as I can see, this could be changed to a move. It looks like this method is called by OnMaybeDequeueOne() and Send(), so it may affect e10s.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → continuation
e10s doesn't introduce any new interrupt messages, so I doubt this is related to the OOMs we're seeing.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink][not e10s-specific]
Assignee | ||
Comment 2•8 years ago
|
||
Fun, MaybeUndeferIncall() copies when pulling a message out of a stack, then does another copy when putting into another queue.
Assignee | ||
Comment 3•8 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac4284261718
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8737322 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 5•8 years ago
|
||
This will let us Move() aMsg in the next patch.
Attachment #8737323 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8737324 -
Flags: review?(wmccloskey)
Attachment #8737322 -
Flags: review?(wmccloskey) → review+
Attachment #8737323 -
Flags: review?(wmccloskey) → review+
Comment on attachment 8737324 [details] [diff] [review] part 3 - Make DispatchInterruptMessage take a move ctor, and avoid a copy. Review of attachment 8737324 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/MessageChannel.cpp @@ +1679,5 @@ > 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)); I suspect this doesn't do what you want. Since aMsg is declared as const, I don't think we'll use the move version. So I think we're still copying; you should be able to verify with printfs or in a debugger or something. And if you remove const from DispatchInterruptMessage, then you're going to have to change a good deal more code. To be honest, this copy is going to be quite rare. It happens in the plugin channel when a particular kind of message race occurs. I'm not sure it's really worth the effort.
Attachment #8737324 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #7) > Since aMsg is declared as const, I don't think we'll use the move version. Ah, that's a fun Move() footgun I was not aware of... > To be honest, this copy is going to be quite rare. It happens in the plugin > channel when a particular kind of message race occurs. I'm not sure it's > really worth the effort. Fair enough. I'd like to move to a world where we prevent accidental copies, but I suppose any place like this that does a rare intentional copy could just explicitly do a copy, rather than moving to full "copy hygeine".
Assignee | ||
Updated•8 years ago
|
Attachment #8737323 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8737324 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Summary: MessageChannel::mDeferred makes a full copy of the message → Avoid two copies in MessageChannel::MaybeUndeferIncall()
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4032b2283068
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•