telemetry: Remove command line args from page test

telemetry: Remove command line args from page set

- Remove command line argument handling from page tests
  (except for extra browser args)
- Allow benchmarks to create customized page tests
- Adapt all measurements and benchmarks

R=tonyg@chromium.org, nednguyen@google.com, nduca@chromium.org, chrishenry@google.com, dtu@chromium.org
BUG=421276
Committed: https://crrev.com/7c0eae7f526aa9af80d5800da7daa50f41290375
Cr-Commit-Position: refs/heads/master@{#304887}

[ernstm, 2014-10-08 01:09:23]

ernstm@chromium.org changed reviewers:
+ nednguyen@google.com
- nednguyen@chromium.org

[ernstm, 2014-10-08 01:09:35]

PTAL

[nednguyen, 2014-10-08 01:44:54]

With this, can you also make sure that test_runner only match "directly
constructable" page tests and directly constructable page sets.

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
This has to be @classmethod so test_runner can process the command line options
defined by page_test before it instantiates  benchmark:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

I would change the @classmethod PageTestClass to @classmethod CreatePageTest().

[ernstm, 2014-10-09 17:16:17]

What do you mean with 'match "directly constructable" page tests'?

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
On 2014/10/08 01:44:54, nednguyen wrote:
> This has to be @classmethod so test_runner can process the command line
options
> defined by page_test before it instantiates  benchmark:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> I would change the @classmethod PageTestClass to @classmethod
CreatePageTest().

But then SetArgumentDefaults and ProcessCommandLineArgs would need to
instantiate the page test instead of just calling a classmethod on the class.

[nednguyen, 2014-10-09 17:37:20]

On 2014/10/09 17:16:17, ernstm wrote:
> What do you mean with 'match "directly constructable" page tests'?

Currently, you can run telemetry test by ./run_measurement <page_test_name>
<page_set_name>

To do this, test_runner.py first searches for classes that match page_test_name
& page_set_name.
Then with the assumption that all page_test & page_set instances can be
constructed directly from their classes, it creates page_test_class() and
page_set_class() instances for the test to run. Once we allow the page_test &
page_set to be parameterizable through constructor, you have to make sure that
the names of those 'not-directly-constructable' classes won't be in the search
space.

The logic of searching for test class can be found here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

> 
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> File tools/telemetry/telemetry/benchmark.py (right):
> 
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
> On 2014/10/08 01:44:54, nednguyen wrote:
> > This has to be @classmethod so test_runner can process the command line
> options
> > defined by page_test before it instantiates  benchmark:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > 
> > I would change the @classmethod PageTestClass to @classmethod
> CreatePageTest().
> 
> But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> instantiate the page test instead of just calling a classmethod on the class.

[ernstm, 2014-10-09 18:33:17]

On 2014/10/09 17:37:20, nednguyen wrote:
> On 2014/10/09 17:16:17, ernstm wrote:
> > What do you mean with 'match "directly constructable" page tests'?
> 
> Currently, you can run telemetry test by ./run_measurement <page_test_name>
> <page_set_name>
> 
> To do this, test_runner.py first searches for classes that match
page_test_name
> & page_set_name.
> Then with the assumption that all page_test & page_set instances can be
> constructed directly from their classes, it creates page_test_class() and
> page_set_class() instances for the test to run. Once we allow the page_test &
> page_set to be parameterizable through constructor, you have to make sure that
> the names of those 'not-directly-constructable' classes won't be in the search
> space.
> 
> The logic of searching for test class can be found here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> > 
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > File tools/telemetry/telemetry/benchmark.py (right):
> > 
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
> > On 2014/10/08 01:44:54, nednguyen wrote:
> > > This has to be @classmethod so test_runner can process the command line
> > options
> > > defined by page_test before it instantiates  benchmark:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > > 
> > > I would change the @classmethod PageTestClass to @classmethod
> > CreatePageTest().
> > 
> > But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> > instantiate the page test instead of just calling a classmethod on the
class.

What are we using run_measurement for? Can we switch to always use run_benchmark
instead?

[nednguyen, 2014-10-09 19:03:36]

On 2014/10/09 18:33:17, ernstm wrote:
> On 2014/10/09 17:37:20, nednguyen wrote:
> > On 2014/10/09 17:16:17, ernstm wrote:
> > > What do you mean with 'match "directly constructable" page tests'?
> > 
> > Currently, you can run telemetry test by ./run_measurement <page_test_name>
> > <page_set_name>
> > 
> > To do this, test_runner.py first searches for classes that match
> page_test_name
> > & page_set_name.
> > Then with the assumption that all page_test & page_set instances can be
> > constructed directly from their classes, it creates page_test_class() and
> > page_set_class() instances for the test to run. Once we allow the page_test
&
> > page_set to be parameterizable through constructor, you have to make sure
that
> > the names of those 'not-directly-constructable' classes won't be in the
search
> > space.
> > 
> > The logic of searching for test class can be found here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > > File tools/telemetry/telemetry/benchmark.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > > tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
> > > On 2014/10/08 01:44:54, nednguyen wrote:
> > > > This has to be @classmethod so test_runner can process the command line
> > > options
> > > > defined by page_test before it instantiates  benchmark:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > > > 
> > > > I would change the @classmethod PageTestClass to @classmethod
> > > CreatePageTest().
> > > 
> > > But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> > > instantiate the page test instead of just calling a classmethod on the
> class.
> 
> What are we using run_measurement for? Can we switch to always use
run_benchmark
> instead?

run_measurement is actually the same as run_benchmark actually. The core issue
here is we still need to support the mode 'run <page test> <page set>'. I am on
the side of deprecating this but it would require more consensus from other
owners.

[nednguyen, 2014-10-09 19:40:08]

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
On 2014/10/09 17:16:17, ernstm wrote:
> On 2014/10/08 01:44:54, nednguyen wrote:
> > This has to be @classmethod so test_runner can process the command line
> options
> > defined by page_test before it instantiates  benchmark:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > 
> > I would change the @classmethod PageTestClass to @classmethod
> CreatePageTest().
> 
> But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> instantiate the page test instead of just calling a classmethod on the class.

There are multiple ways to solve this:

One way is to change SetArgumentDefaults(cls, parser) to
SetArgumentDefaults(cls, parser, page_test_class) and move the instantiation of
page test to test_runner.

Another way is to cache @CreatePageTest() so page test instance that's used by
the benchmark is only created once. 

The hard way to do this is to deprecate the feature of letting page test set
define the command line options. Since page test is configurable through
constructor, people who want the command line option can define it in
benchmark.AddCommandLineArgs(). e.g:

def CreatePageTest(self, options):
   return repaint.Repaint(options.viewport, options.width, ...)

[ernstm, 2014-10-09 22:09:09]

> run_measurement is actually the same as run_benchmark actually. The core issue
> here is we still need to support the mode 'run <page test> <page set>'. I am
on
> the side of deprecating this but it would require more consensus from other
> owners.

Let's see if we can deprecate it. Tony, Nat, what are the reasons for keeping
run_measurement?

[ernstm, 2014-10-09 22:11:27]

> The hard way to do this is to deprecate the feature of letting page test set
> define the command line options. Since page test is configurable through
> constructor, people who want the command line option can define it in
> benchmark.AddCommandLineArgs(). e.g:
> 
> def CreatePageTest(self, options):
>    return repaint.Repaint(options.viewport, options.width, ...)

We want to empower benchmarks. Measurements should be created by benchmarks with
the parameters in the constructor, not by command line flags. Looks like there
is more to clean-up here than I initially thought. But let's do it right. Any
objections to removing command line flags entirely from Measurements?

[jbroman, 2014-10-09 22:21:33]

On 2014/10/09 22:11:27, ernstm wrote:
> > The hard way to do this is to deprecate the feature of letting page test set
> > define the command line options. Since page test is configurable through
> > constructor, people who want the command line option can define it in
> > benchmark.AddCommandLineArgs(). e.g:
> > 
> > def CreatePageTest(self, options):
> >    return repaint.Repaint(options.viewport, options.width, ...)
> 
> We want to empower benchmarks. Measurements should be created by benchmarks
with
> the parameters in the constructor, not by command line flags. Looks like there
> is more to clean-up here than I initially thought. But let's do it right. Any
> objections to removing command line flags entirely from Measurements?

I'm not a reviewer but...

Is the goal to remove access to measurements from the command line? Doesn't this
make it harder to use telemetry from the command line? I often am in a situation
where I want to measure, say, the record time on a key_mobile_sites page. If
that benchmark doesn't happen to exist, will I have to go and add it in Python
before I can do this?

[tonyg, 2014-10-09 22:29:35]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org

[tonyg, 2014-10-09 22:29:43]

[+dtu]

[dtu, 2014-10-10 01:45:19]

On 2014/10/09 22:21:33, jbroman wrote:
> On 2014/10/09 22:11:27, ernstm wrote:
> > > The hard way to do this is to deprecate the feature of letting page test
set
> > > define the command line options. Since page test is configurable through
> > > constructor, people who want the command line option can define it in
> > > benchmark.AddCommandLineArgs(). e.g:
> > > 
> > > def CreatePageTest(self, options):
> > >    return repaint.Repaint(options.viewport, options.width, ...)
> > 
> > We want to empower benchmarks. Measurements should be created by benchmarks
> with
> > the parameters in the constructor, not by command line flags. Looks like
there
> > is more to clean-up here than I initially thought. But let's do it right.
Any
> > objections to removing command line flags entirely from Measurements?

We can also make all the constructor arguments optional. Then the PageTests will
still be directly constructable. The current command-line arguments all have
defaults. (Except for Startup - you have to choose warm or cold startup.)
> 
> I'm not a reviewer but...
> 
> Is the goal to remove access to measurements from the command line? Doesn't
this
> make it harder to use telemetry from the command line? I often am in a
situation
> where I want to measure, say, the record time on a key_mobile_sites page. If
> that benchmark doesn't happen to exist, will I have to go and add it in Python
> before I can do this?

That's certainly not the main goal, but might happen as a side effect of
reducing the complexity of the codebase. In this case, I think you should add a
Python benchmark. If it's an important metric to you, we should track it on the
perf dashboard.

[dtu, 2014-10-10 01:45:28]

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
On 2014/10/09 19:40:08, nednguyen wrote:
> On 2014/10/09 17:16:17, ernstm wrote:
> > On 2014/10/08 01:44:54, nednguyen wrote:
> > > This has to be @classmethod so test_runner can process the command line
> > options
> > > defined by page_test before it instantiates  benchmark:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > > 
> > > I would change the @classmethod PageTestClass to @classmethod
> > CreatePageTest().
> > 
> > But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> > instantiate the page test instead of just calling a classmethod on the
class.
> 
> There are multiple ways to solve this:
> 
> One way is to change SetArgumentDefaults(cls, parser) to
> SetArgumentDefaults(cls, parser, page_test_class) and move the instantiation
of
> page test to test_runner.

To me, the separation between test_runner and benchmark is very clear -
test_runner solely deals with parsing the command-line. All test logic is in
Benchmark. Put another way, test_runner should be able to run multiple
Benchmarks just by adding a for loop. Moving the instantiation of PageTest to
test_runner breaks this separation.
> 
> Another way is to cache @CreatePageTest() so page test instance that's used by
> the benchmark is only created once.

I think Nat and Tony would agree with me that @Cache should never be used to
affect the code flow, only for performance. Its side-effects are very implicit,
and it's already overused.
> 
> The hard way to do this is to deprecate the feature of letting page test set
> define the command line options. Since page test is configurable through
> constructor, people who want the command line option can define it in
> benchmark.AddCommandLineArgs(). e.g:
> 
> def CreatePageTest(self, options):
>    return repaint.Repaint(options.viewport, options.width, ...)

Ah, I think this is something we can do. I think PageTest (Measurement) options
were added before Benchmarks existed.

[nednguyen, 2014-10-10 02:22:25]

On 2014/10/10 01:45:28, dtu wrote:
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> File tools/telemetry/telemetry/benchmark.py (right):
> 
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
> On 2014/10/09 19:40:08, nednguyen wrote:
> > On 2014/10/09 17:16:17, ernstm wrote:
> > > On 2014/10/08 01:44:54, nednguyen wrote:
> > > > This has to be @classmethod so test_runner can process the command line
> > > options
> > > > defined by page_test before it instantiates  benchmark:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > > > 
> > > > I would change the @classmethod PageTestClass to @classmethod
> > > CreatePageTest().
> > > 
> > > But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> > > instantiate the page test instead of just calling a classmethod on the
> class.
> > 
> > There are multiple ways to solve this:
> > 
> > One way is to change SetArgumentDefaults(cls, parser) to
> > SetArgumentDefaults(cls, parser, page_test_class) and move the instantiation
> of
> > page test to test_runner.
> 
> To me, the separation between test_runner and benchmark is very clear -
> test_runner solely deals with parsing the command-line. All test logic is in
> Benchmark. Put another way, test_runner should be able to run multiple
> Benchmarks just by adding a for loop. Moving the instantiation of PageTest to
> test_runner breaks this separation.
> > 
> > Another way is to cache @CreatePageTest() so page test instance that's used
by
> > the benchmark is only created once.
> 
> I think Nat and Tony would agree with me that @Cache should never be used to
> affect the code flow, only for performance. Its side-effects are very
implicit,
> and it's already overused.
> > 
> > The hard way to do this is to deprecate the feature of letting page test set
> > define the command line options. Since page test is configurable through
> > constructor, people who want the command line option can define it in
> > benchmark.AddCommandLineArgs(). e.g:
> > 
> > def CreatePageTest(self, options):
> >    return repaint.Repaint(options.viewport, options.width, ...)
> 
> Ah, I think this is something we can do. I think PageTest (Measurement)
options
> were added before Benchmarks existed.

All right, let's aim for this then. 

Compared with other test framework (python unittest for example), I think
telemetry has been a little bit too generous about command line options, which
may make it a little bit more convenient for our users, but hurts us a lot in
term of complexity and expressiveness (how would you configure the results
filtering function for example).

[ernstm, 2014-10-14 22:18:14]

ernstm@chromium.org changed reviewers:
+ chrishenry@google.com

[ernstm, 2014-10-14 22:24:12]

On 2014/10/10 02:22:25, nednguyen wrote:
> On 2014/10/10 01:45:28, dtu wrote:
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > File tools/telemetry/telemetry/benchmark.py (right):
> > 
> >
>
https://codereview.chromium.org/637153002/diff/1/tools/telemetry/telemetry/be...
> > tools/telemetry/telemetry/benchmark.py:177: def CreatePageTest(self):
> > On 2014/10/09 19:40:08, nednguyen wrote:
> > > On 2014/10/09 17:16:17, ernstm wrote:
> > > > On 2014/10/08 01:44:54, nednguyen wrote:
> > > > > This has to be @classmethod so test_runner can process the command
line
> > > > options
> > > > > defined by page_test before it instantiates  benchmark:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> > > > > 
> > > > > I would change the @classmethod PageTestClass to @classmethod
> > > > CreatePageTest().
> > > > 
> > > > But then SetArgumentDefaults and ProcessCommandLineArgs would need to
> > > > instantiate the page test instead of just calling a classmethod on the
> > class.
> > > 
> > > There are multiple ways to solve this:
> > > 
> > > One way is to change SetArgumentDefaults(cls, parser) to
> > > SetArgumentDefaults(cls, parser, page_test_class) and move the
instantiation
> > of
> > > page test to test_runner.
> > 
> > To me, the separation between test_runner and benchmark is very clear -
> > test_runner solely deals with parsing the command-line. All test logic is in
> > Benchmark. Put another way, test_runner should be able to run multiple
> > Benchmarks just by adding a for loop. Moving the instantiation of PageTest
to
> > test_runner breaks this separation.
> > > 
> > > Another way is to cache @CreatePageTest() so page test instance that's
used
> by
> > > the benchmark is only created once.
> > 
> > I think Nat and Tony would agree with me that @Cache should never be used to
> > affect the code flow, only for performance. Its side-effects are very
> implicit,
> > and it's already overused.
> > > 
> > > The hard way to do this is to deprecate the feature of letting page test
set
> > > define the command line options. Since page test is configurable through
> > > constructor, people who want the command line option can define it in
> > > benchmark.AddCommandLineArgs(). e.g:
> > > 
> > > def CreatePageTest(self, options):
> > >    return repaint.Repaint(options.viewport, options.width, ...)
> > 
> > Ah, I think this is something we can do. I think PageTest (Measurement)
> options
> > were added before Benchmarks existed.
> 
> All right, let's aim for this then. 
> 
> Compared with other test framework (python unittest for example), I think
> telemetry has been a little bit too generous about command line options, which
> may make it a little bit more convenient for our users, but hurts us a lot in
> term of complexity and expressiveness (how would you configure the results
> filtering function for example).

With the latest patch, page tests can't run directly anymore (run_measurement is
gone) and they can't access the command line arguments anymore (except for extra
browser args). Page tests must now be configured through their constructor by
the benchmark. I think this is a cleaner approach than handling all parameters
via the command line. The patch is large, because many measurements and
benchmarks needed to be adapted. PTAL.

