Rename rasterize_and_record_micro.key_mobile_sites{_smooth,} and top_25{_smooth,}

Make rasterize_and_record_micro.key_mobile_sites_smooth actually use KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/eeb7021c7962a466e254557f9164b742ac174c19
Cr-Commit-Position: refs/heads/master@{#423931}

[benjhayden, 2016-10-04 04:10:06]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

BUG=601953
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[benjhayden, 2016-10-04 04:10:49]

benjhayden@chromium.org changed reviewers:
+ nednguyen@google.com, wkorman@chromium.org

[benjhayden, 2016-10-04 04:10:49]

PTAL :-)

[benjhayden, 2016-10-04 04:15:28]

benjhayden@chromium.org changed reviewers:
+ vivianz@chromium.org

[wkorman, 2016-10-04 18:31:02]

Interesting, thanks for catching -- need input from someone who's been
shepherding this benchmark for paint/raster purposes longer than myself. Unless
perchance you (benjhayden@) are that person yourself!

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
File tools/perf/benchmarks/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
tools/perf/benchmarks/rasterize_and_record_micro.py:55: page_set =
page_sets.Top25PageSet
This seems to be similar -- not smooth here, but name below is '_smooth'.

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
tools/perf/benchmarks/rasterize_and_record_micro.py:68: page_set =
page_sets.KeyMobileSitesSmoothPageSet
The other possibility is that the name was a typo, and we intended to keep this
page set but not have '_smooth' in the name.

+vmpstr@ to weigh in.

[wkorman, 2016-10-04 18:31:32]

wkorman@chromium.org changed reviewers:
+ vmpstr@chromium.org

[wkorman, 2016-10-04 18:31:33]

+vmpstr@/chrishtr@ for historical background on page set selection/naming input.

[vmpstr, 2016-10-04 19:00:14]

lgtm

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
File tools/perf/benchmarks/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
tools/perf/benchmarks/rasterize_and_record_micro.py:68: page_set =
page_sets.KeyMobileSitesSmoothPageSet
On 2016/10/04 18:31:02, wkorman wrote:
> The other possibility is that the name was a typo, and we intended to keep
this
> page set but not have '_smooth' in the name.
> 
> +vmpstr@ to weigh in.

I think the benchmark might predate "smooth" pagesets, so maybe we just missed
updating it.

[wkorman, 2016-10-04 21:06:02]

lgtm

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
File tools/perf/benchmarks/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
tools/perf/benchmarks/rasterize_and_record_micro.py:55: page_set =
page_sets.Top25PageSet
On 2016/10/04 18:31:02, wkorman wrote:
> This seems to be similar -- not smooth here, but name below is '_smooth'.

With vmpstr@ feedback I'm supportive of this change then if we switch this as
well, unless there's some reason we shouldn't?

[nednguyen, 2016-10-04 21:12:09]

On 2016/10/04 21:06:02, wkorman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> File tools/perf/benchmarks/rasterize_and_record_micro.py (right):
> 
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> tools/perf/benchmarks/rasterize_and_record_micro.py:55: page_set =
> page_sets.Top25PageSet
> On 2016/10/04 18:31:02, wkorman wrote:
> > This seems to be similar -- not smooth here, but name below is '_smooth'.
> 
> With vmpstr@ feedback I'm supportive of this change then if we switch this as
> well, unless there's some reason we shouldn't?

IIRC, KeyMobileSitesPageSet doesn't do any scrolling, whereas
KeyMobileSitesSmoothPageSet does scroll after loading the page. If so, this CL
will cause change to the metrics got reported. But the change is probably for
good reason that this benchmark should track animation?

[vmpstr, 2016-10-04 21:16:17]

On 2016/10/04 21:12:09, nednguyen wrote:
> On 2016/10/04 21:06:02, wkorman wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> > File tools/perf/benchmarks/rasterize_and_record_micro.py (right):
> > 
> >
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> > tools/perf/benchmarks/rasterize_and_record_micro.py:55: page_set =
> > page_sets.Top25PageSet
> > On 2016/10/04 18:31:02, wkorman wrote:
> > > This seems to be similar -- not smooth here, but name below is '_smooth'.
> > 
> > With vmpstr@ feedback I'm supportive of this change then if we switch this
as
> > well, unless there's some reason we shouldn't?
> 
> IIRC, KeyMobileSitesPageSet doesn't do any scrolling, whereas
> KeyMobileSitesSmoothPageSet does scroll after loading the page. If so, this CL
> will cause change to the metrics got reported. But the change is probably for
> good reason that this benchmark should track animation?

