1092462 - delete Pickle/IPC::Message's copy assignment method

Status: RESOLVED DUPLICATE of bug 1273307

(Core :: IPC, defect)

Description

[Nathan Froyd [:froydnj]]

We shouldn't really be copying messages around when we can mozilla::Move them, so delete them to encourage people to Not Do That.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - don't use IPC::Message::operator=(IPC::Message const&) in MessageChannel.cpp

All of these probably should have been addressed in bug 1092010.

Comment 2

[Nathan Froyd [:froydnj]]

After part 1, deleting the copy constructor/assignment methods runs into problems with ChannelProxy, because the current implementation of ChannelProxy (and associated infrastructure, like Message::Sender and Channel::Listener) pass |const Message&| around.  Which is fine, except when that means you wind up copying the message because you need to re-inject messages into the event-loop:

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/ipc_channel_proxy.cc#62

Not immediately sure how to solve that.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8515448 [details] [diff] [review]
part 1 - don't use IPC::Message::operator=(IPC::Message const&) in MessageChannel.cpp

Review of attachment 8515448 [details] [diff] [review]:
-----------------------------------------------------------------

This is identical to "part 3" in bug 1092010, right?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> After part 1, deleting the copy constructor/assignment methods runs into
> problems with ChannelProxy

If it's too hard, that's fine; I expect you've hit all the places that affect performance significantly. A comment indicating that the move operations are preferable to the copy operations would be good.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Comment on attachment 8515448 [details] [diff] [review]
>
> This is identical to "part 3" in bug 1092010, right?

That's correct; it seemed better to just roll it into that bug once I realized how much architecture work was needed for this bug.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: DMD results

I did some more profiling. Things look better now. I've attached updated cumulative results involving the remaining places where the copy constructor/operator= is used in a way that might be avoided.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> I did some more profiling. Things look better now. I've attached updated
> cumulative results involving the remaining places where the copy
> constructor/operator= is used in a way that might be avoided.

Thanks, that's interesting.  It looks like OnMessageReceivedFromLink is the place to fix everything, but its parameter is |const Message&|, which we can't |Move| out of.  More of that re-architecting things to fix needless copying.  Maybe it's worth taking a look at Chromium's current IPC code to see if they've fixed things...

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> Thanks, that's interesting.  It looks like OnMessageReceivedFromLink is the
> place to fix everything, but its parameter is |const Message&|, which we
> can't |Move| out of.  More of that re-architecting things to fix needless
> copying.  Maybe it's worth taking a look at Chromium's current IPC code to
> see if they've fixed things...

A quick skim through Chromium's code indicates that they have not optimized copying yet (no move constructors, and nothing like |swap()| to do moves by hand).  They do have some Pickle performance improvements, though (removing some trivial checks on every datatype read, and small improvements elsewhere).

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We've diverged enough from them that anything higher-level than pickle/ipc_message is probably not relevant anyway.

Comment 10

[Nicholas Nethercote [inactive]]

> They do have some Pickle performance improvements, though (removing
> some trivial checks on every datatype read, and small improvements
> elsewhere).

Anything worth copying?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> > They do have some Pickle performance improvements, though (removing
> > some trivial checks on every datatype read, and small improvements
> > elsewhere).
> 
> Anything worth copying?

Oh, sure, it'd be good stuff to copy in the abstract.

I'm not sure how much it'd win in terms of actual performance.  In bug 871596, cjones said he'd never seen the pickle stuff come up on profiles, and even though the work in the bug was worthwhile, I don't know that there were measurable performance improvements.

OTOH, Chromium felt that it was worth doing, and fewer instructions executed for every primitive read/write in IPC code has to be some kind of a good thing...I'll put it on my list of things to do.

Comment 12

[Andrew McCreight [:mccr8]]

The copy assignment method is still unused. The ctor is going to be harder to remove, so let's just leave that to some unspecified future time.

Comment 13

[Andrew McCreight [:mccr8]]

This builds just fine on Linux, but the older STL on OSX uses doesn't define move constructor stuff for std::queue. With it, the call to swap() in GeckoChildProcessHost::GetQueuedMessages() apparently triggers a copy (awesome...), plus 4 calls to mPending.erase() in MessageChannel.cpp. erahm pointed out the trick that you can switch to using std::deque instead of std:queue, and it has a swap() method, so I'll try that.

Comment 14

[Andrew McCreight [:mccr8]]

Using deque worked for the swap(), but I'm not sure how to make the calls to mPending.erase() work easily. This will have to wait until OSX STL understands move constructors or somebody wants to make more invasive changes.

Comment 15

[Andrew McCreight [:mccr8]]

Oh and I think somebody said that Android also uses an older STL, so anybody working on this will need to make sure that works, too.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Delete Pickle/IPC::Message's copy assignment method.