Android: do not run telemetry directly.

Android: do not run telemetry directly.

Rather than going directly to telemetry, on Android the perf tests are run in two
stages:
1. build/android/test_runner.py perf <CONFIG>
This script runs all tests using sharding across multiple devices and retries
on different devices in case of failures.
This is the same mechanism as all other functional tests.
It's not an "annotated" step, since it potentially prints the *RESULT output
multiple times.
It only persists to disk the "best" run of each test.

2. build/android/test_runner.py perf --print <STEP>
At this stage, it'll basically "cat" the output previously persisted in (1).

BUG=

[bulach, 2013-10-18 10:55:58]

here's what I'm talking about "converging" the steps with downstream android...

with this patch, rather than calling directly into telemetry.py from the
buildbot, it'd call into "build/android/test_runner.py perf" in two stages, one
to run all tests, and then another one to just "cat" the output..

I'm not too comfortable with up with the master config, but I'm even less
comfortable in keeping the current divergence and applying "hacks" on adb in
different levels of the stack ;)

please let me know your thoughts on this, I think it's pretty important for both
waterfalls to be running the tests in the same way, so we can both leverage the
stabilization efforts... thanks!!

[frankf, 2013-10-18 17:55:19]

overall approach lgtm
I'll defer to others more familiar with buildbot

https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
File masters/master.chromium.perf/chromium_perf_android.json (right):

https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
masters/master.chromium.perf/chromium_perf_android.json:3:
"tools/perf/run_benchmark --browser=android-chromium-testshell
page_cycler.bloat"],
I think you're missing some options compared to downstream:
--show-stdout --keep_test_server_ports

[ghost stip (do not use), 2013-10-21 21:41:17]

I guess I still don't fully understand why the android scripts can't call
runbuild directly? buildbot can specify the location of runtest (and the flags
to run it with) when calling the annotated script. Then things get simplified a
lot.

When we originally discussed the 'have everything dump to a file, then cat
together' I was under the impression that there was a single perf test you were
grouping the results together in. From what I understand, the page cyclers
aren't designed to have multiple perf step outputs (page_cycler.moz, etc) in the
same input stream. You'd have to create multiple runtest steps, which runs into
the master restart requirement before.

Seems like we have two choices here: add conditional support to wrap perf test
commands with runtest if its location is passed to bb_run_bot.py. The other
option is we dynamically add runtest steps in master.cfg, using buildrunner. The
latter adds the compilcation of determining where the result files are -- you'd
have to pass this information back to buildbot somehow.

https://chromiumcodereview.appspot.com/28933002/diff/1/masters/master.chromiu...
File masters/master.chromium.perf/chromium_perf_android.json (right):

https://chromiumcodereview.appspot.com/28933002/diff/1/masters/master.chromiu...
masters/master.chromium.perf/chromium_perf_android.json:3:
"tools/perf/run_benchmark --browser=android-chromium-testshell
page_cycler.bloat"],
why is this file in tools/build? isn't it more appropriate in src?

https://chromiumcodereview.appspot.com/28933002/diff/1/scripts/master/factory...
File scripts/master/factory/chromium_commands.py (right):

https://chromiumcodereview.appspot.com/28933002/diff/1/scripts/master/factory...
scripts/master/factory/chromium_commands.py:931:
self._AddBasicPythonTest('sharded_perf_tests', script=script, args=args)
this doesn't do any RESULT parsing. you need to pass this data through runtest.
self.AddAnnotatedPerfStep() will do what you want

[ghost stip (do not use), 2013-10-21 21:41:56]

On 2013/10/21 21:41:17, stip wrote:
> I guess I still don't fully understand why the android scripts can't call
> runbuild directly? buildbot can specify the location of runtest (and the flags
> to run it with) when calling the annotated script. Then things get simplified
a
> lot.
> 
> When we originally discussed the 'have everything dump to a file, then cat
> together' I was under the impression that there was a single perf test you
were
> grouping the results together in. From what I understand, the page cyclers
> aren't designed to have multiple perf step outputs (page_cycler.moz, etc) in
the
> same input stream. You'd have to create multiple runtest steps, which runs
into
> the master restart requirement before.
> 
> Seems like we have two choices here: add conditional support to wrap perf test
> commands with runtest if its location is passed to bb_run_bot.py. The other
> option is we dynamically add runtest steps in master.cfg, using buildrunner.
The
> latter adds the compilcation of determining where the result files are --
you'd
> have to pass this information back to buildbot somehow.
> 
>
https://chromiumcodereview.appspot.com/28933002/diff/1/masters/master.chromiu...
> File masters/master.chromium.perf/chromium_perf_android.json (right):
> 
>
https://chromiumcodereview.appspot.com/28933002/diff/1/masters/master.chromiu...
> masters/master.chromium.perf/chromium_perf_android.json:3:
> "tools/perf/run_benchmark --browser=android-chromium-testshell
> page_cycler.bloat"],
> why is this file in tools/build? isn't it more appropriate in src?
> 
>
https://chromiumcodereview.appspot.com/28933002/diff/1/scripts/master/factory...
> File scripts/master/factory/chromium_commands.py (right):
> 
>
https://chromiumcodereview.appspot.com/28933002/diff/1/scripts/master/factory...
> scripts/master/factory/chromium_commands.py:931:
> self._AddBasicPythonTest('sharded_perf_tests', script=script, args=args)
> this doesn't do any RESULT parsing. you need to pass this data through
runtest.
> self.AddAnnotatedPerfStep() will do what you want