Actually, I think it's the opposite. This benchmark is not meant to capture
scrolling. It acts on a single commit message after the page is loaded. If we
also start scrolling, then it's possible that this will introduce noise into the
benchmark. In other words, if sometimes we measure one scroll offset and another
time we measure another scroll offset, then we'll end up getting a different
interest rect for recording and a different display list to process. wkorman@,
do you think this is possible? If so, maybe we should instead remove _smooth
from the benchmark name and leave this with a non-scrolling page set.

[wkorman, 2016-10-04 21:33:56]

On 2016/10/04 21:16:17, vmpstr wrote:
> On 2016/10/04 21:12:09, nednguyen wrote:
> > On 2016/10/04 21:06:02, wkorman wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> > > File tools/perf/benchmarks/rasterize_and_record_micro.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2388423002/diff/1/tools/perf/benchmarks/raste...
> > > tools/perf/benchmarks/rasterize_and_record_micro.py:55: page_set =
> > > page_sets.Top25PageSet
> > > On 2016/10/04 18:31:02, wkorman wrote:
> > > > This seems to be similar -- not smooth here, but name below is
'_smooth'.
> > > 
> > > With vmpstr@ feedback I'm supportive of this change then if we switch this
> as
> > > well, unless there's some reason we shouldn't?
> > 
> > IIRC, KeyMobileSitesPageSet doesn't do any scrolling, whereas
> > KeyMobileSitesSmoothPageSet does scroll after loading the page. If so, this
CL
> > will cause change to the metrics got reported. But the change is probably
for
> > good reason that this benchmark should track animation?
> 
> Actually, I think it's the opposite. This benchmark is not meant to capture
> scrolling. It acts on a single commit message after the page is loaded. If we
> also start scrolling, then it's possible that this will introduce noise into
the
> benchmark. In other words, if sometimes we measure one scroll offset and
another
> time we measure another scroll offset, then we'll end up getting a different
> interest rect for recording and a different display list to process. wkorman@,
> do you think this is possible? If so, maybe we should instead remove _smooth
> from the benchmark name and leave this with a non-scrolling page set.

Hmm, ok. I agree, I don't think we want to add scrolling if we've not got it
already. Let me look at the different pagesets locally and talk w/ chrishtr@ and
I will follow up. It will be Thursday as I'm OOO tomorrow.

[wkorman, 2016-10-06 18:48:46]

Current proposal on review of history: fix the Name() methods for
rasterize_and_record_micro.top_25_smooth and .key_mobile_sites_smooth to not
include '_smooth' in the name, and leave the PageSet(s) as-is.

Detail:

I'm working to piece together the history of what happened here with
rasterize_and_record top_25_smooth and key_mobile_sites_smooth. So far what I
have found is:

2015-01-15: http://crrev.com/816353008 introduced the explicit Name() method
with '_smooth' suffixes. At this time rasterize_and_record_micro was actually
using the Smooth PageSet variants.

2015-02-14: http://crrev.com/927763002 changed the page set to
KeyMobileSitesPageSet from KeyMobileSitesSmoothPageSet while also renaming the
json archive, on behalf of http://crbug.com/418375

2015-03-02: http://crrev.com/966283002 changed the page set to Top25PageSet from
Top25SmoothPageSet while also copying the non-smooth json file to a new smooth
suffixed json file.

I'm interpreting the last two as intentional efforts to split apart a separate
Smooth page set class/data from the non-smooth page set class/data. So, it
appears that initially all we had was smooth page sets, and the non-smooth ones
came later.

It does look like today the Smooth page sets perform scrolling, so next I'll
look to see what the state of that was when the above changes were made.
Whatever the case may be, recent benchmarking looks not to have incorporated
scrolling and I don't think we want it.