[nednguyen, 2014-10-14 22:33:37]

I like this approach, but would you mind separating some changes to other
patches?

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/v...
File tools/perf/benchmarks/v8.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/v...
tools/perf/benchmarks/v8.py:14: return
timeline_based_measurement.TimelineBasedMeasurement(
Add your #TODO back?

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurement
File tools/perf/run_measurement (left):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurem...
tools/perf/run_measurement:1: #!/usr/bin/env python
Wow, let makes a separate patch just to remove this script. Are we sure that no
bot uses this?

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark_runner.py:212:
index_by_class_name=True).values()
This change itself also deserves a separate patch.

[dtu, 2014-10-14 22:37:17]

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurement
File tools/perf/run_measurement (left):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurem...
tools/perf/run_measurement:1: #!/usr/bin/env python
On 2014/10/14 22:33:37, nednguyen wrote:
> Wow, let makes a separate patch just to remove this script. Are we sure that
no
> bot uses this?

CrOS is still using run_measurement --print-bootstrap-deps-cros. We may want to
sweep downstream waterfalls, the perf dashboard, etc. for references.

[nednguyen, 2014-10-14 22:39:00]

On 2014/10/14 22:37:17, dtu wrote:
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurement
> File tools/perf/run_measurement (left):
> 
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurem...
> tools/perf/run_measurement:1: #!/usr/bin/env python
> On 2014/10/14 22:33:37, nednguyen wrote:
> > Wow, let makes a separate patch just to remove this script. Are we sure that
> no
> > bot uses this?
> 
> CrOS is still using run_measurement --print-bootstrap-deps-cros. We may want
to
> sweep downstream waterfalls, the perf dashboard, etc. for references.

