Avoid two copies in MessageChannel::MaybeUndeferIncall()

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [MemShrink][not e10s-specific])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 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

3 years ago
Whiteboard: [MemShrink] → [MemShrink][not e10s-specific]
(Assignee)

Comment 2

3 years ago
Fun, MaybeUndeferIncall() copies when pulling a message out of a stack, then does another copy when putting into another queue.
(Assignee)

Comment 4

3 years ago
Created attachment 8737322 [details] [diff] [review]
part 1 - Avoid two Message copies in MaybeUndeferIncall.
Attachment #8737322 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

3 years ago
Created attachment 8737323 [details] [diff] [review]
part 2 - Cache some values in MessageChannel::DispatchMessage().

This will let us Move() aMsg in the next patch.
Attachment #8737323 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

3 years ago
Created attachment 8737324 [details] [diff] [review]
part 3 - Make DispatchInterruptMessage take a move ctor, and avoid a copy.
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

3 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

3 years ago
Attachment #8737323 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8737324 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Summary: MessageChannel::mDeferred makes a full copy of the message → Avoid two copies in MessageChannel::MaybeUndeferIncall()
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4032b2283068
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.