[Telemetry] Reuse shared state between story runs

[Telemetry] Reuse shared state between story runs

This CL removes the benchmark methods:

- ShouldTearDownStateAfterEachStoryRun
- ShouldTearDownStateAfterEachStorySetRun

allowing the shared state to be reused across story runs.

It also provides a new default implementation for the
shared state to always close the browser after each story
(except for ChromeOs).

This is not expected to change performance metrics
(although it might), and should reduce total benchmark
running time.

BUG=chromium:748566, chromium:746251
Review-Url: https://codereview.chromium.org/2998773002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/122dd5e91bf64888049459ce73a8fd34c084cfe0

[perezju, 2017-08-10 12:56:24]

perezju@chromium.org changed reviewers:
+ nednguyen@google.com

[perezju, 2017-08-10 12:56:25]

This prepares src-side benchmarks to use the mechanism that will be introduced
by:
https://chromium-review.googlesource.com/c/610044

[perezju, 2017-08-10 12:56:25]

This prepares src-side benchmarks to use the mechanism that will be introduced
by:
https://chromium-review.googlesource.com/c/610044

[perezju, 2017-08-10 12:59:13]

On 2017/08/10 12:56:25, perezju wrote:
> This prepares src-side benchmarks to use the mechanism that will be introduced
> by:
> https://chromium-review.googlesource.com/c/610044

Actually, it's the other way around :-/ got confused with my tabs.

This CL makes the Telemetry changes, while [1] prepares the src-side benchmarks.
Thus [1] needs to land first.

[1]: https://chromium-review.googlesource.com/c/610044

[nednguyen, 2017-08-10 13:00:43]

lgtm with a comment

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
telemetry/telemetry/story/story_set.py:148: def
ShouldStopBrowserAfterStoryRun(self, story):
This is a bit awkward to put in story_set. Though I also don't want to create a
subclass PageSet of StorySet. 

Maybe just rename this to: ShouldStopApplicationsAfterStoryRun to make it more
general?

[perezju, 2017-08-10 13:12:48]

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
telemetry/telemetry/story/story_set.py:148: def
ShouldStopBrowserAfterStoryRun(self, story):
On 2017/08/10 13:00:43, nednguyen wrote:
> This is a bit awkward to put in story_set. Though I also don't want to create
a
> subclass PageSet of StorySet. 
> 
> Maybe just rename this to: ShouldStopApplicationsAfterStoryRun to make it more
> general?

Hmm, I see how this is actually breaking some abstractions.

As an alternative, maybe we should not be providing this method at all. And (the
very few) benchmarks that want to have a different behavior should subclass and
override the corresponding method on the shared state instead.

By not providing such a simple API we also discourage clients from changing the
default behavior and keep story runs independent as much as possible.

I'll tweak these two CLs as needed.

[nednguyen, 2017-08-10 13:13:55]

On 2017/08/10 13:12:48, perezju wrote:
>
https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
> File telemetry/telemetry/story/story_set.py (right):
> 
>
https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
> telemetry/telemetry/story/story_set.py:148: def
> ShouldStopBrowserAfterStoryRun(self, story):
> On 2017/08/10 13:00:43, nednguyen wrote:
> > This is a bit awkward to put in story_set. Though I also don't want to
create
> a
> > subclass PageSet of StorySet. 
> > 
> > Maybe just rename this to: ShouldStopApplicationsAfterStoryRun to make it
more
> > general?
> 
> Hmm, I see how this is actually breaking some abstractions.
> 
> As an alternative, maybe we should not be providing this method at all. And
(the
> very few) benchmarks that want to have a different behavior should subclass
and
> override the corresponding method on the shared state instead.
> 
> By not providing such a simple API we also discourage clients from changing
the
> default behavior and keep story runs independent as much as possible.

+1. I like this option :-)
> 
> I'll tweak these two CLs as needed.

[perezju, 2017-08-10 13:29:55]

Description was changed from

==========
[Telemetry] Reuse shared state and add new browser-closing mechanism

This CL removes the benchmark methods:

- ShouldTearDownStateAfterEachStoryRun
- ShouldTearDownStateAfterEachStorySetRun

allowing the shared state to be reused across story runs.

It also adds the new story_set method:

- ShouldStopBrowserAfterStoryRun

which defaults to always closing (except for ChromeOs), and allows
individual benchmarks to override that default.

BUG=chromium:748566
==========

to