Definitely this effort worths filing it own bug for tracking purpose.

[dtu, 2014-10-14 22:55:43]

Also look in content/test/gpu/gpu_tests for more measurements.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:136: def Run(self, args):
Remove this too :)

[vmiura, 2014-10-14 23:16:07]

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

[vmiura, 2014-10-14 23:16:08]

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
File tools/perf/benchmarks/repaint.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
tools/perf/benchmarks/repaint.py:24: help='Height of invalidations for
fixed_size mode.')
I'm not sure the approach of removing command line args from the the Measurement
only to add them to the Benchmark is good.

Since the Measurement consumes those command line options, I'd say the
Measurement should add the options.

In this case we will probably want to make RepaintContinuouslyAction add and
consume those options, so that we can decouple that from the measurement.

[dtu, 2014-10-14 23:31:15]

On 2014/10/14 23:16:08, vmiura wrote:
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> File tools/perf/benchmarks/repaint.py (right):
> 
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> tools/perf/benchmarks/repaint.py:24: help='Height of invalidations for
> fixed_size mode.')
> I'm not sure the approach of removing command line args from the the
Measurement
> only to add them to the Benchmark is good.
> 
> Since the Measurement consumes those command line options, I'd say the
> Measurement should add the options.
> 
> In this case we will probably want to make RepaintContinuouslyAction add and
> consume those options, so that we can decouple that from the measurement.

