Adding StringPiece read/write support to pickle.

Adding StringPiece/StringPiece16 read/write support to pickle, and
update unit tests.

The IPC perf tests do a lot of allocations which can, with large block
sizes, harm their performance. The high allocation counts also make
their performance very dependent on the quirks of the Windows heap, as
it applies undocumented heuristics to decide when to decommit memory
and when to save it for later.

And, doing unnecessary allocations is generally always a bad thing.

So, this change adds StringPiece support to PickleIterator (reading)
and Pickle (writing). The StringPiece function now handles all strings
when writing, but must be requested explicitly when reading.

ipc_mojo_perftests does more allocations than ipc_perftests. This
removes one message-sized allocation from both tests.

The unit tests were updated so that WriteString and WriteString16 are
exercised with both string objects and char/char16 arrays (no
allocations required!). Reading into StringPiece and StringPiece16 is
also tested and the tests were verified with:
out\Release\base_unittests --gtest_filter=PickleTest.*

The main performance test command line used was:

out\Release\ipc_mojo_perftests --gtest_filter=MojoChannelPerfTest.ChannelPingPong

Typical test results on HP Z620 (Windows 8.1) with thread affinity and
high-performance power settings prior to this change:
IPC_Channel_Perf_50000x_12      1140.01 ms
IPC_Channel_Perf_50000x_144     1182.55 ms
IPC_Channel_Perf_50000x_1728    1266.42 ms
IPC_Channel_Perf_12000x_20736   1289.19 ms
IPC_Channel_Perf_1000x_248832   584.619 ms

Typical test results with same settings but using base::StringPiece:
IPC_Channel_Perf_50000x_12      1123.33 ms
IPC_Channel_Perf_50000x_144     1164.53 ms
IPC_Channel_Perf_50000x_1728    1242.99 ms
IPC_Channel_Perf_12000x_20736   1186.84 ms
IPC_Channel_Perf_1000x_248832   496.469 ms

Modest improvement with small buffers, but significant speedup with large buffers.

Typical test results with large-blocks only prior to this change:
IPC_Channel_Perf_1000x_248832   1211.33 ms
IPC_Channel_Perf_1000x_248832   961.404 ms
IPC_Channel_Perf_1000x_248832   600.911 ms
IPC_Channel_Perf_1000x_248832   468.356 ms
IPC_Channel_Perf_1000x_248832   430.859 ms
IPC_Channel_Perf_1000x_248832   425.147 ms

Typical test results with large-blocks only (base::StringPiece):
IPC_Channel_Perf_1000x_248832   909.571 ms
IPC_Channel_Perf_1000x_248832   731.435 ms
IPC_Channel_Perf_1000x_248832   493.697 ms
IPC_Channel_Perf_1000x_248832   417.966 ms
IPC_Channel_Perf_1000x_248832   397.377 ms
IPC_Channel_Perf_1000x_248832   397.725 ms

Note that it takes a while for the Windows heap to 'realize' that it
should hang on to some of the large blocks which is why performance
increases over multiple runs.

Chrome will not immediately be affected because StringPiece reading has
to be explicitly selected. Note that the effect on ipc_perftests is
more variable due to the odd Windows heap heuristics.

Reliable results require setting the power plan to high-performance.
On Linux this is done with this command:
sudo cpupower frequency-set --governor performance

The ipc_perftests command-line is:
out/Release/ipc_perftests --gtest_filter=IPCChannelPerfTest.ChannelPingPong

Typical before/after Linux results for ipc_perftests are:
IPC_Channel_Perf_50000x_12      465.271 ms
IPC_Channel_Perf_50000x_144     480.224 ms
IPC_Channel_Perf_50000x_1728    510.871 ms
IPC_Channel_Perf_12000x_20736   318.016 ms
IPC_Channel_Perf_1000x_248832   309.325 ms

IPC_Channel_Perf_50000x_12      459.245 ms
IPC_Channel_Perf_50000x_144     479.347 ms
IPC_Channel_Perf_50000x_1728    506.57  ms
IPC_Channel_Perf_12000x_20736   289.583 ms
IPC_Channel_Perf_1000x_248832   255.083 ms

Before after Linux results for ipc_mojo_perftests:
IPC_Channel_Perf_50000x_12      670.727 ms
IPC_Channel_Perf_50000x_144     713.6   ms
IPC_Channel_Perf_50000x_1728    808.157 ms
IPC_Channel_Perf_12000x_20736   464.221 ms
IPC_Channel_Perf_1000x_248832   365.258 ms

IPC_Channel_Perf_50000x_12      653.12  ms
IPC_Channel_Perf_50000x_144     697.418 ms
IPC_Channel_Perf_50000x_1728    772.575 ms
IPC_Channel_Perf_12000x_20736   446.315 ms
IPC_Channel_Perf_1000x_248832   348.38  ms

So, consistent improvements on Linux.

