[Mac] Add a perf test that counts the number of Mach ports in the browser.

[Mac] Add a perf test that counts the number of Mach ports in the browser.

BUG=105513
TEST=This is one.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119541

[Robert Sesek, 2012-01-23 21:02:45]

mark: Overall review
cmp: Perf FYI and please help me figure out how to set up graphs and
expectations.

[Mark Mentovai, 2012-01-23 21:12:29]

LG otherwise

https://chromiumcodereview.appspot.com/9193024/diff/1/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/base/mach_ipc_mac.mm#ne...
base/mach_ipc_mac.mm:346: kr = vm_deallocate(task_port,
346 and 349 should use mach_task_self.

[cmp, 2012-01-23 21:20:51]

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
File chrome/common/automation_messages_internal.h (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
chrome/common/automation_messages_internal.h:1512: int /* number of Mach ports
*/)
iirc new messages should be added at the end of this file, otherwise testing
against the reference build will break.  don't break that unless absolutely
necessary. :)  (see line 12)

you should add running your test using the current reference build as one of the
verifications for your CL so we know what to expect.

your change will most likely eventually require updating the reference build.  i
would treat that as a separate process and so design this change as being able
to land without also needing an immediate reference build update.

it would be even better if you can find a way to not need a reference build
change at all.  or if you're adding a new type of data, thinking ahead to how
other kinds of this data may be added in the future without requiring a
reference build change.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
File chrome/test/perf/mach_ports_test.cc (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:29:
FilePath(FILE_PATH_LITERAL("data/page_cycler/moz"))) {
Please create a new data set that's named to match your test.  Feel free to base
it on data/page_cycler/moz.  Please keep it in data/mach_ports/moz, too, in the
internal repo.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:32: virtual void SetUp() OVERRIDE {
I believe SetUp and TearDown can both be defined as void NAME as opposed to
virtual void NAME OVERRIDE.  I would follow our current conventions for this
(see chrome/test/perf/*_test.cc, eg) where possible.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:102: }
Can the values returned by this test vary across test runs in copies of Chrome
from the same source tree at the same revision?  If so, do they vary much under
system load?

Most of our tests define a RunTest method which allows us to pass a different
class name into the fixture.  We use this to allow creating a fixture that uses
the reference build.

If you need a reference build version of this test, then please move the
majority of 69-102 into a RunTest method in MachPortsTest, then call the RunTest
method from this fixture.  Let me know if you have any questions about that.

[cmp, 2012-01-23 21:23:01]

By adding to performance_ui_tests and using PrintResultList, the output format
will match what the Buildbot expects, so you'll be mostly set from this side.

After this lands, there will need to be some Buildbot changes to run the test on
the chromium.perf waterfall.  Once those changes are made, the perf dashboard
will be mostly receiving updates.

After the dashboard is working, then we'll set up the expectations.  Let me know
what kind of expectations you're measuring against.  That is to ask: is this a
stable test like sizes that we can measure absolutely, or is it a test that
requires a reference build that we'll measure a diff against.

[Robert Sesek, 2012-01-23 21:58:22]

On 2012/01/23 21:23:01, cmp wrote:
> By adding to performance_ui_tests and using PrintResultList, the output format
> will match what the Buildbot expects, so you'll be mostly set from this side.
> 
> After this lands, there will need to be some Buildbot changes to run the test
on
> the chromium.perf waterfall.  Once those changes are made, the perf dashboard
> will be mostly receiving updates.

Cool!

> After the dashboard is working, then we'll set up the expectations.  Let me
know
> what kind of expectations you're measuring against.  That is to ask: is this a
> stable test like sizes that we can measure absolutely, or is it a test that
> requires a reference build that we'll measure a diff against.

This is pretty similar to sizes in that it's measuring something absolute. The
mean may have some variance, but there's definitely a maximum threshold which it
will absolutely not pass unless a regression is introduced.

[Robert Sesek, 2012-01-23 21:58:40]

https://chromiumcodereview.appspot.com/9193024/diff/1/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/base/mach_ipc_mac.mm#ne...
base/mach_ipc_mac.mm:346: kr = vm_deallocate(task_port,
On 2012/01/23 21:12:30, Mark Mentovai wrote:
> 346 and 349 should use mach_task_self.

Done.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
File chrome/common/automation_messages_internal.h (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
chrome/common/automation_messages_internal.h:1512: int /* number of Mach ports
*/)
On 2012/01/23 21:20:51, cmp wrote:
> iirc new messages should be added at the end of this file, otherwise testing
> against the reference build will break.  don't break that unless absolutely
> necessary. :)  (see line 12)

I *think* this is the right place, based on line 1538-41.

> you should add running your test using the current reference build as one of
the
> verifications for your CL so we know what to expect.

OK. The reference build will likely contain a bug for which this test is
protecting from regressions. Its numbers will be higher than what's reported by
trunk.

> your change will most likely eventually require updating the reference build. 
i
> would treat that as a separate process and so design this change as being able
> to land without also needing an immediate reference build update.
> 
> it would be even better if you can find a way to not need a reference build
> change at all.  or if you're adding a new type of data, thinking ahead to how
> other kinds of this data may be added in the future without requiring a
> reference build change.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
File chrome/test/perf/mach_ports_test.cc (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:29:
FilePath(FILE_PATH_LITERAL("data/page_cycler/moz"))) {
On 2012/01/23 21:20:51, cmp wrote:
> Please create a new data set that's named to match your test.  Feel free to
base
> it on data/page_cycler/moz.  Please keep it in data/mach_ports/moz, too, in
the
> internal repo.

Really? Why do I need a separate data set if this has exactly what I want (some
decently complex sample webpages)?

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:32: virtual void SetUp() OVERRIDE {
On 2012/01/23 21:20:51, cmp wrote:
> I believe SetUp and TearDown can both be defined as void NAME as opposed to
> virtual void NAME OVERRIDE.  I would follow our current conventions for this
> (see chrome/test/perf/*_test.cc, eg) where possible.

In UITest, they're defined as OVERRIDE.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:102: }
On 2012/01/23 21:20:51, cmp wrote:
> Can the values returned by this test vary across test runs in copies of Chrome
> from the same source tree at the same revision?  If so, do they vary much
under
> system load?
> 
> Most of our tests define a RunTest method which allows us to pass a different
> class name into the fixture.  We use this to allow creating a fixture that
uses
> the reference build.
> 
> If you need a reference build version of this test, then please move the
> majority of 69-102 into a RunTest method in MachPortsTest, then call the
RunTest
> method from this fixture.  Let me know if you have any questions about that.

The values will vary slightly, but the mean (what I will end up measuring) is
pretty consistent in my own testing. There will be differences in the numbers
(of statistical significance) across OS versions, though.

[cmp, 2012-01-23 22:20:56]

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
File chrome/common/automation_messages_internal.h (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/common/automatio...
chrome/common/automation_messages_internal.h:1512: int /* number of Mach ports
*/)
On 2012/01/23 21:58:41, rsesek wrote:
> On 2012/01/23 21:20:51, cmp wrote:
> > iirc new messages should be added at the end of this file, otherwise testing
> > against the reference build will break.  don't break that unless absolutely
> > necessary. :)  (see line 12)
> 
> I *think* this is the right place, based on line 1538-41.

Okay, sgtm.

> > you should add running your test using the current reference build as one of
> the
> > verifications for your CL so we know what to expect.
> 
> OK. The reference build will likely contain a bug for which this test is
> protecting from regressions. Its numbers will be higher than what's reported
by
> trunk.

Okay, since you won't need to run this test in the reference build (as you say
later it's more like sizes) let's only make sure that, say, the PageCyclerTest
will still work in the reference build after your change.  Other tests may have
broken but it's too hard to tell right now which ones, so just focus on running
PageCyclerTest.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
File chrome/test/perf/mach_ports_test.cc (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:29:
FilePath(FILE_PATH_LITERAL("data/page_cycler/moz"))) {
On 2012/01/23 21:58:41, rsesek wrote:
> Really? Why do I need a separate data set if
> this has exactly what I want (some decently
> complex sample webpages)?

Sure, it would technically be a separate data set, but really it's just a fresh
copy of the moz data set.  My concern is that your test can be impacted by
changes to the moz data set that are related only to page cycler changes, and
that those changes will be unexpected by the person making them.  By copying the
data to a new location just for this test, this test will own that data and
there won't be any concern that some unrelated change breaks it.

Have you tried other data sets, like morejs?  morejs is even more complex than
moz, and that may be desirable.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:32: virtual void SetUp() OVERRIDE {
On 2012/01/23 21:58:41, rsesek wrote:
> On 2012/01/23 21:20:51, cmp wrote:
> > I believe SetUp and TearDown can both be defined as void NAME as opposed to
> > virtual void NAME OVERRIDE.  I would follow our current conventions for this
> > (see chrome/test/perf/*_test.cc, eg) where possible.
> 
> In UITest, they're defined as OVERRIDE.

cmp@terakrom:/work/chrome/trunk/src/chrome/test/perf$ git gs OVERRIDE
browser_perf_test.h:20:  virtual void SetUpCommandLine(CommandLine*
command_line) OVERRIDE;
rendering/latency_tests.cc:121:  virtual void SetUpCommandLine(CommandLine*
command_line) OVERRIDE;
rendering/throughput_tests.cc:40:  virtual void SetUpCommandLine(CommandLine*
command_line) OVERRIDE {

I am uncertain why all of these tests were written without that modifier, and
some docs confirm that the "overridden base method must have the same signature
as the virtual method."  I suspect g++ (for example) will just ignore the lack
of the virtual+OVERRIDE modifiers, but I'm guessing.  Mark, do you understand if
the tests in chrome/test/perf/ are doing something wrong as-written?

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:102: }
On 2012/01/23 21:58:41, rsesek wrote:
> The values will vary slightly, but the mean (what I will end up measuring) is
> pretty consistent in my own testing. There will be differences in the numbers
> (of statistical significance) across OS versions, though.

Based on this, I will assume you won't need any reference builds running your
test.  That would be great!

[Robert Sesek, 2012-01-27 19:10:41]

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
File chrome/test/perf/mach_ports_test.cc (right):

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:29:
FilePath(FILE_PATH_LITERAL("data/page_cycler/moz"))) {
On 2012/01/23 22:20:56, cmp wrote:
> On 2012/01/23 21:58:41, rsesek wrote:
> > Really? Why do I need a separate data set if
> > this has exactly what I want (some decently
> > complex sample webpages)?
> 
> Sure, it would technically be a separate data set, but really it's just a
fresh
> copy of the moz data set.  My concern is that your test can be impacted by
> changes to the moz data set that are related only to page cycler changes, and
> that those changes will be unexpected by the person making them.  By copying
the
> data to a new location just for this test, this test will own that data and
> there won't be any concern that some unrelated change breaks it.
> 
> Have you tried other data sets, like morejs?  morejs is even more complex than
> moz, and that may be desirable.

Added a new data set per internal CL.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:32: virtual void SetUp() OVERRIDE {
On 2012/01/23 22:20:56, cmp wrote:
> On 2012/01/23 21:58:41, rsesek wrote:
> > On 2012/01/23 21:20:51, cmp wrote:
> > > I believe SetUp and TearDown can both be defined as void NAME as opposed
to
> > > virtual void NAME OVERRIDE.  I would follow our current conventions for
this
> > > (see chrome/test/perf/*_test.cc, eg) where possible.
> > 
> > In UITest, they're defined as OVERRIDE.
> 
> cmp@terakrom:/work/chrome/trunk/src/chrome/test/perf$ git gs OVERRIDE
> browser_perf_test.h:20:  virtual void SetUpCommandLine(CommandLine*
> command_line) OVERRIDE;
> rendering/latency_tests.cc:121:  virtual void SetUpCommandLine(CommandLine*
> command_line) OVERRIDE;
> rendering/throughput_tests.cc:40:  virtual void SetUpCommandLine(CommandLine*
> command_line) OVERRIDE {
> 
> I am uncertain why all of these tests were written without that modifier, and
> some docs confirm that the "overridden base method must have the same
signature
> as the virtual method."  I suspect g++ (for example) will just ignore the lack
> of the virtual+OVERRIDE modifiers, but I'm guessing.  Mark, do you understand
if
> the tests in chrome/test/perf/ are doing something wrong as-written?

The modifier is rather new, so old code may not have it. New code should,
though.

https://chromiumcodereview.appspot.com/9193024/diff/1/chrome/test/perf/mach_p...
chrome/test/perf/mach_ports_test.cc:102: }
On 2012/01/23 22:20:56, cmp wrote:
> On 2012/01/23 21:58:41, rsesek wrote:
> > The values will vary slightly, but the mean (what I will end up measuring)
is
> > pretty consistent in my own testing. There will be differences in the
numbers
> > (of statistical significance) across OS versions, though.
> 
> Based on this, I will assume you won't need any reference builds running your
> test.  That would be great!

I think that's correct, yes.

[cmp, 2012-01-27 20:10:37]

lgtm

[Mark Mentovai, 2012-01-27 20:16:27]

lgTM

[commit-bot: I haz the power, 2012-01-27 20:19:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9193024/9001

[commit-bot: I haz the power, 2012-01-27 20:44:38]

Try job failure for 9193024-9001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-27 20:49:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/9193024/8004

[commit-bot: I haz the power, 2012-01-28 00:51:46]

Change committed as 119541

[Robert Sesek, 2012-01-31 20:00:19]

Chase: what's the last step I need to do to get this to appear on the
waterfall/graphs?

[cmp, 2012-01-31 20:44:06]

First grab the right checkout with 'svn co
svn://svn.chromium.org/chrome/trunk/tools/build/'.

Edit the config in masters/master.chromium.perf/master.cfg to run a new test on
the right bots and send me the CL.

Edit the files in scripts/master/factory/chromium* to know your test name and
what command the bots should run to invoke them.  Model this on AddStartupTests,
for example.  Send me the CL.