I would like to see us reduce the number of options configurable from the
command-line. Having many options makes it easier to mess up when trying to
reproduce a test scenario. A lot of what are currently Measurement command-line
option should really be hard-coded into the Benchmark to produce a
self-contained test case.

[vmiura, 2014-10-15 00:45:20]

On 2014/10/14 23:31:15, dtu wrote:
> On 2014/10/14 23:16:08, vmiura wrote:
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > File tools/perf/benchmarks/repaint.py (right):
> > 
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > tools/perf/benchmarks/repaint.py:24: help='Height of invalidations for
> > fixed_size mode.')
> > I'm not sure the approach of removing command line args from the the
> Measurement
> > only to add them to the Benchmark is good.
> > 
> > Since the Measurement consumes those command line options, I'd say the
> > Measurement should add the options.
> > 
> > In this case we will probably want to make RepaintContinuouslyAction add and
> > consume those options, so that we can decouple that from the measurement.
> 
> I would like to see us reduce the number of options configurable from the
> command-line. Having many options makes it easier to mess up when trying to
> reproduce a test scenario. A lot of what are currently Measurement
command-line
> option should really be hard-coded into the Benchmark to produce a
> self-contained test case.

I agree, I just wasn't sure about the move of the command-line parsing from one
layer to another.

I'd say:
1. If command-line options don't need to be command-line options, nuke them.
2. If command-line options are specific to a Menchmark put them on a benchmark.
3. If command-line options tweak something internal to a Measurement or Action
then put them on the Measurement or Action.

