Fix tab_switching to support flag --pageset-repeat properly

Fix tab_switching to support flag --pageset-repeat properly

BUG=689388

[vovoy, 2017-04-23 06:17:13]

The CQ bit was checked by vovoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-23 06:17:16]

Dry run: CQ is trying da patch.

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

[vovoy, 2017-04-23 06:17:44]

Description was changed from

==========
Fix tab_switching to support flag --pageset-repeat properly

BUG=689388
==========

to

==========
Fix tab_switching to support flag --pageset-repeat properly

BUG=689388
==========

[vovoy, 2017-04-23 06:17:44]

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

[commit-bot: I haz the power, 2017-04-23 07:19:46]

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

[commit-bot: I haz the power, 2017-04-23 07:19:48]

Dry run: This issue passed the CQ dry run.

[vovoy, 2017-04-23 10:38:19]

ptal

[vovoy, 2017-04-24 06:46:50]

On 2017/04/23 10:38:19, vovoy wrote:
> ptal

ref: discussion on https://codereview.chromium.org/2819423002/

[vovoy, 2017-04-24 09:42:11]

The CQ bit was checked by vovoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 09:42:26]

Dry run: 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-04-24 09:48:38]

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

[commit-bot: I haz the power, 2017-04-24 09:48:39]

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

[vovoy, 2017-04-24 09:59:03]

The CQ bit was checked by vovoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 09:59:09]

Dry run: CQ is trying da patch.

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

[vovoy, 2017-04-24 10:02:26]

If tab switching on every pageset, the metrics data would be a mix of tab
switching on different tab counts (24, 48, 72...) and is meaningless.

I tried to only tab switching on the last pageset, but the current patch is
hacky. Is there any other way to get --pageset-repeat value in a story?

[nednguyen, 2017-04-24 11:25:48]

On 2017/04/24 10:02:26, vovoy wrote:
> If tab switching on every pageset, the metrics data would be a mix of tab
> switching on different tab counts (24, 48, 72...) and is meaningless.
> 
> I tried to only tab switching on the last pageset, but the current patch is
> hacky. Is there any other way to get --pageset-repeat value in a story?

Oh, you're right that we have this problem. 
+Juan: how do the long running memory stress test deal with this?

[perezju, 2017-04-24 12:03:51]

On 2017/04/24 11:25:48, nednguyen wrote:
> On 2017/04/24 10:02:26, vovoy wrote:
> > If tab switching on every pageset, the metrics data would be a mix of tab
> > switching on different tab counts (24, 48, 72...) and is meaningless.
> > 
> > I tried to only tab switching on the last pageset, but the current patch is
> > hacky. Is there any other way to get --pageset-repeat value in a story?
> 
> Oh, you're right that we have this problem. 
> +Juan: how do the long running memory stress test deal with this?

On that test each story opens a single page; and the default mechanism of the
story runner for --storyset-repeat will repeat those.

I think this option is being awkward to write because it breaks our current
abstractions of what story/story-set's are.

Ideally stories should be independent units of measurement, which can be
grouped into story sets, and repeated as needed to obtain more samples.

We sometimes drop the "independent" requirement (as in the current long
running benchmarks), allowing the browser to stay open between stories.

But I think we shouldn't drop the "unit of measurement"; that is each story
should delineate the "what" we want to measure.

With this CL as is, issuing --pageset-repeat 3 will yield:

- open 24 tabs -> measure
- open 24 more tabs, 48 are open -> measure
- open 24 more tabs, 72 are open, switch through all the tabs -> measure

And then average the 3 measurements. That's surely not what we want.

The more I think about it, the more I think that the only reasonable
solution is to have a --tabset-repeat as in the previous CL, and then
have the whole "open 72 tabs, switch through them, and measure" as a
single story.

Or, can you think of other alternatives?

[vovoy, 2017-04-24 12:06:19]

On 2017/04/24 11:25:48, nednguyen wrote:
> On 2017/04/24 10:02:26, vovoy wrote:
> > If tab switching on every pageset, the metrics data would be a mix of tab
> > switching on different tab counts (24, 48, 72...) and is meaningless.
> > 
> > I tried to only tab switching on the last pageset, but the current patch is
> > hacky. Is there any other way to get --pageset-repeat value in a story?
> 
> Oh, you're right that we have this problem. 
> +Juan: how do the long running memory stress test deal with this?

another possible way is send option.pageset_repeat as a arguement to
MultiTabTypical24Story() constructor.
maybe better than getattr(shared_state, '_finder_options').

