Move smoothness to the new API

Move smoothness to the new API

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256661

[nednguyen, 2014-02-24 18:57:45]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-02-24 18:59:12]

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

[commit-bot: I haz the power, 2014-02-24 18:59:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[nduca, 2014-03-07 20:05:56]

some feedback but overall looks really close

https://codereview.chromium.org/170183004/diff/80001/tools/perf/measurements/...
File tools/perf/measurements/smoothness.py (right):

https://codereview.chromium.org/170183004/diff/80001/tools/perf/measurements/...
tools/perf/measurements/smoothness.py:20: def
_GetSyntheticDelayCategoriesFromPage(page):
Lets put this code somewhere common. Maybe we pass in synthetic_delays into the
browser.StartTracing call and then behind that we convert it to categories. And
then timeline, smoothness and thread_times metrics all pass in the synthetic
delays if its on the page.

https://codereview.chromium.org/170183004/diff/80001/tools/perf/measurements/...
tools/perf/measurements/smoothness.py:49: # Start tracing for smoothness metric
end with period.

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
File tools/perf/metrics/timeline_based_metric.py (right):

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
tools/perf/metrics/timeline_based_metric.py:6: class _ResultsWrapper(object):
this class should be in TimelineBasedMeasurement

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
tools/perf/metrics/timeline_based_metric.py:37: @WrapResults
i'd rather that the measurement construct this wrapper just before calling
AddResults no the metric

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
File tools/perf/metrics/timeline_interaction_record.py (right):

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
tools/perf/metrics/timeline_interaction_record.py:50: self.start = start
i'd like logical_name to be mandatory

https://codereview.chromium.org/170183004/diff/80001/tools/perf/metrics/timel...
tools/perf/metrics/timeline_interaction_record.py:81: def FromBounds(bounds):
logical name is required

[nduca, 2014-03-12 18:07:23]

lgtm awesome!

https://codereview.chromium.org/170183004/diff/180001/tools/perf/metrics/smoo...
File tools/perf/metrics/smoothness.py (right):

https://codereview.chromium.org/170183004/diff/180001/tools/perf/metrics/smoo...
tools/perf/metrics/smoothness.py:83: # We use 19ms as a somewhat looser
threshold, instead of 1000.0/60.0.
lets file a followup bug and cc:you cc:ernstm to land a simple unittest that
creates the smoothness metric and verifies that it produces plausible data.
right now i think render_stats provides most of the unittest coverage but that
makes me a bit nervous.

[nednguyen, 2014-03-12 18:12:19]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-03-12 18:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nednguyen@google.com/170183004/190001

[commit-bot: I haz the power, 2014-03-12 19:28:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nednguyen@google.com/170183004/190001

[commit-bot: I haz the power, 2014-03-12 21:44:43]

Change committed as 256661

[Sami, 2014-03-13 17:10:48]

Looks like this broke tests where the set of actions run by each page isn't the
same, e.g.
http://build.chromium.org/p/chromium.perf/builders/Win%208%20Perf%20%282%29/b...
(split_animations.html is the only one in that set that doesn't scroll). This
seems to be because we never reset the list of actions in
measurement/smoothness.py between pages.

Dominik and I were also a bit unclear on the motivation here. Why are we merging
separate actions into a single range for the smoothness metric instead of
looking at them individually like before?

[nednguyen, 2014-03-13 20:13:14]

On 2014/03/13 17:10:48, Sami wrote:
> Looks like this broke tests where the set of actions run by each page isn't
the
> same, e.g.
>
http://build.chromium.org/p/chromium.perf/builders/Win%25208%2520Perf%2520%25...
> (split_animations.html is the only one in that set that doesn't scroll). This
> seems to be because we never reset the list of actions in
> measurement/smoothness.py between pages.
> 
> Dominik and I were also a bit unclear on the motivation here. Why are we
merging
> separate actions into a single range for the smoothness metric instead of
> looking at them individually like before?

Hi Sami,
I initially thought smoothness did not support getting the metric for multiple
actions very well due to the fact that there are time gaps between actions due
to telemetry's delays. So to look at each action individually, once would use
timeline_based_measurement which can return the metric for each action.

[Dominik Grewe, 2014-03-13 20:57:46]

On 2014/03/13 20:13:14, nednguyen wrote:
> On 2014/03/13 17:10:48, Sami wrote:
> > Looks like this broke tests where the set of actions run by each page isn't
> the
> > same, e.g.
> >
>
http://build.chromium.org/p/chromium.perf/builders/Win%2525208%252520Perf%252...
> > (split_animations.html is the only one in that set that doesn't scroll).
This
> > seems to be because we never reset the list of actions in
> > measurement/smoothness.py between pages.
> > 
> > Dominik and I were also a bit unclear on the motivation here. Why are we
> merging
> > separate actions into a single range for the smoothness metric instead of
> > looking at them individually like before?
> 
> Hi Sami,
> I initially thought smoothness did not support getting the metric for multiple
> actions very well due to the fact that there are time gaps between actions due
> to telemetry's delays. So to look at each action individually, once would use
> timeline_based_measurement which can return the metric for each action.

Synthetic gestures, such as scrolls, have pretty precise boundaries (we set a
timeline marker in Chrome itself in addition to the Telemetry one). And if I
understand correctly, TImelineBasedMeasurement relies on the page itself to
demarcate the actions, which doesn't seem applicable for third-party pages. But
maybe I'm missing something.

[nednguyen, 2014-03-13 21:14:02]

On 2014/03/13 20:57:46, Dominik Grewe wrote:
> On 2014/03/13 20:13:14, nednguyen wrote:
> > On 2014/03/13 17:10:48, Sami wrote:
> > > Looks like this broke tests where the set of actions run by each page
isn't
> > the
> > > same, e.g.
> > >
> >
>
http://build.chromium.org/p/chromium.perf/builders/Win%252525208%25252520Perf...
> > > (split_animations.html is the only one in that set that doesn't scroll).
> This
> > > seems to be because we never reset the list of actions in
> > > measurement/smoothness.py between pages.
> > > 
> > > Dominik and I were also a bit unclear on the motivation here. Why are we
> > merging
> > > separate actions into a single range for the smoothness metric instead of
> > > looking at them individually like before?
> > 
> > Hi Sami,
> > I initially thought smoothness did not support getting the metric for
multiple
> > actions very well due to the fact that there are time gaps between actions
due
> > to telemetry's delays. So to look at each action individually, once would
use
> > timeline_based_measurement which can return the metric for each action.
> 
> Synthetic gestures, such as scrolls, have pretty precise boundaries (we set a
> timeline marker in Chrome itself in addition to the Telemetry one). And if I
> understand correctly, TImelineBasedMeasurement relies on the page itself to
> demarcate the actions, which doesn't seem applicable for third-party pages.
But
> maybe I'm missing something.

How does smoothness due with more than one actions? For example: scroll,
[telemetry latency], click, [telemetry latency], scroll.

It look like the current smoothness metric implementation flattens the lists of
frame times collected for each action, and then compute the jank metrics from
there. This can create false positive janks if each action is 60fps, but the
telemetry latency is more than 16ms.

The timelineBasedMeasure can be extended to read the markers that already
produced by the synthetic gestures. So instead of outputting 1 set of number for
the case above, it will outputting 2 set of numbers, one for each actions (may
be we should ignore the click action).

[Sami, 2014-03-14 11:54:37]

On 2014/03/13 21:14:02, nednguyen wrote:
> How does smoothness due with more than one actions? For example: scroll,
> [telemetry latency], click, [telemetry latency], scroll.
> 
> It look like the current smoothness metric implementation flattens the lists
of
> frame times collected for each action, and then compute the jank metrics from
> there. This can create false positive janks if each action is 60fps, but the
> telemetry latency is more than 16ms.

Isn't the bigger bug here that we're collection actions across _pages_ (and
that's turning a couple of bots red)? Each page is supposed to be independent,
so I'm not sure when this would make sense.

> The timelineBasedMeasure can be extended to read the markers that already
> produced by the synthetic gestures. So instead of outputting 1 set of number
for
> the case above, it will outputting 2 set of numbers, one for each actions (may
> be we should ignore the click action).

Per-action metrics seems overkill to me; I think we usually care about the
overall smoothness of a particular page and action list combination. If we want
to see individual numbers, we can always add a dedicated test for them.

[nednguyen, 2014-03-14 13:18:14]

On 2014/03/14 11:54:37, Sami wrote:
> On 2014/03/13 21:14:02, nednguyen wrote:
> > How does smoothness due with more than one actions? For example: scroll,
> > [telemetry latency], click, [telemetry latency], scroll.
> > 
> > It look like the current smoothness metric implementation flattens the lists
> of
> > frame times collected for each action, and then compute the jank metrics
from
> > there. This can create false positive janks if each action is 60fps, but the
> > telemetry latency is more than 16ms.
> 
> Isn't the bigger bug here that we're collection actions across _pages_ (and
> that's turning a couple of bots red)? Each page is supposed to be independent,
> so I'm not sure when this would make sense.

Yes, it makes no sense to collect actions across pages. This is a bug in my
implementation, and I will  upload a patch to fix it.

> > The timelineBasedMeasure can be extended to read the markers that already
> > produced by the synthetic gestures. So instead of outputting 1 set of number
> for
> > the case above, it will outputting 2 set of numbers, one for each actions
(may
> > be we should ignore the click action).
> 
> Per-action metrics seems overkill to me; I think we usually care about the
> overall smoothness of a particular page and action list combination. If we
want
> to see individual numbers, we can always add a dedicated test for them.

[Sami, 2014-03-14 13:48:04]

On 2014/03/14 13:18:14, nednguyen wrote:
> Yes, it makes no sense to collect actions across pages. This is a bug in my
> implementation, and I will  upload a patch to fix it.

Thanks!