Add benchmark that imitates news reading.

Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq

Committed: https://crrev.com/5b1847df818e5be4592e9a6ac4832350bb663023
Cr-Commit-Position: refs/heads/master@{#405212}

[ulan, 2016-07-04 13:41:15]

Description was changed from

==========
WIP: benchmarks emulating news reading workflow.


1. Open new web page (currently reddit and washington post)
2. Click a link.
3. Wait for some time.
4. Scroll a page.
5. Wait for some time.
6. Go back and repeat from step 2.

BUG=
==========

to

==========
WIP: benchmarks emulating news reading workflow.


1. Open new web page (currently reddit and washington post)
2. Click a link.
3. Wait for some time.
4. Scroll a page.
5. Wait for some time.
6. Go back and repeat from step 2.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-04 18:10:11]

Description was changed from

==========
WIP: benchmarks emulating news reading workflow.


1. Open new web page (currently reddit and washington post)
2. Click a link.
3. Wait for some time.
4. Scroll a page.
5. Wait for some time.
6. Go back and repeat from step 2.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
WIP: benchmarks emulating news reading workflow.


1. Open new web page
2. Click a link.
3. Wait for some time.
4. Scroll a page.
5. Wait for some time.
6. Go back and repeat from step 2.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-05 13:25:30]

Description was changed from

==========
WIP: benchmarks emulating news reading workflow.


1. Open new web page
2. Click a link.
3. Wait for some time.
4. Scroll a page.
5. Wait for some time.
6. Go back and repeat from step 2.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that emulate news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of website crash with OOM on svelte device.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-05 13:27:17]

Description was changed from

==========
Add benchmark that emulate news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of website crash with OOM on svelte device.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that emulates news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of website crash with OOM on svelte device.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-05 13:30:24]

Description was changed from

==========
Add benchmark that emulates news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of website crash with OOM on svelte device.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that emulates news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-05 13:39:34]

ulan@chromium.org changed reviewers:
+ hpayer@chromium.org, petrcermak@chromium.org, primiano@chromium.org

[ulan, 2016-07-05 13:39:35]

petrcermak@, primiano@, wdyt about this benchmark?

I can add it as standalone or as part of system health if you think that it
would interesting for system health.

If it looks good, I will record the pages and add desktop versions.

[ulan, 2016-07-05 13:39:47]

Patchset #1 (id:1) has been deleted

[ulan, 2016-07-05 13:39:54]

Patchset #1 (id:20001) has been deleted

[ulan, 2016-07-07 14:15:58]

Description was changed from

==========
Add benchmark that emulates news reading workflow.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitate news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-07 14:16:06]

Description was changed from

==========
Add benchmark that imitate news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[petrcermak, 2016-07-07 14:55:55]

petrcermak@chromium.org changed reviewers:
+ nednguyen@google.com, sullivan@chromium.org

[petrcermak, 2016-07-07 14:55:56]

Looks good overall. As I explained in my inline comments, I think that the
stories should be added to the system_health folder straightaway.

Please add a bug to the patch description. If you don't have one, please use
589726. In fact, even if you have your own bug, please add 589726 to the BUG=
line.

+sullivan: Will the infrastructure be able to handle the extra benchmark?

+nednguyen: FYI (System Health Stories).

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:17: _MEMORY_STATS_RE =
re.compile(r'renderer_processes')
You can replace this by |'renderer_processes' in value.name|. No need for a
regex.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:21:
r'_(idle_deadline_overrun|percentage_idle|outside_idle)')
Should this only match the end of the name ("$" at the end of the regular
expression)?

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:23: r'v8-gc-full-mark-compactor_|'
Let's use the power of regular expressions when you're using them ;-)

r'v8-gc-((full|latency|memory)-mark-compactor|incremental-(finalize|step)|scavenger|total)_'

I suppose the inner simplifications might make the regex too unreadable, so the
following might be better (your call):

r'v8-gc-(full-mark-compactor|latency-mark-compactor|memory-mark-compactor|incremental-finalize|incremental-step|scavenger|total)_'

Also, shouldn't this only match the beginning of the name ("^" at the beginning
of the regular expression)?

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:33: """ Base class for news browsing benchmarks.
nit: Remove space after quotes

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:72: not
_IGNORED_MEMORY_STATS_RE.search(value.name))
You have this in both statements. I suggest you factor it out:

if _IGNORED_MEMORY_STATS_RE.search(value.name):
  return False
if 'memory:chrome' in value.name:
  return bool(_MEMORY_STATS_RE.search(value.name))
else:
  return bool(_V8_GC_HIGH_LEVEL_STATS_RE.search(value.name))

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/page_sets/n...
File tools/perf/page_sets/news_stories.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/page_sets/n...
tools/perf/page_sets/news_stories.py:18: class _NewsStory(page.Page):
I can see that this class is modelled very closely after SinglePageStory. My
suggestion is that you:

1) Move this file to tools/perf/page_sets/system_health/browsing_stories.py.
2) Do the same for https://codereview.chromium.org/2124823002/ (merge the
stories into browsing_stories).
3) Change single_page_stories.py to export the SinglePageStory subclass (remove
underscore from name) so that you can re-use the code.
4) Re-name the stories to 'browse:news:XYZ' and 'browse:media:XYZ' to be in line
with the remaining SH stories.

Please do NOT add the stories to the SystemHealthStorySet just yet because your
stories rely on periodic dumps while SH stories use explicit dumps, so we need
to figure out how to make this work for everyone.

How does that sound?

[ulan, 2016-07-07 17:34:38]

Thank you. I address local comments and will upload a separate patch set that
moves the files.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:17: _MEMORY_STATS_RE =
re.compile(r'renderer_processes')
On 2016/07/07 14:55:55, petrcermak wrote:
> You can replace this by |'renderer_processes' in value.name|. No need for a
> regex.

Done.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:21:
r'_(idle_deadline_overrun|percentage_idle|outside_idle)')
On 2016/07/07 14:55:56, petrcermak wrote:
> Should this only match the end of the name ("$" at the end of the regular
> expression)?

We can have outside_idle_(max|sum|min...).

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:23: r'v8-gc-full-mark-compactor_|'
On 2016/07/07 14:55:55, petrcermak wrote:
> Let's use the power of regular expressions when you're using them ;-)
> 
>
r'v8-gc-((full|latency|memory)-mark-compactor|incremental-(finalize|step)|scavenger|total)_'
> 
> I suppose the inner simplifications might make the regex too unreadable, so
the
> following might be better (your call):
> 
>
r'v8-gc-(full-mark-compactor|latency-mark-compactor|memory-mark-compactor|incremental-finalize|incremental-step|scavenger|total)_'
> 
> Also, shouldn't this only match the beginning of the name ("^" at the
beginning
> of the regular expression)?

Thanks, I chose the second option as it seems more readable to me.
And added "^".

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:33: """ Base class for news browsing benchmarks.
On 2016/07/07 14:55:55, petrcermak wrote:
> nit: Remove space after quotes

Done.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:72: not
_IGNORED_MEMORY_STATS_RE.search(value.name))
On 2016/07/07 14:55:55, petrcermak wrote:
> You have this in both statements. I suggest you factor it out:
> 
> if _IGNORED_MEMORY_STATS_RE.search(value.name):
>   return False
> if 'memory:chrome' in value.name:
>   return bool(_MEMORY_STATS_RE.search(value.name))
> else:
>   return bool(_V8_GC_HIGH_LEVEL_STATS_RE.search(value.name))

Note that the branch below has ignored V8 stats, not memory stats.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/page_sets/n...
File tools/perf/page_sets/news_stories.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/page_sets/n...
tools/perf/page_sets/news_stories.py:18: class _NewsStory(page.Page):
On 2016/07/07 14:55:56, petrcermak wrote:
> I can see that this class is modelled very closely after SinglePageStory. My
> suggestion is that you:
> 
> 1) Move this file to tools/perf/page_sets/system_health/browsing_stories.py.
> 2) Do the same for https://codereview.chromium.org/2124823002/ (merge the
> stories into browsing_stories).
> 3) Change single_page_stories.py to export the SinglePageStory subclass
(remove
> underscore from name) so that you can re-use the code.
> 4) Re-name the stories to 'browse:news:XYZ' and 'browse:media:XYZ' to be in
line
> with the remaining SH stories.
> 
> Please do NOT add the stories to the SystemHealthStorySet just yet because
your
> stories rely on periodic dumps while SH stories use explicit dumps, so we need
> to figure out how to make this work for everyone.
> 
> How does that sound?

Sounds good. I will do this change tomorrow.

[petrcermak, 2016-07-08 09:17:53]

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:17: _MEMORY_STATS_RE =
re.compile(r'renderer_processes')
On 2016/07/07 17:34:38, ulan wrote:
> On 2016/07/07 14:55:55, petrcermak wrote:
> > You can replace this by |'renderer_processes' in value.name|. No need for a
> > regex.
> 
> Done.

There's no point in having the regular expression anymore ;-)

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:21:
r'_(idle_deadline_overrun|percentage_idle|outside_idle)')
On 2016/07/07 17:34:38, ulan wrote:
> On 2016/07/07 14:55:56, petrcermak wrote:
> > Should this only match the end of the name ("$" at the end of the regular
> > expression)?
> 
> We can have outside_idle_(max|sum|min...).

