[Telemetry] Replace RunStressMemorey with RunPageInteraction

[Telemetry] Replace RunStressMemorey with RunPageInteraction

BUG=418375
Committed: https://crrev.com/7eae1996f473949a11f3ba6200056bc0c2db3d6f
Cr-Commit-Position: refs/heads/master@{#298883}

[nednguyen, 2014-10-06 20:55:30]

nednguyen@google.com changed reviewers:
+ dtu@chromium.org, tonyg@chromium.org

[tonyg, 2014-10-06 21:06:37]

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25.py
File tools/perf/page_sets/top_25.py (right):

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
tools/perf/page_sets/top_25.py:48: def RunPageInteractions(self, action_runner):
This was a nice, general purpose page set. But now we're closely coupling it to
the memory measurement's script. Is there another way we can preserve the
generality of page sets like this?

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
tools/perf/page_sets/top_25.py:134: def RunSmoothness(self, action_runner):
Case-in point, I'm not sure how we're going to handle these two different sets
of actions with this approach.

[nednguyen, 2014-10-06 21:40:40]

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

[nednguyen, 2014-10-06 21:40:41]

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25.py
File tools/perf/page_sets/top_25.py (right):

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
tools/perf/page_sets/top_25.py:48: def RunPageInteractions(self, action_runner):
On 2014/10/06 21:06:36, tonyg wrote:
> This was a nice, general purpose page set. But now we're closely coupling it
to
> the memory measurement's script. Is there another way we can preserve the
> generality of page sets like this?

The coupling already exists with these RunStressMemory function that can be run
if we use memory measurement. 

To me, the real bug that causes the coupling is that we have the mindset "write
a user story A for measurement Y", whereas the better model should be: user
stories describe what user actually do in the real world, and we take various
measurements around each of them.

Even though the URLs are the same, the user stories in this page set are
actually different for memory vs smoothness. This movement will only reduce the
coupling, make the page set *more* general since it allows other measurement to
run user stories with actions which once were only issued in 'RunStressMemory".

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
tools/perf/page_sets/top_25.py:134: def RunSmoothness(self, action_runner):
On 2014/10/06 21:06:36, tonyg wrote:
> Case-in point, I'm not sure how we're going to handle these two different sets
> of actions with this approach.

In the short term, we will keep the general top_25 for smoothness, repaint ...
and other metrics that are already migrated to timeline based measurement. For
memory, we can make a top_25_memory_intensive page set. 

Once we move memory to timeline based measurement, then we can consider either
change memory benchmark to use the top_25 that other measurements use, or extend
the pages in top_25 to include memory intensive scenarios.

[nduca, 2014-10-07 17:46:48]

some rambling thoughts

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25.py
File tools/perf/page_sets/top_25.py (right):

https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
tools/perf/page_sets/top_25.py:48: def RunPageInteractions(self, action_runner):
backin gup... maybe the thing to think of here is that a user story has a single
run method... you call run on it and it runs the story. start to finish.

so, a user story set is a list of many stories. some scroll. some just navigate.
some just open and close gmail.

pagesets are just sets of user stories about web pages.


right now we have "one page can has many stories in it" --- thats the core thing
I think we want to do away with. Rather, if you want the same URL to be
scrollable and be able to open/close discard, then make it have two user
stories.


some measurements make no sense on some kinds of stories. e.g. there's no point
in calculating memory levels on smoothness. so, then you need a mechanism to
tell if a story is useful to a measurement. i think that's whats being lost in
this refactoring --- ned, how does memory measurement's canRunForPage know that
a page stresses memory?

[nednguyen, 2014-10-07 17:59:07]

On 2014/10/07 17:46:48, nduca wrote:
> some rambling thoughts
> 
>
https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25.py
> File tools/perf/page_sets/top_25.py (right):
> 
>
https://codereview.chromium.org/632013002/diff/1/tools/perf/page_sets/top_25....
> tools/perf/page_sets/top_25.py:48: def RunPageInteractions(self,
action_runner):
> backin gup... maybe the thing to think of here is that a user story has a
single
> run method... you call run on it and it runs the story. start to finish.
> 
> so, a user story set is a list of many stories. some scroll. some just
navigate.
> some just open and close gmail.
> 
> pagesets are just sets of user stories about web pages.
> 
> 
> right now we have "one page can has many stories in it" --- thats the core
thing
> I think we want to do away with. Rather, if you want the same URL to be
> scrollable and be able to open/close discard, then make it have two user
> stories.
> 
> 
> some measurements make no sense on some kinds of stories. e.g. there's no
point
> in calculating memory levels on smoothness. so, then you need a mechanism to
> tell if a story is useful to a measurement. i think that's whats being lost in
> this refactoring --- ned, how does memory measurement's canRunForPage know
that
> a page stresses memory?

Chatted offline. I think we can solve this problem with a tagging mechanism. 
UserStory.__init__(tags)

def  Memory::CanRunForPage(page):
  return page.tags contains 'stress_memory'

[nednguyen, 2014-10-07 21:37:55]

Patchset #2 (id:20001) has been deleted

[nednguyen, 2014-10-07 21:38:47]

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

[ernstm, 2014-10-07 22:36:19]

On 2014/10/07 21:38:48, nednguyen wrote:

Why do we need the tagging mechanism? If we split page sets such that a page set
only contains interactions of a certain type (scrolling, repainting,
stress_memory), then all we need to do is pair measurements with suitable page
sets in benchmarks. For this particular case, we should create a
top_25_stress_memory page set and later top_25_scroll and top_25_repaint.

[nednguyen, 2014-10-07 22:44:09]

On 2014/10/07 22:36:19, ernstm wrote:
> On 2014/10/07 21:38:48, nednguyen wrote:
> 
> Why do we need the tagging mechanism? If we split page sets such that a page
set
> only contains interactions of a certain type (scrolling, repainting,
> stress_memory), then all we need to do is pair measurements with suitable page
> sets in benchmarks. For this particular case, we should create a
> top_25_stress_memory page set and later top_25_scroll and top_25_repaint.

Tony & Nat's concern here is we want some sane way for the measurement to tells
whether it can run against some pages. If someone makes a mistake and pair
memory measurement with top_25_smoothness, we need a way to tell them "there is
no thing interesting about running memory measurement on these pages". 

I think tagging is the middle ground that allows both the sanity check mentioned
and ease of reusing a page across multiple measurements (e.g just specify tags =
['smoothness', 'repaint', 'v8',...])

[ernstm, 2014-10-07 22:55:08]

On 2014/10/07 22:44:09, nednguyen wrote:
> On 2014/10/07 22:36:19, ernstm wrote:
> > On 2014/10/07 21:38:48, nednguyen wrote:
> > 
> > Why do we need the tagging mechanism? If we split page sets such that a page
> set
> > only contains interactions of a certain type (scrolling, repainting,
> > stress_memory), then all we need to do is pair measurements with suitable
page
> > sets in benchmarks. For this particular case, we should create a
> > top_25_stress_memory page set and later top_25_scroll and top_25_repaint.
> 
> Tony & Nat's concern here is we want some sane way for the measurement to
tells
> whether it can run against some pages. If someone makes a mistake and pair
> memory measurement with top_25_smoothness, we need a way to tell them "there
is
> no thing interesting about running memory measurement on these pages". 
> 
> I think tagging is the middle ground that allows both the sanity check
mentioned
> and ease of reusing a page across multiple measurements (e.g just specify tags
=
> ['smoothness', 'repaint', 'v8',...])

I think this creates an unnecessary coupling between page sets and measurements.
We decided that we want page sets to be purely about the interactions / user
stories. We should allow coupling those with any measurement. In the worst case,
you are not able to measure anything interesting if you run a measurement with
an interaction that doesn't do anything that's relevant for it.

[nednguyen, 2014-10-08 22:37:27]

On 2014/10/07 22:55:08, ernstm wrote:
> On 2014/10/07 22:44:09, nednguyen wrote:
> > On 2014/10/07 22:36:19, ernstm wrote:
> > > On 2014/10/07 21:38:48, nednguyen wrote:
> > > 
> > > Why do we need the tagging mechanism? If we split page sets such that a
page
> > set
> > > only contains interactions of a certain type (scrolling, repainting,
> > > stress_memory), then all we need to do is pair measurements with suitable
> page
> > > sets in benchmarks. For this particular case, we should create a
> > > top_25_stress_memory page set and later top_25_scroll and top_25_repaint.
> > 
> > Tony & Nat's concern here is we want some sane way for the measurement to
> tells
> > whether it can run against some pages. If someone makes a mistake and pair
> > memory measurement with top_25_smoothness, we need a way to tell them "there
> is
> > no thing interesting about running memory measurement on these pages". 
> > 
> > I think tagging is the middle ground that allows both the sanity check
> mentioned
> > and ease of reusing a page across multiple measurements (e.g just specify
tags
> =
> > ['smoothness', 'repaint', 'v8',...])
> 
> I think this creates an unnecessary coupling between page sets and
measurements.
> We decided that we want page sets to be purely about the interactions / user
> stories. We should allow coupling those with any measurement. In the worst
case,
> you are not able to measure anything interesting if you run a measurement with
> an interaction that doesn't do anything that's relevant for it.

Remove the tagging after we reached consensus when discussed offline. PTAL.

[tonyg, 2014-10-09 01:07:44]

lgtm

I like this new approach a lot. Thanks!

https://codereview.chromium.org/632013002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/632013002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:63: def
CheckNoMixedInBetweenLegacyRunMethodsAndRunPageInteractions(
Love these sorts of tests, thanks :)

[nednguyen, 2014-10-09 01:36:53]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-09 01:40:33]

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

