Closed Bug 1261099 Opened 5 years ago Closed 5 years ago

Avoid two copies in MessageChannel::MaybeUndeferIncall()

Categories

(Core :: IPC, defect)

defect
Not set
normal

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)

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: nobody → continuation
e10s doesn't introduce any new interrupt messages, so I doubt this is related to the OOMs we're seeing.
Whiteboard: [MemShrink] → [MemShrink][not e10s-specific]
Fun, MaybeUndeferIncall() copies when pulling a message out of a stack, then does another copy when putting into another queue.
This will let us Move() aMsg in the next patch.
Attachment #8737323 - 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)
(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".
Attachment #8737323 - Attachment is obsolete: true
Attachment #8737324 - Attachment is obsolete: true
Summary: MessageChannel::mDeferred makes a full copy of the message → Avoid two copies in MessageChannel::MaybeUndeferIncall()
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4032b2283068
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.