[vmiura, 2014-10-15 00:46:05]

Menchmark -> Benchmark :-)

[nednguyen, 2014-10-15 02:10:51]

On 2014/10/15 00:45:20, vmiura wrote:
> On 2014/10/14 23:31:15, dtu wrote:
> > On 2014/10/14 23:16:08, vmiura wrote:
> > >
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > > File tools/perf/benchmarks/repaint.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > > tools/perf/benchmarks/repaint.py:24: help='Height of invalidations for
> > > fixed_size mode.')
> > > I'm not sure the approach of removing command line args from the the
> > Measurement
> > > only to add them to the Benchmark is good.
> > > 
> > > Since the Measurement consumes those command line options, I'd say the
> > > Measurement should add the options.
> > > 
> > > In this case we will probably want to make RepaintContinuouslyAction add
and
> > > consume those options, so that we can decouple that from the measurement.
> > 
> > I would like to see us reduce the number of options configurable from the
> > command-line. Having many options makes it easier to mess up when trying to
> > reproduce a test scenario. A lot of what are currently Measurement
> command-line
> > option should really be hard-coded into the Benchmark to produce a
> > self-contained test case.
> 
> I agree, I just wasn't sure about the move of the command-line parsing from
one
> layer to another.
> 
> I'd say:
> 1. If command-line options don't need to be command-line options, nuke them.
> 2. If command-line options are specific to a benchmark put them on a
benchmark.
> 3. If command-line options tweak something internal to a Measurement or Action
> then put them on the Measurement or Action.

I would say there is no clear boundary between 2 & 3.  If the model is already
that you always need page set + measurement + benchmark to run telemetry test,
there is not much benefit in support defining flag options at the measurement
layer.  Each flag adds a lot of maintaining burdens since it's global, hard to
unittested, hard to track down who use it, so I totally support delegating the
job of defining measurements' flag options to benchmark authors.

[chrishenry, 2014-10-15 17:58:53]

