Add telemetry test for paper calculator hit testing performance.

Add telemetry test for paper calculator hit testing performance.

This CL adds a new page set |key_hit_test_cases|, the reason to have a separate
set is that we only get meaningful data for |thread_times| and adding it to
|key_silk_cases| would clutter the smoothness numbers.

The main metric is |renderer_main_cpu_time_per_frame|, since the page doesn't
do any rendering, all the hit tests get counted under a single frame budget,
which can be used as a signal for the total cost.

BUG=426406
Committed: https://crrev.com/fafaad6de1e2441262a274385de89e662a2df605
Cr-Commit-Position: refs/heads/master@{#308052}

[Zeeshan Qureshi, 2014-12-10 00:31:59]

zeeshanq@chromium.org changed reviewers:
+ rbyers@chromium.org

[Zeeshan Qureshi, 2014-12-10 00:31:59]

Rick, WDYT?

[Rick Byers, 2014-12-10 02:56:43]

Thanks for getting this up Zeeshan!  I'm excited to have some perf coverage for
hit-testing independent from rendering.

You need to add a new pageset for this (rather than, for example, just adding it
to key_silk_cases) because it wouldn't make sense (would pollute the dashboard)
to collect normal smoothness measurements (such as input latency) here, right? 
The page's RunSmoothness action is designed to provide useful data ONLY to the
thread_times measurement, not to the smoothness measurement.  That makes sense,
but seems a little unfortunate.

Perhaps it would make a little more sense if we renamed key_tap_cases to
key_hit_test_cases?  That way the unifying principle of the pageset is not so
much that there's taps involved, but that there's hit tests without rendering
(and so smoothness makes no sense).

I could imagine, for example, adding tapping on quantum calculator to
key_silk_cases because we want to measure the tap input latency and animation
frame rate.

miletus@ / jdduke@ any input?

https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
File tools/perf/benchmarks/thread_times.py (right):

https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
tools/perf/benchmarks/thread_times.py:27: class
ThreadTimesKeyTapCases(_ThreadTimes):
add a comment like the others.

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
File tools/perf/page_sets/key_tap_cases.py (right):

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
tools/perf/page_sets/key_tap_cases.py:32:
url=('http://zqureshi.github.io/paper-calculator/paper-calculator/'
rather than put this up on github and take a wpr of it, we should probably just
check the files into the tree directly.  That way if someone needs, for example,
to change how you've disabled rendering, it's easy.  WPR is really for live
websites we don't control.  See, for example, key_silk_cases and
tough_scrolling_cases in src/tools/perf/page_sets/.

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
tools/perf/page_sets/key_tap_cases.py:36: def RunSmoothness(self,
action_runner):
If you're going to have a key_tap_caess pageset, then I think this function (and
TapButton below) belong on the base class.  You don't want people to have to
repeat them for any other pages added to the set...

[Yufeng Shen (Slow to review), 2014-12-10 20:17:38]

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

[Yufeng Shen (Slow to review), 2014-12-10 20:17:39]

https://codereview.chromium.org/789003002/diff/20001/tools/perf/page_sets/key...
File tools/perf/page_sets/key_hit_test_cases.py (right):

https://codereview.chromium.org/789003002/diff/20001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:19: def RunSmoothness(self,
action_runner):
we probably don't need this. No page in this pageset would ever
want a default scroll action right ?

[Yufeng Shen (Slow to review), 2014-12-10 20:21:52]

On 2014/12/10 02:56:43, Rick Byers wrote:
> Thanks for getting this up Zeeshan!  I'm excited to have some perf coverage
for
> hit-testing independent from rendering.
> 
> You need to add a new pageset for this (rather than, for example, just adding
it
> to key_silk_cases) because it wouldn't make sense (would pollute the
dashboard)
> to collect normal smoothness measurements (such as input latency) here, right?

> The page's RunSmoothness action is designed to provide useful data ONLY to the
> thread_times measurement, not to the smoothness measurement.  That makes
sense,
> but seems a little unfortunate.
> 

Yes, it is a little bit confusing here.  RunSmoothness is more like an interface
for measurement to call, e.g. both thread_times and smoothness will call
RunSmoothness.
And theread_times will compute the metrics it wants, and smoothness will compute
other metrics it wants like input_latency.


