1092010 - |Pickle| should use moves instead of copies

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nicholas Nethercote [inactive]]

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

Comment 1

[Nathan Froyd [:froydnj]]

I'll look at this.

Comment 2

[Nathan Froyd [:froydnj]]

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!

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add move semantics to Pickle and IPC::Message

Straightforward move constructors and move assignment operators.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 5

[Nicholas Nethercote [inactive]]

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 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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

Comment 8

[Nathan Froyd [:froydnj]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Nathan Froyd [:froydnj]]

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

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

Comment 11

[Nathan Froyd [:froydnj]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/545657c82f22

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/789d38312b9e

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/789d38312b9e

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: add and use move semantics for Pickle and IPC::Message (rollup for b2g34)

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 16

[Nathan Froyd [:froydnj]]

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.

Comment 17

[Nathan Froyd [:froydnj]]

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

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: add and use move semantics for Pickle and IPC::Message (rollup for aurora)

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 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

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?

Comment 20

[Nathan Froyd [:froydnj]]

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

Comment 21

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

We should just stop using STL! :)

Comment 22

[Nathan Froyd [:froydnj]]

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.

Comment 23

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

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

And b2g!

Comment 24

[Nathan Froyd [:froydnj]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/975ffc36f1de

Comment 26

[Andrew Overholt [:overholt]]

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

Comment 27

[Nathan Froyd [:froydnj]]

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

Comment 28

[Andrew Overholt [:overholt]]

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

Comment 29

[bhavana bajaj [:bajaj]]

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 ?

Comment 30

[Eric Rahm [:erahm]]

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

Comment 31

[[:fabrice] Fabrice Desré]

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

Comment 32

[Eric Rahm [:erahm]]

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

Comment 33

[[:fabrice] Fabrice Desré]

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

Comment 34

[bhavana bajaj [:bajaj]]

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.

Comment 35

[(Away)]

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.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 37

[(Away)]

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

Comment 38

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

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

Comment 39

[Nicholas Nethercote [inactive]]

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

Comment 40

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

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

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 42

[Andrew McCreight [:mccr8]]

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