[commit-bot: I haz the power, 2014-10-09 03:13:45]

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

[commit-bot: I haz the power, 2014-10-09 03:13:46]

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

[nednguyen, 2014-10-09 04:13:27]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-09 04:14:42]

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

[commit-bot: I haz the power, 2014-10-09 05:03:34]

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

[commit-bot: I haz the power, 2014-10-09 05:03:35]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nednguyen, 2014-10-09 14:50:29]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-09 14:51:29]

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

[commit-bot: I haz the power, 2014-10-09 15:53:52]

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

[commit-bot: I haz the power, 2014-10-09 15:53:54]

Failed to commit the patch.

[commit-bot: I haz the power, 2014-10-09 15:54:56]

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

[nednguyen, 2014-10-09 15:55:00]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-09 15:57:00]

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

[commit-bot: I haz the power, 2014-10-09 15:59:39]

Committed patchset #5 (id:100001) as 216c11762d508ac9530f0e0aa6800805e7df0bee

[commit-bot: I haz the power, 2014-10-09 16:00:50]

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

[chrishenry, 2014-10-09 16:30:03]

Looks nice, lgtm. Very much liking naming the page not based on measurement but
on what it's doing (the actual interactions).

That said, please address the naming comment below.

https://codereview.chromium.org/632013002/diff/100001/tools/perf/measurements...
File tools/perf/measurements/memory.py (right):

