Explicitly setting story names for all smoothness benchmarks

Explicitly setting story names for all smoothness benchmarks

BUG=chromium:720002
Review-Url: https://codereview.chromium.org/2926633002
Cr-Commit-Position: refs/heads/master@{#478284}
Committed: https://chromium.googlesource.com/chromium/src/+/11ffc63960b436ad6a73fcbd1d1621188be057b9

[ashleymarie1, 2017-06-06 19:40:53]

ashleymarie@chromium.org changed reviewers:
+ ajuma@chromium.org, alancutter@chromium.org, bokan@chromium.org,
cblume@chromium.org, ericrk@chromium.org, junov@google.com, kbr@chromium.org,
nednguyen@google.com, picksi@google.com, rbyers@chromium.org,
reveman@chromium.org, senorblanco@google.com, skyostil@chromium.org,
ssid@chromium.org, tdresser@chromium.org, vmiura@chromium.org, zmo@google.com

[ashleymarie1, 2017-06-06 19:41:01]

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

[commit-bot: I haz the power, 2017-06-06 19:41:27]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-06 19:44:30]

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

[commit-bot: I haz the power, 2017-06-06 19:44:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ashleymarie1, 2017-06-06 19:56:55]

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

[commit-bot: I haz the power, 2017-06-06 19:57:26]

Dry run: CQ is trying da patch.

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

[Ken Russell (switch to Gerrit), 2017-06-06 19:59:22]

webgl cases lgtm

[nednguyen, 2017-06-06 19:59:50]

On 2017/06/06 19:59:22, Ken Russell wrote:
> webgl cases lgtm

lgtm

[cblume, 2017-06-06 20:10:59]

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/im...
File tools/perf/page_sets/image_decoding_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/im...
tools/perf/page_sets/image_decoding_cases.py:29:
self.AddStory(ImageDecodingCasesPage(url, self, name='yuv_decoding.html'))
The only url in the urls_list is yuv_decoding.
This line hard-codes that name in.

But if we added another url to the urls_list, would this hard-coded value then
be incorrect?

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_desktop_move_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_desktop_move_cases.py:125:
self.AddStory(GoogleMapsPage(self))
In this case a name is not explicitly specified.
Instead, the url is used if no name is provided.

This differs from what happens in image_decoding_cases.py. Plus this will be the
full url where over there it is just the page name.

This also differs from tough_ad_cases.py where this patch splits the full url to
use the page name as the name.

And it differs again I think in tough_canvas_cases.py, where we seem to trim off
the leading "http://".

Perhaps we want to consolidate onto one method?

In this case (where this comment is) the url doesn't really have a page name. So
maybe we should consolidate onto the full url? And if so, maybe it is okay to
consolidate into using the url as a name if no name is explicitly specified
(like in this .py)?

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ma...
File tools/perf/page_sets/maps.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ma...
tools/perf/page_sets/maps.py:19: name='http://localhost:8000/performance.html')
I am bad at python but is there a way to have both url= and name= reference the
same value? Maybe a constant somewhere?

Somebody might come later and update one value without updating the other.

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_filters_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_filters_cases.py:55: 'Pirates/Default.html')))
Here is another spot where perhaps we could use a constant value instead of
specifying the value twice.

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_path_rendering_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_path_rendering_cases.py:51:
name='https://testdrive-archive.azurewebsites.net/performance/chalkboard/'))
Here is another spot where perhaps we could use a constant value instead of
specifying the value twice.

[bokan, 2017-06-06 20:12:10]

Just a comment but no need to wait on my approval if telemetry OWNERs have
stamped as I don't have the context to understand this change.

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_pinch_zoom_cases.py (right):

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_pinch_zoom_cases.py:13: name = url
Not sure I understand, this looks to me to be the opposite of what the commit
message and bug describe.

From the bug: "We do not currently always explicitly set the name of stories in
story sets. It then defaults to using the URL". Isn't this what the bug is
trying to get away from?

