The default bug view has changed. See this FAQ.

Shrink Channel::ChannelImpl::input_overflow_buf_ after each use

RESOLVED FIXED in Firefox 18

Status

()

Core
IPC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla20
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

(Whiteboard: [MemShrink][slim:<256KB-per-process])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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)

Comment 1

4 years ago
Created attachment 681328 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

patch
Attachment #681328 - Flags: review?(jones.chris.g)
(Assignee)

Updated

4 years ago
Assignee: nobody → n.nethercote
(Assignee)

Updated

4 years ago
Attachment #681328 - Flags: feedback?(luke)

Updated

4 years ago
Attachment #681328 - Flags: feedback?(luke) → feedback+
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 3

4 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

4 years ago
Yes, we serialize the entire preferences database and send it to any new content process right after launching it.
(Assignee)

Comment 5

4 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

4 years ago
Created attachment 683415 [details] [diff] [review]
Shrink the IPC message buffer after each message is processed.

New version, removes the bogus call.
Attachment #683415 - Flags: review?(jones.chris.g)
(Assignee)

Updated

4 years ago
Attachment #681328 - Attachment is obsolete: true
Attachment #683415 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac86426a764e
(Assignee)

Comment 10

4 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?
https://hg.mozilla.org/mozilla-central/rev/ac86426a764e
Status: NEW → RESOLVED
Last Resolved: 4 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.
Attachment #683415 - Flags: approval-mozilla-beta?
Attachment #683415 - Flags: approval-mozilla-beta+
Attachment #683415 - Flags: approval-mozilla-aurora?
Attachment #683415 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/91d6bf883dfc
https://hg.mozilla.org/releases/mozilla-beta/rev/0e1c415ec2ef
status-firefox18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
You need to log in before you can comment on or make changes to this bug.