On 2014/10/15 02:10:51, nednguyen wrote:
> On 2014/10/15 00:45:20, vmiura wrote:
> > On 2014/10/14 23:31:15, dtu wrote:
> > > On 2014/10/14 23:16:08, vmiura wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > > > File tools/perf/benchmarks/repaint.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
> > > > tools/perf/benchmarks/repaint.py:24: help='Height of invalidations for
> > > > fixed_size mode.')
> > > > I'm not sure the approach of removing command line args from the the
> > > Measurement
> > > > only to add them to the Benchmark is good.
> > > > 
> > > > Since the Measurement consumes those command line options, I'd say the
> > > > Measurement should add the options.
> > > > 
> > > > In this case we will probably want to make RepaintContinuouslyAction add
> and
> > > > consume those options, so that we can decouple that from the
measurement.
> > > 
> > > I would like to see us reduce the number of options configurable from the
> > > command-line. Having many options makes it easier to mess up when trying
to
> > > reproduce a test scenario. A lot of what are currently Measurement
> > command-line
> > > option should really be hard-coded into the Benchmark to produce a
> > > self-contained test case.
> > 
> > I agree, I just wasn't sure about the move of the command-line parsing from
> one
> > layer to another.
> > 
> > I'd say:
> > 1. If command-line options don't need to be command-line options, nuke them.
> > 2. If command-line options are specific to a benchmark put them on a
> benchmark.
> > 3. If command-line options tweak something internal to a Measurement or
Action
> > then put them on the Measurement or Action.
> 
> I would say there is no clear boundary between 2 & 3.  If the model is already
> that you always need page set + measurement + benchmark to run telemetry test,
> there is not much benefit in support defining flag options at the measurement
> layer.  Each flag adds a lot of maintaining burdens since it's global, hard to
> unittested, hard to track down who use it, so I totally support delegating the
> job of defining measurements' flag options to benchmark authors.

+1
I think adding command-line on benchmark is sufficient. We don't need to add
further command-line flags in measurement (they should be class configuration,
i.e., constructor arguments or setters).

[chrishenry, 2014-10-15 18:26:09]

Overall, this is pretty nice. I'm a bit concern that we're doing too many things
in a single patch though. As others have suggested, maybe split changes that can
be split out and submit them separately (interactive, run_measurement, discover
classes changes).

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
File tools/perf/benchmarks/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
tools/perf/benchmarks/rasterize_and_record_micro.py:31: help='Whether to report
additional detailed results.')
(Not blocking patch, just commenting)

Augh... Do we really need all of these command-line flags?

Maybe some of these should just be hard-coded constants that people can modify
if desired.