Acknowledged.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:23: r'v8-gc-full-mark-compactor_|'
On 2016/07/07 17:34:38, ulan wrote:
> On 2016/07/07 14:55:55, petrcermak wrote:
> > Let's use the power of regular expressions when you're using them ;-)
> > 
> >
>
r'v8-gc-((full|latency|memory)-mark-compactor|incremental-(finalize|step)|scavenger|total)_'
> > 
> > I suppose the inner simplifications might make the regex too unreadable, so
> the
> > following might be better (your call):
> > 
> >
>
r'v8-gc-(full-mark-compactor|latency-mark-compactor|memory-mark-compactor|incremental-finalize|incremental-step|scavenger|total)_'
> > 
> > Also, shouldn't this only match the beginning of the name ("^" at the
> beginning
> > of the regular expression)?
> 
> Thanks, I chose the second option as it seems more readable to me.
> And added "^".

Acknowledged.

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:72: not
_IGNORED_MEMORY_STATS_RE.search(value.name))
On 2016/07/07 17:34:38, ulan wrote:
> On 2016/07/07 14:55:55, petrcermak wrote:
> > You have this in both statements. I suggest you factor it out:
> > 
> > if _IGNORED_MEMORY_STATS_RE.search(value.name):
> >   return False
> > if 'memory:chrome' in value.name:
> >   return bool(_MEMORY_STATS_RE.search(value.name))
> > else:
> >   return bool(_V8_GC_HIGH_LEVEL_STATS_RE.search(value.name))
> 
> Note that the branch below has ignored V8 stats, not memory stats.

Ack. Sorry, I didn't realize that.

[ulan, 2016-07-08 09:50:24]

I moved the news browsing stories to system health. PTAL.

Instead of using SinglePageStory I extracted common parts to SystemHealthStory
(mainly because NewsBrowsingStory is not a single page story).

I also moved platform constants into a separate module, so that we can refer to
them as platforms.DESKTOP, platforms.MOBILE, etc.

[ulan, 2016-07-08 09:50:57]

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/180001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:17: _MEMORY_STATS_RE =
re.compile(r'renderer_processes')
On 2016/07/08 09:17:53, petrcermak wrote:
> On 2016/07/07 17:34:38, ulan wrote:
> > On 2016/07/07 14:55:55, petrcermak wrote:
> > > You can replace this by |'renderer_processes' in value.name|. No need for
a
> > > regex.
> > 
> > Done.
> 
> There's no point in having the regular expression anymore ;-)

Done.

[petrcermak, 2016-07-08 10:04:32]

I really like this :-)

Not sure if it was deliberate, but you haven't included your new page set in the
patch (forgotten `git add`)?

Thanks a lot!
Petr

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:7: ALL_PLATFORMS =
frozenset({DESKTOP, MOBILE})
supernit: Can you please add a blank line above ALL_PLATFORMS (to separate the 2
elements and 4 subsets)?

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/system_health_stories.py (right):

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/system_health_stories.py:6: 
Remove blank line and sort the imports alphabetically (platforms before
single_page_stories).

[ulan, 2016-07-08 10:34:22]

I uploaded the missing file :) sorry about that.

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:7: ALL_PLATFORMS =
frozenset({DESKTOP, MOBILE})
On 2016/07/08 10:04:31, petrcermak wrote:
> supernit: Can you please add a blank line above ALL_PLATFORMS (to separate the
2
> elements and 4 subsets)?

Done.

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/system_health_stories.py (right):

https://codereview.chromium.org/2118293002/diff/220001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/system_health_stories.py:6: 
On 2016/07/08 10:04:31, petrcermak wrote:
> Remove blank line and sort the imports alphabetically (platforms before
> single_page_stories).

Done.

[ulan, 2016-07-08 10:50:55]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
This one is not calling the SystemHealthStory.RunPageInteraction, which is not
nice.

I could introduce _RunPageInteractions, which is called by
SystemHealthStory.RunPageInteraction() after loading the document and override 
_RunPageInteractions here. wdyt?

[petrcermak, 2016-07-08 12:46:03]

Looks great overall. Here are a few more comments.

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:29: def _Item(self,
index):
nit: I think that this should be called something like _BuildItemSelector

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:30: return
'document.querySelectorAll(\'%s\')[%d]' % (
nit: I suggest you use " inside the string to avoid the backslashes ;-)

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:33: def
_WaitForItem(self, action_runner, index):
Do you expect any subclass to override _WaitForItem, _ClickItem or
_WaitForNavigation? If not, then I suggest you just inline them into
_NavigateToNewsItem (which I suggested you move to this class).

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
On 2016/07/08 10:50:55, ulan wrote:
> This one is not calling the SystemHealthStory.RunPageInteraction, which is not
> nice.
> 
> I could introduce _RunPageInteractions, which is called by
> SystemHealthStory.RunPageInteraction() after loading the document and override

> _RunPageInteractions here. wdyt?

I think that the best solution would be to move the for loop into
_DidLoadDocument and not override RunPageInteractions at all. How does that
sound?

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:73: def
_NavigateToNewsItem(self, action_runner, index):
Why is this method not defined on _BrowsingStory? It doesn't seem to be news
related.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:74:
self._WaitForItem(action_runner, index)
These methods build the item selector twice unnecessarily. Plus they both have
to know how selectors are created. I suggest you build the selector first and
then pass it to them:

item_selector = 'document.querySelectorAll("%s")[%d]' % (self.ITEM_SELECTOR,
index)
action_runner.WaitForElement(element_function=item_selector)
action_runner.ClickElement(element_function=item_selector)
if not self.IS_SINGLE_PAGE_APP:
  action_runner.WaitForNavigate()

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:193: 
nit: please add a second blank line here (there should be two blank lines
between top-level statements).

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:194: def
IterAllStoryClasses():
This will also return the _NewsBrowsingStory abstract class, which is
undesirable. We'll need to figure out a more scalable approach so that we don't
need to define this kind of function in every single file in system_health/.
Therefore, I suggest you remove this method now and we add it later when we
actually add the stories to the SH set.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:199: def
IterAllNewsBrowsingStoryClasses():
I'd make this private for the time being (leading underscore).

[nednguyen, 2016-07-08 12:50:47]

On 2016/07/08 12:46:03, petrcermak wrote:
> Looks great overall. Here are a few more comments.
> 
> Thanks,
> Petr
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/browsing_stories.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:29: def _Item(self,
> index):
> nit: I think that this should be called something like _BuildItemSelector
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:30: return
> 'document.querySelectorAll(\'%s\')[%d]' % (
> nit: I suggest you use " inside the string to avoid the backslashes ;-)
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:33: def
> _WaitForItem(self, action_runner, index):
> Do you expect any subclass to override _WaitForItem, _ClickItem or
> _WaitForNavigation? If not, then I suggest you just inline them into
> _NavigateToNewsItem (which I suggested you move to this class).
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:64: def
> RunPageInteractions(self, action_runner):
> On 2016/07/08 10:50:55, ulan wrote:
> > This one is not calling the SystemHealthStory.RunPageInteraction, which is
not
> > nice.
> > 
> > I could introduce _RunPageInteractions, which is called by
> > SystemHealthStory.RunPageInteraction() after loading the document and
override
> 
> > _RunPageInteractions here. wdyt?
> 
> I think that the best solution would be to move the for loop into
> _DidLoadDocument and not override RunPageInteractions at all. How does that
> sound?
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:73: def
> _NavigateToNewsItem(self, action_runner, index):
> Why is this method not defined on _BrowsingStory? It doesn't seem to be news
> related.
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:74:
> self._WaitForItem(action_runner, index)
> These methods build the item selector twice unnecessarily. Plus they both have
> to know how selectors are created. I suggest you build the selector first and
> then pass it to them:
> 
> item_selector = 'document.querySelectorAll("%s")[%d]' % (self.ITEM_SELECTOR,
> index)
> action_runner.WaitForElement(element_function=item_selector)
> action_runner.ClickElement(element_function=item_selector)
> if not self.IS_SINGLE_PAGE_APP:
>   action_runner.WaitForNavigate()
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:193: 
> nit: please add a second blank line here (there should be two blank lines
> between top-level statements).
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:194: def
> IterAllStoryClasses():
> This will also return the _NewsBrowsingStory abstract class, which is
> undesirable. We'll need to figure out a more scalable approach so that we
don't
> need to define this kind of function in every single file in system_health/.
> Therefore, I suggest you remove this method now and we add it later when we
> actually add the stories to the SH set.
> 
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:199: def
> IterAllNewsBrowsingStoryClasses():
> I'd make this private for the time being (leading underscore).

Sorry for silly question but which row in
https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw...
does these stories fit into? It's ok if there is none, but I think we should
mark this great work in the system health stories database :-)

[nednguyen, 2016-07-08 12:54:42]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
Looks like we don't cover any non-US news pages here. I think we can punt on
reddit & hacknews for some international news pages?

[sullivan, 2016-07-08 12:58:50]

On 2016/07/08 12:50:47, nednguyen (ooo til 7-11) wrote:
> On 2016/07/08 12:46:03, petrcermak wrote:
> > Looks great overall. Here are a few more comments.
> > 
> > Thanks,
> > Petr
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > File tools/perf/page_sets/system_health/browsing_stories.py (right):
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:29: def _Item(self,
> > index):
> > nit: I think that this should be called something like _BuildItemSelector
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:30: return
> > 'document.querySelectorAll(\'%s\')[%d]' % (
> > nit: I suggest you use " inside the string to avoid the backslashes ;-)
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:33: def
> > _WaitForItem(self, action_runner, index):
> > Do you expect any subclass to override _WaitForItem, _ClickItem or
> > _WaitForNavigation? If not, then I suggest you just inline them into
> > _NavigateToNewsItem (which I suggested you move to this class).
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:64: def
> > RunPageInteractions(self, action_runner):
> > On 2016/07/08 10:50:55, ulan wrote:
> > > This one is not calling the SystemHealthStory.RunPageInteraction, which is
> not
> > > nice.
> > > 
> > > I could introduce _RunPageInteractions, which is called by
> > > SystemHealthStory.RunPageInteraction() after loading the document and
> override
> > 
> > > _RunPageInteractions here. wdyt?
> > 
> > I think that the best solution would be to move the for loop into
> > _DidLoadDocument and not override RunPageInteractions at all. How does that
> > sound?
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:73: def
> > _NavigateToNewsItem(self, action_runner, index):
> > Why is this method not defined on _BrowsingStory? It doesn't seem to be news
> > related.
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:74:
> > self._WaitForItem(action_runner, index)
> > These methods build the item selector twice unnecessarily. Plus they both
have
> > to know how selectors are created. I suggest you build the selector first
and
> > then pass it to them:
> > 
> > item_selector = 'document.querySelectorAll("%s")[%d]' % (self.ITEM_SELECTOR,
> > index)
> > action_runner.WaitForElement(element_function=item_selector)
> > action_runner.ClickElement(element_function=item_selector)
> > if not self.IS_SINGLE_PAGE_APP:
> >   action_runner.WaitForNavigate()
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:193: 
> > nit: please add a second blank line here (there should be two blank lines
> > between top-level statements).
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:194: def
> > IterAllStoryClasses():
> > This will also return the _NewsBrowsingStory abstract class, which is
> > undesirable. We'll need to figure out a more scalable approach so that we
> don't
> > need to define this kind of function in every single file in system_health/.
> > Therefore, I suggest you remove this method now and we add it later when we
> > actually add the stories to the SH set.
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:199: def
> > IterAllNewsBrowsingStoryClasses():
> > I'd make this private for the time being (leading underscore).
> 
> Sorry for silly question but which row in
>
https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw...
> does these stories fit into? It's ok if there is none, but I think we should
> mark this great work in the system health stories database :-)

+1

Also what is the running time on linux perf trybots? Ideally new stories are
under 5 minutes.

[petrcermak, 2016-07-08 12:59:29]

On 2016/07/08 12:50:47, nednguyen (ooo til 7-11) wrote:
> Sorry for silly question but which row in
>
https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw...
> does these stories fit into? It's ok if there is none, but I think we should
> mark this great work in the system health stories database :-)