==========
[Telemetry] Reuse shared state between story runs

This CL removes the benchmark methods:

- ShouldTearDownStateAfterEachStoryRun
- ShouldTearDownStateAfterEachStorySetRun

allowing the shared state to be reused across story runs.

It also provides a new default implementation for the
shared state to always close the browser after each story
(except for ChromeOs).

This is not expected to change performance metrics
(although it might), and should reduce total benchmark
running time.

BUG=chromium:748566,chromium:746251
==========

[perezju, 2017-08-10 14:32:32]

PTAL

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
File telemetry/telemetry/story/story_set.py (right):

https://codereview.chromium.org/2998773002/diff/1/telemetry/telemetry/story/s...
telemetry/telemetry/story/story_set.py:148: def
ShouldStopBrowserAfterStoryRun(self, story):
On 2017/08/10 13:12:48, perezju wrote:
> On 2017/08/10 13:00:43, nednguyen wrote:
> > This is a bit awkward to put in story_set. Though I also don't want to
create
> a
> > subclass PageSet of StorySet. 
> > 
> > Maybe just rename this to: ShouldStopApplicationsAfterStoryRun to make it
more
> > general?
> 
> Hmm, I see how this is actually breaking some abstractions.
> 
> As an alternative, maybe we should not be providing this method at all. And
(the
> very few) benchmarks that want to have a different behavior should subclass
and
> override the corresponding method on the shared state instead.
> 
> By not providing such a simple API we also discourage clients from changing
the
> default behavior and keep story runs independent as much as possible.
> 
> I'll tweak these two CLs as needed.

Done.

[nednguyen, 2017-08-10 14:40:49]

lgtm

https://codereview.chromium.org/2998773002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2998773002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:173: effective_max_failures =
finder_options.max_failures
Hey, turn out we still have this "--max-failures" thing in the code base. Maybe
we should just set this to 5 for ref build.

(very unrelated :-p)

[perezju, 2017-08-10 14:46:39]

https://codereview.chromium.org/2998773002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2998773002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:173: effective_max_failures =
finder_options.max_failures
On 2017/08/10 14:40:49, nednguyen wrote:
> Hey, turn out we still have this "--max-failures" thing in the code base.
Maybe
> we should just set this to 5 for ref build.
> 
> (very unrelated :-p) 

:) will have a look after finishing this cleanup.

[perezju, 2017-08-14 07:34:26]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2017-08-14 07:34:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-14 07:35:44]

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

[commit-bot: I haz the power, 2017-08-14 07:35:45]

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[perezju, 2017-08-14 07:55:54]

The CQ bit was checked by perezju@chromium.org

[perezju, 2017-08-14 07:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2998773002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-08-14 07:56:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-14 08:10:13]

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

[commit-bot: I haz the power, 2017-08-14 08:10:15]

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)

[perezju, 2017-08-14 12:01:33]

The CQ bit was checked by perezju@chromium.org

[perezju, 2017-08-14 12:01:33]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2998773002/#ps60001
(title: "fix unittest")

[commit-bot: I haz the power, 2017-08-14 12:01:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-14 12:28:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1502712093212760,
"parent_rev": "5db51352aa5cca18e236d28ce85ba8b8a818fe12", "commit_rev":
"122dd5e91bf64888049459ce73a8fd34c084cfe0"}

[commit-bot: I haz the power, 2017-08-14 12:28:55]

Description was changed from

==========
[Telemetry] Reuse shared state between story runs

This CL removes the benchmark methods:

- ShouldTearDownStateAfterEachStoryRun
- ShouldTearDownStateAfterEachStorySetRun

allowing the shared state to be reused across story runs.

It also provides a new default implementation for the
shared state to always close the browser after each story
(except for ChromeOs).

This is not expected to change performance metrics
(although it might), and should reduce total benchmark
running time.

BUG=chromium:748566,chromium:746251
==========

to

==========
[Telemetry] Reuse shared state between story runs

This CL removes the benchmark methods:

- ShouldTearDownStateAfterEachStoryRun
- ShouldTearDownStateAfterEachStorySetRun

allowing the shared state to be reused across story runs.

It also provides a new default implementation for the
shared state to always close the browser after each story
(except for ChromeOs).

This is not expected to change performance metrics
(although it might), and should reduce total benchmark
running time.

BUG=chromium:748566,chromium:746251

Review-Url: https://codereview.chromium.org/2998773002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-08-14 12:28:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...