[telemetry] Add new measurement that counts number of GCs needed to free V8 context.

[telemetry] Add new measurement that counts number of GCs needed to free V8 context.
The lower this number, the more efficient GC.

Add benchmark that reloads top pages and tracks this measurement.

BUG=chromium:450824
Committed: https://crrev.com/c9e8c5b4f21742741f3664faacedda8d769d944e
Cr-Commit-Position: refs/heads/master@{#317551}

[ulan, 2015-02-17 16:33:14]

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

[ulan, 2015-02-17 16:33:15]

PTAL. This depends on https://codereview.chromium.org/927423002/
and https://codereview.chromium.org/934773002/

[ulan, 2015-02-17 16:37:29]

On 2015/02/17 16:33:15, ulan wrote:
> PTAL. This depends on https://codereview.chromium.org/927423002/
> and https://codereview.chromium.org/934773002/

Should I upload data/page_reload_cases.json or is it generated automatically?

[Sami, 2015-02-17 17:14:07]

Thanks, added some suggestions.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
File tools/perf/measurements/v8_detached_context_age_in_gc.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:1: # Copyright 2012 The
Chromium Authors. All rights reserved.
year += 3 :)

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:16: 
nit: add an extra blank line here.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:21: return
max(map(lambda x: x['high'], buckets)) if buckets else 0
nit: max(x['high'] for x in buckets) works too.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:24: class
V8DetachedContextAgeInGC(page_test.PageTest):
Could you please add a unit test for this too?

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:1: # Copyright 2014 The Chromium
Authors. All rights reserved.
year++

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:13: action_runner.Wait(2)
Any significance to the two seconds here? Is there any specific event we could
wait for instead?

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:22: 
nit: add an extra blank line here.

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:30:
archive_data_file='data/page_reload_cases.json',
I think we can reuse the top_pages archive file here. It doesn't look like this
benchmark needs anything special from the recorded pages.

[ulan, 2015-02-18 10:35:56]

Thank for review!

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
File tools/perf/measurements/v8_detached_context_age_in_gc.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:1: # Copyright 2012 The
Chromium Authors. All rights reserved.
On 2015/02/17 17:14:06, Sami wrote:
> year += 3 :)

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:16: 
On 2015/02/17 17:14:06, Sami wrote:
> nit: add an extra blank line here.

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:21: return
max(map(lambda x: x['high'], buckets)) if buckets else 0
On 2015/02/17 17:14:06, Sami wrote:
> nit: max(x['high'] for x in buckets) works too.

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/measurements/v8_d...
tools/perf/measurements/v8_detached_context_age_in_gc.py:24: class
V8DetachedContextAgeInGC(page_test.PageTest):
On 2015/02/17 17:14:06, Sami wrote:
> Could you please add a unit test for this too?

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:1: # Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/02/17 17:14:06, Sami wrote:
> year++

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:13: action_runner.Wait(2)
On 2015/02/17 17:14:06, Sami wrote:
> Any significance to the two seconds here? Is there any specific event we could
> wait for instead?

2 seconds is just an arbitrary number. Is it possible to wait for "page-loaded"
event?

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:22: 
On 2015/02/17 17:14:06, Sami wrote:
> nit: add an extra blank line here.

Done.

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:30:
archive_data_file='data/page_reload_cases.json',
On 2015/02/17 17:14:06, Sami wrote:
> I think we can reuse the top_pages archive file here. It doesn't look like
this
> benchmark needs anything special from the recorded pages.

Done.

[rmcilroy, 2015-02-18 11:20:31]

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

[rmcilroy, 2015-02-18 11:20:31]

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

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:22: return
max(x['high'] for x in buckets) if buckets else 0
is '0' a good value for when we don't find buckets?  If the benchmark starts
failing this might get ignored since it would appear as a perf improvement of
the results to '0'.  Should we just fail here explicitly or can there
legitimately be no data for the V8.DetachedContextAgeInGC measurement?

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:40: for _ in xrange(8):
nit - pull '8' out into a constant.

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:43: delta =
histogram_util.SubtractHistogram(data, self._data_start)
nit - I would do the getHistogram / SubtractHistogram in the
_MaxDetachedContextAge (maybe renamed _GetMaxDetachedContextAge) to keep the
code related to V8.DetachedContextAgeInGC together.

https://codereview.chromium.org/930333002/diff/20001/tools/perf/page_sets/pag...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/20001/tools/perf/page_sets/pag...
tools/perf/page_sets/page_reload_cases.py:11: for _ in xrange(7):
nit - is '7' chosen for an particular reason (I'm assuming it is for the same
reason we do 7 GCs in emergency GCs)?  Could you pull it out into a constant
with a more descriptive name?

[ulan, 2015-02-18 13:33:16]

Thank you, Ross!

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

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:22: return
max(x['high'] for x in buckets) if buckets else 0
On 2015/02/18 11:20:31, rmcilroy wrote:
> is '0' a good value for when we don't find buckets?  If the benchmark starts
> failing this might get ignored since it would appear as a perf improvement of
> the results to '0'.  Should we just fail here explicitly or can there
> legitimately be no data for the V8.DetachedContextAgeInGC measurement?

Good point. There can be no detached contexts. If, for example, the page set
consists of a single page that is not reloaded. I replaced 0 with SkipValue (I
hope it works as the name suggests).

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:40: for _ in xrange(8):
On 2015/02/18 11:20:31, rmcilroy wrote:
> nit - pull '8' out into a constant.

