Add ipc_mojo_perftests

Add ipc_mojo_perftests

This change adds ipc_mojo_perftests that runs the same benchmark
as of ipc_perftests. Now head-to-head comparison becomes possible.

For this change, whole ipc_perftests logic is extracted to
ipc_perftest_support.cc to make it reusable by ipc_mojo_perftests.

TEST=none
BUG=none
R=jam@chromium.org, darin@chromium.org, yuzhu@chromium.org

Committed: https://crrev.com/373af03b7c0e1932c7ea3b832918b7767d47ff89
Cr-Commit-Position: refs/heads/master@{#293988}

[Hajime Morrita, 2014-09-03 22:41:55]

This needs yuzhu's fix https://codereview.chromium.org/538593002/ before
landing.

[yzshen1, 2014-09-03 22:49:28]

On 2014/09/03 22:41:55, morrita wrote:
> This needs yuzhu's fix https://codereview.chromium.org/538593002/ before
> landing.

Have you considered using value parameterized tests? In that way, we can run
tests using different configs easily.
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

(I just learnt it from sky. :))

[Hajime Morrita, 2014-09-04 21:10:46]

Thanks for the review! PTAL?

On 2014/09/03 22:49:28, yzshen1 wrote:
> On 2014/09/03 22:41:55, morrita wrote:
> > This needs yuzhu's fix https://codereview.chromium.org/538593002/ before
> > landing.
> 
> Have you considered using value parameterized tests? In that way, we can run
> tests using different configs easily.
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...
> 
> (I just learnt it from sky. :))

Done.
It also gets rid of copy-n-pasted parameters.

[yzshen1, 2014-09-04 23:44:08]

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

[yzshen1, 2014-09-04 23:44:09]