Very good question. I think it falls into '"Back" button' case under "Intra-tab
stories". It's basically the yellow "Back" column in
https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw....

I originally intended to do only a single navigation back, but now I think that
this (repeat back navigation) is better.

Ulan: Is there any chance we could align the "load:*" (single_page_stories.py)
and "browse:*" (browsing_stories.py) stories? It would be nice to have the same
sets for both so that we can compare "loading only" and "loading + reading". I
would start by making sure that there's a loading story for for every browsing
story. Note that this would also address Ned's point since there are non-US
pages in single_page_stories.py.

Thanks,
Petr

[Hannes Payer (out of office), 2016-07-08 13:52:55]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> Looks like we don't cover any non-US news pages here. I think we can punt on
> reddit & hacknews for some international news pages?
> 

Hacker news is nice because it goes to other webpages when you click on a link.
Kind of nice to have this behavior as well.

[ulan, 2016-07-08 13:57:57]

Thank you!

> Also what is the running time on linux perf trybots? Ideally new stories are
under 5 minutes.
Desktop set currently finishes in 10 minutes on linux (cnn and washingtonpost
take half of that time).
I can remove washingtonpost from desktop because desktop version is not wpa.

Mobile set finishes in 5 minutes on nexus 6p (cnn is currently disabled for
mobile due to OOMs).

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:29: def _Item(self,
index):
On 2016/07/08 12:46:03, petrcermak wrote:
> nit: I think that this should be called something like _BuildItemSelector

I inlined this function in NavigateToItem.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:30: return
'document.querySelectorAll(\'%s\')[%d]' % (
On 2016/07/08 12:46:03, petrcermak wrote:
> nit: I suggest you use " inside the string to avoid the backslashes ;-)

Done.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:33: def
_WaitForItem(self, action_runner, index):
On 2016/07/08 12:46:03, petrcermak wrote:
> Do you expect any subclass to override _WaitForItem, _ClickItem or
> _WaitForNavigation? If not, then I suggest you just inline them into
> _NavigateToNewsItem (which I suggested you move to this class).

Done. I thought that the media browsing benchmark needs WaitForItem and
ClickItem. But now I think that media browsing benchmark could also use higher
level NavigateToItem method.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
On 2016/07/08 12:46:03, petrcermak wrote:
> On 2016/07/08 10:50:55, ulan wrote:
> > This one is not calling the SystemHealthStory.RunPageInteraction, which is
not
> > nice.
> > 
> > I could introduce _RunPageInteractions, which is called by
> > SystemHealthStory.RunPageInteraction() after loading the document and
override
> 
> > _RunPageInteractions here. wdyt?
> 
> I think that the best solution would be to move the for loop into
> _DidLoadDocument and not override RunPageInteractions at all. How does that
> sound?

I am using _DidLoadDocument as a hook to do special actions like closing pop-up
(see WashingtonPostMobileStory) before the actual interactions.

If I move everything to _DidLoadDocument, I would need another hook that runs
before _DidLoadDocument but after loading the document.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:73: def
_NavigateToNewsItem(self, action_runner, index):
On 2016/07/08 12:46:03, petrcermak wrote:
> Why is this method not defined on _BrowsingStory? It doesn't seem to be news
> related.

Done.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:74:
self._WaitForItem(action_runner, index)
On 2016/07/08 12:46:03, petrcermak wrote:
> These methods build the item selector twice unnecessarily. Plus they both have
> to know how selectors are created. I suggest you build the selector first and
> then pass it to them:
> 
> item_selector = 'document.querySelectorAll("%s")[%d]' % (self.ITEM_SELECTOR,
> index)
> action_runner.WaitForElement(element_function=item_selector)
> action_runner.ClickElement(element_function=item_selector)
> if not self.IS_SINGLE_PAGE_APP:
>   action_runner.WaitForNavigate()

Done, but I left WaitForNavigation in separate function because it is used in
NavigateBack().

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> Looks like we don't cover any non-US news pages here. I think we can punt on
> reddit & hacknews for some international news pages?
> 

Petr suggested to sync load:news and browse:news.

Our motivation for browse:news pages that are not in load:news:
- Nytimes and hackernews have the largest V8 heaps in mobile browse:news
workload (except CNN which crashes with OOM).
- Washingtonpost is WPA.
- Facebook and twitter are super-popular pages.

Any strong opinion for removing one of these pages? Otherwise, I'll add these
pages to load:news.

Now pages in load:news that are not in browse:new: bbc, qq, sohu, wikipedia.

Should I move qq and sohu to browse:news?

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:193: 
On 2016/07/08 12:46:03, petrcermak wrote:
> nit: please add a second blank line here (there should be two blank lines
> between top-level statements).

Done.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:194: def
IterAllStoryClasses():
On 2016/07/08 12:46:03, petrcermak wrote:
> This will also return the _NewsBrowsingStory abstract class, which is
> undesirable. We'll need to figure out a more scalable approach so that we
don't
> need to define this kind of function in every single file in system_health/.
> Therefore, I suggest you remove this method now and we add it later when we
> actually add the stories to the SH set.

Done.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:199: def
IterAllNewsBrowsingStoryClasses():
On 2016/07/08 12:46:03, petrcermak wrote:
> I'd make this private for the time being (leading underscore).

Done.

[nednguyen, 2016-07-08 14:01:35]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 13:52:54, Hannes Payer wrote:
> On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > Looks like we don't cover any non-US news pages here. I think we can punt on
> > reddit & hacknews for some international news pages?
> > 
> 
> Hacker news is nice because it goes to other webpages when you click on a
link.
> Kind of nice to have this behavior as well. 

Ah, that's a good point. Those I think we should use Google news search for this
behavior because google news's page content has a lot more things than
hackernews.

[ulan, 2016-07-08 14:14:59]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[nednguyen, 2016-07-08 14:17:14]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 13:57:57, ulan wrote:
> On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > Looks like we don't cover any non-US news pages here. I think we can punt on
> > reddit & hacknews for some international news pages?
> > 
> 
> Petr suggested to sync load:news and browse:news.
> 
> Our motivation for browse:news pages that are not in load:news:
> - Nytimes and hackernews have the largest V8 heaps in mobile browse:news
> workload (except CNN which crashes with OOM).
> - Washingtonpost is WPA.
> - Facebook and twitter are super-popular pages.
> 
> Any strong opinion for removing one of these pages? Otherwise, I'll add these
> pages to load:news.
> 
> Now pages in load:news that are not in browse:new: bbc, qq, sohu, wikipedia.
> 
> Should I move qq and sohu to browse:news?
> 

Keeping reddit is fine.
I suggest we replace hacker news with google news search for the reason stated. 
For qq & sohu, you can just copy them here instead of removing them from
load:news. I can buy that we may be move all the loading stories to browsing
stories one day, but not now :-)

