Last Comment Bug 811596 - Shrink Channel::ChannelImpl::input_overflow_buf_ after each use
: Shrink Channel::ChannelImpl::input_overflow_buf_ after each use
Status: RESOLVED FIXED
[MemShrink][slim:<256KB-per-process]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: slim-fast B2GDarkMatter
  Show dependency treegraph
 
Reported: 2012-11-13 18:31 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-11-21 14:40 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Shrink the IPC message buffer after each message is processed. (3.27 KB, patch)
2012-11-13 18:32 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
luke: feedback+
Details | Diff | Review
Shrink the IPC message buffer after each message is processed. (2.42 KB, patch)
2012-11-19 18:45 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
cjones.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-13 18:31:00 PST
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 [:njn] (on vacation until July 11) 2012-11-13 18:32:51 PST
Created attachment 681328 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

patch
Comment 2 Gabriele Svelto [:gsvelto] 2012-11-14 04:27:05 PST
(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 [:njn] (on vacation until July 11) 2012-11-14 13:57:45 PST
> > 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] 2012-11-15 03:39:33 PST
Yes, we serialize the entire preferences database and send it to any new content process right after launching it.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-15 22:02:01 PST
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 2012-11-19 17:29:33 PST
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 2012-11-19 17:30:22 PST
Sorry for the review lag, I was traveling last week and mostly without internet access.
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-19 18:45:24 PST
Created attachment 683415 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

New version, removes the bogus call.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-20 01:00:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac86426a764e
Comment 10 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-20 01:03:20 PST
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] 2012-11-20 07:03:34 PST
https://hg.mozilla.org/mozilla-central/rev/ac86426a764e
Comment 12 Alex Keybl [:akeybl] 2012-11-20 11:32:37 PST
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.

Note You need to log in before you can comment on or make changes to this bug.