[ashleymarie1, 2017-06-06 20:35:01]

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/im...
File tools/perf/page_sets/image_decoding_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/im...
tools/perf/page_sets/image_decoding_cases.py:29:
self.AddStory(ImageDecodingCasesPage(url, self, name='yuv_decoding.html'))
On 2017/06/06 20:10:58, cblume wrote:
> The only url in the urls_list is yuv_decoding.
> This line hard-codes that name in.
> 
> But if we added another url to the urls_list, would this hard-coded value then
> be incorrect?
Good point! I update the urls_list to be a list of tuples url, name
PTAL at patchset 6+

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_desktop_move_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_desktop_move_cases.py:125:
self.AddStory(GoogleMapsPage(self))
On 2017/06/06 20:10:58, cblume wrote:
> In this case a name is not explicitly specified.
> Instead, the url is used if no name is provided.
> 
> This differs from what happens in image_decoding_cases.py. Plus this will be
the
> full url where over there it is just the page name.
> 
> This also differs from tough_ad_cases.py where this patch splits the full url
to
> use the page name as the name.
> 
> And it differs again I think in tough_canvas_cases.py, where we seem to trim
off
> the leading "http://".
> 
> Perhaps we want to consolidate onto one method?
> 
> In this case (where this comment is) the url doesn't really have a page name.
So
> maybe we should consolidate onto the full url? And if so, maybe it is okay to
> consolidate into using the url as a name if no name is explicitly specified
> (like in this .py)?
So what I'm trying to do with all these different methods is keep the generated
names the same as what has been historically generated in telemetry code to
prevent the need for migrations on the dashboard. I agree it would be much nicer
if all of them were consistent but that would require weeks or months of
migrations. For the benchmarks that we know aren't going to be changing / are
stable, we are doing the migrations, but as far as I'm aware, smoothness isn't
on the list of fully stabilized yet.

What we're doing with this specific bug is taking the weird logic generation out
of telemetry and requiring the name be provided to telemetry when the story is
initiated.

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ma...
File tools/perf/page_sets/maps.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ma...
tools/perf/page_sets/maps.py:19: name='http://localhost:8000/performance.html')
On 2017/06/06 20:10:58, cblume wrote:
> I am bad at python but is there a way to have both url= and name= reference
the
> same value? Maybe a constant somewhere?
> 
> Somebody might come later and update one value without updating the other.

Done.

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_filters_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_filters_cases.py:55: 'Pirates/Default.html')))
On 2017/06/06 20:10:58, cblume wrote:
> Here is another spot where perhaps we could use a constant value instead of
> specifying the value twice.

Done.

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_path_rendering_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_path_rendering_cases.py:51:
name='https://testdrive-archive.azurewebsites.net/performance/chalkboard/'))
On 2017/06/06 20:10:58, cblume wrote:
> Here is another spot where perhaps we could use a constant value instead of
> specifying the value twice.

Done.

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_pinch_zoom_cases.py (right):

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_pinch_zoom_cases.py:13: name = url
On 2017/06/06 20:12:10, bokan wrote:
> Not sure I understand, this looks to me to be the opposite of what the commit
> message and bug describe.
> 
> From the bug: "We do not currently always explicitly set the name of stories
in
> story sets. It then defaults to using the URL". Isn't this what the bug is
> trying to get away from?

Sorry for the confusion. The goal of the bug is to push name generation logic
out of telemetry and into story initialization. Right now, there are 5+ ways
that telemetry identifies a story: name, url, id, display_name, etc. We'd like
to pull that back into one or two ways. display_name does a lot of funky stuff
to come up with a reasonable name for a nameless story so we're gonna kill off
that logic and just require all stories have names. If story sets want to
generate page names dynamically, that is fine. We just don't want to do it on
the telemetry side.

[bokan, 2017-06-06 20:45:08]

touch_pinch_zoom_cases lgtm

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_pinch_zoom_cases.py (right):