[petrcermak, 2016-07-08 15:06:28]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
On 2016/07/08 13:57:57, ulan wrote:
> On 2016/07/08 12:46:03, petrcermak wrote:
> > On 2016/07/08 10:50:55, ulan wrote:
> > > This one is not calling the SystemHealthStory.RunPageInteraction, which is
> not
> > > nice.
> > > 
> > > I could introduce _RunPageInteractions, which is called by
> > > SystemHealthStory.RunPageInteraction() after loading the document and
> override
> > 
> > > _RunPageInteractions here. wdyt?
> > 
> > I think that the best solution would be to move the for loop into
> > _DidLoadDocument and not override RunPageInteractions at all. How does that
> > sound?
> 
> I am using _DidLoadDocument as a hook to do special actions like closing
pop-up
> (see WashingtonPostMobileStory) before the actual interactions.
> 
> If I move everything to _DidLoadDocument, I would need another hook that runs
> before _DidLoadDocument but after loading the document.

I don't think that's true. You get
action_runner.tab.WaitForDocumentReadyStateToBeComplete() for free as it's in
SystemHealthStory.RunPageInteractions. All you'd have to do is:

class _NewsBrowsingStory:

  def _DidLoadDocument(self, action_runner):
    for i in xrange(self.ITEMS_TO_VISIT):
      ...

class WashingtonPostMobileStory:

  def _DidLoadDocument(self, action_runner):
    action_runner.ClickElement(selector='.close')
    super(WashingtonPostMobileStory, self)._DidLoadDocument(action_runner)

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 13:57:57, ulan wrote:
> On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > Looks like we don't cover any non-US news pages here. I think we can punt on
> > reddit & hacknews for some international news pages?
> > 
> 
> Petr suggested to sync load:news and browse:news.
> 
> Our motivation for browse:news pages that are not in load:news:
> - Nytimes and hackernews have the largest V8 heaps in mobile browse:news
> workload (except CNN which crashes with OOM).
> - Washingtonpost is WPA.
> - Facebook and twitter are super-popular pages.

Facebook and Twitter are in load:social. I suggest you put your stirues under
browse:social:facebook and browse:social:twitter.

> Any strong opinion for removing one of these pages? Otherwise, I'll add these
> pages to load:news.

Not really. If you have a strong opinion for adding a browse:X:Y, I suggest you
just make sure that there's load:X:Y as well. If you think that some of the
existing loading stories doesn't make sense (because it's superseded by you new
load:X:Y story), then you can remove it.

> Now pages in load:news that are not in browse:new: bbc, qq, sohu, wikipedia.
> Should I move qq and sohu to browse:news?

That would be great as it would address the problem with non-US pages.

Note that load:X:Y should load the same URL as the initial url of the
corresponding browse:X:Y (so that the two can be "subtracted"). We can do that
in a follow up patch though if you want ;-)

[petrcermak, 2016-07-08 15:06:29]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
On 2016/07/08 13:57:57, ulan wrote:
> On 2016/07/08 12:46:03, petrcermak wrote:
> > On 2016/07/08 10:50:55, ulan wrote:
> > > This one is not calling the SystemHealthStory.RunPageInteraction, which is
> not
> > > nice.
> > > 
> > > I could introduce _RunPageInteractions, which is called by
> > > SystemHealthStory.RunPageInteraction() after loading the document and
> override
> > 
> > > _RunPageInteractions here. wdyt?
> > 
> > I think that the best solution would be to move the for loop into
> > _DidLoadDocument and not override RunPageInteractions at all. How does that
> > sound?
> 
> I am using _DidLoadDocument as a hook to do special actions like closing
pop-up
> (see WashingtonPostMobileStory) before the actual interactions.
> 
> If I move everything to _DidLoadDocument, I would need another hook that runs
> before _DidLoadDocument but after loading the document.

I don't think that's true. You get
action_runner.tab.WaitForDocumentReadyStateToBeComplete() for free as it's in
SystemHealthStory.RunPageInteractions. All you'd have to do is:

class _NewsBrowsingStory:

  def _DidLoadDocument(self, action_runner):
    for i in xrange(self.ITEMS_TO_VISIT):
      ...

class WashingtonPostMobileStory:

  def _DidLoadDocument(self, action_runner):
    action_runner.ClickElement(selector='.close')
    super(WashingtonPostMobileStory, self)._DidLoadDocument(action_runner)

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 13:57:57, ulan wrote:
> On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > Looks like we don't cover any non-US news pages here. I think we can punt on
> > reddit & hacknews for some international news pages?
> > 
> 
> Petr suggested to sync load:news and browse:news.
> 
> Our motivation for browse:news pages that are not in load:news:
> - Nytimes and hackernews have the largest V8 heaps in mobile browse:news
> workload (except CNN which crashes with OOM).
> - Washingtonpost is WPA.
> - Facebook and twitter are super-popular pages.

Facebook and Twitter are in load:social. I suggest you put your stirues under
browse:social:facebook and browse:social:twitter.

> Any strong opinion for removing one of these pages? Otherwise, I'll add these
> pages to load:news.

Not really. If you have a strong opinion for adding a browse:X:Y, I suggest you
just make sure that there's load:X:Y as well. If you think that some of the
existing loading stories doesn't make sense (because it's superseded by you new
load:X:Y story), then you can remove it.

> Now pages in load:news that are not in browse:new: bbc, qq, sohu, wikipedia.
> Should I move qq and sohu to browse:news?

That would be great as it would address the problem with non-US pages.

Note that load:X:Y should load the same URL as the initial url of the
corresponding browse:X:Y (so that the two can be "subtracted"). We can do that
in a follow up patch though if you want ;-)

[ulan, 2016-07-08 15:40:48]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:64: def
RunPageInteractions(self, action_runner):
On 2016/07/08 15:06:28, petrcermak wrote:
> On 2016/07/08 13:57:57, ulan wrote:
> > On 2016/07/08 12:46:03, petrcermak wrote:
> > > On 2016/07/08 10:50:55, ulan wrote:
> > > > This one is not calling the SystemHealthStory.RunPageInteraction, which
is
> > not
> > > > nice.
> > > > 
> > > > I could introduce _RunPageInteractions, which is called by
> > > > SystemHealthStory.RunPageInteraction() after loading the document and
> > override
> > > 
> > > > _RunPageInteractions here. wdyt?
> > > 
> > > I think that the best solution would be to move the for loop into
> > > _DidLoadDocument and not override RunPageInteractions at all. How does
that
> > > sound?
> > 
> > I am using _DidLoadDocument as a hook to do special actions like closing
> pop-up
> > (see WashingtonPostMobileStory) before the actual interactions.
> > 
> > If I move everything to _DidLoadDocument, I would need another hook that
runs
> > before _DidLoadDocument but after loading the document.
> 
> I don't think that's true. You get
> action_runner.tab.WaitForDocumentReadyStateToBeComplete() for free as it's in
> SystemHealthStory.RunPageInteractions. All you'd have to do is:
> 
> class _NewsBrowsingStory:
> 
>   def _DidLoadDocument(self, action_runner):
>     for i in xrange(self.ITEMS_TO_VISIT):
>       ...
> 
> class WashingtonPostMobileStory:
> 
>   def _DidLoadDocument(self, action_runner):
>     action_runner.ClickElement(selector='.close')
>     super(WashingtonPostMobileStory, self)._DidLoadDocument(action_runner)

Very nice, thank you! I didn't think about that. Done.

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 15:06:29, petrcermak wrote:
> On 2016/07/08 13:57:57, ulan wrote:
> > On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > > Looks like we don't cover any non-US news pages here. I think we can punt
on
> > > reddit & hacknews for some international news pages?
> > > 
> > 
> > Petr suggested to sync load:news and browse:news.
> > 
> > Our motivation for browse:news pages that are not in load:news:
> > - Nytimes and hackernews have the largest V8 heaps in mobile browse:news
> > workload (except CNN which crashes with OOM).
> > - Washingtonpost is WPA.
> > - Facebook and twitter are super-popular pages.
> 
> Facebook and Twitter are in load:social. I suggest you put your stirues under
> browse:social:facebook and browse:social:twitter.
> 
> > Any strong opinion for removing one of these pages? Otherwise, I'll add
these
> > pages to load:news.
> 
> Not really. If you have a strong opinion for adding a browse:X:Y, I suggest
you
> just make sure that there's load:X:Y as well. If you think that some of the
> existing loading stories doesn't make sense (because it's superseded by you
new
> load:X:Y story), then you can remove it.
> 
> > Now pages in load:news that are not in browse:new: bbc, qq, sohu, wikipedia.
> > Should I move qq and sohu to browse:news?
> 
> That would be great as it would address the problem with non-US pages.
> 
> Note that load:X:Y should load the same URL as the initial url of the
> corresponding browse:X:Y (so that the two can be "subtracted"). We can do that
> in a follow up patch though if you want ;-)