Done.

https://codereview.chromium.org/930333002/diff/20001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc.py:43: delta =
histogram_util.SubtractHistogram(data, self._data_start)
On 2015/02/18 11:20:31, rmcilroy wrote:
> nit - I would do the getHistogram / SubtractHistogram in the
> _MaxDetachedContextAge (maybe renamed _GetMaxDetachedContextAge) to keep the
> code related to V8.DetachedContextAgeInGC together.

Done.

https://codereview.chromium.org/930333002/diff/20001/tools/perf/page_sets/pag...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/20001/tools/perf/page_sets/pag...
tools/perf/page_sets/page_reload_cases.py:11: for _ in xrange(7):
On 2015/02/18 11:20:31, rmcilroy wrote:
> nit - is '7' chosen for an particular reason (I'm assuming it is for the same
> reason we do 7 GCs in emergency GCs)?  Could you pull it out into a constant
> with a more descriptive name?

Done. Added a comment.

[Sami, 2015-02-18 15:30:27]

Thanks for the fixes.

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:13: action_runner.Wait(2)
On 2015/02/18 10:35:56, ulan wrote:
> On 2015/02/17 17:14:06, Sami wrote:
> > Any significance to the two seconds here? Is there any specific event we
could
> > wait for instead?
> 
> 2 seconds is just an arbitrary number. Is it possible to wait for
"page-loaded"
> event?

ReloadPage() already waits for the page to become interactive which is probably
good enough. If you think the sleep will help to make things less noisy, then
let's leave it in.

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
File tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py (right):

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:29: return
"{}"
nit: single quotes

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:37: page =
FakePage("file://blank.html")
ditto

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:48: return
dict(list((v.name, v)) for v in results.all_page_specific_values)
nit: "list()" is redundant

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:67: def
testWithData(self):
Could we write this test in a way that uses the real browser? That way we can
catch breakage where the browser stops sending us the necessary data for some
reason.

[ulan, 2015-02-18 15:58:42]

Thank you, Sami!

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
File tools/perf/page_sets/page_reload_cases.py (right):

https://codereview.chromium.org/930333002/diff/1/tools/perf/page_sets/page_re...
tools/perf/page_sets/page_reload_cases.py:13: action_runner.Wait(2)
On 2015/02/18 15:30:26, Sami wrote:
> On 2015/02/18 10:35:56, ulan wrote:
> > On 2015/02/17 17:14:06, Sami wrote:
> > > Any significance to the two seconds here? Is there any specific event we
> could
> > > wait for instead?
> > 
> > 2 seconds is just an arbitrary number. Is it possible to wait for
> "page-loaded"
> > event?
> 
> ReloadPage() already waits for the page to become interactive which is
probably
> good enough. If you think the sleep will help to make things less noisy, then
> let's leave it in.

I see. OK, let's leave it.

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
File tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py (right):

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:29: return
"{}"
On 2015/02/18 15:30:26, Sami wrote:
> nit: single quotes

Done.

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:37: page =
FakePage("file://blank.html")
On 2015/02/18 15:30:26, Sami wrote:
> ditto

Done.

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:48: return
dict(list((v.name, v)) for v in results.all_page_specific_values)
On 2015/02/18 15:30:27, Sami wrote:
> nit: "list()" is redundant

Done.

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:67: def
testWithData(self):
On 2015/02/18 15:30:26, Sami wrote:
> Could we write this test in a way that uses the real browser? That way we can
> catch breakage where the browser stops sending us the necessary data for some
> reason.

Do you mean: create a real html page, let the browser run it, do GC and collect
data?
Dependency on GC would make this test non-deterministic and might require us to
adjust expectation if GC implementation changes. If browser breaks we would see
that in perf graphs anyway, right?

[Sami, 2015-02-18 16:20:57]

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
File tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py (right):

https://codereview.chromium.org/930333002/diff/40001/tools/perf/measurements/...
tools/perf/measurements/v8_detached_context_age_in_gc_unittest.py:67: def
testWithData(self):
On 2015/02/18 15:58:41, ulan wrote:
> On 2015/02/18 15:30:26, Sami wrote:
> > Could we write this test in a way that uses the real browser? That way we
can
> > catch breakage where the browser stops sending us the necessary data for
some
> > reason.
> 
> Do you mean: create a real html page, let the browser run it, do GC and
collect
> data?
> Dependency on GC would make this test non-deterministic and might require us
to
> adjust expectation if GC implementation changes. If browser breaks we would
see
> that in perf graphs anyway, right? 

Breaking a unit test instead triggering a perf regression is better because the
bad patch never gets to land in the first scenario. Maybe we can do what Oilpan
did and verify that we get at least some data from the browser instead of
expecting exact values:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...

[ulan, 2015-02-19 10:31:50]

> Maybe we can do what Oilpan did and verify that we get at least some data from
the browser instead of expecting exact values
Thanks for the pointer. Uploaded such test.

[Sami, 2015-02-19 19:11:11]

Thank you, lgtm.

[rmcilroy, 2015-02-20 18:35:10]

lgtm

[ulan, 2015-02-23 07:11:00]

The CQ bit was checked by ulan@chromium.org

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

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

[commit-bot: I haz the power, 2015-02-23 08:06:39]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-23 08:07:12]

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