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

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

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 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?

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

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

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.

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

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

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

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

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

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.

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.

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.

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.