Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(22)

Issue 927183002: Adding StringPiece read/write support to pickle. (Closed)

Created:
5 years, 10 months ago by brucedawson
Modified:
5 years, 9 months ago
CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, Hajime Morrita
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add ReadStringPiece16, change WriteString16 to use StringPiece, and add unit tests. #

Patch Set 3 : Fixed null termination. 0 != '0'. D'oh! #

Patch Set 4 : Fix indentation to make it consistent. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -7 lines) Patch
M base/pickle.h View 1 3 chunks +7 lines, -2 lines 0 comments Download
M base/pickle.cc View 1 2 3 4 chunks +27 lines, -2 lines 0 comments Download
M base/pickle_unittest.cc View 1 2 3 chunks +14 lines, -1 line 0 comments Download
M ipc/ipc_perftest_support.cc View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 33 (11 generated)
brucedawson
Does this seem worthwhile? Enabling fewer allocations seems like a good direction to go in.
5 years, 10 months ago (2015-02-17 21:53:38 UTC) #2
Lei Zhang
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) ...
5 years, 10 months ago (2015-02-17 22:37:56 UTC) #3
Lei Zhang
On 2015/02/17 21:53:38, brucedawson wrote: > Does this seem worthwhile? Enabling fewer allocations seems like ...
5 years, 10 months ago (2015-02-17 22:48:58 UTC) #4
brucedawson
PTAL All suggestions addressed, and the updated unit test is passing. Being able to write ...
5 years, 10 months ago (2015-02-18 21:41:59 UTC) #5
brucedawson
Adding morrita@ for approval/notification of ipc_perftest change.
5 years, 10 months ago (2015-02-18 21:43:17 UTC) #7
Lei Zhang
lgtm
5 years, 10 months ago (2015-02-18 22:35:54 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/927183002/20001
5 years, 10 months ago (2015-02-18 23:04:34 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/43659)
5 years, 10 months ago (2015-02-18 23:10:35 UTC) #12
brucedawson
Adding cpu@ for required approval of change to ipc test. Moving morrita@ to CC.
5 years, 10 months ago (2015-02-19 01:12:19 UTC) #14
cpu_(ooo_6.6-7.5)
lgtm
5 years, 10 months ago (2015-02-19 03:51:40 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/927183002/20001
5 years, 10 months ago (2015-02-19 17:18:19 UTC) #17
commit-bot: I haz the power
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_asan_rel/builds/37147)
5 years, 10 months ago (2015-02-19 18:26:42 UTC) #19
brucedawson
On 2015/02/19 18:26:42, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
5 years, 10 months ago (2015-02-19 22:18:56 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/927183002/20001
5 years, 10 months ago (2015-02-19 22:20:53 UTC) #22
commit-bot: I haz the power
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_asan_rel/builds/37316)
5 years, 10 months ago (2015-02-19 23:12:12 UTC) #24
brucedawson
This is still pending due to intermittent link errors caused by an apparent pnacl dependency ...
5 years, 10 months ago (2015-02-24 19:40:25 UTC) #26
Lei Zhang
On 2015/02/24 19:40:25, brucedawson wrote: > This is still pending due to intermittent link errors ...
5 years, 9 months ago (2015-03-04 20:36:07 UTC) #27
brucedawson
> The bug got fixed today. Shall we land this? Yes. I ran a "git ...
5 years, 9 months ago (2015-03-04 21:13:46 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/927183002/60001
5 years, 9 months ago (2015-03-04 21:14:33 UTC) #30
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 9 months ago (2015-03-04 21:18:07 UTC) #31
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/fcfde7d98209569fba81de4f1b26d0e26cd95848 Cr-Commit-Position: refs/heads/master@{#319128}
5 years, 9 months ago (2015-03-04 21:21:33 UTC) #32
brucedawson
5 years, 9 months ago (2015-03-05 01:27:23 UTC) #33
Message was sent while issue was closed.
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..

Powered by Google App Engine
This is Rietveld 408576698