From d68c7ff8ce39423e03c5d2cbfe3aadd112afd82a Mon Sep 17 00:00:00 2001 From: brucedawson Date: Wed, 4 Mar 2015 17:54:50 -0800 Subject: 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 | 29 ++--------------------------- base/pickle.h | 9 ++------- base/pickle_unittest.cc | 15 +-------------- 3 files changed, 5 insertions(+), 48 deletions(-) (limited to 'base') diff --git a/base/pickle.cc b/base/pickle.cc index e66c5ba..d461f41 100644 --- a/base/pickle.cc +++ b/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(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(value.size()))) return false; @@ -311,7 +286,7 @@ bool Pickle::WriteWString(const std::wstring& value) { static_cast(value.size() * sizeof(wchar_t))); } -bool Pickle::WriteString16(const base::StringPiece16& value) { +bool Pickle::WriteString16(const string16& value) { if (!WriteInt(static_cast(value.size()))) return false; diff --git a/base/pickle.h b/base/pickle.h index b9b4126..f2a198e 100644 --- a/base/pickle.h +++ b/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); diff --git a/base/pickle_unittest.cc b/base/pickle_unittest.cc index 466d32a..1fa1f32 100644 --- a/base/pickle_unittest.cc +++ b/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); -- cgit v1.1