811596 - Shrink Channel::ChannelImpl::input_overflow_buf_ after each use

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nicholas Nethercote [inactive]]

Channel::ChannelImpl::input_overflow_buf_ is where IPC messages are received and processed.  It grows as necessary, but is never shrunk.  It's regularly 128 KiB or 256 KiB in child processes (see bug 810142).  (That's measured on b2g-desktop64, but I'm guessing that the sizes of the messages are the same on 32-bit.)

The forthcoming patch changes things so that when after calling clear() on the buffer (done once each message is finished with) we also reduce its capacity to 4096.  This is using a swap trick that I've been told (by the web and Luke Wagner) is the most reliable way to shrink a string's capacity.

The benefit:  On my b2g-desktop build this reduces the amount of memory allocated for the string to 8 KiB.  (It's not 4 KiB because this machine's std::string seems to need 57 bytes for string header stuff.  I could reduce the capacity from 4096 to something like 4000 but it doesn't seem worth chasing this.)

The downside:  We do some additional reallocs on large messages, but they seem to be rare (except for the big one we always get when a child starts up, but that already requires reallocs).

If this patch lands, we won't need to measure this buffer any more, i.e. bug 810142 will become moot.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Shrink the IPC message buffer after each message is processed.

patch

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> The downside:  We do some additional reallocs on large messages, but they
> seem to be rare (except for the big one we always get when a child starts
> up, but that already requires reallocs).

I would imagine that is the first screenshot we take after having launched the application. I suppose that as long as we use IPC to send screenshots between processes we'll always have to grow that particular buffer to accommodate them but then again the cost of taking the screenshot is probably vastly larger than the one of allocating a buffer for moving it around.

Comment 3

[Nicholas Nethercote [inactive]]

> > We do some additional reallocs on large messages, but they
> > seem to be rare (except for the big one we always get when a child starts
> > up, but that already requires reallocs).
> 
> I would imagine that is the first screenshot we take after having launched
> the application.

I actually tried printing the message contents.  A lot of was binary gobbledygook, but the parts I could read seemed to be mostly preference names (i.e. those from about:config).

Comment 4

[Josh Matthews [:jdm]]

Yes, we serialize the entire preferences database and send it to any new content process right after launching it.

Comment 5

[Nicholas Nethercote [inactive]]

cjones: review ping?  This is a decent-sized win -- 248 or 120 KiB per child process -- for something so simple.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 681328 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

>diff --git a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc b/ipc/chromium/src/chrome/common/ipc_channel_posix.cc

>@@ -541,17 +551,17 @@ bool Channel::ChannelImpl::ProcessIncomi
>                          << " channel:" << this
>                          << " message-type:" << m.type()
>                          << " header()->num_fds:" << m.header()->num_fds
>                          << " num_fds:" << num_fds
>                          << " fds_i:" << fds_i;
>             // close the existing file descriptors so that we don't leak them
>             for (unsigned i = fds_i; i < num_fds; ++i)
>               HANDLE_EINTR(close(fds[i]));
>-            input_overflow_fds_.clear();
>+            ClearAndShrink(input_overflow_buf_, Channel::kReadBufferSize);

This is a different buffer.  Typo?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry for the review lag, I was traveling last week and mostly without internet access.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Shrink the IPC message buffer after each message is processed.

New version, removes the bogus call.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac86426a764e

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 683415 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

[Approval Request Comment]

Bug caused by (feature/regressing bug #):  N/A.

User impact if declined:  120 or 248 KiB extra memory consumption per child process on B2G.

Testing completed (on m-c, etc.):   Just landed on m-c.

Risk to taking this patch (and alternatives if risky):  low.  Simple change, in IPC code that is only used on B2G.

String or UUID changes made by this patch:  N/A.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac86426a764e

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 683415 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

[Triage Comment]
Low risk, b2g-only patch in support of lower memory requirements. Approving for Aurora/Beta.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91d6bf883dfc
https://hg.mozilla.org/releases/mozilla-beta/rev/0e1c415ec2ef