Add ChannelProxy benchmark to ipc_perftests.

Add ChannelProxy benchmark to ipc_perftests.

This CL add a ChannelProxy equivalent of the Channel benchmark
to ipc_perftests.

To use it in the benchmark, TestIOThread is extracted from
mojo/system/ to base/test

TEST=IPChannelPerfTest.ChannelProxyPingPong
R=viettrungluu@chromium.org,brettw@chromium.org,darin@chromium.org
BUG=none

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

[viettrungluu, 2014-08-27 16:47:18]

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread.cc
File base/test/test_io_thread.cc (right):

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
nit: "Copyright 2014" (note: no "(c)").

(You might argue for 2013, if this is just a file move.)

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread.h
File base/test/test_io_thread.h (right):

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.h:18: class TestIOThread {
Could use a class-level comment.

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.h:31: void PostTask(const tracked_objects::Location&
from_here,
Probably these should have comments, especially |PostTaskAndWait()| (the comment
to which should probably have some warning about potential deadlocking).

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_perftests.cc
File ipc/ipc_perftests.cc (right):

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_perftests.cc#new...
ipc/ipc_perftests.cc:35: return make_scoped_ptr(new base::MessageLoop());
nit: Same nit as below.

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_perftests.cc#new...
ipc/ipc_perftests.cc:290: base::TestIOThread
io_thread((base::TestIOThread::kAutoStart));
nit: I don't think there's any need for the extra parentheses (no vexing parse
here).

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:47: return make_scoped_ptr<base::MessageLoop>(new
base::MessageLoopForIO());
Nit: If you have to give the template parameter, there's no point in using
make_scoped_ptr, i.e., you may as well just write
scoped_ptr<base::MessageLoop>(...).

[viettrungluu, 2014-08-27 16:52:12]

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread.h
File base/test/test_io_thread.h (right):

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.h:1: // Copyright (c) 2014 The Chromium Authors. All
rights reserved.
nit: Also, no "(c)".

[Hajime Morrita, 2014-08-27 17:47:43]

Thanks for the review! PTAL?

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread.cc
File base/test/test_io_thread.cc (right):

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2014/08/27 16:47:18, viettrungluu wrote:
> nit: "Copyright 2014" (note: no "(c)").
> 
> (You might argue for 2013, if this is just a file move.)

Done.

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread.h
File base/test/test_io_thread.h (right):

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.h:18: class TestIOThread {
On 2014/08/27 16:47:18, viettrungluu wrote:
> Could use a class-level comment.

Done.

https://codereview.chromium.org/488003003/diff/40001/base/test/test_io_thread...
base/test/test_io_thread.h:31: void PostTask(const tracked_objects::Location&
from_here,
On 2014/08/27 16:47:18, viettrungluu wrote:
> Probably these should have comments, especially |PostTaskAndWait()| (the
comment
> to which should probably have some warning about potential deadlocking).

Done.

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_perftests.cc
File ipc/ipc_perftests.cc (right):

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_perftests.cc#new...
ipc/ipc_perftests.cc:290: base::TestIOThread
io_thread((base::TestIOThread::kAutoStart));
On 2014/08/27 16:47:18, viettrungluu wrote:
> nit: I don't think there's any need for the extra parentheses (no vexing parse
> here).

Done.

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/488003003/diff/40001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:47: return make_scoped_ptr<base::MessageLoop>(new
base::MessageLoopForIO());
On 2014/08/27 16:47:18, viettrungluu wrote:
> Nit: If you have to give the template parameter, there's no point in using
> make_scoped_ptr, i.e., you may as well just write
> scoped_ptr<base::MessageLoop>(...).

Done.

[viettrungluu, 2014-08-28 00:07:31]

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread.h
File base/test/test_io_thread.h (right):

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread...
base/test/test_io_thread.h:20: // It also provides some ideaomatic API like
PostTaskAndWait().
spelling: ideaomatic -> ideomatic

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread...
base/test/test_io_thread.h:37: // Posts |task| to the IO-thread, spins the
thread's message loop until
This description is inaccurate. It actually waits on a WaitableEvent.

(IIRC, it's done this way since the main/creation thread may not have a message
loop, especially in tests.)

[Hajime Morrita, 2014-08-28 00:24:35]

PTAL?

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread.h
File base/test/test_io_thread.h (right):

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread...
base/test/test_io_thread.h:20: // It also provides some ideaomatic API like
PostTaskAndWait().
On 2014/08/28 00:07:31, viettrungluu wrote:
> spelling: ideaomatic -> ideomatic

Done.

https://codereview.chromium.org/488003003/diff/60001/base/test/test_io_thread...
base/test/test_io_thread.h:37: // Posts |task| to the IO-thread, spins the
thread's message loop until
On 2014/08/28 00:07:31, viettrungluu wrote:
> This description is inaccurate. It actually waits on a WaitableEvent.
> 
> (IIRC, it's done this way since the main/creation thread may not have a
message
> loop, especially in tests.)

Ugh right. fixed.

[Hajime Morrita, 2014-08-28 17:26:35]

Trung: Ping?
Brett, Darin: Could you rubberstamp for base/test? The TestIOThread class is
moved from mojo/common/ to get used from ipc_perftests.

[viettrungluu, 2014-08-28 19:15:44]

lgtm

[brettw, 2014-08-28 19:47:01]

I don't really understand the new setter in ipc_test_base. It looks like that
test creates a message loop in the Setup method, and then you allow tests to set
a new one which deletes the old one? This seems weird, or I'm misunderstanding
something.

[Hajime Morrita, 2014-08-28 20:18:42]

On 2014/08/28 19:47:01, brettw wrote:
> I don't really understand the new setter in ipc_test_base. It looks like that
> test creates a message loop in the Setup method, and then you allow tests to
set
> a new one which deletes the old one? This seems weird, or I'm misunderstanding
> something.

By default, it creates MessageLoopForIO. A new test wanted to create non-IO
message loop for the main thread
as it creates IO message loop in a different thread. To avoid CHECK() failure on
the main thread. 
I reset the IO message loop in the main thread with non-IO message loop.

We could add an virtual method to make the message loop creation polymorphic.
I thought it's bit a overkill though.

[brettw, 2014-08-28 21:17:31]

LGTM.

Can you put a detailed comment on the setter about what this does (i.e. it
overwrites the existing one), and mention that it shouldn't be called after you
start doing anything with the thread (I assume it will get messed up then).

[Hajime Morrita, 2014-08-28 21:34:05]

On 2014/08/28 21:17:31, brettw wrote:
> LGTM.
> 
> Can you put a detailed comment on the setter about what this does (i.e. it
> overwrites the existing one), and mention that it shouldn't be called after
you
> start doing anything with the thread (I assume it will get messed up then).
Sure! Thanks for the review.

[Hajime Morrita, 2014-08-28 21:39:02]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-28 21:40:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/488003003/100001

[jam, 2014-08-28 21:49:51]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2014-08-28 21:49:52]

lgtm

[commit-bot: I haz the power, 2014-08-28 23:22:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53157)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 00:21:44]

Committed patchset #6 (id:100001) as 93cd24e4350ece15ce08d4015d5cd1b62a5333f7

[commit-bot: I haz the power, 2014-09-10 03:04:14]

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