Sorry, first line should read 'I guess I still don't fully understand why the
android scripts can't call *runtest* directly?'

[bulach, 2013-10-22 10:45:52]

high level clarifications :) I think we should probably setup another meeting so
we all understand the problems here.. I can explain the android side, but for
sure I lack the buildbot side knowledge...

Anyways,
Here's the scenario, let's assume perf tests [PTA, PTB, PTC], and devices [D1,
D2]...

PTA, PTB and PTC are entirely independent, say, octane, page cycler and
smoothness...
(sorry if I didn't make this point clearer before! yes, the tests and their
results/ stream are entirely independent, never meant to be combined...).

The first stage, we run:
First Stage Step:
- PTA on D1
- PTB on D2
- PTC on "whatever finishes first".
optionally:
- Run failed tests on other devices, say:
-- PTB on D1.

Note that the first two steps happen in parallel..
From the buildbot point of view, this is a single "First Stage Step"....
The stdout of PTA, PTB and PTC are interleaved (but again, it doesn't mean they
will be combined).
At this stage, buildbot can't parse them and upload to the dashboard in any
meaningful way: not only PTA and PTB are interleaved, PTB was actually executed
twice: one failed on D2, for whatever reason, but it did potentially print
*RESULT= lines..

This first stage then persists the output of [PTA/D1, PTB/D1, PTC/whatever].
The second stage is then buildbot asking:

Second stage, multiple steps:
- PTA
- PTB
- PTC


At this stage, we'd just "cat" the previously saved "clean and final" result for
each test (regardless of how many retries happened, which device it ran,
etc..)...

The json could certainly be in src/ the only reason I didn't put right now is
that it'd make matter worse: until chromium.perf is converted to recipes, we'd
still need to change its master.cfg to add / remove tests, so it seems logical
to keep the json together with master.cfg to avoid two-sided patches.

The main problem here is that "runtest.py" supports only a single step, which is
fine for the "second stage"... 

On the first stage however, we want to run multiple steps simultaneously,
possibly with internal retries... The first stage output is meaningless for
dashboards and whatnot, due to interleaving and retries.

If there's any other way to do this, please let me know!! :)

[tonyg, 2013-10-24 15:12:30]

https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
File masters/master.chromium.perf/chromium_perf_android.json (right):

https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
masters/master.chromium.perf/chromium_perf_android.json:3:
"tools/perf/run_benchmark --browser=android-chromium-testshell
page_cycler.bloat"],
There are two important goals in our perfbot configs right now:
1. Bring Android and desktop into greater synergy.
2. Bring upstream and downstream Android into greater synergy.

I feel like the introduction of this file is a huge step forward for #2, but
also a step backwards for #1. Is there any way we could accomplish #2 without
having to sacrifice #1?

Or maybe #2 is more immediately critical and we could still work towards #1
subsequently?

[bulach, 2013-10-24 16:55:32]

On 2013/10/24 15:12:30, tonyg wrote:
>
https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
> File masters/master.chromium.perf/chromium_perf_android.json (right):
> 
>
https://codereview.chromium.org/28933002/diff/1/masters/master.chromium.perf/...
> masters/master.chromium.perf/chromium_perf_android.json:3:
> "tools/perf/run_benchmark --browser=android-chromium-testshell
> page_cycler.bloat"],
> There are two important goals in our perfbot configs right now:
> 1. Bring Android and desktop into greater synergy.
> 2. Bring upstream and downstream Android into greater synergy.
> 
> I feel like the introduction of this file is a huge step forward for #2, but
> also a step backwards for #1. Is there any way we could accomplish #2 without
> having to sacrifice #1?
> 
> Or maybe #2 is more immediately critical and we could still work towards #1
> subsequently?

I fully agree with the stated goals.
The "recipes" may bring a bit more harmony for #1, but even with that, I don't
think we'd get away of this "two phase steps"...

The underlying problems are:
- all the retry logic / device switching is at the build/android/test_runner.py
level.
- less pressing, but also important, it seems undesirable to have multiple
devices and use a single one.
- we're bumping at each other with our efforts for chromium.perf and clank.perf
:-/


I can think of the following options:
1) Push all platforms to use this "two phase steps" with a configuration for all
the steps..
+ would better unify across platform.
- would make more complicated for all other platforms.

or
2) Make the changes less "dramatic": for example, rather than having this json
config that would run all the tests at once, we may just run each step twice:
once with the retry, and another that would just "cat" the last result...
+ slightly less intrusive
- would still be different from downstream, so in the end, achieves even less
goals :)



Thoughts??

[tonyg, 2013-11-06 16:24:16]

Is it possible to make telemetry.py call build/android/test_runner.py without
using the json file? That is the hack that I don't think we should introduce
upstream.

[bulach, 2013-11-06 18:24:04]

that's a possibility, but here's the -ve points:

- will not shard, and would still be different from downstream.
- this part of the code in b/a/test_runner.py would still be "forked", i.e.,
only exercised upstream...
- would still need to call twice: one for executing (but somewhat not passing
the stdout up otherwise the retries would mess up with the log parser /
uploader) and another one to actually print the stdout...

I can try to hack that up, but I'd really like to have a solution where down and
upstream get closer in the long term..

[ghost stip (do not use), 2013-11-12 00:33:52]

should we close this since https://chromiumcodereview.appspot.com/62563003/ is
the way forward?

[bulach, 2013-11-12 09:11:48]

thanks stip! closing without landing..