Trim IPC ChannelReader's buffer.

Trim IPC ChannelReader's buffer after processing large messages.

Most IPC messages are small, but there are few ones that can be quite large.
With this change ChannelReader trims its input buffer after processing large
messages.

BUG=529940
Committed: https://crrev.com/06a2e65a4f3610ec17dbc5988c0b16a95825240a
Cr-Commit-Position: refs/heads/master@{#357708}

[Dmitry Skiba, 2015-09-28 18:53:17]

dskiba@google.com changed reviewers:
+ jam@chromium.org, mariakhomenko@chromium.org, sievers@chromium.org

[Dmitry Skiba, 2015-09-28 18:53:17]

Tests are coming, please check the approach.

[jam, 2015-09-28 20:27:42]

it seems this change is good on its own (i.e. if someone sends a very large
string).

what about fixing all the places that are sending unbounded data, i.e.
domstorage I think was one example that you mentioned? Are there bugs against
relevant owners to change all these to shared memory?

[Maria, 2015-09-28 21:05:40]

On 2015/09/28 20:27:42, jam wrote:
> it seems this change is good on its own (i.e. if someone sends a very large
> string).
> 
> what about fixing all the places that are sending unbounded data, i.e.
> domstorage I think was one example that you mentioned? Are there bugs against
> relevant owners to change all these to shared memory?

Here's the bug against DOM storage:
https://code.google.com/p/chromium/issues/detail?id=532653. I chatted with the
guys working on it and they are onboard -- will hopefully see it happen in Q4.

I don't think we filed any other bugs, but we should track down owners for other
large messages in that spreadsheet. Will do.

[no sievers, 2015-09-28 22:54:44]

I also think handling this generally would be better, because
- why not have everyone benefit?
- if it causes problems (like affects performance negatively), why do we want to
further punish devices that are already disadvantaged?
- am worried about maintainability and test coverage of doing subtle special
casing here and there

[Dmitry Skiba, 2015-09-29 17:48:40]

Well, this change trades some performance (since we are trimming and then
potentially resizing again) for some memory (since huge messages are rare). This
tradeoff makes perfect sense on a low-end Android device where running out of
memory (and triggering low memory signal, GC, OOM killer, etc.) costs us far
more. I'm not sure that we really need that on other platforms. Besides, current
max buffer size is 64 KiB because this is what we found from analyzing message
dumps. Different platforms may have different message size distribution (for
example, x64 might be different).

And performance impact is not a problem here. I ran some tests yesterday and it
looks like on average we spend ~200 microseconds resizing / trimming the buffer
for huge messages. Considering that (1) huge messages are rare, and (2) we spend
milliseconds actually deserializing each huge message, that is tolerable. On the
other hand, we save at least ~450 KiB when casually browsing, according to the
message size histogram:
https://docs.google.com/spreadsheets/d/11-mGTHS9HD8GZ6kXN2jxe6ytFN_KW00_4GKRU...
I'll gather more data today.

Actually, the previous change, "Resize IPC input buffer to fit the next
message." is the one that really improves performance, and it's available to all
platforms. It also slightly reduces memory usage too.

As for test coverage - there are tests, but I'll be happy to add more if you can
think of a particular uncovered case.

[Dmitry Skiba, 2015-09-29 18:30:09]

After talking to Maria about this we decided to start implementing UMA histogram
for message sizes (only for the browser part of browser-renderer IPC channel).
That will give us more information about real message sizes on various
platforms, and will help to decide on suitable max buffer size.

Regarding this change, it looks like we have two options:

1. Push this change (for Android only), and then expand it to all platforms.

2. Wait for data from UMA and amend this change to affect all platform at once.


I don't know how long it will take us to collect useful data, so I would prefer
to go with option 1.

[jam, 2015-09-29 19:34:37]

On 2015/09/29 18:30:09, Dmitry Skiba wrote:
> After talking to Maria about this we decided to start implementing UMA
histogram
> for message sizes (only for the browser part of browser-renderer IPC channel).
> That will give us more information about real message sizes on various
> platforms, and will help to decide on suitable max buffer size.
> 
> Regarding this change, it looks like we have two options:
> 
> 1. Push this change (for Android only), and then expand it to all platforms.
> 
> 2. Wait for data from UMA and amend this change to affect all platform at
once.
> 
> 
> I don't know how long it will take us to collect useful data, so I would
prefer
> to go with option 1.

we'll get data from canary channel pretty quickly? that is desktop only, but
most IPCs (i.e. all the stuff quoted so far) are cross platform.

[Dmitry Skiba, 2015-10-28 21:19:15]

Description was changed from

==========
Shrink IPC channel's buffer on low-end Android devices.

Most IPC messages are small, but there are few ones that can occasionally
grow pretty large (~1 MiB). On low-end Android device that is ~5-7% of all
memory, so it makes sense to shrink IPC buffers once large message is
processed.

BUG=529940
==========

to

==========
Trim IPC ChannelReader's buffer after processing large messages.

Most IPC messages are small, but there are few ones that can be quite large.
With this change ChannelReader trims its input buffer after processing large
messages.

BUG=529940
==========

[Dmitry Skiba, 2015-10-28 21:19:28]

Guys, please review.

[jam, 2015-10-29 21:30:57]

lgtm

[Dmitry Skiba, 2015-10-30 01:37:23]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 01:37:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377483003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377483003/100001

[commit-bot: I haz the power, 2015-10-30 01:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 01:56:13]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Dmitry Skiba, 2015-10-30 20:49:58]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2015-10-30 20:49:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1377483003/#ps140001
(title: "Fix for COW std::strings")

[commit-bot: I haz the power, 2015-10-30 20:50:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377483003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377483003/140001

[commit-bot: I haz the power, 2015-10-30 21:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 21:29:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dmitry Skiba, 2015-11-04 00:11:39]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2015-11-04 00:11:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1377483003/#ps180001
(title: "Fix USE_ATTACHMENT_BROKER && defined(OS_MACOSX) && !defined(OS_IOS)
case")

[commit-bot: I haz the power, 2015-11-04 00:14:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377483003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377483003/180001

[commit-bot: I haz the power, 2015-11-04 01:25:06]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-04 01:26:39]

Patchset 10 (id:??) landed as
https://crrev.com/06a2e65a4f3610ec17dbc5988c0b16a95825240a
Cr-Commit-Position: refs/heads/master@{#357708}