https://codereview.chromium.org/632013002/diff/100001/tools/perf/measurements...
tools/perf/measurements/memory.py:11: super(Memory,
self).__init__('RunPageInteractions')
Eh, can we not name it RunPageInteractions? This seems to be adding work to
telemetry v2 refactoring bucket (Page -> UserStory rename). Run/RunInteractions
will do right?

[nednguyen, 2014-10-09 16:41:45]

On 2014/10/09 16:30:03, chrishenry wrote:
> Looks nice, lgtm. Very much liking naming the page not based on measurement
but
> on what it's doing (the actual interactions).
> 
> That said, please address the naming comment below.
> 
>
https://codereview.chromium.org/632013002/diff/100001/tools/perf/measurements...
> File tools/perf/measurements/memory.py (right):
> 
>
https://codereview.chromium.org/632013002/diff/100001/tools/perf/measurements...
> tools/perf/measurements/memory.py:11: super(Memory,
> self).__init__('RunPageInteractions')
> Eh, can we not name it RunPageInteractions? This seems to be adding work to
> telemetry v2 refactoring bucket (Page -> UserStory rename).
Run/RunInteractions
> will do right?

The distinction between RunNavigateSteps & RunPageInteractions for legacy hooks
to work gonna be around a while. So:

def UserStory::Run():
...

def Page::Run(page_test):
   page_test.WillRunNavigate()
   self.RunNavigateSteps()
   page_test.DidRunNavigate()
   page_test.WillRunTest() # Probably should merge this with DidRunNavigate
   self.RunPageInteractions()
   ...

[chromium-reviews, 2014-10-09 17:31:25]

On Thu, Oct 9, 2014 at 9:41 AM, <nednguyen@google.com> wrote:

> On 2014/10/09 16:30:03, chrishenry wrote:
>
>> Looks nice, lgtm. Very much liking naming the page not based on
>> measurement
>>
> but
>
>> on what it's doing (the actual interactions).
>>
>
>  That said, please address the naming comment below.
>>
>
>
> https://codereview.chromium.org/632013002/diff/100001/
> tools/perf/measurements/memory.py
>
>> File tools/perf/measurements/memory.py (right):
>>
>
>
> https://codereview.chromium.org/632013002/diff/100001/
> tools/perf/measurements/memory.py#newcode11
>
>> tools/perf/measurements/memory.py:11: super(Memory,
>> self).__init__('RunPageInteractions')
>> Eh, can we not name it RunPageInteractions? This seems to be adding work
>> to
>> telemetry v2 refactoring bucket (Page -> UserStory rename).
>>
> Run/RunInteractions
>
>> will do right?
>>
>
> The distinction between RunNavigateSteps & RunPageInteractions for legacy
> hooks
> to work gonna be around a while. So:
>
> def UserStory::Run():
> ...
>
> def Page::Run(page_test):
>    page_test.WillRunNavigate()
>    self.RunNavigateSteps()
>    page_test.DidRunNavigate()
>    page_test.WillRunTest() # Probably should merge this with DidRunNavigate
>    self.RunPageInteractions()
>    ...
>