https://codereview.chromium.org/637153002/diff/150001/tools/perf/measurements...
File tools/perf/measurements/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/measurements...
tools/perf/measurements/rasterize_and_record_micro.py:62:
str(self._rasterize_repeat) + """
nit: if you're feeling generous, can you refactor this into """...""" %
(self._record_repeat, self._rasterize_repeat), instead of this hard to read
multi-line string concatentation.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark.py:1: # Copyright 2014 The Chromium Authors.
All rights reserved.
Is there no unit test update necessary?

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark.py:58: if hasattr(cls,
'AddTestCommandLineArgs'):
Can you add a TODO to evaluate whether this is still needed? Unclear to me why
gpu_tests want to use AddTestCommandLineArgs instead of AddCommandLineArgs
directly.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark_runner.py (left):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark_runner.py:218: if not issubclass(test_class,
profile_creator.ProfileCreator)]
So, are we sure that no bot is currently running run_measurement page_test
foo_page_set.py?

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (left):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:294: if interactive:
Unclear to me whether you need to remove this. Can you explain why we can't
support this? (Granted, this is probably not as useful as, e.g., integrating
interactive with ActionRunner.)

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:155: elif
self._overhead_level == V8_OVERHEAD_LEVEL:
Just to confirm: is there any plan to fold the necessary trace events from
V8_OVERHEAD_LEVEL to NO_OVERHEAD_LEVEL?

[vmiura, 2014-10-15 19:36:34]

> > I agree, I just wasn't sure about the move of the command-line parsing from
> > one layer to another.
> > 
> > I'd say:
> > 1. If command-line options don't need to be command-line options, nuke them.
> > 2. If command-line options are specific to a benchmark put them on a
> benchmark.
> > 3. If command-line options tweak something internal to a Measurement or
Action
> > then put them on the Measurement or Action.
> 
> I would say there is no clear boundary between 2 & 3.  If the model is already
> that you always need page set + measurement + benchmark to run telemetry test,
> there is not much benefit in support defining flag options at the measurement
> layer.  Each flag adds a lot of maintaining burdens since it's global, hard to
> unittested, hard to track down who use it, so I totally support delegating the
> job of defining measurements' flag options to benchmark authors.

I don't think we need to keep measurement layer usable on it's own, but there is
value to keeping measures/actions 'composable' and uniform.  We should be able
to unit test measurements & page actions on their own.

Part of the motivation of this re-factoring was to allow us to compose for
example
thread_times + repaint on a pageset without having to duplicate
'repaint' and 'thread_times' logic into a new measurement.

Apply the same logic to benchmarks.

This CL adds parser options in:

  benchmarks/repaint.py: class _ThreadTimes(Benchmark)
  benchmarks/thread_times.py: class _Repaint(Benchmark)

For a 'thread_times + repaint' benchmark, it doesn't sound great to duplicate
this code, or to have inconsistency in options between benchmarks.

Anyway, not an owner... my 2c :).

[vmiura, 2014-10-15 20:09:48]

>   benchmarks/repaint.py: class _ThreadTimes(Benchmark)
>   benchmarks/thread_times.py: class _Repaint(Benchmark)

Reversed...

  benchmarks/repaint.py: class _Repaint(Benchmark)
  benchmarks/thread_times.py: class _ThreadTimes(Benchmark)

[ernstm, 2014-10-16 22:32:13]

I think I have addressed all of the comments in the latest patch. However, I
don't feel 100% convinced anymore that getting rid of the command line args
altogether is the right approach. kbr@ and vmiura@ made some good arguments for
keeping support for command line arguments on the measurements. We might want to
configure measurement parameters that can be hard coded in a benchmark through
constructor arguments, and those that truly need to be command line arguments as
such. I'll explore in a separate patch what that would look like.

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
File tools/perf/benchmarks/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/r...
tools/perf/benchmarks/rasterize_and_record_micro.py:31: help='Whether to report
additional detailed results.')
On 2014/10/15 18:26:09, chrishenry wrote:
> (Not blocking patch, just commenting)
> 
> Augh... Do we really need all of these command-line flags?
> 
> Maybe some of these should just be hard-coded constants that people can modify
> if desired.

There is no way to find out if anybody is still using those. Let's remove them
in a separate patch and see if anybody complains.

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/v...
File tools/perf/benchmarks/v8.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/benchmarks/v...
tools/perf/benchmarks/v8.py:14: return
timeline_based_measurement.TimelineBasedMeasurement(
On 2014/10/14 22:33:37, nednguyen wrote:
> Add your #TODO back?

Done.

https://codereview.chromium.org/637153002/diff/150001/tools/perf/measurements...
File tools/perf/measurements/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/measurements...
tools/perf/measurements/rasterize_and_record_micro.py:62:
str(self._rasterize_repeat) + """
On 2014/10/15 18:26:09, chrishenry wrote:
> nit: if you're feeling generous, can you refactor this into """...""" %
> (self._record_repeat, self._rasterize_repeat), instead of this hard to read
> multi-line string concatentation.

Done.

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurement
File tools/perf/run_measurement (left):

https://codereview.chromium.org/637153002/diff/150001/tools/perf/run_measurem...
tools/perf/run_measurement:1: #!/usr/bin/env python
On 2014/10/14 22:37:17, dtu wrote:
> On 2014/10/14 22:33:37, nednguyen wrote:
> > Wow, let makes a separate patch just to remove this script. Are we sure that
> no
> > bot uses this?
> 
> CrOS is still using run_measurement --print-bootstrap-deps-cros. We may want
to
> sweep downstream waterfalls, the perf dashboard, etc. for references.

Separate patch here: https://codereview.chromium.org/655563007/

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark.py:1: # Copyright 2014 The Chromium Authors.
All rights reserved.
On 2014/10/15 18:26:09, chrishenry wrote:
> Is there no unit test update necessary?

There is no unit test :-(

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark.py:58: if hasattr(cls,
'AddTestCommandLineArgs'):
On 2014/10/15 18:26:09, chrishenry wrote:
> Can you add a TODO to evaluate whether this is still needed? Unclear to me why
> gpu_tests want to use AddTestCommandLineArgs instead of AddCommandLineArgs
> directly.

AddTestCommandLineArgs nicely groups the options for a benchmark.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark_runner.py (left):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark_runner.py:218: if not issubclass(test_class,
profile_creator.ProfileCreator)]
On 2014/10/15 18:26:09, chrishenry wrote:
> So, are we sure that no bot is currently running run_measurement page_test
> foo_page_set.py?

Separate patch for removing run_measurement:
https://codereview.chromium.org/655563007/

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/benchmark_runner.py:212:
index_by_class_name=True).values()
On 2014/10/14 22:33:37, nednguyen wrote:
> This change itself also deserves a separate patch.

Separate patch here: https://codereview.chromium.org/659673002/

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (left):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:294: if interactive:
On 2014/10/15 18:26:09, chrishenry wrote:
> Unclear to me whether you need to remove this. Can you explain why we can't
> support this? (Granted, this is probably not as useful as, e.g., integrating
> interactive with ActionRunner.)

Not strictly necessary. I added the feature back. It would still be nice to not
need to know about options in page runner. We can to this as a follow up patch.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:136: def Run(self, args):
On 2014/10/14 22:55:43, dtu wrote:
> Remove this too :)

