Closed
Bug 811596
Opened 12 years ago
Closed 12 years ago
Shrink Channel::ChannelImpl::input_overflow_buf_ after each use
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink][slim:<256KB-per-process])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
cjones
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → n.nethercote
Assignee | ||
Updated•12 years ago
|
Attachment #681328 -
Flags: feedback?(luke)
Updated•12 years ago
|
Attachment #681328 -
Flags: feedback?(luke) → feedback+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink][slim:<256KB-per-process]
Comment 2•12 years ago
|
||
(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.
Assignee | ||
Comment 3•12 years ago
|
||
> > 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•12 years ago
|
||
Yes, we serialize the entire preferences database and send it to any new content process right after launching it.
Assignee | ||
Comment 5•12 years ago
|
||
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?
Attachment #681328 -
Flags: review?(jones.chris.g)
Sorry for the review lag, I was traveling last week and mostly without internet access.
Assignee | ||
Comment 8•12 years ago
|
||
New version, removes the bogus call.
Attachment #683415 -
Flags: review?(jones.chris.g)
Assignee | ||
Updated•12 years ago
|
Attachment #681328 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #683415 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
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.
Attachment #683415 -
Flags: approval-mozilla-beta?
Attachment #683415 -
Flags: approval-mozilla-aurora?
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 12•12 years ago
|
||
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.
Attachment #683415 -
Flags: approval-mozilla-beta?
Attachment #683415 -
Flags: approval-mozilla-beta+
Attachment #683415 -
Flags: approval-mozilla-aurora?
Attachment #683415 -
Flags: approval-mozilla-aurora+
Comment 13•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•