|Pickle| should use moves instead of copies

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: njn, Assigned: froydnj)

Tracking

(Blocks 2 bugs)

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox34 wontfix, firefox35+ fixed, firefox36 fixed, b2g-v2.1 affected, b2g-v2.2 fixed)

Details

(Whiteboard: [MemShrink], crash signature)

Attachments

(5 attachments)

|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.
Assignee: nobody → nfroyd
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.
Attachment #8515083 - Flags: review?(dvander)
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.
Attachment #8515086 - Flags: review?(dvander)
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.
Attachment #8515083 - Flags: review?(dvander) → review+
Attachment #8515086 - Flags: review?(dvander) → review+
Whiteboard: [MemShrink]
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.
I don't know why I thought these remaining instances in MessageChannel.cpp were
so hard to get rid of before.
Attachment #8515459 - Flags: review?(dvander)
https://hg.mozilla.org/mozilla-central/rev/86941614b59a
https://hg.mozilla.org/mozilla-central/rev/545657c82f22
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8515459 - Flags: review?(dvander) → review+
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+.
Attachment #8518257 - Flags: review+
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.
Attachment #8518257 - Flags: approval-mozilla-b2g34?
[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.
blocking-b2g: --- → 2.1?
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.
Attachment #8518402 - Flags: review?(dvander)
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?
Attachment #8518402 - Flags: review?(dvander) → review+
(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.
Attachment #8518402 - Flags: approval-mozilla-aurora?
(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...
Attachment #8518402 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
34 is marked as wontfix yet B2G 2.1 is based on 34 so I'm confused ...
Flags: needinfo?(nfroyd)
(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?
Flags: needinfo?(nfroyd) → needinfo?(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.
Flags: needinfo?(overholt) → needinfo?(bbajaj)
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 ?
Flags: needinfo?(bbajaj) → needinfo?(erahm)
I saw about a 3-4% speedup in launch time of a test app.
Flags: needinfo?(erahm)
(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.
blocking-b2g: 2.1? → ---
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.
Attachment #8518257 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
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.
Blocks: 1101264
Crash Signature: [@ OOM | large | NS_ABORT_OOM(unsigned int) | Pickle::operator=(Pickle const&) ] [@ OOM | large | NS_ABORT_OOM(unsigned int) | Pickle::Pickle(Pickle const&) ]
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.
You need to log in before you can comment on or make changes to this bug.