Changed facebook and twitter to browse:social.

I'll discuss the pages with Hannes and update the patch.

Let's sync the url in a separate change.

[petrcermak, 2016-07-08 15:58:39]

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:93: 
On 2016/07/08 15:40:47, ulan wrote:
> On 2016/07/08 15:06:29, petrcermak wrote:
> > On 2016/07/08 13:57:57, ulan wrote:
> > > On 2016/07/08 12:54:42, nednguyen (ooo til 7-11) wrote:
> > > > Looks like we don't cover any non-US news pages here. I think we can
punt
> on
> > > > reddit & hacknews for some international news pages?
> > > > 
> > > 
> > > Petr suggested to sync load:news and browse:news.
> > > 
> > > Our motivation for browse:news pages that are not in load:news:
> > > - Nytimes and hackernews have the largest V8 heaps in mobile browse:news
> > > workload (except CNN which crashes with OOM).
> > > - Washingtonpost is WPA.
> > > - Facebook and twitter are super-popular pages.
> > 
> > Facebook and Twitter are in load:social. I suggest you put your stirues
under
> > browse:social:facebook and browse:social:twitter.
> > 
> > > Any strong opinion for removing one of these pages? Otherwise, I'll add
> these
> > > pages to load:news.
> > 
> > Not really. If you have a strong opinion for adding a browse:X:Y, I suggest
> you
> > just make sure that there's load:X:Y as well. If you think that some of the
> > existing loading stories doesn't make sense (because it's superseded by you
> new
> > load:X:Y story), then you can remove it.
> > 
> > > Now pages in load:news that are not in browse:new: bbc, qq, sohu,
wikipedia.
> > > Should I move qq and sohu to browse:news?
> > 
> > That would be great as it would address the problem with non-US pages.
> > 
> > Note that load:X:Y should load the same URL as the initial url of the
> > corresponding browse:X:Y (so that the two can be "subtracted"). We can do
that
> > in a follow up patch though if you want ;-)
> 
> Changed facebook and twitter to browse:social.
> 
> I'll discuss the pages with Hannes and update the patch.
> 
> Let's sync the url in a separate change.

SGTM. Thanks :-)

[ulan, 2016-07-11 17:42:03]

I added mobile qq.com to the story set. This uncovered a bug in chromium which
causes the story to crash during scrolling (see crbug.com/627123).

To avoid being blocked on the bug, we can either
1) land workaround for scrolling in telemetry
(https://codereview.chromium.org/2138883002/)
2) or disable qq.com until the chrome bug is fixed. Note that the crash is very
flaky, so it is possible that other pages also have this problem.

wdyt?

[nednguyen, 2016-07-11 17:43:52]

On 2016/07/11 17:42:03, ulan wrote:
> I added mobile http://qq.com to the story set. This uncovered a bug in
chromium which
> causes the story to crash during scrolling (see crbug.com/627123).
> 
> To avoid being blocked on the bug, we can either
> 1) land workaround for scrolling in telemetry
> (https://codereview.chromium.org/2138883002/)
> 2) or disable http://qq.com until the chrome bug is fixed. Note that the crash
is very
> flaky, so it is possible that other pages also have this problem.
> 
> wdyt?

Let disable http://qq.com until the chrome bug is fixed. Crash while scrolling
seems a serious bug that should be P1 & there is no need to add extra complexity
to telemetry to work around that bug being fixed.

[ulan, 2016-07-11 17:59:09]

On 2016/07/11 17:43:52, nednguyen wrote:
> On 2016/07/11 17:42:03, ulan wrote:
> > I added mobile http://qq.com to the story set. This uncovered a bug in
> chromium which
> > causes the story to crash during scrolling (see crbug.com/627123).
> > 
> > To avoid being blocked on the bug, we can either
> > 1) land workaround for scrolling in telemetry
> > (https://codereview.chromium.org/2138883002/)
> > 2) or disable http://qq.com until the chrome bug is fixed. Note that the
crash
> is very
> > flaky, so it is possible that other pages also have this problem.
> > 
> > wdyt?
> 
> Let disable http://qq.com until the chrome bug is fixed. Crash while scrolling
> seems a serious bug that should be P1 & there is no need to add extra
complexity
> to telemetry to work around that bug being fixed.

bokan@ responded in crbug.com/627123, looks like telemetry shouldn't use
innerHeight/innerWidth for scrolling.

[petrcermak, 2016-07-12 12:33:39]

On 2016/07/11 17:59:09, ulan wrote:
> On 2016/07/11 17:43:52, nednguyen wrote:
> > On 2016/07/11 17:42:03, ulan wrote:
> > > I added mobile http://qq.com to the story set. This uncovered a bug in
> > chromium which
> > > causes the story to crash during scrolling (see crbug.com/627123).
> > > 
> > > To avoid being blocked on the bug, we can either
> > > 1) land workaround for scrolling in telemetry
> > > (https://codereview.chromium.org/2138883002/)
> > > 2) or disable http://qq.com until the chrome bug is fixed. Note that the
> crash
> > is very
> > > flaky, so it is possible that other pages also have this problem.
> > > 
> > > wdyt?
> > 
> > Let disable http://qq.com until the chrome bug is fixed. Crash while
scrolling
> > seems a serious bug that should be P1 & there is no need to add extra
> complexity
> > to telemetry to work around that bug being fixed.
> 
> bokan@ responded in crbug.com/627123, looks like telemetry shouldn't use
> innerHeight/innerWidth for scrolling.

I agree with disabling qq.com for the time being (only the 'browse' story, NOT
the 'load' one).

Thanks,
Petr

[nednguyen, 2016-07-12 12:42:17]

On 2016/07/12 12:33:39, petrcermak wrote:
> On 2016/07/11 17:59:09, ulan wrote:
> > On 2016/07/11 17:43:52, nednguyen wrote:
> > > On 2016/07/11 17:42:03, ulan wrote:
> > > > I added mobile http://qq.com to the story set. This uncovered a bug in
> > > chromium which
> > > > causes the story to crash during scrolling (see crbug.com/627123).
> > > > 
> > > > To avoid being blocked on the bug, we can either
> > > > 1) land workaround for scrolling in telemetry
> > > > (https://codereview.chromium.org/2138883002/)
> > > > 2) or disable http://qq.com until the chrome bug is fixed. Note that the
> > crash
> > > is very
> > > > flaky, so it is possible that other pages also have this problem.
> > > > 
> > > > wdyt?
> > > 
> > > Let disable http://qq.com until the chrome bug is fixed. Crash while
> scrolling
> > > seems a serious bug that should be P1 & there is no need to add extra
> > complexity
> > > to telemetry to work around that bug being fixed.
> > 
> > bokan@ responded in crbug.com/627123, looks like telemetry shouldn't use
> > innerHeight/innerWidth for scrolling.
> 
> I agree with disabling http://qq.com for the time being (only the 'browse'
story, NOT
> the 'load' one).
> 
> Thanks,
> Petr

Note that in order for these stories to be covered by system_health_smoke
benchmark test, we need a memory benchmark in benchmarks/system_health.py. 
Since the benchmark ulan is creating also covers memory usage, I think we should
move it to benchmarks/system_health.py rather than create extra system health
memory benchmarks that also use these stories. Thoughts?

[petrcermak, 2016-07-12 12:53:19]

I think that we are almost done here. Once the list of stories is finalized (I'm
not sure what the exact state is), I suggest you record and upload the WPRs (let
me know if you need help) and then upload a final patch.

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/260001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:74:
self._WaitForItem(action_runner, index)
On 2016/07/08 13:57:56, ulan wrote:
> On 2016/07/08 12:46:03, petrcermak wrote:
> > These methods build the item selector twice unnecessarily. Plus they both
have
> > to know how selectors are created. I suggest you build the selector first
and
> > then pass it to them:
> > 
> > item_selector = 'document.querySelectorAll("%s")[%d]' % (self.ITEM_SELECTOR,
> > index)
> > action_runner.WaitForElement(element_function=item_selector)
> > action_runner.ClickElement(element_function=item_selector)
> > if not self.IS_SINGLE_PAGE_APP:
> >   action_runner.WaitForNavigate()
> 
> Done, but I left WaitForNavigation in separate function because it is used in
> NavigateBack().

Acknowledged.

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:22: ITEM_SELECTOR =
NotImplemented
Sorry to keep changing my mind. Now that I look at this, it seems strange that
_BrowsingStory has knowledge of items (given that iteration over items actually
happens inside _NewsBrowsingStory). Therefore, I suggest you move ITEM_SELECTOR
and ITEM_TO_VISIT to _NewsBrowsingStory. In addition, please change
_NavigateToItem to (but keep it inside _BrowsingStory):

