Convert V8GCTimes measurement to timeline based measurement.

Convert V8GCTimes measurement to timeline based measurement.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

Committed: https://crrev.com/29f246f9b289f23d319ffd5c47b024dcda5a96b8
Cr-Commit-Position: refs/heads/master@{#355268}

[ulan, 2015-10-15 09:50:45]

ulan@chromium.org changed reviewers:
+ nednguyen@google.com, skyostil@chromium.org

[ulan, 2015-10-15 09:50:45]

Hi Ned, Sami.

Could you please take a look?

The motivation for this CL is to make V8GCTimes compatible with other TMB
metrics such as smoothness and memory.

[Sami, 2015-10-15 10:21:02]

Looks reasonable to me, but I'll let Ned have the final word.

https://codereview.chromium.org/1400083003/diff/1/tools/perf/measurements/v8_...
File tools/perf/measurements/v8_gc_times.py (right):

https://codereview.chromium.org/1400083003/diff/1/tools/perf/measurements/v8_...
tools/perf/measurements/v8_gc_times.py:1: # Copyright 2013 The Chromium Authors.
All rights reserved.
Going back in time? :)

[ulan, 2015-10-15 10:45:24]

ulan@chromium.org changed reviewers:
+ eakuefner@chromium.org

[ulan, 2015-10-15 10:45:25]

Thanks, Sami. I fixed the date. :)

Ethan, may I redirect this CL to you since Ned is out of office?

[eakuefner, 2015-10-15 15:58:35]

Take a look at https://codereview.chromium.org/1368533002 to see how we'd like
timeline-based benchmarks to be structured. For background, this will eventually
turn into a codelab, but the code stands as-is. High-level overview:

1. Check your TimelineBasedMetric into telemetry/web_perf/metrics.
1. Create a benchmark that uses TimelineBasedMeasurement (which collects all
metrics), and then
3. Use SetTimelineBasedMetrics to filter out the metrics you want within
SetTimelineBasedMeasurementOptions (rather than ValueCanBeAddedPredicate, which
is no longer the recommended approach for this use case).

In particular, you shouldn't actually need to add your own measurement.

https://codereview.chromium.org/1400083003/diff/20001/tools/perf/measurements...
File tools/perf/measurements/v8_gc_times.py (right):

https://codereview.chromium.org/1400083003/diff/20001/tools/perf/measurements...
tools/perf/measurements/v8_gc_times.py:11: class
_CustomResultsWrapper(timeline_based_measurement.ResultsWrapperInterface):
Please don't use a results wrapper. This has been a stopgap that we've used to
temporarily ensure continuity of smoothness data while we've been working on
launching TBM. Nowadays, timeline-based metrics should specify an interaction
record label for each value, when adding a value.

Check out go/dashboard-tbm-announce to see how results are displayed with their
interaction records on the dashboard now.

This will change the test paths for the data being uploaded, so if continuity of
data is a concern, check out go/dashboard-reports-announce to see how you can
use the new reports page to combine data from multiple tests in one chart.

[ulan, 2015-10-15 17:39:59]

Thanks for the helpful instructions, Ethan.

I moved the metric to telemetry/web_perf/metrics/v8_latency.py and tested that
it works on a dummy benchmark (see v8.py in patch set 3) without creating a new
measurement.

I will check-in a real benchmark once (crbug.com/543564) is fixed.

The new benchmark will replace v8.top25_smooth, so I am leaving v8.top25_smooth
unchanged until then.

[ulan, 2015-10-15 17:42:42]

Ross, Hannes: I am converting V8GCTimes to TBM and changing its name to
V8Lantency. Are you ok with the new name?

[Hannes Payer (out of office), 2015-10-15 19:09:25]

On 2015/10/15 17:42:42, ulan wrote:
> Ross, Hannes: I am converting V8GCTimes to TBM and changing its name to
> V8Lantency. Are you ok with the new name?

What about V8GCLatency, because it is just about GC latency?

[ulan, 2015-10-15 19:31:15]

> What about V8GCLatency, because it is just about GC latency?

Done.

[ulan, 2015-10-16 14:09:08]

I am adding support for interaction records (the old V8GCTimes metric was
ignoring them).

