[telemetry] Add a V8GCTimes measurement and benchmarks.

[telemetry] Add a V8GCTimes measurement and benchmarks.

Replaces the v8_stats metric with a v8_gc_times
measurement.  This new measurement measures the amount of
time spent in GC during the whole benchmark rather than
just during gestures.

The following gc metrics are measured:
  v8_gc_incremental_marking
  v8_gc_incremental_marking_idle_deadline_overrun
  v8_gc_incremental_marking_outside_idle
  v8_gc_incremental_marking_percentage_idle
  v8_gc_mark_compactor
  v8_gc_mark_compactor_idle_deadline_overrun
  v8_gc_mark_compactor_outside_idle
  v8_gc_mark_compactor_percentage_idle
  v8_gc_scavenger
  v8_gc_scavenger_idle_deadline_overrun
  v8_gc_scavenger_outside_idle
  v8_gc_scavenger_percentage_idle
  v8_gc_total
  v8_gc_total_idle_deadline_overrun
  v8_gc_total_outside_idle

It also measures the duration of the measurement and the
amount of CPU time used.

The v8_garbage_collection_cases benchmark is modified to
use this new v8_gc_times measurement, and v8.top_25_smooth
and v8.key_mobile_sites_smooth benchmarks have been added.

Since v8_garbage_collection_cases was the only benchmark to
use the is_fast metric type, and these measurements are now
done as part of the v8_gc_times measurement now, this CL
also removes the fast metric type.

Perf Sherrifs: Since this CL changes he way in which the GC
measurements are done it may cause a movement in the
v8_garbage_collection_cases benchmarks.

Committed: https://crrev.com/452de5927e54e1da0d57a4b2df8b447f4d442360
Cr-Commit-Position: refs/heads/master@{#314868}

Committed: https://crrev.com/e65bf3eeb5227de0bb05f08169719d06e518de8b
Cr-Commit-Position: refs/heads/master@{#315550}

Committed: https://crrev.com/a398f4b4d5836a37ded04049378860636b430291
Cr-Commit-Position: refs/heads/master@{#315756}

[rmcilroy, 2015-02-04 12:03:53]

rmcilroy@chromium.org changed reviewers:
+ skyostil@chromium.org

[rmcilroy, 2015-02-04 12:03:54]

Sami: Could you please take a look, thanks.

Jochen/Hannes/Simon: FYI but comments are welcome.

[Hannes Payer (out of office), 2015-02-04 12:14:48]

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

[Hannes Payer (out of office), 2015-02-04 12:14:48]

Awesome!!!

[Sami, 2015-02-04 19:19:29]

Looks great! Couple of dumb questions below.

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

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:28: 'gc_incremental_marking',
I wonder if we should have v8 somewhere in the name here? If you want to have
this measurement plus the oilpan one[1] active simultaneously, the results might
look confusing.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:74: if event_stat:
nit: this and the one below would be a little neater as early-outs.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:81: idle_task_overrun =
event.thread_duration - allotted_idle_time
Should we be looking at the wall clock here instead? That's what defines the
deadline and if we miss it because of descheduling, it's still kinda our fault,
right?

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:91: 'Total thread duration spent in ' +
v8_event_stat.result_description))
nit: string interpolation would be a little easier to read (below too).

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:122: 'Total thread duration of all
garbage collection events outside of idle'
Missing a space at end of string.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:126: 'Total idle task deadline overrun
for all idle tasks garbage collection'
Missing a space at end of string.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:130: 'Percentage of the thread duration
of all garbage collection events'
Missing a space at end of string.

[Hannes Payer (out of office), 2015-02-05 10:51:35]

+ulan

[rmcilroy, 2015-02-05 16:23:25]

Thanks for the review.

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

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:28: 'gc_incremental_marking',
On 2015/02/04 19:19:28, Sami wrote:
> I wonder if we should have v8 somewhere in the name here? If you want to have
> this measurement plus the oilpan one[1] active simultaneously, the results
might
> look confusing.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...

Sounds good, done.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:74: if event_stat:
On 2015/02/04 19:19:28, Sami wrote:
> nit: this and the one below would be a little neater as early-outs.

Agree for this one, but not the one below.  If you feel strongly I'll change
that one to though.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:81: idle_task_overrun =
event.thread_duration - allotted_idle_time
On 2015/02/04 19:19:28, Sami wrote:
> Should we be looking at the wall clock here instead? That's what defines the
> deadline and if we miss it because of descheduling, it's still kinda our
fault,
> right?

Hmm yeah, good point.  I've done this, but it means we are comparing
thread_duration and wall_duration which doesn't seem good.  Do avoid this I've
assumed that the amount of "inside_idle" thread duration time is proportional to
the ratio of idle wall duration time (simplistic but hopefully reasonable
assumption in general).

I've added an extra test to check this.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:91: 'Total thread duration spent in ' +
v8_event_stat.result_description))
On 2015/02/04 19:19:29, Sami wrote:
> nit: string interpolation would be a little easier to read (below too).