def _ClickLink(self, action_runner, element_function):
  action_runner.WaitForElement(element_function)
  ...

The query selector logic should be in _NewsBrowsingStory:

def _NavigateToItem(self, action_runner, index):
  item_selector = ...
  return self._ClickLink(action_runner, element_function=item_selector)

[ulan, 2016-07-12 13:44:15]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-12 14:05:48]

I uploaded new patch set that syncs browse stories with load stories.

I am ensuring that browse stories are subset of load stories.

I am also changing the urls in load stories so that they match the urls in
browse stories. By the way, we cannot compare memory usage of load stories and
browse stories because they use different memory measurement (load forces GC).
Does it still make sense to sync urls?

I can also extract the load changes into separate CL and land that CL before
this CL, wdyt?

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:22: ITEM_SELECTOR =
NotImplemented
On 2016/07/12 12:53:19, petrcermak wrote:
> Sorry to keep changing my mind. Now that I look at this, it seems strange that
> _BrowsingStory has knowledge of items (given that iteration over items
actually
> happens inside _NewsBrowsingStory). Therefore, I suggest you move
ITEM_SELECTOR
> and ITEM_TO_VISIT to _NewsBrowsingStory. In addition, please change
> _NavigateToItem to (but keep it inside _BrowsingStory):
> 
> def _ClickLink(self, action_runner, element_function):
>   action_runner.WaitForElement(element_function)
>   ...
> 
> The query selector logic should be in _NewsBrowsingStory:
> 
> def _NavigateToItem(self, action_runner, index):
>   item_selector = ...
>   return self._ClickLink(action_runner, element_function=item_selector)

I extracted _ClinkLink.

Item related code in _BrowsingStory will be useful later when we add media
browsing stories. They also navigate to items, but do not use the back button.

https://codereview.chromium.org/2118293002/diff/340001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/single_page_stories.py (right):

https://codereview.chromium.org/2118293002/diff/340001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/single_page_stories.py:152: URL =
'http://news.qq.com'
SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY is missing.

Should I add it? Or remove 'Mobile' from story class name?

[nednguyen, 2016-07-12 14:11:16]

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:80: class DesktopNewsBenchmark(_NewsBenchmark):
Ping on considering moving these benchmarks to benchmarks/system_health.py.

[ulan, 2016-07-12 14:16:22]

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:80: class DesktopNewsBenchmark(_NewsBenchmark):
On 2016/07/12 14:11:15, nednguyen wrote:
> Ping on considering moving these benchmarks to benchmarks/system_health.py. 

SGTM, I'll wait for Petr's reply before moving.
Any suggestions for benchnmark name? system_health.news?

[petrcermak, 2016-07-12 15:30:55]

I DON'T think that the new benchmark should be added to system_health.py. The
reason for this is that it uses V8-specific set up and metrics (for example, the
memory reported in this benchmark only includes memory dumps that happen after
V8 is initialized).

