Revert of Adding StringPiece read/write support to pickle. (patchset #4...

Revert of Adding StringPiece read/write support to pickle. (patchset #4 id:60001 of https://codereview.chromium.org/927183002/)

Reason for revert:
Causing crashes on Linux. Investigating.
Bug 464180.

Original issue's description:
> 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}

TBR=thestig@chromium.org,cpu@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review URL: https://codereview.chromium.org/981853002

Cr-Commit-Position: refs/heads/master@{#319191}

base/pickle.cc

@@ -152,18 +152,6 @@ bool PickleIterator::ReadString(std::string* result) {
   return true;
 }
 
-bool PickleIterator::ReadStringPiece(base::StringPiece* result) {
-  int len;
-  if (!ReadInt(&len))
-    return false;
-  const char* read_from = GetReadPointerAndAdvance(len);
-  if (!read_from)
-    return false;
-
-  *result = base::StringPiece(read_from, len);
-  return true;
-}
-
 bool PickleIterator::ReadWString(std::wstring* result) {
   int len;
   if (!ReadInt(&len))
@@ -188,19 +176,6 @@ bool PickleIterator::ReadString16(string16* result) {
   return true;
 }
 
-bool PickleIterator::ReadStringPiece16(base::StringPiece16* result) {
-  int len;
-  if (!ReadInt(&len))
-    return false;
-  const char* read_from = GetReadPointerAndAdvance(len, sizeof(char16));
-  if (!read_from)
-    return false;
-
-  *result = base::StringPiece16(reinterpret_cast<const char16*>(read_from),
-                                len);
-  return true;
-}
-
 bool PickleIterator::ReadData(const char** data, int* length) {
   *length = 0;
   *data = 0;
@@ -296,7 +271,7 @@ Pickle& Pickle::operator=(const Pickle& other) {
   return *this;
 }
 
-bool Pickle::WriteString(const base::StringPiece& value) {
+bool Pickle::WriteString(const std::string& value) {
   if (!WriteInt(static_cast<int>(value.size())))
     return false;
 
@@ -311,7 +286,7 @@ bool Pickle::WriteWString(const std::wstring& value) {
                     static_cast<int>(value.size() * sizeof(wchar_t)));
 }
 
-bool Pickle::WriteString16(const base::StringPiece16& value) {
+bool Pickle::WriteString16(const string16& value) {
   if (!WriteInt(static_cast<int>(value.size())))
     return false;
 

base/pickle.h

@@ -13,7 +13,6 @@
 #include "base/gtest_prod_util.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
-#include "base/strings/string_piece.h"
 
 class Pickle;
 
@@ -40,12 +39,8 @@ class BASE_EXPORT PickleIterator {
   bool ReadFloat(float* result) WARN_UNUSED_RESULT;
   bool ReadDouble(double* result) WARN_UNUSED_RESULT;
   bool ReadString(std::string* result) WARN_UNUSED_RESULT;
-  // The StringPiece data will only be valid for the lifetime of the message.
-  bool ReadStringPiece(base::StringPiece* result) WARN_UNUSED_RESULT;
   bool ReadWString(std::wstring* result) WARN_UNUSED_RESULT;
   bool ReadString16(base::string16* result) WARN_UNUSED_RESULT;
-  // The StringPiece16 data will only be valid for the lifetime of the message.
-  bool ReadStringPiece16(base::StringPiece16* result) WARN_UNUSED_RESULT;
 
   // A pointer to the data will be placed in |*data|, and the length will be
   // placed in |*length|. The pointer placed into |*data| points into the
@@ -200,9 +195,9 @@ class BASE_EXPORT Pickle {
   bool WriteDouble(double value) {
     return WritePOD(value);
   }
-  bool WriteString(const base::StringPiece& value);
+  bool WriteString(const std::string& value);
   bool WriteWString(const std::wstring& value);
-  bool WriteString16(const base::StringPiece16& value);
+  bool WriteString16(const base::string16& value);
   // "Data" is a blob with a length. When you read it out you will be given the
   // length. See also WriteBytes.
   bool WriteData(const char* data, int length);

base/pickle_unittest.cc

@@ -30,13 +30,10 @@ const double testdouble = 2.71828182845904523;
 const std::string teststring("Hello world");  // note non-aligned string length
 const std::wstring testwstring(L"Hello, world");
 const base::string16 teststring16(base::ASCIIToUTF16("Hello, world"));
-const char testrawstring[] = "Hello new world"; // Test raw string writing
-// Test raw char16 writing, assumes UTF16 encoding is ANSI for alpha chars.
-const base::char16 testrawstring16[] = {'A', 'l', 'o', 'h', 'a', 0};
 const char testdata[] = "AAA\0BBB\0";
 const int testdatalen = arraysize(testdata) - 1;
 
-// checks that the results can be read correctly from the Pickle
+// checks that the result
 void VerifyResult(const Pickle& pickle) {
   PickleIterator iter(pickle);
 
@@ -94,14 +91,6 @@ void VerifyResult(const Pickle& pickle) {
   EXPECT_TRUE(iter.ReadString16(&outstring16));
   EXPECT_EQ(teststring16, outstring16);
 
-  base::StringPiece outstringpiece;
-  EXPECT_TRUE(iter.ReadStringPiece(&outstringpiece));
-  EXPECT_EQ(testrawstring, outstringpiece);
-
-  base::StringPiece16 outstringpiece16;
-  EXPECT_TRUE(iter.ReadStringPiece16(&outstringpiece16));
-  EXPECT_EQ(testrawstring16, outstringpiece16);
-
   const char* outdata;
   int outdatalen;
   EXPECT_TRUE(iter.ReadData(&outdata, &outdatalen));
@@ -132,8 +121,6 @@ TEST(PickleTest, EncodeDecode) {
   EXPECT_TRUE(pickle.WriteString(teststring));
   EXPECT_TRUE(pickle.WriteWString(testwstring));
   EXPECT_TRUE(pickle.WriteString16(teststring16));
-  EXPECT_TRUE(pickle.WriteString(testrawstring));
-  EXPECT_TRUE(pickle.WriteString16(testrawstring16));
   EXPECT_TRUE(pickle.WriteData(testdata, testdatalen));
   VerifyResult(pickle);
 

ipc/ipc_perftest_support.cc

@@ -103,8 +103,8 @@ class ChannelReflectorListener : public Listener {
     EXPECT_TRUE(iter.ReadInt64(&time_internal));
     int msgid;
     EXPECT_TRUE(iter.ReadInt(&msgid));
-    base::StringPiece payload;
-    EXPECT_TRUE(iter.ReadStringPiece(&payload));
+    std::string payload;
+    EXPECT_TRUE(iter.ReadString(&payload));
 
     // Include message deserialization in latency.
     base::TimeTicks now = base::TimeTicks::Now();