https://codereview.chromium.org/2926633002/diff/80001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_pinch_zoom_cases.py:13: name = url
On 2017/06/06 20:35:00, ashleymarie1 wrote:
> On 2017/06/06 20:12:10, bokan wrote:
> > Not sure I understand, this looks to me to be the opposite of what the
commit
> > message and bug describe.
> > 
> > From the bug: "We do not currently always explicitly set the name of stories
> in
> > story sets. It then defaults to using the URL". Isn't this what the bug is
> > trying to get away from?
> 
> Sorry for the confusion. The goal of the bug is to push name generation logic
> out of telemetry and into story initialization. Right now, there are 5+ ways
> that telemetry identifies a story: name, url, id, display_name, etc. We'd like
> to pull that back into one or two ways. display_name does a lot of funky stuff
> to come up with a reasonable name for a nameless story so we're gonna kill off
> that logic and just require all stories have names. If story sets want to
> generate page names dynamically, that is fine. We just don't want to do it on
> the telemetry side.

Got it, thanks for the explanation!

[cblume, 2017-06-06 21:07:17]

non-owner LGTM

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_desktop_move_cases.py (right):

https://codereview.chromium.org/2926633002/diff/40001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_desktop_move_cases.py:125:
self.AddStory(GoogleMapsPage(self))
On 2017/06/06 20:35:00, ashleymarie1 wrote:
> On 2017/06/06 20:10:58, cblume wrote:
> > In this case a name is not explicitly specified.
> > Instead, the url is used if no name is provided.
> > 
> > This differs from what happens in image_decoding_cases.py. Plus this will be
> the
> > full url where over there it is just the page name.
> > 
> > This also differs from tough_ad_cases.py where this patch splits the full
url
> to
> > use the page name as the name.
> > 
> > And it differs again I think in tough_canvas_cases.py, where we seem to trim
> off
> > the leading "http://".
> > 
> > Perhaps we want to consolidate onto one method?
> > 
> > In this case (where this comment is) the url doesn't really have a page
name.
> So
> > maybe we should consolidate onto the full url? And if so, maybe it is okay
to
> > consolidate into using the url as a name if no name is explicitly specified
> > (like in this .py)?
> So what I'm trying to do with all these different methods is keep the
generated
> names the same as what has been historically generated in telemetry code to
> prevent the need for migrations on the dashboard. I agree it would be much
nicer
> if all of them were consistent but that would require weeks or months of
> migrations. For the benchmarks that we know aren't going to be changing / are
> stable, we are doing the migrations, but as far as I'm aware, smoothness isn't
> on the list of fully stabilized yet.
> 
> What we're doing with this specific bug is taking the weird logic generation
out
> of telemetry and requiring the name be provided to telemetry when the story is
> initiated.

Ah okay, I follow.
Thank you for the clarification. :D

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/i...
File tools/perf/page_sets/image_decoding_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/i...
tools/perf/page_sets/image_decoding_cases.py:29:
self.AddStory(ImageDecodingCasesPage(url, self, name=name))
Once again, I am a python noob.
But I think you meant just "name" here and not "name=name"?

[Justin Novosad, 2017-06-06 21:32:26]

canvas cases lgtm

[ssid, 2017-06-06 23:51:07]

Description was changed from

==========
Explicitly setting story names for all smoothness benchmarks

BUG=chromium:720002
==========

to

==========
Explicitly setting story names for all smoothness benchmarks

BUG=chromium:720002
==========

[ssid, 2017-06-06 23:51:07]

ssid@chromium.org changed reviewers:
- ssid@chromium.org

[alancutter (OOO until 2018), 2017-06-07 07:32:19]

alancutter@chromium.org changed reviewers:
+ junov@chromium.org

[alancutter (OOO until 2018), 2017-06-07 07:32:19]

tough_animation_cases rs lgtm.

[Sami, 2017-06-07 09:55:05]

rs lgtm.