Having said that, the stories, which are almost completely independent, should
be part of the SH story set so that they could be re-used for other metrics. The
reason why we don't add them to the SH story set straightaway is because they
currently rely on periodic memory dumps (whereas the existing SH stories rely on
explicit memory dumps). Once this is resolved (see the TODO in Juan's patch
https://codereview.chromium.org/2123713005/), the new stories will be included
in system_health.memory_* benchmarks.

=== Summary ===

  Current situation:

    * system_health.memory_* = memoryMetric & explicit dumps
                             × load:* stories (single_page_stories.py)

  After this patch lands:

    * system_health.memory_* = memoryMetric & explicit dumps
                             × load:* stories (single_page_stories.py)

    * news_*                 = v8AndMemoryMetrics & periodic dumps
                             × browse:* stories (browsing_stories.py)

  Near future (after measuring is a no-op with memory-infra disabled):

    * system_health.memory_* = memoryMetric & explicit dumps
                             × load:* stories (single_page_stories.py) &
                               browse:* stories (browsing_stories.py)

    * news_*                 = v8AndMemoryMetrics & periodic dumps
                             × browse:* stories (browsing_stories.py)

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:22: ITEM_SELECTOR =
NotImplemented
On 2016/07/12 14:05:47, ulan wrote:
> On 2016/07/12 12:53:19, petrcermak wrote:
> > Sorry to keep changing my mind. Now that I look at this, it seems strange
that
> > _BrowsingStory has knowledge of items (given that iteration over items
> actually
> > happens inside _NewsBrowsingStory). Therefore, I suggest you move
> ITEM_SELECTOR
> > and ITEM_TO_VISIT to _NewsBrowsingStory. In addition, please change
> > _NavigateToItem to (but keep it inside _BrowsingStory):
> > 
> > def _ClickLink(self, action_runner, element_function):
> >   action_runner.WaitForElement(element_function)
> >   ...
> > 
> > The query selector logic should be in _NewsBrowsingStory:
> > 
> > def _NavigateToItem(self, action_runner, index):
> >   item_selector = ...
> >   return self._ClickLink(action_runner, element_function=item_selector)
> 
> I extracted _ClinkLink.
> 
> Item related code in _BrowsingStory will be useful later when we add media
> browsing stories. They also navigate to items, but do not use the back button.

I see. In that case I'm fine either way (extracted or not extracted).

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:63: return 'news_%s' % cls.page_set.PLATFORM
Given that this benchmark focuses on V8, shouldn't it be called "v8.news_*"?

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:197: class
_NewsBrowsingStorySet(story.StorySet):
Could this be called _BrowsingSystemHealthStorySet (and the sub-classes
DesktopBrowsingSystemHealthStorySet and MobileBrowsingSystemHealthStorySet)?

[ulan, 2016-07-12 17:14:48]

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/320001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:22: ITEM_SELECTOR =
NotImplemented
On 2016/07/12 15:30:54, petrcermak wrote:
> On 2016/07/12 14:05:47, ulan wrote:
> > On 2016/07/12 12:53:19, petrcermak wrote:
> > > Sorry to keep changing my mind. Now that I look at this, it seems strange
> that
> > > _BrowsingStory has knowledge of items (given that iteration over items
> > actually
> > > happens inside _NewsBrowsingStory). Therefore, I suggest you move
> > ITEM_SELECTOR
> > > and ITEM_TO_VISIT to _NewsBrowsingStory. In addition, please change
> > > _NavigateToItem to (but keep it inside _BrowsingStory):
> > > 
> > > def _ClickLink(self, action_runner, element_function):
> > >   action_runner.WaitForElement(element_function)
> > >   ...
> > > 
> > > The query selector logic should be in _NewsBrowsingStory:
> > > 
> > > def _NavigateToItem(self, action_runner, index):
> > >   item_selector = ...
> > >   return self._ClickLink(action_runner, element_function=item_selector)
> > 
> > I extracted _ClinkLink.
> > 
> > Item related code in _BrowsingStory will be useful later when we add media
> > browsing stories. They also navigate to items, but do not use the back
button.
> 
> I see. In that case I'm fine either way (extracted or not extracted).

Acknowledged.

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
File tools/perf/benchmarks/news.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/benchmarks/...
tools/perf/benchmarks/news.py:63: return 'news_%s' % cls.page_set.PLATFORM
On 2016/07/12 15:30:54, petrcermak wrote:
> Given that this benchmark focuses on V8, shouldn't it be called "v8.news_*"?

Done. I renamed the file too.

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:197: class
_NewsBrowsingStorySet(story.StorySet):
On 2016/07/12 15:30:55, petrcermak wrote:
> Could this be called _BrowsingSystemHealthStorySet (and the sub-classes
> DesktopBrowsingSystemHealthStorySet and MobileBrowsingSystemHealthStorySet)?

I renamed it to _NewsBrowsingSystemHealthStorySet. There will be also
_MediaBrowsingSystemHealthStorySet in future. By the way, we could fold them
into one browsing story set and have only one browsing benchmark (instead of
news browsing and media browsing), but then the benchmark will run for ~10
minutes. Is it better to have two 5minute benchmarks or one 10 minute benchmark?

[nednguyen, 2016-07-12 17:19:15]

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:197: class
_NewsBrowsingStorySet(story.StorySet):
On 2016/07/12 17:14:48, ulan wrote:
> On 2016/07/12 15:30:55, petrcermak wrote:
> > Could this be called _BrowsingSystemHealthStorySet (and the sub-classes
> > DesktopBrowsingSystemHealthStorySet and MobileBrowsingSystemHealthStorySet)?
> 
> I renamed it to _NewsBrowsingSystemHealthStorySet. There will be also
> _MediaBrowsingSystemHealthStorySet in future. By the way, we could fold them
> into one browsing story set and have only one browsing benchmark (instead of
> news browsing and media browsing), but then the benchmark will run for ~10
> minutes. Is it better to have two 5minute benchmarks or one 10 minute
benchmark?

More benchmarks will make it more confusing for users to decide what to run for
checking performance. In general, we want to reduce the number of benchmarks to
as few as possible.

[petrcermak, 2016-07-12 17:23:25]

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:197: class
_NewsBrowsingStorySet(story.StorySet):
On 2016/07/12 17:19:15, nednguyen wrote:
> On 2016/07/12 17:14:48, ulan wrote:
> > On 2016/07/12 15:30:55, petrcermak wrote:
> > > Could this be called _BrowsingSystemHealthStorySet (and the sub-classes
> > > DesktopBrowsingSystemHealthStorySet and
MobileBrowsingSystemHealthStorySet)?
> > 
> > I renamed it to _NewsBrowsingSystemHealthStorySet. There will be also
> > _MediaBrowsingSystemHealthStorySet in future. By the way, we could fold them
> > into one browsing story set and have only one browsing benchmark (instead of
> > news browsing and media browsing), but then the benchmark will run for ~10
> > minutes. Is it better to have two 5minute benchmarks or one 10 minute
> benchmark?
> 
> More benchmarks will make it more confusing for users to decide what to run
for
> checking performance. In general, we want to reduce the number of benchmarks
to
> as few as possible.

I agree with Ned. Let's have a single _BrowsingSystemHealthStorySet.

[ulan, 2016-07-12 17:59:14]

On 2016/07/12 17:23:25, petrcermak wrote:
>
https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/browsing_stories.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/360001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:197: class
> _NewsBrowsingStorySet(story.StorySet):
> On 2016/07/12 17:19:15, nednguyen wrote:
> > On 2016/07/12 17:14:48, ulan wrote:
> > > On 2016/07/12 15:30:55, petrcermak wrote:
> > > > Could this be called _BrowsingSystemHealthStorySet (and the sub-classes
> > > > DesktopBrowsingSystemHealthStorySet and
> MobileBrowsingSystemHealthStorySet)?
> > > 
> > > I renamed it to _NewsBrowsingSystemHealthStorySet. There will be also
> > > _MediaBrowsingSystemHealthStorySet in future. By the way, we could fold
them
> > > into one browsing story set and have only one browsing benchmark (instead
of
> > > news browsing and media browsing), but then the benchmark will run for ~10
> > > minutes. Is it better to have two 5minute benchmarks or one 10 minute
> > benchmark?
> > 
> > More benchmarks will make it more confusing for users to decide what to run
> for
> > checking performance. In general, we want to reduce the number of benchmarks
> to
> > as few as possible.
> 
> I agree with Ned. Let's have a single _BrowsingSystemHealthStorySet.

We'll make one benchmark then. Renamed story set to
_BrowsingSystemHealthStorySet and benchmark to v8.browsing

[ulan, 2016-07-12 18:00:40]

Do changes in load:news stories look good?
I am going to record wpr for browse and load story sets tomorrow.

[petrcermak, 2016-07-12 18:07:50]

Modified load:* stories lg2me.

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
File tools/perf/benchmarks/v8_browsing.py (right):

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:80: class
DesktopNewsBenchmark(_BrowsingBenchmark):
This should be DesktopBrowsingBenchmark

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:88: class
MobileNewsBenchmark(_BrowsingBenchmark):
This should be MobileBrowsingBenchmark

[ulan, 2016-07-13 12:47:32]

I uploaded wpr snapshots.

Each story was recorded separately to keep the difference between recording
times of (browse|load)_(desktop|mobile) versions as small as possible.


Running with snapshots I get the following results.

v8.browsing_desktop on Linux:
[       OK ] browse:news:cnn (77408 ms)
[       OK ] browse:news:hackernews (42587 ms)
[       OK ] browse:news:nytimes (68400 ms)
[       OK ] browse:news:reddit (69901 ms)
[       OK ] browse:social:twitter (45682 ms)
Total time of run_benchmark: 5:21.92
results.html of 3 runs:
https://4183efcaf5788dde32c066f68b69ecbb88460d40-www.googledrive.com/host/0B5...


v8.browsing_mobile on Nexus 6P:
[       OK ] browse:news:cnn (126611 ms) (will be disabled due to OOM on svelte)
[       OK ] browse:social:facebook (52290 ms)
[       OK ] browse:news:hackernews (42713 ms)
[       OK ] browse:news:nytimes (60707 ms)
[       OK ] browse:news:qq (45011 ms) (will be disabled due to bug in
telemetry)
[       OK ] browse:news:reddit (49458 ms)
[       OK ] browse:social:twitter (45268 ms)
[       OK ] browse:news:washingtonpost (44730 ms)
Total time of run_benchmark: 8:33.54

results.html of 3 runs
https://830a8114727e349d39d880f92ec79c0b18f5b401-ww.googledrive.com/host/0B5n...


system_health.memory_desktop on Linux (only stories changed in this CL):
[       OK ] load:news:cnn (18159 ms)
[       OK ] load:social:facebook (16111 ms)
[       OK ] load:news:hackernews (13143 ms)
[       OK ] load:news:nytimes (15064 ms)
[       OK ] load:news:qq (20241 ms)
[       OK ] load:news:reddit (15576 ms)
[       OK ] load:social:twitter (14957 ms)
Total time of run_benchmark: 2:05.13
results.html of 3 runs
https://c18dd98ddb27d39bd32f1285993261b0f12cee82-www.googledrive.com/host/0B5...


system_health.memory_mobile on Linux (only stories changed in this CL):
[       OK ] load:news:cnn (29850 ms)
[       OK ] load:social:facebook (16926 ms)
[       OK ] load:news:hackernews (14214 ms)
[       OK ] load:news:nytimes (18415 ms)
[       OK ] load:news:qq (15809 ms)
[       OK ] load:news:reddit (16848 ms)
[       OK ] load:social:twitter (15654 ms)
[       OK ] load:news:washingtonpost (16417 ms)
Total time of run_benchmark: 2:40.76
results.html of 3 runs
https://cddea2bd89ccbbe359e086c3f1c3b46496a0a8bc-www.googledrive.com/host/0B5...

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
File tools/perf/benchmarks/v8_browsing.py (right):

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:80: class
DesktopNewsBenchmark(_BrowsingBenchmark):
On 2016/07/12 18:07:49, petrcermak wrote:
> This should be DesktopBrowsingBenchmark

Done.

https://codereview.chromium.org/2118293002/diff/400001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:88: class
MobileNewsBenchmark(_BrowsingBenchmark):
On 2016/07/12 18:07:50, petrcermak wrote:
> This should be MobileBrowsingBenchmark

Done.

[ulan, 2016-07-13 12:54:01]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

Note for perf sheriffs: regressions are expected in
system_health.memory_mobile and system_health.memory_desktop
for the following stories:
load:news:cnn
load:social:facebook
load:news:qq
load:news:reddit
load:social:twitter


BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[ulan, 2016-07-13 13:08:51]

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

[commit-bot: I haz the power, 2016-07-13 13:09:00]

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, 2016-07-13 13:09:01]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[nednguyen, 2016-07-13 13:10:49]

lgtm with nits

I like this new structuring a lot. Thanks for your great work!

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:96: SUPPORTED_PLATFORMS =
platforms.DESKTOP_ONLY
nist: can you rearrange the stories in the "news browsing stories" so they are
grouped by "support all platforms", "support desktop", "support mobile"?

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:11: NO_PLATFORMS = frozenset()
nits: can you add a comment saying this is used for disabling a story?

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/single_page_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/single_page_stories.py:4: 
maybe rename this file to loading_stories.py to make it consistent with
browsing_stories?

[nednguyen, 2016-07-13 13:11:23]

On 2016/07/13 13:10:49, nednguyen wrote:
> lgtm with nits
> 
> I like this new structuring a lot. Thanks for your great work!
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/browsing_stories.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/browsing_stories.py:96: SUPPORTED_PLATFORMS
=
> platforms.DESKTOP_ONLY
> nist: can you rearrange the stories in the "news browsing stories" so they are
> grouped by "support all platforms", "support desktop", "support mobile"?
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/platforms.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/platforms.py:11: NO_PLATFORMS = frozenset()
> nits: can you add a comment saying this is used for disabling a story?
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/single_page_stories.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/single_page_stories.py:4: 
> maybe rename this file to loading_stories.py to make it consistent with
> browsing_stories?

cc'ed Charlie & Randy since this patch change some of the stories your battor
benchmarks are using

[nednguyen, 2016-07-13 13:13:01]

On 2016/07/13 13:11:23, nednguyen wrote:
> On 2016/07/13 13:10:49, nednguyen wrote:
> > lgtm with nits
> > 
> > I like this new structuring a lot. Thanks for your great work!
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > File tools/perf/page_sets/system_health/browsing_stories.py (right):
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/browsing_stories.py:96:
SUPPORTED_PLATFORMS
> =
> > platforms.DESKTOP_ONLY
> > nist: can you rearrange the stories in the "news browsing stories" so they
are
> > grouped by "support all platforms", "support desktop", "support mobile"?
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > File tools/perf/page_sets/system_health/platforms.py (right):
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/platforms.py:11: NO_PLATFORMS =
frozenset()
> > nits: can you add a comment saying this is used for disabling a story?
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > File tools/perf/page_sets/system_health/single_page_stories.py (right):
> > 
> >
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> > tools/perf/page_sets/system_health/single_page_stories.py:4: 
> > maybe rename this file to loading_stories.py to make it consistent with
> > browsing_stories?
> 
> cc'ed Charlie & Randy since this patch change some of the stories your battor
> benchmarks are using

