Simplify LayoutMetric.

tools/telemetry/telemetry/web_perf/metrics/layout.py

Index: tools/telemetry/telemetry/web_perf/metrics/layout.py
diff --git a/tools/telemetry/telemetry/web_perf/metrics/layout.py b/tools/telemetry/telemetry/web_perf/metrics/layout.py
index baa5295a7aa2a35b73531b03dd292c87b8b76074..a8494e17e72dd40bd4888874ef2d3eafac991b4b 100644
--- a/tools/telemetry/telemetry/web_perf/metrics/layout.py
+++ b/tools/telemetry/telemetry/web_perf/metrics/layout.py
@@ -3,55 +3,53 @@
 # found in the LICENSE file.
 
 import math
-from telemetry.value import scalar
+from telemetry.value import list_of_scalar_values
 from telemetry.web_perf.metrics import timeline_based_metric
 
 
 class LayoutMetric(timeline_based_metric.TimelineBasedMetric):
-  """Computes metrics that measure layout performance, specifically,
-  avg and stddev of CPU time of layout-related trace events:
+  """Reports directly durations of FrameView::performLayout events.
 
-    layout_total_{avg,stddev}: FrameView::layout
-    layout_{avg,stddev}: FrameView::performLayout
-    pre_layout_{avg,stddev}: FrameView::performPreLayoutTasks
-    post_layout_{avg,stddev}: FrameView::performPostLayoutTasks
-    layer_positions_{avg,stddev}: RenderLayer::updateLayerPositionsAfterLayout
+    layout_load: Layout events caused by page load.
+    layout_animation: Layout events caused by user interaction.
 
   Layout happens no more than once per frame, so per-frame-ness is implied.
-  Not all pages have interactions, so interaction records are not required.
   """
-  EVENTS = {
-    'FrameView::layout': 'layout_total',
-    'FrameView::performLayout': 'layout',
-    'FrameView::performPreLayoutTasks': 'pre_layout',
-    'FrameView::performPostLayoutTasks': 'post_layout',
-    'RenderLayer::updateLayerPositionsAfterLayout': 'layer_positions',
-  }
 
   def __init__(self):
     super(LayoutMetric, self).__init__()
 
-  def AddResults(self, _model, renderer_thread, _interaction_records, results):
-    self._AddResults(renderer_thread.parent.IterAllSlices(), results)
-
-  def _AddResults(self, events, results):
-    metrics = dict((long_name, (short_name, [])) for long_name, short_name in
-        self.EVENTS.iteritems())
+  def AddResults(self, _model, renderer_thread, interactions, results):
+    self._AddResultsInternal(renderer_thread.parent.IterAllSlices(),
+                             interactions, results)
 
+  def _AddResultsInternal(self, events, interactions, results):
+    layouts = []
     for event in events:
-      if event.name in metrics:
-        metrics[event.name][1].append(event.end - event.start)
-
-    for long_name, (short_name, durations) in metrics.iteritems():
-      count = len(durations)
-      avg = 0.0
-      stddev = 0.0
-      if count:
-        avg = sum(durations) / count
-        stddev = math.sqrt(sum((d - avg) ** 2 for d in durations) / count)
-      results.AddValue(scalar.ScalarValue(results.current_page,
-          short_name + '_avg', 'ms', avg,
-          description='Average duration of %s events' % long_name))
-      results.AddValue(scalar.ScalarValue(results.current_page,
-          short_name + '_stddev', 'ms', stddev,
-          description='stddev of duration of %s events' % long_name))
+      if ((event.name == 'FrameView::performLayout') and
+          ((not interactions) or any(
+              interaction.start <= event.start < interaction.end
+              for interaction in interactions))):
+        layouts.append(event.end - event.start)
+    if not layouts:
+      return
+    description = 'List of durations of layouts'
+    label = 'layout'
+    if interactions:
+      if interactions[0].label == 'load':

[nednguyen, 2015/03/31 22:49:03]

This sounds about right. However, to check whether an interaction is load, I
think we should rely on something more strictly like type rather than using its
label. Note that anyone can define interaction with any labels in the pages, so
we can get name collision problem with "load".

[benjhayden, 2015/04/01 20:22:45]

Let's punt the load event to another CL.

[nednguyen, 2015/04/01 20:25:50]

I like this. Fixing the TODO is add the Load interaction record may be a sizable
patch.

+        # Loading a page should take no more than 1000ms, so load layout may
+        # take some hundreds of ms total.
+        label += '_load'

[nednguyen, 2015/03/31 22:49:03]

label is supposed to be just interaction record label.

[benjhayden, 2015/04/01 20:22:45]

It sounds like you're saying that the result label should be exactly equal to
the interaction label, but if the result label is exactly equal to the
interaction label, then all of the results would have the same label, and that
label wouldn't say anything about what the results are ("layout"). Right?

So I assume you mean that result labels should include the interaction label
(e.g. "ScrollAction") as well as something to indicate the meaning of the
result, e.g. "layout".

I assume that is recommended because there may be multiple kinds of interactions
in a single page, and TBM would call LayoutMetric once for each kind of
interaction, and the different results should not all have exactly the same
label: neither "layout" nor "ScrollAction", but rather "layout_ScrollAction". Is
that correct?

Here is the problem with including the interaction record labels in the result
labels.

KeySilkCases uses 8 different interaction record names:
ScrollAction
SwipeAction
Action_TapAction
TapAction
Wait
Action_SwipeAction
animation_interaction
RunSmoothAllActions

So some pages would report their animation layout durations as
"layout_ScrollAction", some as "layout_SwipeAction", etc. which would be
confusing since they're all conceptually the same thing, i.e. animation layout
durations. chromeperf would not be able to present them together in a single
graph. If I have to look at 8 different graphs when I should be able to look at
one, then I probably won't ever look at any of them. We might as well not have
any layout metrics.

Should I relabel all of the interactions in KeySilkCases to have the same label?
Or teach TBM to call LayoutMetric once with all of the interaction records
together?
Or is there another way to let chromeperf group all the animation layout
durations from all the pages together?

[nednguyen, 2015/04/01 20:25:50]

label is interaction record label, name is for the metric name.

+      else:
+        # We don't care if the interactions are Smoothness or Click or Swipe or
+        # Scroll or whatever, all we care about is that the layout events were
+        # caused by user interaction rather than load.
+        # Animations should take no more than 150ms to paint the first frame
+        # after an interaction begins, then no more than 16ms for each frame
+        # after that. So the first layout after an interaction begins should
+        # cost no more than 100ms or so, and every layout after that should cost
+        # no more than 5ms or so.
+        label += '_animation'
+      description += ' that start in response to user interaction'
+    results.AddValue(list_of_scalar_values.ListOfScalarValues(
+      results.current_page, label, 'ms', layouts, description=description))