Please do not review until I upload new PS.

[rmcilroy, 2015-10-16 14:10:29]

V8GCLatency SGTM, thanks.

[rmcilroy, 2015-10-16 14:10:29]

V8GCLatency SGTM, thanks.

[ulan, 2015-10-19 11:12:19]

I uploaded the final version.

Ethan, could you please check the style and rubber-stamp the rest? Since the
metric is v8 specific, there is no need for you to spend time understanding it.

This CL will not break existing benchmarks because it will be used in the new V8
benchmark.

I added a new test that checks this metric with multiple interaction records.

I tested the metric locally in the new V8 benchmark.

[eakuefner, 2015-10-19 16:57:59]

Looking good -- personally I'm fine with the approach of landing the metric
before the benchmark, since these types of CLs will become two-sided anyway as
soon as Telemetry is moved to Catapult.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py (right):

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:47: def
AddResults(self, model, renderer_thread, interaction_records, results):
Move self.VerifyNonOverlappedRecords here and remove the conditional below.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:52: if not
interaction_records:
Remove this (as per above)

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:58: for (label,
events) in events_by_label.iteritems():
nit: no parens here (just write for label, events ...)

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:90:
results.current_page, v8_event_stat.result_name, 'ms',
We're working on making all values specify an improvement_direction, so please
give all your values an improvement_direction as follows:

from telemetry.value import improvement_direction

...

results.AddValue(...(..., improvement_direction=improvement_direction.{UP,
DOWN}))

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:169: v8_event_names
= {x.src_event_name : True for x in self._V8_EVENT_STATS}
Maybe you want to build a list instead of a dict here, since the value is the
same for all keys?

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:173: def
EventIsInInterval(event, interval):
I'm concerned about this pattern -- it seems like you've gone for pure
efficiency here but it makes the code harder to read and less Pythonic.

Instead of sorting and filtering up front, use for loops and prefer filtering on
the fly. This may result in some duplicated computations, but will make the code
much less verbose and easier to read.

I think the structure of blob_timeline is a good example here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

Note in this example how dmurph has a number of predicates that he uses to check
events as he sees them, uses IsEventInInteraction (which we'd now replace with
timeline_based_metric.IsEventInInteractions, which has been added since), and
liberally uses the built-in any predicate. The result is a relatively small
amount of code that is easy to read.

[eakuefner, 2015-10-19 19:24:09]

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py (right):

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:48:
self._AddV8MetricsToResults(renderer_thread, interaction_records, results)
One more note: please move to querying the model, rather than renderer_thread
directly, if possible. AddResults currently still has a renderer_thread as a
fallback for smoothness, but in most cases you should just be able to look at
the model directly (again see blob_metrics).

We're in the process of changing how TBM works slightly, so that soon, you will
write your metric's code as a Performance Insights 'mapper', written in JS,
which you'll call from the benchmark, and querying the model now will make it
much easier to port this metric to JS.

[ulan, 2015-10-20 11:10:19]

Thank you for the review, Ethan!

I uploaded a new patch set.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py (right):

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:47: def
AddResults(self, model, renderer_thread, interaction_records, results):
On 2015/10/19 16:57:58, eakuefner wrote:
> Move self.VerifyNonOverlappedRecords here and remove the conditional below.

Done.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:48:
self._AddV8MetricsToResults(renderer_thread, interaction_records, results)
On 2015/10/19 19:24:09, eakuefner wrote:
> One more note: please move to querying the model, rather than renderer_thread
> directly, if possible. AddResults currently still has a renderer_thread as a
> fallback for smoothness, but in most cases you should just be able to look at
> the model directly (again see blob_metrics).
> 
> We're in the process of changing how TBM works slightly, so that soon, you
will
> write your metric's code as a Performance Insights 'mapper', written in JS,
> which you'll call from the benchmark, and querying the model now will make it
> much easier to port this metric to JS.

Done.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:52: if not
interaction_records:
On 2015/10/19 16:57:58, eakuefner wrote:
> Remove this (as per above)

Done.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:58: for (label,
events) in events_by_label.iteritems():
On 2015/10/19 16:57:58, eakuefner wrote:
> nit: no parens here (just write for label, events ...)