Also I would suggest you add a note to sheriff in the description that looks
similar to the one in https://codereview.chromium.org/2115623002/

[petrcermak, 2016-07-13 13:20:59]

LGTM once Ned's comments are addressed.

Thanks a lot for all the hard work!
Petr

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:96: SUPPORTED_PLATFORMS =
platforms.DESKTOP_ONLY
On 2016/07/13 13:10:48, nednguyen wrote:
> nist: can you rearrange the stories in the "news browsing stories" so they are
> grouped by "support all platforms", "support desktop", "support mobile"?

I don't agree with this. I would like to keep the ordering in browsing_stories,
loading_stories, etc the same. However, there are cases when the loading page
works on all platforms, but the browsing one only on desktop. Furthermore, I
would like to keep mobile and desktop versions of the same page next to each
other.

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/single_page_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/single_page_stories.py:4: 
On 2016/07/13 13:10:48, nednguyen wrote:
> maybe rename this file to loading_stories.py to make it consistent with
> browsing_stories?

I agree that this should be renamed to loading stories and the class to
_LoadingStory. I'm happy to do this in a subsequent patch if you prefer.

[ulan, 2016-07-13 13:22:10]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

Note for perf sheriffs: regressions are expected in
system_health.memory_mobile and system_health.memory_desktop
for the following stories:
load:news:cnn
load:social:facebook
load:news:qq
load:news:reddit
load:social:twitter


BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[nednguyen, 2016-07-13 13:26:04]

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:96: SUPPORTED_PLATFORMS =
platforms.DESKTOP_ONLY
On 2016/07/13 13:20:58, petrcermak wrote:
> On 2016/07/13 13:10:48, nednguyen wrote:
> > nist: can you rearrange the stories in the "news browsing stories" so they
are
> > grouped by "support all platforms", "support desktop", "support mobile"?
> 
> I don't agree with this. I would like to keep the ordering in
browsing_stories,
> loading_stories, etc the same. However, there are cases when the loading page
> works on all platforms, but the browsing one only on desktop. Furthermore, I
> would like to keep mobile and desktop versions of the same page next to each
> other.

Ah, makes sense to keep mobile & desktop versions of same page next to each
other.

[ulan, 2016-07-13 13:32:46]

Yay, landing soon.

Thank you all for great suggestions and comments!

> Also I would suggest you add a note to sheriff in the description that looks
similar to the one in https://codereview.chromium.org/2115623002/
Done.

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/browsing_stories.py:96: SUPPORTED_PLATFORMS =
platforms.DESKTOP_ONLY
On 2016/07/13 13:26:04, nednguyen wrote:
> On 2016/07/13 13:20:58, petrcermak wrote:
> > On 2016/07/13 13:10:48, nednguyen wrote:
> > > nist: can you rearrange the stories in the "news browsing stories" so they
> are
> > > grouped by "support all platforms", "support desktop", "support mobile"?
> > 
> > I don't agree with this. I would like to keep the ordering in
> browsing_stories,
> > loading_stories, etc the same. However, there are cases when the loading
page
> > works on all platforms, but the browsing one only on desktop. Furthermore, I
> > would like to keep mobile and desktop versions of the same page next to each
> > other.
> 
> Ah, makes sense to keep mobile & desktop versions of same page next to each
> other.

Acknowledged.

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:11: NO_PLATFORMS = frozenset()
On 2016/07/13 13:10:48, nednguyen wrote:
> nits: can you add a comment saying this is used for disabling a story?

Done.

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/single_page_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/single_page_stories.py:4: 
On 2016/07/13 13:20:58, petrcermak wrote:
> On 2016/07/13 13:10:48, nednguyen wrote:
> > maybe rename this file to loading_stories.py to make it consistent with
> > browsing_stories?
> 
> I agree that this should be renamed to loading stories and the class to
> _LoadingStory. I'm happy to do this in a subsequent patch if you prefer.

Done.

[nednguyen, 2016-07-13 13:34:39]

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/single_page_stories.py (right):

https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/single_page_stories.py:4: 
On 2016/07/13 13:32:46, ulan wrote:
> On 2016/07/13 13:20:58, petrcermak wrote:
> > On 2016/07/13 13:10:48, nednguyen wrote:
> > > maybe rename this file to loading_stories.py to make it consistent with
> > > browsing_stories?
> > 
> > I agree that this should be renamed to loading stories and the class to
> > _LoadingStory. I'm happy to do this in a subsequent patch if you prefer.
> 
> Done.

Looks like you forgot to upload your loading_stories.py?

[ulan, 2016-07-13 13:35:50]

On 2016/07/13 13:34:39, nednguyen wrote:
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> File tools/perf/page_sets/system_health/single_page_stories.py (right):
> 
>
https://codereview.chromium.org/2118293002/diff/420001/tools/perf/page_sets/s...
> tools/perf/page_sets/system_health/single_page_stories.py:4: 
> On 2016/07/13 13:32:46, ulan wrote:
> > On 2016/07/13 13:20:58, petrcermak wrote:
> > > On 2016/07/13 13:10:48, nednguyen wrote:
> > > > maybe rename this file to loading_stories.py to make it consistent with
> > > > browsing_stories?
> > > 
> > > I agree that this should be renamed to loading stories and the class to
> > > _LoadingStory. I'm happy to do this in a subsequent patch if you prefer.
> > 
> > Done.
> 
> Looks like you forgot to upload your loading_stories.py?

Right, just uploaded it :)

[ulan, 2016-07-13 13:36:49]

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

[commit-bot: I haz the power, 2016-07-13 13:37:03]

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, 2016-07-13 13:37:06]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[commit-bot: I haz the power, 2016-07-13 14:03:19]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[commit-bot: I haz the power, 2016-07-13 14:31:38]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[commit-bot: I haz the power, 2016-07-13 15:01:32]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[petrcermak, 2016-07-13 15:03:28]

LGTM with one more comment. Let's do this!

Thanks,
Petr

https://codereview.chromium.org/2118293002/diff/460001/tools/perf/benchmarks/...
File tools/perf/benchmarks/v8_browsing.py (right):

https://codereview.chromium.org/2118293002/diff/460001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:31: class
_BrowsingBenchmark(perf_benchmark.PerfBenchmark):
I suppose this should be called _V8BrowingBenchmark (to match 'v8.browsing_*')?
The same goes for its Desktop and Mobile subclasses.

[ulan, 2016-07-13 15:30:31]

https://codereview.chromium.org/2118293002/diff/460001/tools/perf/benchmarks/...
File tools/perf/benchmarks/v8_browsing.py (right):

https://codereview.chromium.org/2118293002/diff/460001/tools/perf/benchmarks/...
tools/perf/benchmarks/v8_browsing.py:31: class
_BrowsingBenchmark(perf_benchmark.PerfBenchmark):
On 2016/07/13 15:03:27, petrcermak wrote:
> I suppose this should be called _V8BrowingBenchmark (to match
'v8.browsing_*')?
> The same goes for its Desktop and Mobile subclasses.

Done.

[ulan, 2016-07-13 15:30:59]

The CQ bit was checked by ulan@chromium.org

[ulan, 2016-07-13 15:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/2118293002/#ps480001
(title: "rename benchmarks")

[commit-bot: I haz the power, 2016-07-13 15:31:51]

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, 2016-07-13 15:31:54]

There were warnings when CQ was processing your CL:
 * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use
CQ_INCLUDE_TRYBOTS instead.
 * CQ is not running the android_s5_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']
 * CQ is not running the mac_retina_perf_cq trybot, per your CQ_INCLUDE_TRYBOTS
flag request. That bot was already specified, with these tests: ['defaulttests']

[nednguyen, 2016-07-13 15:34:22]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[commit-bot: I haz the power, 2016-07-13 17:38:14]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[commit-bot: I haz the power, 2016-07-13 17:38:18]

Committed patchset #23 (id:480001)

[commit-bot: I haz the power, 2016-07-13 17:38:48]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 17:39:39]

Description was changed from

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Add benchmark that imitates news reading.

Each story in the benchmark does the following:
1. Load the main page.
2. Open and scroll the first news item.
3. Go back to the main page and scroll it.
4. Open and scroll the second news item.
5. Go back to the main page and scroll it.
6. etc.

This workflow is interesting because it is close to real world
and generates a lot of garbage by creating and discarding pages.

A couple of websites crash with OOM on svelte device, so by
tracking and optimizing for this benchmark we can improve
user experience on low-memory devices.

*** NOTE TO SHERRIF ***
Regressions are expected in system_health.memory_mobile and
system_health.memory_desktop for the following stories:
- load:news:cnn
- load:social:facebook
- load:news:qq
- load:news:reddit
- load:social:twitter

If there are failing tests in form of
benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.*
please do not revert this CL but instead disabling failing
test by adding them into the _DISABLE_TESTS list in
tools/perf/benchmarks/system_health_smoke_test.py

BUG=589726
CQ_INCLUDE_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq

Committed: https://crrev.com/5b1847df818e5be4592e9a6ac4832350bb663023
Cr-Commit-Position: refs/heads/master@{#405212}
==========

[commit-bot: I haz the power, 2016-07-13 17:39:42]

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