[Stephen White, 2017-06-07 13:29:29]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2017-06-07 13:29:30]

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
File tools/perf/page_sets/tough_filters_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
tools/perf/page_sets/tough_filters_cases.py:54:
self.AddStory(PirateMarkPage(pirate_url, self, name=pirate_url))
This one is pretty stable. Would you mind giving it the proper name "IE
PirateMark"? I realize this will cause a break in the graph data, but I think
it's worth it to get rid of that ugly URL.

Similarly,

"http://rawgit.com/WebKit/webkit/master..." -> "MotionMark Focus"

"http://static.bobdo.net/Analog_Clock.svg" -> "Analog Clock SVG"

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
File tools/perf/page_sets/tough_path_rendering_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
tools/perf/page_sets/tough_path_rendering_cases.py:51:
self.AddStory(ChalkboardPage(chalkboard_url, self, name=chalkboard_url))
Same here: display name should be "IE Chalkboard".

[ashleymarie1, 2017-06-07 20:52:52]

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/i...
File tools/perf/page_sets/image_decoding_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/i...
tools/perf/page_sets/image_decoding_cases.py:29:
self.AddStory(ImageDecodingCasesPage(url, self, name=name))
On 2017/06/06 21:07:16, cblume wrote:
> Once again, I am a python noob.
> But I think you meant just "name" here and not "name=name"?

I'm a python noob as well and you're right! Since I made the name field required
in ImageDecodingCasesPage init, I don't need to say name=name and can just say
name

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
File tools/perf/page_sets/tough_filters_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
tools/perf/page_sets/tough_filters_cases.py:54:
self.AddStory(PirateMarkPage(pirate_url, self, name=pirate_url))
On 2017/06/07 13:29:30, Stephen White wrote:
> This one is pretty stable. Would you mind giving it the proper name "IE
> PirateMark"? I realize this will cause a break in the graph data, but I think
> it's worth it to get rid of that ugly URL.
> 
> Similarly,
> 
> "http://rawgit.com/WebKit/webkit/master..." -> "MotionMark Focus"
> 
> "http://static.bobdo.net/Analog_Clock.svg" -> "Analog Clock SVG"

Done.

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
File tools/perf/page_sets/tough_path_rendering_cases.py (right):

https://codereview.chromium.org/2926633002/diff/120001/tools/perf/page_sets/t...
tools/perf/page_sets/tough_path_rendering_cases.py:51:
self.AddStory(ChalkboardPage(chalkboard_url, self, name=chalkboard_url))
On 2017/06/07 13:29:30, Stephen White wrote:
> Same here: display name should be "IE Chalkboard".

Done.

[Stephen White, 2017-06-07 21:08:20]

filters and path_rendering LGTM

Thanks for the new names!

[Rick Byers, 2017-06-09 01:02:29]

RS LGTM

[ashleymarie1, 2017-06-09 13:37:51]

The CQ bit was checked by ashleymarie@chromium.org

[ashleymarie1, 2017-06-09 13:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
nednguyen@google.com, cblume@chromium.org, skyostil@chromium.org,
alancutter@chromium.org, bokan@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2926633002/#ps160001
(title: "Explicitly setting story names for all smoothness benchmarks")

[commit-bot: I haz the power, 2017-06-09 13:38:01]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-09 15:25:02]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497015471627190,
"parent_rev": "ab9603551915d6e2efe32d56e09963e82ce12dbe", "commit_rev":
"11ffc63960b436ad6a73fcbd1d1621188be057b9"}

[commit-bot: I haz the power, 2017-06-09 15:25:14]

Description was changed from

==========
Explicitly setting story names for all smoothness benchmarks

BUG=chromium:720002
==========

to

==========
Explicitly setting story names for all smoothness benchmarks

BUG=chromium:720002

Review-Url: https://codereview.chromium.org/2926633002
Cr-Commit-Position: refs/heads/master@{#478284}
Committed:
https://chromium.googlesource.com/chromium/src/+/11ffc63960b436ad6a73fcbd1d16...
==========

[commit-bot: I haz the power, 2017-06-09 15:25:15]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/11ffc63960b436ad6a73fcbd1d16...