Done.

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/637153002/diff/150001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:155: elif
self._overhead_level == V8_OVERHEAD_LEVEL:
On 2014/10/15 18:26:09, chrishenry wrote:
> Just to confirm: is there any plan to fold the necessary trace events from
> V8_OVERHEAD_LEVEL to NO_OVERHEAD_LEVEL?

Yes, this is waiting for Fadi's work to teach V8 about categories.

[chrishenry, 2014-10-17 23:57:09]

lgtm, but let's finish the conversation on
https://codereview.chromium.org/641823004/

https://codereview.chromium.org/637153002/diff/170001/tools/perf/benchmarks/p...
File tools/perf/benchmarks/page_cycler.py (right):

https://codereview.chromium.org/637153002/diff/170001/tools/perf/benchmarks/p...
tools/perf/benchmarks/page_cycler.py:14: def AddCommandLineArgs(cls, parser):
Should this be AddTestCommandLineArgs then? Similar elsewhere.

[ernstm, 2014-11-19 18:04:44]

ernstm@chromium.org changed reviewers:
+ kbr@chromium.org

[ernstm, 2014-11-19 18:04:46]

We decided to move forward with this CL instead of
https://codereview.chromium.org/641823004/.

+kbr for the (small) changes to GPU tests.

https://codereview.chromium.org/637153002/diff/170001/tools/perf/benchmarks/p...
File tools/perf/benchmarks/page_cycler.py (right):

https://codereview.chromium.org/637153002/diff/170001/tools/perf/benchmarks/p...
tools/perf/benchmarks/page_cycler.py:14: def AddCommandLineArgs(cls, parser):
On 2014/10/17 23:57:09, chrishenry wrote:
> Should this be AddTestCommandLineArgs then? Similar elsewhere.

Done. Renamed AddTestCommandLineArgs to AddBenchmarkCommandLineArgs.

[Ken Russell (switch to Gerrit), 2014-11-19 18:10:59]

content/test/gpu LGTM as long as this passes the trybots.

[ernstm, 2014-11-19 18:15:23]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-19 18:16:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637153002/230001

[commit-bot: I haz the power, 2014-11-19 18:23:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 18:23:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ernstm, 2014-11-19 19:13:19]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-19 19:14:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637153002/250001

[ernstm, 2014-11-19 19:39:23]

The CQ bit was unchecked by ernstm@chromium.org

[ernstm, 2014-11-19 19:40:49]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-19 19:42:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637153002/270001

[nednguyen, 2014-11-19 19:44:16]

https://codereview.chromium.org/637153002/diff/270001/tools/perf/measurements...
File tools/perf/measurements/smooth_gesture_util_unittest.py (right):

https://codereview.chromium.org/637153002/diff/270001/tools/perf/measurements...
tools/perf/measurements/smooth_gesture_util_unittest.py:123: # pylint:
disable=E1003
Can you change this to disable=bad-super-call instead? It seems like this the
new way to suppress it.

[ernstm, 2014-11-19 19:56:19]

The CQ bit was unchecked by ernstm@chromium.org

[ernstm, 2014-11-19 20:02:44]

https://codereview.chromium.org/637153002/diff/270001/tools/perf/measurements...
File tools/perf/measurements/smooth_gesture_util_unittest.py (right):

https://codereview.chromium.org/637153002/diff/270001/tools/perf/measurements...
tools/perf/measurements/smooth_gesture_util_unittest.py:123: # pylint:
disable=E1003
On 2014/11/19 19:44:16, nednguyen wrote:
> Can you change this to disable=bad-super-call instead? It seems like this the
> new way to suppress it.

Done.

[ernstm, 2014-11-19 20:02:51]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-11-19 20:04:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637153002/290001

[commit-bot: I haz the power, 2014-11-19 21:02:07]

Committed patchset #9 (id:290001)

[commit-bot: I haz the power, 2014-11-19 21:04:28]

Patchset 9 (id:??) landed as
https://crrev.com/7c0eae7f526aa9af80d5800da7daa50f41290375
Cr-Commit-Position: refs/heads/master@{#304887}

[jeremy, 2014-11-20 14:36:15]

A revert of this CL (patchset #9 id:290001) has been created in
https://codereview.chromium.org/737403002/ by jeremy@chromium.org.

The reason for reverting is: Looks like this might have hosed generate_profiles
on the Mac perf bots - crbug.com/435108 ..

[rmistry, 2014-11-21 20:21:10]

This CL broke Skia's bot:
http://build.chromium.org/p/client.skia.fyi/builders/Housekeeper-Nightly-Recr...

Looks like the --skp-output flag in the measurement:
https://codereview.chromium.org/637153002/diff/290001/tools/perf/measurements...
was not moved over to the benchmark:
https://codereview.chromium.org/637153002/diff/290001/tools/perf/benchmarks/s...