|Pickle| has a variable-sized buffer. It's often small, but occasionally gets large, e.g. 500 KiB or more. Pickle objects are copied a lot. Here's a hot case, from MessageChannel::DequeueOne(): > *recvd = mPending.front(); > mPending.pop_front(); The first line triggers Pickle::operator=, which copies the buffer. The second line then deletes the copied Pickle. For this case alone, if you start up an e10s browser and open a few sites, you'll hit 10s of MiBs worth of unnecessary buffer copies. There are other, similar cases scattered around. I'll look at this more on Monday. But first I will have to re-read one of those 20,000 word tutorials about how C++ moves work, so if someone else wants to take this on in the meantime please do.

I'll look at this.

I think Bill probably took care of a lot of this in bug 1062713 already; he had even written some of the same patches I had in my queue this morning!

Straightforward move constructors and move assignment operators.

These are the only places where copies occurred; I guess I could have gotten by with just move assignment in part 1, but it seemed good to have the constructors for completeness. I verified that running the following commands in ${objdir}/ipc: for o in $(find . -name '*.o'); do echo $o; objdump -dr $o |c++filt|fgrep 'IPC::Message::Message(IPC::Message&)'; done for o in $(find . -name '*.o'); do echo $o; objdump -dr $o |c++filt|fgrep 'Pickle(Pickle&)'; done produce no hits.

Comment on attachment 8515083 [details] [diff] [review] part 1 - add move semantics to Pickle and IPC::Message Review of attachment 8515083 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for taking this. ::: ipc/chromium/src/base/pickle.h @@ +47,5 @@ > Pickle(const char* data, int data_len); > > // Initializes a Pickle as a deep copy of another Pickle. > Pickle(const Pickle& other); > + Pickle(Pickle&& other); The comment about deep copy doesn't apply to the move constructor, so maybe put a blank line between these two? @@ +52,4 @@ > > // Performs a deep copy. > Pickle& operator=(const Pickle& other); > + Pickle& operator=(Pickle&& other); Ditto.

Comment on attachment 8515086 [details] [diff] [review] part 2 - use move semantics in MessageChannel.cpp Review of attachment 8515086 [details] [diff] [review]: ----------------------------------------------------------------- I think these were the only two places that showed up in my profiling. I'll check again on Monday if there was anywhere else.

(In reply to Nathan Froyd (:froydnj) from comment #4) > Created attachment 8515086 [details] [diff] [review] > part 2 - use move semantics in MessageChannel.cpp > > These are the only places where copies occurred; I guess I could have gotten > by > with just move assignment in part 1, but it seemed good to have the > constructors for completeness. > > I verified that running the following commands in ${objdir}/ipc: > > for o in $(find . -name '*.o'); do echo $o; objdump -dr $o |c++filt|fgrep > 'IPC::Message::Message(IPC::Message&)'; done > for o in $(find . -name '*.o'); do echo $o; objdump -dr $o |c++filt|fgrep > 'Pickle(Pickle&)'; done > > produce no hits. I was planning to delete the copy constructor and non-moving operator= if possible. It seems like a good idea to make it impossible to accidentally do deep copying when moving is, AFAICT, always the preferred option. Could you try doing that? Thank you.

Committed with the whitespace tweak suggested in comment 5: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d36cf13ac1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d4914ea11a (In reply to Nicholas Nethercote [:njn] from comment #7) > I was planning to delete the copy constructor and non-moving operator= if > possible. It seems like a good idea to make it impossible to accidentally do > deep copying when moving is, AFAICT, always the preferred option. Could you > try doing that? Thank you. That does seem like a good idea; I will file a followup for that.

Backed out for non-unified bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/b5703e3ef5de https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3498289&repo=mozilla-inbound

I don't know why I thought these remaining instances in MessageChannel.cpp were so hard to get rid of before.

https://hg.mozilla.org/integration/mozilla-inbound/rev/86941614b59a Relanded with appropriate #includes: https://hg.mozilla.org/integration/mozilla-inbound/rev/545657c82f22

https://hg.mozilla.org/mozilla-central/rev/86941614b59a https://hg.mozilla.org/mozilla-central/rev/545657c82f22

This is a rollup patch for b2g34. I don't know whether or what sort of patches are being taken for b2g34, but eliminating memory churn for all IPC messages seems very worthwhile. I did have to make a few changes (there's been a lot of churn in MessageChannel.cpp between 34 and central!), but I don't think any of it was particularly unusual, so carrying over r+.

Comment on attachment 8518257 [details] [diff] [review] add and use move semantics for Pickle and IPC::Message (rollup for b2g34) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): None User impact if declined: Slower apps, rendering, etc. Testing completed: Original patch has been on m-c, etc. Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: None. This patch eliminates most copying of IPC messages within the IPC subsystem, reducing memory churn and improving performance.

[Blocking Requested - why for this release]: IPC improvements for B2G [Tracking Requested - why for this release]: General IPC improvements, plus IndexedDB is now using IPC for many of its operations, and eliminating copying in those codepaths will make IndexedDB operations faster.

This patch contains the improvements from this bug and imports some improvements inspired by bug 1062713, without importing all the bits there. I'm sad that we had to write out the loop in ~AutoDeferMessages, but I didn't see a good way around that. Asking for re-review, just to make sure I didn't screw something up. Will nom for aurora assuming the review looks good.