So non-Page UserStory only works with TimelineBasedMeasurement and not
legacy measurements?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nednguyen, 2014-10-09 17:47:44]

On 2014/10/09 17:31:25, chromium-reviews wrote:
> On Thu, Oct 9, 2014 at 9:41 AM, <mailto:nednguyen@google.com> wrote:
> 
> > On 2014/10/09 16:30:03, chrishenry wrote:
> >
> >> Looks nice, lgtm. Very much liking naming the page not based on
> >> measurement
> >>
> > but
> >
> >> on what it's doing (the actual interactions).
> >>
> >
> >  That said, please address the naming comment below.
> >>
> >
> >
> > https://codereview.chromium.org/632013002/diff/100001/
> > tools/perf/measurements/memory.py
> >
> >> File tools/perf/measurements/memory.py (right):
> >>
> >
> >
> > https://codereview.chromium.org/632013002/diff/100001/
> > tools/perf/measurements/memory.py#newcode11
> >
> >> tools/perf/measurements/memory.py:11: super(Memory,
> >> self).__init__('RunPageInteractions')
> >> Eh, can we not name it RunPageInteractions? This seems to be adding work
> >> to
> >> telemetry v2 refactoring bucket (Page -> UserStory rename).
> >>
> > Run/RunInteractions
> >
> >> will do right?
> >>
> >
> > The distinction between RunNavigateSteps & RunPageInteractions for legacy
> > hooks
> > to work gonna be around a while. So:
> >
> > def UserStory::Run():
> > ...
> >
> > def Page::Run(page_test):
> >    page_test.WillRunNavigate()
> >    self.RunNavigateSteps()
> >    page_test.DidRunNavigate()
> >    page_test.WillRunTest() # Probably should merge this with DidRunNavigate
> >    self.RunPageInteractions()
> >    ...
> >
> 
> So non-Page UserStory only works with TimelineBasedMeasurement and not
> legacy measurements?

Yes, we will live with that for a while. Migrating the act of turning on some
profilers from DidRunNavigateSteps to WillRunUserStory() may cause some
regressions, so let have all eyes focus on it after telemetry v2 is in a better
shape.

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-10-09 18:00:46]

On Thu, Oct 9, 2014 at 10:47 AM, nednguyen via telemetry <
telemetry@chromium.org> wrote:

> On 2014/10/09 17:31:25, chromium-reviews wrote:
>
>  On Thu, Oct 9, 2014 at 9:41 AM, <mailto:nednguyen@google.com> wrote:
>>
>
>  > On 2014/10/09 16:30:03, chrishenry wrote:
>> >
>> >> Looks nice, lgtm. Very much liking naming the page not based on
>> >> measurement
>> >>
>> > but
>> >
>> >> on what it's doing (the actual interactions).
>> >>
>> >
>> >  That said, please address the naming comment below.
>> >>
>> >
>> >
>> > https://codereview.chromium.org/632013002/diff/100001/
>> > tools/perf/measurements/memory.py
>> >
>> >> File tools/perf/measurements/memory.py (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/632013002/diff/100001/
>> > tools/perf/measurements/memory.py#newcode11
>> >
>> >> tools/perf/measurements/memory.py:11: super(Memory,
>> >> self).__init__('RunPageInteractions')
>> >> Eh, can we not name it RunPageInteractions? This seems to be adding
>> work
>> >> to
>> >> telemetry v2 refactoring bucket (Page -> UserStory rename).
>> >>
>> > Run/RunInteractions
>> >
>> >> will do right?
>> >>
>> >
>> > The distinction between RunNavigateSteps & RunPageInteractions for
>> legacy
>> > hooks
>> > to work gonna be around a while. So:
>> >
>> > def UserStory::Run():
>> > ...
>> >
>> > def Page::Run(page_test):
>> >    page_test.WillRunNavigate()
>> >    self.RunNavigateSteps()
>> >    page_test.DidRunNavigate()
>> >    page_test.WillRunTest() # Probably should merge this with
>> DidRunNavigate
>> >    self.RunPageInteractions()
>> >    ...
>> >
>>
>
>  So non-Page UserStory only works with TimelineBasedMeasurement and not
>> legacy measurements?
>>
>
> Yes, we will live with that for a while. Migrating the act of turning on
> some
> profilers from DidRunNavigateSteps to WillRunUserStory() may cause some
> regressions, so let have all eyes focus on it after telemetry v2 is in a
> better
> shape.


Ok, sounds reasonable.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.