Committed: https://crrev.com/fcfde7d98209569fba81de4f1b26d0e26cd95848
Cr-Commit-Position: refs/heads/master@{#319128}

[brucedawson, 2015-02-17 21:53:37]

brucedawson@chromium.org changed reviewers:
+ thestig@chromium.org

[brucedawson, 2015-02-17 21:53:38]

Does this seem worthwhile? Enabling fewer allocations seems like a good
direction to go in.

[Lei Zhang, 2015-02-17 22:37:56]

Exercise the new methods in PickleTest.EncodeDecode?

https://codereview.chromium.org/927183002/diff/1/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode43
base/pickle.h:43: bool ReadStringPiece(base::StringPiece* result)
WARN_UNUSED_RESULT;
Do you feel it's obvious that the StringPiece's only valid as long as the pickle
is valid?

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode45
base/pickle.h:45: bool ReadString16(base::string16* result) WARN_UNUSED_RESULT;
Add ReadStringPiece16()?

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode202
base/pickle.h:202: bool WriteString16(const base::string16& value);
Change this to base::StringPiece16 as well? There's no std::wstring version of
StringPiece.

[Lei Zhang, 2015-02-17 22:48:58]

On 2015/02/17 21:53:38, brucedawson wrote:
> Does this seem worthwhile? Enabling fewer allocations seems like a good
> direction to go in.

To answer this, yes. One of the hesitations that prevent StringPiece use is that
if a class takes a StringPiece, but then passes to some other function that does
not take a StringPiece, you lose out on the benefits of StringPiece. Since
Pickle is an end consumer, it can definitely benefit from using StringPiece.

After this CL, I can go through the ReadString* callers and see if any of them
can take advantage of ReadStringPiece* and make sure there's no WriteString
callers that needlessly create a string object.

[brucedawson, 2015-02-18 21:41:59]

PTAL

All suggestions addressed, and the updated unit test is passing.

Being able to write char[] and char16[] strings is certainly handy.

https://codereview.chromium.org/927183002/diff/1/base/pickle.h
File base/pickle.h (right):

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode43
base/pickle.h:43: bool ReadStringPiece(base::StringPiece* result)
WARN_UNUSED_RESULT;
It doesn't hurt to explicitly mention the validity restrictions, given the
importance. Done.

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode45
base/pickle.h:45: bool ReadString16(base::string16* result) WARN_UNUSED_RESULT;
On 2015/02/17 22:37:56, Lei Zhang (Soon to be OOO) wrote:
> Add ReadStringPiece16()?

Done.

https://codereview.chromium.org/927183002/diff/1/base/pickle.h#newcode202
base/pickle.h:202: bool WriteString16(const base::string16& value);
On 2015/02/17 22:37:56, Lei Zhang (Soon to be OOO) wrote:
> Change this to base::StringPiece16 as well? There's no std::wstring version of
> StringPiece.

Done.

[brucedawson, 2015-02-18 21:42:40]

brucedawson@chromium.org changed reviewers:
+ morrita@chromium.org

[brucedawson, 2015-02-18 21:43:17]

Adding morrita@ for approval/notification of ipc_perftest change.

[Lei Zhang, 2015-02-18 22:35:54]

lgtm

[brucedawson, 2015-02-18 23:03:11]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:04:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927183002/20001

[commit-bot: I haz the power, 2015-02-18 23:10:34]

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

[commit-bot: I haz the power, 2015-02-18 23:10:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brucedawson, 2015-02-19 01:12:18]

brucedawson@chromium.org changed reviewers:
+ cpu@chromium.org
- morrita@chromium.org

[brucedawson, 2015-02-19 01:12:19]

Adding cpu@ for required approval of change to ipc test. Moving morrita@ to CC.

[cpu_(ooo_6.6-7.5), 2015-02-19 03:51:40]

lgtm

[brucedawson, 2015-02-19 17:17:16]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-02-19 17:18:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927183002/20001

[commit-bot: I haz the power, 2015-02-19 18:26:41]

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

[commit-bot: I haz the power, 2015-02-19 18:26:42]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brucedawson, 2015-02-19 22:18:56]

On 2015/02/19 18:26:42, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Well that's odd. The linux_chromium_asan_rel build step worked last time:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

The failure suggests an unreliability in the goma builds. Trying again.

[brucedawson, 2015-02-19 22:19:02]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-02-19 22:20:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927183002/20001

[commit-bot: I haz the power, 2015-02-19 23:12:11]

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

[commit-bot: I haz the power, 2015-02-19 23:12:12]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brucedawson, 2015-02-20 00:10:45]

New patchsets have been uploaded after l-g-t-m from
thestig@chromium.org,cpu@chromium.org

[brucedawson, 2015-02-24 19:40:25]

This is still pending due to intermittent link errors caused by an apparent
pnacl dependency bug:

https://code.google.com/p/chromium/issues/detail?id=461422

At this point the only way to safely land this change would be to add a landmine
to force a clobber build, but that seems inappropriate.

[Lei Zhang, 2015-03-04 20:36:07]

On 2015/02/24 19:40:25, brucedawson wrote:
> This is still pending due to intermittent link errors caused by an apparent
> pnacl dependency bug:
> 
> https://code.google.com/p/chromium/issues/detail?id=461422
> 
> At this point the only way to safely land this change would be to add a
landmine
> to force a clobber build, but that seems inappropriate.

The bug got fixed today. Shall we land this?

[brucedawson, 2015-03-04 21:13:46]

> The bug got fixed today. Shall we land this?

Yes. I ran a "git cl try" to confirm the bug fix and all 24 builds passed.
Committing now.

[brucedawson, 2015-03-04 21:13:52]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-03-04 21:14:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927183002/60001

[commit-bot: I haz the power, 2015-03-04 21:18:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-04 21:21:33]

Patchset 4 (id:??) landed as
https://crrev.com/fcfde7d98209569fba81de4f1b26d0e26cd95848
Cr-Commit-Position: refs/heads/master@{#319128}

[brucedawson, 2015-03-05 01:27:23]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/981853002/ by brucedawson@chromium.org.

The reason for reverting is: Causing crashes on Linux. Investigating.
Bug 464180..