Comment on attachment 8518402 [details] [diff] [review] add and use move semantics for Pickle and IPC::Message (rollup for aurora) Review of attachment 8518402 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this - there was a reason for all these copies originally but they've long-since been refactored away. ::: ipc/glue/MessageChannel.cpp @@ +710,5 @@ > + Message dummy; > + for (size_t i = mDeferred.length(); i != 0; --i) { > + mQueue.push_front(dummy); > + Message& first = mQueue.front(); > + first = Move(mDeferred[i-1]); Is an rvalue-ref capable push_front also not in stlport?

(In reply to David Anderson [:dvander] from comment #19) > Is an rvalue-ref capable push_front also not in stlport? Not in our version, AFAICS: http://mxr.mozilla.org/mozilla-central/source/build/stlport/stlport/stl/_deque.h#679 I don't think our version of stlport supports basic rvalue-ref functions for many things. This is probably going to start biting us hard at some point.

We should just stop using STL! :)

Comment on attachment 8518402 [details] [diff] [review] add and use move semantics for Pickle and IPC::Message (rollup for aurora) This patch removes a lot of copying from IPC code paths. As IndexedDB uses IPC for its operations on Aurora, we felt it was worthwhile to uplift this, in addition to the benefits that e10s users will see. Approval Request Comment [Feature/regressing bug #]: bug 994190, sort of [User impact if declined]: Slower operations, more memory churn. [Describe test coverage new/current, TBPL]: Green on m-c, tested by e10s, IndexedDB [Risks and why]: Low risk [String/UUID change made/needed]: None.

(In reply to Nathan Froyd (:froydnj) from comment #22) > in addition to the benefits that e10s users will see. And b2g!

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #21) > We should just stop using STL! :) Oh man, we need to talk...

34 is marked as wontfix yet B2G 2.1 is based on 34 so I'm confused ...

(In reply to Andrew Overholt [:overholt] from comment #26) > 34 is marked as wontfix yet B2G 2.1 is based on 34 so I'm confused ... Don't look at me, I didn't set the status flags for 34! :) I'm unfamiliar with the b2g 2.1 processes here; I assumed that it was a bit late to get this patch into beta, and it's not that beneficial for beta anyway. But it is beneficial for b2g (Eric Rahm measured a nice drop in j2me app startup), and I figured there was a b2g-specific tree to land on for b2g34. How many of my assumptions are incorrect?

(In reply to Nathan Froyd (:froydnj) from comment #27) > (In reply to Andrew Overholt [:overholt] from comment #26) > > 34 is marked as wontfix yet B2G 2.1 is based on 34 so I'm confused ... > > Don't look at me, I didn't set the status flags for 34! :) :) > I'm unfamiliar with the b2g 2.1 processes here; I assumed that it was a bit > late to get this patch into beta, and it's not that beneficial for beta > anyway. But it is beneficial for b2g (Eric Rahm measured a nice drop in > j2me app startup), and I figured there was a b2g-specific tree to land on > for b2g34. > > How many of my assumptions are incorrect? None. It's possible to uplift this to the b2g-specific 34 tree, we just need justification to do so and an approval-b2g34 (or whatever) request. Bhavana can help with that.

Given where we are in the 2.1 release cycle and keeping in mind we are hitting the performance targets there I am not sure I want to uplift this at this point unless the win is huge and there are no fallouts. Looks like Eric has done analysis here. Eric, can you please point out if this landing significantly helped any improve app launchtime or other perf metrics ?

I saw about a 3-4% speedup in launch time of a test app.

(In reply to Eric Rahm [:erahm] from comment #30) > I saw about a 3-4% speedup in launch time of a test app. Which mean how many ms?

(In reply to Fabrice Desré [:fabrice] from comment #31) > (In reply to Eric Rahm [:erahm] from comment #30) > > I saw about a 3-4% speedup in launch time of a test app. > > Which mean how many ms? ~1000ms, this app has a particularly long launch time.

Hm, ok I see :P So I agree with Bhavana to just ride the trains.

Comment on attachment 8518257 [details] [diff] [review] add and use move semantics for Pickle and IPC::Message (rollup for b2g34) Considering Eric's i/p and comment #33, removing the approval and this can ride the trains.

Nice fix! On desktop the majority of Pickle OOMs are between 6-7 megs. This will definitely help things on machines where contiguous address space is at a premium.

Can we figure out what we're sending over IPC that's several MB? Because that's quite large.

This user frequently fails on such allocations, maybe they could help you figure out the cause: https://support.mozilla.org/en-US/questions/1029898?fpa=1

(In reply to David Major [:dmajor] (UTC+13) from comment #37) > https://support.mozilla.org/en-US/questions/1029898?fpa=1 Wow, that thread is depressing...

> This user frequently fails on such allocations, maybe they could help you > figure out the cause: > https://support.mozilla.org/en-US/questions/1029898?fpa=1 The user is on 33, and so (apparently) doesn't have e10s enabled... so why is Pickle being used? Maybe by the thumbnails process?

We also use this code so that the main thread can talk to the compositor thread. That's probably the biggest use without e10s.

Hrm, maybe we're transferring large bits of data via the message manager for the thumbnail stuff? https://crash-stats.mozilla.com/report/index/e6946448-80bf-44ac-a285-4ad022141108 The stacks don't make this very clear, unfortunately.

Let's discuss the OOMs from large IPC messages or whatever in bug 1101264.