Removed this code.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:90:
results.current_page, v8_event_stat.result_name, 'ms',
On 2015/10/19 16:57:58, eakuefner wrote:
> We're working on making all values specify an improvement_direction, so please
> give all your values an improvement_direction as follows:
> 
> from telemetry.value import improvement_direction
> 
> ...
> 
> results.AddValue(...(..., improvement_direction=improvement_direction.{UP,
> DOWN}))

Done.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:169: v8_event_names
= {x.src_event_name : True for x in self._V8_EVENT_STATS}
On 2015/10/19 16:57:58, eakuefner wrote:
> Maybe you want to build a list instead of a dict here, since the value is the
> same for all keys?

Removed this code.

https://codereview.chromium.org/1400083003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:173: def
EventIsInInterval(event, interval):
On 2015/10/19 16:57:58, eakuefner wrote:
> I'm concerned about this pattern -- it seems like you've gone for pure
> efficiency here but it makes the code harder to read and less Pythonic.
> 
> Instead of sorting and filtering up front, use for loops and prefer filtering
on
> the fly. This may result in some duplicated computations, but will make the
code
> much less verbose and easier to read.
> 
> I think the structure of blob_timeline is a good example here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> Note in this example how dmurph has a number of predicates that he uses to
check
> events as he sees them, uses IsEventInInteraction (which we'd now replace with
> timeline_based_metric.IsEventInInteractions, which has been added since), and
> liberally uses the built-in any predicate. The result is a relatively small
> amount of code that is easy to read.

Done. I replaced O(|interaction_record| + |events|) with shorter and simpler
O(|interaction_record|*|events|) algorithm. I don't expect the number of
interaction records to be large, so it is not much slower.

[eakuefner, 2015-10-20 22:17:27]

lgtm

[eakuefner, 2015-10-20 22:46:29]

https://codereview.chromium.org/1400083003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py (right):

https://codereview.chromium.org/1400083003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:48: def
AddResults(self, model, renderer_thread, interaction_records, results):
Actually, looking at your computations below I think this may still not be right
-- AddResults is called in a loop by TBM, which passes a list of interaction
records that all have the same label.

So you probably don't need to iterate over the interaction records and add one
value for each interaction. Instead, for each value you want to create, you can
sum/max/average/etc. across all the interaction records in the list.

[ulan, 2015-10-21 09:40:16]

https://codereview.chromium.org/1400083003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py (right):

https://codereview.chromium.org/1400083003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/v8_gc_latency.py:48: def
AddResults(self, model, renderer_thread, interaction_records, results):
On 2015/10/20 22:46:29, eakuefner wrote:
> Actually, looking at your computations below I think this may still not be
right
> -- AddResults is called in a loop by TBM, which passes a list of interaction
> records that all have the same label.
> 
> So you probably don't need to iterate over the interaction records and add one
> value for each interaction. Instead, for each value you want to create, you
can
> sum/max/average/etc. across all the interaction records in the list.

You're right, thanks for catching this. I thought that AddResults gets all the
interactions.

I fixed the code.

[ulan, 2015-10-21 09:41:18]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-10-21 09:41:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org
Link to the patchset: https://codereview.chromium.org/1400083003/#ps170001
(title: "Fix adding results for multiple interaction records")

[commit-bot: I haz the power, 2015-10-21 09:41:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1400083003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1400083003/170001

[commit-bot: I haz the power, 2015-10-21 09:48:58]

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

[commit-bot: I haz the power, 2015-10-21 09:48:59]

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

[ulan, 2015-10-21 09:51:34]

Sami, Ned, could you please rubber-stamp as owners?

[Sami, 2015-10-21 10:11:30]

rs lgtm.

[ulan, 2015-10-21 10:59:18]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2015-10-21 10:59:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1400083003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1400083003/170001

[commit-bot: I haz the power, 2015-10-21 11:03:33]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-10-21 11:04:13]

Patchset 10 (id:??) landed as
https://crrev.com/29f246f9b289f23d319ffd5c47b024dcda5a96b8
Cr-Commit-Position: refs/heads/master@{#355268}