def CreateStorySet(self, options):
  story_set = story.StorySet(
  archive_data_file='../page_sets/data/system_health_desktop.json',
        base_dir=os.path.dirname(os.path.abspath(__file__)),
        cloud_storage_bucket=story.PARTNER_BUCKET)
  story_set.AddStory(multi_tab_stories.MultiTabTypical24Story(
        story_set, False, options.pageset_repeat))
  return story_set

[nednguyen, 2017-04-24 12:12:02]

On 2017/04/24 12:03:51, perezju wrote:
> On 2017/04/24 11:25:48, nednguyen wrote:
> > On 2017/04/24 10:02:26, vovoy wrote:
> > > If tab switching on every pageset, the metrics data would be a mix of tab
> > > switching on different tab counts (24, 48, 72...) and is meaningless.
> > > 
> > > I tried to only tab switching on the last pageset, but the current patch
is
> > > hacky. Is there any other way to get --pageset-repeat value in a story?
> > 
> > Oh, you're right that we have this problem. 
> > +Juan: how do the long running memory stress test deal with this?
> 
> On that test each story opens a single page; and the default mechanism of the
> story runner for --storyset-repeat will repeat those.
> 
> I think this option is being awkward to write because it breaks our current
> abstractions of what story/story-set's are.
> 
> Ideally stories should be independent units of measurement, which can be
> grouped into story sets, and repeated as needed to obtain more samples.
> 
> We sometimes drop the "independent" requirement (as in the current long
> running benchmarks), allowing the browser to stay open between stories.
> 
> But I think we shouldn't drop the "unit of measurement"; that is each story
> should delineate the "what" we want to measure.
> 
> With this CL as is, issuing --pageset-repeat 3 will yield:
> 
> - open 24 tabs -> measure
> - open 24 more tabs, 48 are open -> measure
> - open 24 more tabs, 72 are open, switch through all the tabs -> measure
> 
> And then average the 3 measurements. That's surely not what we want.
> 
> The more I think about it, the more I think that the only reasonable
> solution is to have a --tabset-repeat as in the previous CL, and then
> have the whole "open 72 tabs, switch through them, and measure" as a
> single story.
> 
> Or, can you think of other alternatives?

I totally agree with this. I also open a bug to kill is_multi_tab_test in
https://github.com/catapult-project/catapult/issues/3514

Sorry for the back n forth, vovoy@. I think we should just use your other CL.

[vovoy, 2017-04-24 12:16:41]

On 2017/04/24 12:12:02, nednguyen wrote:
> On 2017/04/24 12:03:51, perezju wrote:
> > On 2017/04/24 11:25:48, nednguyen wrote:
> > > On 2017/04/24 10:02:26, vovoy wrote:
> > > > If tab switching on every pageset, the metrics data would be a mix of
tab
> > > > switching on different tab counts (24, 48, 72...) and is meaningless.
> > > > 
> > > > I tried to only tab switching on the last pageset, but the current patch
> is
> > > > hacky. Is there any other way to get --pageset-repeat value in a story?
> > > 
> > > Oh, you're right that we have this problem. 
> > > +Juan: how do the long running memory stress test deal with this?
> > 
> > On that test each story opens a single page; and the default mechanism of
the
> > story runner for --storyset-repeat will repeat those.
> > 
> > I think this option is being awkward to write because it breaks our current
> > abstractions of what story/story-set's are.
> > 
> > Ideally stories should be independent units of measurement, which can be
> > grouped into story sets, and repeated as needed to obtain more samples.
> > 
> > We sometimes drop the "independent" requirement (as in the current long
> > running benchmarks), allowing the browser to stay open between stories.
> > 
> > But I think we shouldn't drop the "unit of measurement"; that is each story
> > should delineate the "what" we want to measure.
> > 
> > With this CL as is, issuing --pageset-repeat 3 will yield:
> > 
> > - open 24 tabs -> measure
> > - open 24 more tabs, 48 are open -> measure
> > - open 24 more tabs, 72 are open, switch through all the tabs -> measure
> > 
> > And then average the 3 measurements. That's surely not what we want.
> > 
> > The more I think about it, the more I think that the only reasonable
> > solution is to have a --tabset-repeat as in the previous CL, and then
> > have the whole "open 72 tabs, switch through them, and measure" as a
> > single story.
> > 
> > Or, can you think of other alternatives?
> 
> I totally agree with this. I also open a bug to kill is_multi_tab_test in
> https://github.com/catapult-project/catapult/issues/3514
> 
> Sorry for the back n forth, vovoy@. I think we should just use your other CL.

OK, I will work on the other CL.

[commit-bot: I haz the power, 2017-04-24 12:52:02]

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

[commit-bot: I haz the power, 2017-04-24 12:52:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)