> Perhaps it would make a little more sense if we renamed key_tap_cases to
> key_hit_test_cases?  That way the unifying principle of the pageset is not so
> much that there's taps involved, but that there's hit tests without rendering
> (and so smoothness makes no sense).
> 
> I could imagine, for example, adding tapping on quantum calculator to
> key_silk_cases because we want to measure the tap input latency and animation
> frame rate.
> 
> miletus@ / jdduke@ any input?
> 
>
https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
> File tools/perf/benchmarks/thread_times.py (right):
> 
>
https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
> tools/perf/benchmarks/thread_times.py:27: class
> ThreadTimesKeyTapCases(_ThreadTimes):
> add a comment like the others.
> 
>
https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
> File tools/perf/page_sets/key_tap_cases.py (right):
> 
>
https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
> tools/perf/page_sets/key_tap_cases.py:32:
> url=('http://zqureshi.github.io/paper-calculator/paper-calculator/'
> rather than put this up on github and take a wpr of it, we should probably
just
> check the files into the tree directly.  That way if someone needs, for
example,
> to change how you've disabled rendering, it's easy.  WPR is really for live
> websites we don't control.  See, for example, key_silk_cases and
> tough_scrolling_cases in src/tools/perf/page_sets/.
> 
>
https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
> tools/perf/page_sets/key_tap_cases.py:36: def RunSmoothness(self,
> action_runner):
> If you're going to have a key_tap_caess pageset, then I think this function
(and
> TapButton below) belong on the base class.  You don't want people to have to
> repeat them for any other pages added to the set...

[Yufeng Shen (Slow to review), 2014-12-10 20:22:19]

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

[Yufeng Shen (Slow to review), 2014-12-10 20:22:49]

+ Ned who's expert on telemetry.

[Zeeshan Qureshi, 2014-12-10 20:34:26]

Thanks, Rick. Changed the name to |key_hit_test_cases|, I think we agreed on
that during our discussion but I lost track of it.

Yes, the reason to have a separate page set is that we only get meaningful data
for thread_times and if we were to add it to key_silk_cases we'd clutter the
smoothness numbers.

https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
File tools/perf/benchmarks/thread_times.py (right):

https://codereview.chromium.org/789003002/diff/1/tools/perf/benchmarks/thread...
tools/perf/benchmarks/thread_times.py:27: class
ThreadTimesKeyTapCases(_ThreadTimes):
On 2014/12/10 02:56:43, Rick Byers wrote:
> add a comment like the others.

Done.

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
File tools/perf/page_sets/key_tap_cases.py (right):

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
tools/perf/page_sets/key_tap_cases.py:32:
url=('http://zqureshi.github.io/paper-calculator/paper-calculator/'
On 2014/12/10 02:56:43, Rick Byers wrote:
> rather than put this up on github and take a wpr of it, we should probably
just
> check the files into the tree directly.  That way if someone needs, for
example,
> to change how you've disabled rendering, it's easy.  WPR is really for live
> websites we don't control.  See, for example, key_silk_cases and
> tough_scrolling_cases in src/tools/perf/page_sets/.

Done, and the file's not too big after all.

https://codereview.chromium.org/789003002/diff/1/tools/perf/page_sets/key_tap...
tools/perf/page_sets/key_tap_cases.py:36: def RunSmoothness(self,
action_runner):
On 2014/12/10 02:56:43, Rick Byers wrote:
> If you're going to have a key_tap_caess pageset, then I think this function
(and
> TapButton below) belong on the base class.  You don't want people to have to
> repeat them for any other pages added to the set...

Since the tap action for each page will be slightly different, I'd rather leave
it to the person updating it to move this up into base.

[Zeeshan Qureshi, 2014-12-10 20:36:25]

https://codereview.chromium.org/789003002/diff/20001/tools/perf/page_sets/key...
File tools/perf/page_sets/key_hit_test_cases.py (right):

https://codereview.chromium.org/789003002/diff/20001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:19: def RunSmoothness(self,
action_runner):
On 2014/12/10 20:17:39, Yufeng Shen wrote:
> we probably don't need this. No page in this pageset would ever
> want a default scroll action right ? 

Done.

[jdduke (slow), 2014-12-10 22:27:13]

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

[jdduke (slow), 2014-12-10 22:27:14]

What do the numbers look like here when you test locally? e.g., if you double
the hit test time synthetically, is that dwarfed by noise from repeated runs?

[nednguyen, 2014-12-10 22:33:50]

With jdduke's comment: please check to make sure that you get meaningful metrics
out of the benchmark. If not, there is no need to add extra test cases.

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
File tools/perf/page_sets/key_hit_test_cases.py (right):

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:3: # found in the LICENSE file.
nit: add a blank line here

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:25: class
PaperCalculatorHitTest(KeyHitTestCasesPage):
You only need this if you plan to create other pageset classes that subclass
KeyHitTestCasesPage. Otherwise, I would just merge  KeyHitTestCasesPage with
this one.

[Zeeshan Qureshi, 2014-12-11 19:17:30]

The main metric is |renderer_main_cpu_time_per_frame| since the page doesn't do
any rendering, all the hit tests get counted under a single frame budget. I used
this to bisect a regression in RenderView::hitTest (http://crbug.com/440590).

With the offending CL in, the main thread time was 2368ms on a Nexus 7 2013 with
a +=3% variation. Now, with the CL reverted, the number fell down to 1732ms. So
a roughly 70% regression in hitTest showed up as 40% change in the metric, which
I think is stable enough.

[Zeeshan Qureshi, 2014-12-11 19:19:31]

Each run included repeating the page 10 times.

[jdduke (slow), 2014-12-11 19:23:22]

On 2014/12/11 19:17:30, Zeeshan Qureshi wrote:
> The main metric is |renderer_main_cpu_time_per_frame| since the page doesn't
do
> any rendering, all the hit tests get counted under a single frame budget. I
used
> this to bisect a regression in RenderView::hitTest (http://crbug.com/440590).
> 
> With the offending CL in, the main thread time was 2368ms on a Nexus 7 2013
with
> a +=3% variation. Now, with the CL reverted, the number fell down to 1732ms.
So
> a roughly 70% regression in hitTest showed up as 40% change in the metric,
which
> I think is stable enough.

Perfect, yes, that's exactly what we want.

[Rick Byers, 2014-12-11 22:21:26]

LGTM

Please add some text to the CL description (or possibly better - as a comment in
the pageset) which explains how to re-create this HTML file in case someone
needs to update it later.  In particular, a link to your GitHub commit which
modifies the polymer paper calculator would probably be ideal. 

Don't forget to remove "WIP" from the subject and description too.

[Zeeshan Qureshi, 2014-12-12 00:10:52]

On 2014/12/11 22:21:26, Rick Byers wrote:
> LGTM
> 
> Please add some text to the CL description (or possibly better - as a comment
in
> the pageset) which explains how to re-create this HTML file in case someone
> needs to update it later.  In particular, a link to your GitHub commit which
> modifies the polymer paper calculator would probably be ideal. 

Already in the comments in file.

> Don't forget to remove "WIP" from the subject and description too.

Will do.

[Zeeshan Qureshi, 2014-12-12 01:05:29]

zeeshanq@chromium.org changed reviewers:
+ nduca@chromium.org

[Zeeshan Qureshi, 2014-12-12 01:05:30]

+Nat for OWNERS

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
File tools/perf/page_sets/key_hit_test_cases.py (right):

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:3: # found in the LICENSE file.
On 2014/12/10 22:33:49, nednguyen wrote:
> nit: add a blank line here

Done.

https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
tools/perf/page_sets/key_hit_test_cases.py:25: class
PaperCalculatorHitTest(KeyHitTestCasesPage):
On 2014/12/10 22:33:50, nednguyen wrote:
> You only need this if you plan to create other pageset classes that subclass
> KeyHitTestCasesPage. Otherwise, I would just merge  KeyHitTestCasesPage with
> this one.

I believe that's the plan, as we identify more hit testing scenarios we'll add
more pages/subclasses to this set. Unless I'm understanding you  wrong?

[nednguyen, 2014-12-12 02:00:45]

On 2014/12/12 01:05:30, Zeeshan Qureshi wrote:
> +Nat for OWNERS
> 
>
https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
> File tools/perf/page_sets/key_hit_test_cases.py (right):
> 
>
https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
> tools/perf/page_sets/key_hit_test_cases.py:3: # found in the LICENSE file.
> On 2014/12/10 22:33:49, nednguyen wrote:
> > nit: add a blank line here
> 
> Done.
> 
>
https://codereview.chromium.org/789003002/diff/40001/tools/perf/page_sets/key...
> tools/perf/page_sets/key_hit_test_cases.py:25: class
> PaperCalculatorHitTest(KeyHitTestCasesPage):
> On 2014/12/10 22:33:50, nednguyen wrote:
> > You only need this if you plan to create other pageset classes that subclass
> > KeyHitTestCasesPage. Otherwise, I would just merge  KeyHitTestCasesPage with
> > this one.
> 
> I believe that's the plan, as we identify more hit testing scenarios we'll add
> more pages/subclasses to this set. Unless I'm understanding you  wrong?

Ok if that is your plan. I can do owner stamp if you want. lgtm

[Zeeshan Qureshi, 2014-12-12 02:47:29]

> Ok if that is your plan. I can do owner stamp if you want. lgtm

Thanks!

[Zeeshan Qureshi, 2014-12-12 02:48:13]

The CQ bit was checked by zeeshanq@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:49:07]

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

[commit-bot: I haz the power, 2014-12-12 04:55:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 04:56:22]

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

[Rick Byers, 2014-12-12 16:57:04]

On 2014/12/12 00:10:52, Zeeshan Qureshi wrote:
> On 2014/12/11 22:21:26, Rick Byers wrote:
> > LGTM
> > 
> > Please add some text to the CL description (or possibly better - as a
comment
> in
> > the pageset) which explains how to re-create this HTML file in case someone
> > needs to update it later.  In particular, a link to your GitHub commit which
> > modifies the polymer paper calculator would probably be ideal. 
> 
> Already in the comments in file.

Duh, sorry - not sure how I missed that.  Looks great!