Closed Bug 811596 Opened 7 years ago Closed 7 years ago
Impl::input _overflow _buf _ after each use
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.
Attachment #681328 - Flags: feedback?(luke)
Whiteboard: [MemShrink] → [MemShrink][slim:<256KB-per-process]
(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.
> > 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).
Yes, we serialize the entire preferences database and send it to any new content process right after launching it.
cjones: review ping? This is a decent-sized win -- 248 or 120 KiB per child process -- for something so simple.
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?
Sorry for the review lag, I was traveling last week and mostly without internet access.
New version, removes the bogus call.
Attachment #683415 - Flags: review?(jones.chris.g)
Attachment #681328 - Attachment is obsolete: true
Attachment #683415 - Flags: review?(jones.chris.g) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.
You need to log in before you can comment on or make changes to this bug.