Done.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:122: 'Total thread duration of all
garbage collection events outside of idle'
On 2015/02/04 19:19:29, Sami wrote:
> Missing a space at end of string.

Done.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:126: 'Total idle task deadline overrun
for all idle tasks garbage collection'
On 2015/02/04 19:19:28, Sami wrote:
> Missing a space at end of string.

Done.

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:130: 'Percentage of the thread duration
of all garbage collection events'
On 2015/02/04 19:19:28, Sami wrote:
> Missing a space at end of string.

Done.

[Sami, 2015-02-05 17:29:31]

Thanks, lgtm!

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

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:74: if event_stat:
On 2015/02/05 16:23:25, rmcilroy wrote:
> On 2015/02/04 19:19:28, Sami wrote:
> > nit: this and the one below would be a little neater as early-outs.
> 
> Agree for this one, but not the one below.  If you feel strongly I'll change
> that one to though.

Ok :)

https://codereview.chromium.org/898673005/diff/1/tools/perf/measurements/v8_g...
tools/perf/measurements/v8_gc_times.py:81: idle_task_overrun =
event.thread_duration - allotted_idle_time
On 2015/02/05 16:23:25, rmcilroy wrote:
> On 2015/02/04 19:19:28, Sami wrote:
> > Should we be looking at the wall clock here instead? That's what defines the
> > deadline and if we miss it because of descheduling, it's still kinda our
> fault,
> > right?
> 
> Hmm yeah, good point.  I've done this, but it means we are comparing
> thread_duration and wall_duration which doesn't seem good.  Do avoid this I've
> assumed that the amount of "inside_idle" thread duration time is proportional
to
> the ratio of idle wall duration time (simplistic but hopefully reasonable
> assumption in general).
> 
> I've added an extra test to check this.

That seems like a reasonable assumption given the numbers we have to work with.

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

https://codereview.chromium.org/898673005/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_gc_times.py:91: event.duration -
idle_task_wall_overrun, event.duration)
nit: this is the same as DivideIfPossibleOrZero(allotted_idle_time,
event.duration)

[rmcilroy, 2015-02-05 18:06:19]

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

https://codereview.chromium.org/898673005/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_gc_times.py:91: event.duration -
idle_task_wall_overrun, event.duration)
On 2015/02/05 17:29:31, Sami wrote:
> nit: this is the same as DivideIfPossibleOrZero(allotted_idle_time,
> event.duration)

It's not if "event.duration > allotted_idle_time", in which case
idle_task_wall_overrun is '0', not event.duration - allotted_idle_time.

[Sami, 2015-02-05 18:13:01]

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

https://codereview.chromium.org/898673005/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_gc_times.py:91: event.duration -
idle_task_wall_overrun, event.duration)
On 2015/02/05 18:06:19, rmcilroy wrote:
> On 2015/02/05 17:29:31, Sami wrote:
> > nit: this is the same as DivideIfPossibleOrZero(allotted_idle_time,
> > event.duration)
> 
> It's not if "event.duration > allotted_idle_time", in which case
> idle_task_wall_overrun is '0', not event.duration - allotted_idle_time.

Ack, I misread this as being inside the |if| block. nvm :)

[rmcilroy, 2015-02-05 19:22:54]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-02-05 19:24:00]

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

[commit-bot: I haz the power, 2015-02-05 20:49:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-05 20:50:10]

Patchset 2 (id:??) landed as
https://crrev.com/452de5927e54e1da0d57a4b2df8b447f4d442360
Cr-Commit-Position: refs/heads/master@{#314868}

[Sami, 2015-02-06 15:05:14]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/904903002/ by skyostil@chromium.org.

The reason for reverting is: Failing on mac:

http://chromegw/i/chromium.perf/builders/Mac%2010.9%20Perf%20%281%29/builds/2017
.

[rmcilroy, 2015-02-10 12:38:35]

New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org

[rmcilroy, 2015-02-10 12:39:39]

On 2015/02/06 15:05:14, Sami wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/904903002/ by mailto:skyostil@chromium.org.
> 
> The reason for reverting is: Failing on mac:
> 
>
http://chromegw/i/chromium.perf/builders/Mac%2010.9%20Perf%20%281%29/builds/2017
> .

Relanding this since the telemetry issues on desktop should be fixed now.

[rmcilroy, 2015-02-10 12:39:44]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-02-10 12:40:20]

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

