1261099 - Avoid two copies in MessageChannel::MaybeUndeferIncall()

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

e10s doesn't introduce any new interrupt messages, so I doubt this is related to the OOMs we're seeing.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac4284261718

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Avoid two Message copies in MaybeUndeferIncall.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Cache some values in MessageChannel::DispatchMessage().

This will let us Move() aMsg in the next patch.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make DispatchInterruptMessage take a move ctor, and avoid a copy.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 8

[Andrew McCreight [:mccr8]]

(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".

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4032b2283068

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4032b2283068