[benjhayden, 2016-10-06 20:21:35]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[benjhayden, 2016-10-06 20:25:15]

PTAL :-)

[benjhayden, 2016-10-06 20:25:38]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[nednguyen, 2016-10-06 20:33:53]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[nednguyen, 2016-10-06 20:33:53]

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

[nednguyen, 2016-10-06 20:34:13]

On 2016/10/06 20:25:15, benjhayden wrote:
> PTAL :-)

lgtm

Though you may want to file a bug about benchmark renaming.

[wkorman, 2016-10-06 20:37:12]

On 2016/10/06 20:34:13, nednguyen wrote:
> On 2016/10/06 20:25:15, benjhayden wrote:
> > PTAL :-)
> 
> lgtm
> 
> Though you may want to file a bug about benchmark renaming.

Yes, LGTM and please file a bug copy me and let's ask nduca@ whether it makes
sense for us to add actual _smooth variants for top_25 and key_mobile_sites.

My current belief from repo spelunking is that we unintentionally changed this
benchmark to non-smooth, thus not incorporating scrolling, in Feb/Mar 2015 per
the changes I previously linked.

It would seem to me currently that there's value to having both. And in any
case, the name should match the page set, so this current change makes sense to
me.

[benjhayden, 2016-10-06 20:38:56]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=601953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[benjhayden, 2016-10-06 20:40:59]

On 2016/10/06 at 20:37:12, wkorman wrote:
> On 2016/10/06 20:34:13, nednguyen wrote:
> > On 2016/10/06 20:25:15, benjhayden wrote:
> > > PTAL :-)
> > 
> > lgtm
> > 
> > Though you may want to file a bug about benchmark renaming.
> 
> Yes, LGTM and please file a bug copy me and let's ask nduca@ whether it makes
sense for us to add actual _smooth variants for top_25 and key_mobile_sites.
> 
> My current belief from repo spelunking is that we unintentionally changed this
benchmark to non-smooth, thus not incorporating scrolling, in Feb/Mar 2015 per
the changes I previously linked.
> 
> It would seem to me currently that there's value to having both. And in any
case, the name should match the page set, so this current change makes sense to
me.

Updated the BUG=.
I'll email nduca.
Committing.
Thanks!

[benjhayden, 2016-10-06 20:55:25]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-10-06 20:55:26]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2388423002/#ps20001
(title: "not smooth")

[commit-bot: I haz the power, 2016-10-06 20:56:09]

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-10-06 22:57:30]

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

[commit-bot: I haz the power, 2016-10-06 22:57:31]

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

[benjhayden, 2016-10-07 03:56:43]

The CQ bit was checked by benjhayden@chromium.org

[commit-bot: I haz the power, 2016-10-07 03:56:56]

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-10-07 12:22:38]

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

[commit-bot: I haz the power, 2016-10-07 12:22:39]

Try jobs failed on following builders:
  android_s5_perf_cq on master.tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_...)

[benjhayden, 2016-10-07 19:10:35]

The CQ bit was checked by benjhayden@chromium.org

[commit-bot: I haz the power, 2016-10-07 19:11:12]

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

[benjhayden, 2016-10-07 19:26:53]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[commit-bot: I haz the power, 2016-10-07 19:34:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 19:35:41]

Description was changed from

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Make rasterize_and_record_micro.key_mobile_sites_smooth actually use
KeyMobileSites*Smooth*PageSet.

This benchmark currently uses KeyMobileSitesPageSet despite the name.

This change will cause data stoppage alerts for
rasterize_and_record_micro.key_mobile_sites_smooth and
rasterize_and_record_micro.top_25_smooth

The new benchmarks will need monitoring updated.

BUG=653670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/eeb7021c7962a466e254557f9164b742ac174c19
Cr-Commit-Position: refs/heads/master@{#423931}
==========

[commit-bot: I haz the power, 2016-10-07 19:35:43]

Patchset 2 (id:??) landed as
https://crrev.com/eeb7021c7962a466e254557f9164b742ac174c19
Cr-Commit-Position: refs/heads/master@{#423931}