[commit-bot: I haz the power, 2015-02-10 13:41:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-10 13:41:53]

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

[rmcilroy, 2015-02-10 17:18:45]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/908223003/ by rmcilroy@chromium.org.

The reason for reverting is: Broke the perf bots again due to
http://crbug.com/456845.

[Sami, 2015-02-10 17:29:16]

On 2015/02/10 at 17:18:45, rmcilroy wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/908223003/ by rmcilroy@chromium.org.
> 
> The reason for reverting is: Broke the perf bots again due to
http://crbug.com/456845.

Hmm, are we hitting the limit on the reference builds or did the fix not make it
everywhere yet?

[rmcilroy, 2015-02-10 18:04:13]

On 2015/02/10 17:29:16, Sami wrote:
> On 2015/02/10 at 17:18:45, rmcilroy wrote:
> > A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/908223003/ by mailto:rmcilroy@chromium.org.
> > 
> > The reason for reverting is: Broke the perf bots again due to
> http://crbug.com/456845.
> 
> Hmm, are we hitting the limit on the reference builds or did the fix not make
it
> everywhere yet?

Yes it was the ref builds which were failing.  I'll roll a new version of the
ref build tomorrow and try again...

[rmcilroy, 2015-02-11 11:15:36]

On 2015/02/10 18:04:13, rmcilroy wrote:
> On 2015/02/10 17:29:16, Sami wrote:
> > On 2015/02/10 at 17:18:45, rmcilroy wrote:
> > > A revert of this CL (patchset #3 id:40001) has been created in
> > https://codereview.chromium.org/908223003/ by mailto:rmcilroy@chromium.org.
> > > 
> > > The reason for reverting is: Broke the perf bots again due to
> > http://crbug.com/456845.
> > 
> > Hmm, are we hitting the limit on the reference builds or did the fix not
make
> it
> > everywhere yet?
> 
> Yes it was the ref builds which were failing.  I'll roll a new version of the
> ref build tomorrow and try again...

Let's try this again, this time with ref-build runs disabled since the fix
hasn't ended up in the reference build yet (and Annie and Ryan don't want to
update the ref build just yet due to stability concerns).

[rmcilroy, 2015-02-11 11:15:40]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-02-11 11:16:13]

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

[commit-bot: I haz the power, 2015-02-11 13:28:22]

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

[commit-bot: I haz the power, 2015-02-11 13:28:23]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12...)
  win_gpu_triggered_tests on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[rmcilroy, 2015-02-11 13:32:43]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-02-11 13:33:37]

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

[commit-bot: I haz the power, 2015-02-11 14:31:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-11 14:31:35]

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

[nduca, 2015-02-17 17:30:04]

nduca@chromium.org changed reviewers:
+ nduca@chromium.org, nednguyen@chromium.org

[nduca, 2015-02-17 17:30:05]

why are we moving stuff to perf/? That seems backward from the trend to have
timeline metrics in telemetry

[nduca, 2015-02-17 17:32:14]

also i think we should discuss the rationale to move away from tbm instead of
enhancing tbm to do what was desired :(

[rmcilroy, 2015-02-18 09:25:12]

On 2015/02/17 17:32:14, nduca wrote:
> also i think we should discuss the rationale to move away from tbm instead of
> enhancing tbm to do what was desired :(

I wasn't aware of a trend to get stuff out of perf/ and into timeline metrics -
I was basing these benchmarks on the recent task_execution_times benchmark,
which is also in perf/.  Is there any mail / bug explaining the rational /
advantages of moving things out of perf/ into tbm?

The reason I moved this to perf/ was that we wanted the benchmark to calculate
the metric across the whole page loading, not just during gestures as the
previous approach did.  We also wanted to run the top_25 and key_mobile sites
benchmarks which hadn't been possible due the hard-coding of the interaction
records (i.e., requiring is_fast for the interaction records for those pages),
and due to the fact that they would need to run with V8 overhead level which the
TODOs suggested should go away.  We couldn't wait for the interaction record
refactoring because we had been missing some GC regressions due to the fact that
we weren't running the GC benchmarks on real page-sets, so this is what we ended
up with.

Happy to discuss in person about how we can move this back to tbm once the
necessary refactoring has been done on tbm.