https://codereview.chromium.org/536213002/diff/40001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/BUILD.gn#newcode164
ipc/BUILD.gn:164: "ipc_perftest_support.h",
nit: please order alphabetically.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc.gyp
File ipc/ipc.gyp (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc.gyp#newcode133
ipc/ipc.gyp:133: 'ipc_perftest_support.h',
nit: please order alphabetically.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support.cc
File ipc/ipc_perftest_support.cc (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support...
ipc/ipc_perftest_support.cc:330: //ChannelReflectorListener listener;
Please remove these two lines.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support.h
File ipc/ipc_perftest_support.h (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support...
ipc/ipc_perftest_support.h:32: public:
nit: wrong indent.

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

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:79: CHECK(!channel_.get());
It is not obvious to me that this CHECK is necessary.
Because this method doesn't set channel_.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_test_base.h
File ipc/ipc_test_base.h (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_test_base.h#newc...
ipc/ipc_test_base.h:55: // Use this instead of CreateChannel() if you want to
use some different
The seems a little confusing: this method no longer sets channel_ as
CreateChannel() does. Besides, this is a virtual method that is intended to be
overridden by subclasses.

I think it broke IPCChannelTest.ChannelTestExistingPipe, too.

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
File ipc/mojo/ipc_mojo_perftest.cc (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
ipc/mojo/ipc_mojo_perftest.cc:24: 
One empty line is sufficient.

[yzshen1, 2014-09-05 18:20:13]

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
File ipc/mojo/ipc_mojo_perftest.cc (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
ipc/mojo/ipc_mojo_perftest.cc:81:
RunTestChannelProxyPingPong(GetDefaultTestParams());
I think this calls into IPCTestBase::CreateChannelProxy(), which doesn't call
CreateChannelFromChannelHandle() you defined above. The outcome is that
MojoChannelPerfTest.ChannelProxyPingPong hangs.

[Hajime Morrita, 2014-09-08 21:13:26]

On 2014/09/05 18:20:13, yzshen1 wrote:
>
https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
> File ipc/mojo/ipc_mojo_perftest.cc (right):
> 
>
https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
> ipc/mojo/ipc_mojo_perftest.cc:81:
> RunTestChannelProxyPingPong(GetDefaultTestParams());
> I think this calls into IPCTestBase::CreateChannelProxy(), which doesn't call
> CreateChannelFromChannelHandle() you defined above. The outcome is that
> MojoChannelPerfTest.ChannelProxyPingPong hangs.

Right. I changed it to make it work.

[Hajime Morrita, 2014-09-08 22:28:58]

Jam, Darin: PTAL? This has IPCTestBase refactoring and add another benchmark
based on it.
I need the refactoring part for my Windows work.

[jam, 2014-09-09 00:27:24]

lgtm

[Hajime Morrita, 2014-09-09 00:40:27]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:43:07]

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

[Hajime Morrita, 2014-09-09 00:51:58]

The CQ bit was unchecked by morrita@chromium.org

[Hajime Morrita, 2014-09-09 01:03:18]

Yuzhu: PTAL?

After looking at all your comment, I found that It'd be nice to have another
review from you, as there are some non-style changes I made. 

Thanks!

https://codereview.chromium.org/536213002/diff/40001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/BUILD.gn#newcode164
ipc/BUILD.gn:164: "ipc_perftest_support.h",
On 2014/09/04 23:44:08, yzshen1 wrote:
> nit: please order alphabetically.

Done.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc.gyp
File ipc/ipc.gyp (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc.gyp#newcode133
ipc/ipc.gyp:133: 'ipc_perftest_support.h',
On 2014/09/04 23:44:08, yzshen1 wrote:
> nit: please order alphabetically.

Done.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support.cc
File ipc/ipc_perftest_support.cc (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support...
ipc/ipc_perftest_support.cc:330: //ChannelReflectorListener listener;
On 2014/09/04 23:44:09, yzshen1 wrote:
> Please remove these two lines.

Done.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support.h
File ipc/ipc_perftest_support.h (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_perftest_support...
ipc/ipc_perftest_support.h:32: public:
On 2014/09/04 23:44:09, yzshen1 wrote:
> nit: wrong indent.

Done.

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_test_base.h
File ipc/ipc_test_base.h (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/ipc_test_base.h#newc...
ipc/ipc_test_base.h:55: // Use this instead of CreateChannel() if you want to
use some different
On 2014/09/04 23:44:09, yzshen1 wrote:
> The seems a little confusing: this method no longer sets channel_ as
> CreateChannel() does. Besides, this is a virtual method that is intended to be
> overridden by subclasses.
> 
> I think it broke IPCChannelTest.ChannelTestExistingPipe, too.
> 
Now it's gone.

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
File ipc/mojo/ipc_mojo_perftest.cc (right):

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
ipc/mojo/ipc_mojo_perftest.cc:24: 
On 2014/09/04 23:44:09, yzshen1 wrote:
> One empty line is sufficient.

Done.

https://codereview.chromium.org/536213002/diff/40001/ipc/mojo/ipc_mojo_perfte...
ipc/mojo/ipc_mojo_perftest.cc:81:
RunTestChannelProxyPingPong(GetDefaultTestParams());
On 2014/09/05 18:20:13, yzshen1 wrote:
> I think this calls into IPCTestBase::CreateChannelProxy(), which doesn't call
> CreateChannelFromChannelHandle() you defined above. The outcome is that
> MojoChannelPerfTest.ChannelProxyPingPong hangs.

Done.

[yzshen1, 2014-09-09 07:45:29]

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_unittest.cc (right):

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_unittest.cc:58: class ListenerThatShouldBeNeverCalled
: public IPC::Listener {
Is this class still needed?

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_unittest.cc:100: IPCTestBase::SetUp();
It seems you don't need this override, right?

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
File ipc/mojo/ipc_mojo_perftest.cc (right):

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
ipc/mojo/ipc_mojo_perftest.cc:27: class NullListener : public IPC::Listener {
Is it still needed?

[Hajime Morrita, 2014-09-09 16:56:09]

PTAL?

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_unittest.cc (right):

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_unittest.cc:58: class ListenerThatShouldBeNeverCalled
: public IPC::Listener {
On 2014/09/09 07:45:28, yzshen1 wrote:
> Is this class still needed?
No. Deleted.

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_unittest.cc:100: IPCTestBase::SetUp();
On 2014/09/09 07:45:28, yzshen1 wrote:
> It seems you don't need this override, right?
Right. Removed.

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
File ipc/mojo/ipc_mojo_perftest.cc (right):

https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
ipc/mojo/ipc_mojo_perftest.cc:27: class NullListener : public IPC::Listener {
On 2014/09/09 07:45:28, yzshen1 wrote:
> Is it still needed?
No! Thanks for the catch.

[yzshen1, 2014-09-09 16:57:08]

On 2014/09/09 16:56:09, morrita wrote:
> PTAL?
> 
>
https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
> File ipc/mojo/ipc_channel_mojo_unittest.cc (right):
> 
>
https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
> ipc/mojo/ipc_channel_mojo_unittest.cc:58: class
ListenerThatShouldBeNeverCalled
> : public IPC::Listener {
> On 2014/09/09 07:45:28, yzshen1 wrote:
> > Is this class still needed?
> No. Deleted.
> 
>
https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_channel_mo...
> ipc/mojo/ipc_channel_mojo_unittest.cc:100: IPCTestBase::SetUp();
> On 2014/09/09 07:45:28, yzshen1 wrote:
> > It seems you don't need this override, right?
> Right. Removed.
> 
>
https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
> File ipc/mojo/ipc_mojo_perftest.cc (right):
> 
>
https://codereview.chromium.org/536213002/diff/120001/ipc/mojo/ipc_mojo_perft...
> ipc/mojo/ipc_mojo_perftest.cc:27: class NullListener : public IPC::Listener {
> On 2014/09/09 07:45:28, yzshen1 wrote:
> > Is it still needed?
> No! Thanks for the catch.

LGTM Thanks!

[Hajime Morrita, 2014-09-09 17:04:16]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-09 17:13:29]

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

[commit-bot: I haz the power, 2014-09-09 19:36:33]

Committed patchset #8 (id:140001) as dadfdeace9c74a6f1cacc8bd43033388dfd6050c

[commit-bot: I haz the power, 2014-09-10 03:54:59]

Patchset 8 (id:??) landed as
https://crrev.com/373af03b7c0e1932c7ea3b832918b7767d47ff89
Cr-Commit-Position: refs/heads/master@{#293988}