Add cpuTimeMetric to BattOr benchmark.

Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/68b995c4d3eb37217ce143c225a7d2eed81d320a
Cr-Commit-Position: refs/heads/master@{#419624}

[erikchen, 2016-09-12 18:12:36]

Description was changed from

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
==========

to

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[erikchen, 2016-09-12 18:13:03]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 18:13:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2016-09-12 18:14:11]

erikchen@chromium.org changed reviewers:
+ benjhayden@chromium.org, charliea@chromium.org, nednguyen@google.com,
rnephew@chromium.org

[erikchen, 2016-09-12 18:14:12]

nednguyen: Please review.

[nednguyen, 2016-09-12 18:15:29]

https://codereview.chromium.org/2338433002/diff/1/tools/perf/benchmarks/batto...
File tools/perf/benchmarks/battor.py (right):

https://codereview.chromium.org/2338433002/diff/1/tools/perf/benchmarks/batto...
tools/perf/benchmarks/battor.py:19:
options.config.chrome_trace_config.category_filter.AddFilterString('rail')
have you check whether this tracing category is good enough to get
cpuTimeMetric?

IIRC, thread_times relies on "toplevel" to work

[nednguyen, 2016-09-12 18:22:34]

Description was changed from

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[nednguyen, 2016-09-12 18:22:34]

nednguyen@google.com changed reviewers:
+ vmiura@chromium.org

[nednguyen, 2016-09-12 18:23:32]

+Victor who may have some idea about which trace categories are required to get
accurate data for thread_times metric. For the context, cpuTimeMetric is the new
implementation of thread_times using TBMv2 framework.

[erikchen, 2016-09-12 19:15:36]

https://codereview.chromium.org/2338433002/diff/1/tools/perf/benchmarks/batto...
File tools/perf/benchmarks/battor.py (right):

https://codereview.chromium.org/2338433002/diff/1/tools/perf/benchmarks/batto...
tools/perf/benchmarks/battor.py:19:
options.config.chrome_trace_config.category_filter.AddFilterString('rail')
On 2016/09/12 18:15:29, nednguyen wrote:
> have you check whether this tracing category is good enough to get
> cpuTimeMetric?
> 
> IIRC, thread_times relies on "toplevel" to work

Your'e right. We should be using the LowOverheadFilter.

[erikchen, 2016-09-12 19:15:39]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 19:15:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nednguyen, 2016-09-12 20:02:05]

nednguyen@google.com changed reviewers:
+ tdresser@chromium.org

[nednguyen, 2016-09-12 20:02:06]

I defer to Charlie & Tim as reviewers as they know more about this area than me

[tdresser, 2016-09-12 20:16:01]

-me, others have more context here.

[rnephew (Reviews Here), 2016-09-12 20:28:45]

I know for battor tests we turned 'rails' tracing only so that we would stop
having OOM problems while processing the traces in python. If we turn on more
categories there is a chance we will start having OOM problems again.

[erikchen, 2016-09-12 20:31:24]

On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> I know for battor tests we turned 'rails' tracing only so that we would stop
> having OOM problems while processing the traces in python. If we turn on more
> categories there is a chance we will start having OOM problems again.

OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be a real
problem for Macs?

[erikchen, 2016-09-12 20:31:32]

On 2016/09/12 20:31:24, erikchen wrote:
> On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > I know for battor tests we turned 'rails' tracing only so that we would stop
> > having OOM problems while processing the traces in python. If we turn on
more
> > categories there is a chance we will start having OOM problems again.
> 
> OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be a real
> problem for Macs?

s/OOO/OOM

[rnephew (Reviews Here), 2016-09-12 20:39:25]

On 2016/09/12 20:31:32, erikchen wrote:
> On 2016/09/12 20:31:24, erikchen wrote:
> > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > I know for battor tests we turned 'rails' tracing only so that we would
stop
> > > having OOM problems while processing the traces in python. If we turn on
> more
> > > categories there is a chance we will start having OOM problems again.
> > 
> > OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be a
real
> > problem for Macs?
> 
> s/OOO/OOM

Some of our testing platforms use 32b python, its python itself that is running
out of memory it is allowed to use (IIRC) when it is processing the python
(dumping the json to be more specific). If the platform is using 64b python I do
not think there is a problem.

[charliea (OOO until 10-5), 2016-09-12 20:51:16]

On 2016/09/12 20:39:25, rnephew (Reviews Here) wrote:
> On 2016/09/12 20:31:32, erikchen wrote:
> > On 2016/09/12 20:31:24, erikchen wrote:
> > > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > > I know for battor tests we turned 'rails' tracing only so that we would
> stop
> > > > having OOM problems while processing the traces in python. If we turn on
> > more
> > > > categories there is a chance we will start having OOM problems again.
> > > 
> > > OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be a
> real
> > > problem for Macs?
> > 
> > s/OOO/OOM
> 
> Some of our testing platforms use 32b python, its python itself that is
running
> out of memory it is allowed to use (IIRC) when it is processing the python
> (dumping the json to be more specific). If the platform is using 64b python I
do
> not think there is a problem.

Sorry for the delay in the review.

As Randy said, I think that it's possible we'll run into a situation where the
low overhead filter, and maybe even just "toplevel", will contain too many
events for a 30s trace to handle. If the first fails, try the second, and if
that fails, we'll work with you to find something else that will work.

This is a general problem that we've just started running up against: the
concept of TBMv2 metrics is really powerful, but we lack a great way to tell
Chrome exactly which trace events we want recorded.

[commit-bot: I haz the power, 2016-09-12 21:17:52]

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

[commit-bot: I haz the power, 2016-09-12 21:17:53]

Dry run: Try jobs failed on following builders:
  mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build
URL)

[erikchen, 2016-09-12 21:26:29]

On 2016/09/12 20:51:16, charliea wrote:
> On 2016/09/12 20:39:25, rnephew (Reviews Here) wrote:
> > On 2016/09/12 20:31:32, erikchen wrote:
> > > On 2016/09/12 20:31:24, erikchen wrote:
> > > > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > > > I know for battor tests we turned 'rails' tracing only so that we
would
> > stop
> > > > > having OOM problems while processing the traces in python. If we turn
on
> > > more
> > > > > categories there is a chance we will start having OOM problems again.
> > > > 
> > > > OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be a
> > real
> > > > problem for Macs?
> > > 
> > > s/OOO/OOM
> > 
> > Some of our testing platforms use 32b python, its python itself that is
> running
> > out of memory it is allowed to use (IIRC) when it is processing the python
> > (dumping the json to be more specific). If the platform is using 64b python
I
> do
> > not think there is a problem.
> 
> Sorry for the delay in the review.
> 
> As Randy said, I think that it's possible we'll run into a situation where the
> low overhead filter, and maybe even just "toplevel", will contain too many
> events for a 30s trace to handle. If the first fails, try the second, and if
> that fails, we'll work with you to find something else that will work.
> 
> This is a general problem that we've just started running up against: the
> concept of TBMv2 metrics is really powerful, but we lack a great way to tell
> Chrome exactly which trace events we want recorded.

charliea: Based on your most recent response, can you give this CL a L.G.T.M?

[nednguyen, 2016-09-13 01:40:30]

On 2016/09/12 21:26:29, erikchen wrote:
> On 2016/09/12 20:51:16, charliea wrote:
> > On 2016/09/12 20:39:25, rnephew (Reviews Here) wrote:
> > > On 2016/09/12 20:31:32, erikchen wrote:
> > > > On 2016/09/12 20:31:24, erikchen wrote:
> > > > > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > > > > I know for battor tests we turned 'rails' tracing only so that we
> would
> > > stop
> > > > > > having OOM problems while processing the traces in python. If we
turn
> on
> > > > more
> > > > > > categories there is a chance we will start having OOM problems
again.
> > > > > 
> > > > > OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't be
a
> > > real
> > > > > problem for Macs?
> > > > 
> > > > s/OOO/OOM
> > > 
> > > Some of our testing platforms use 32b python, its python itself that is
> > running
> > > out of memory it is allowed to use (IIRC) when it is processing the python
> > > (dumping the json to be more specific). If the platform is using 64b
python
> I
> > do
> > > not think there is a problem.
> > 
> > Sorry for the delay in the review.
> > 
> > As Randy said, I think that it's possible we'll run into a situation where
the
> > low overhead filter, and maybe even just "toplevel", will contain too many
> > events for a 30s trace to handle. If the first fails, try the second, and if
> > that fails, we'll work with you to find something else that will work.
> > 
> > This is a general problem that we've just started running up against: the
> > concept of TBMv2 metrics is really powerful, but we lack a great way to tell
> > Chrome exactly which trace events we want recorded.
> 
> charliea: Based on your most recent response, can you give this CL a L.G.T.M?

In
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf...,
http://abcnews.go.com/ is failing because the trace too big :-(

INFO:root:Trace (TraceDataPart("tabIds")) of size 40 bytes saved.
INFO:root:Trace (TraceDataPart("telemetry")) of size 1082 bytes saved.
INFO:root:Trace (TraceDataPart("traceEvents")) of size 336850828 bytes saved.
INFO:root:Trace (TraceDataPart("cpuSnapshots")) of size 25702 bytes saved.
[ RUN      ] /tmp/tmpRLRPC0.html
TraceImportError: Invalid string length

[charliea (OOO until 10-5), 2016-09-13 16:47:00]

On 2016/09/13 01:40:30, nednguyen wrote:
> On 2016/09/12 21:26:29, erikchen wrote:
> > On 2016/09/12 20:51:16, charliea wrote:
> > > On 2016/09/12 20:39:25, rnephew (Reviews Here) wrote:
> > > > On 2016/09/12 20:31:32, erikchen wrote:
> > > > > On 2016/09/12 20:31:24, erikchen wrote:
> > > > > > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > > > > > I know for battor tests we turned 'rails' tracing only so that we
> > would
> > > > stop
> > > > > > > having OOM problems while processing the traces in python. If we
> turn
> > on
> > > > > more
> > > > > > > categories there is a chance we will start having OOM problems
> again.
> > > > > > 
> > > > > > OOO issues on Androids, Macs, or both? I imagine that OOO shouldn't
be
> a
> > > > real
> > > > > > problem for Macs?
> > > > > 
> > > > > s/OOO/OOM
> > > > 
> > > > Some of our testing platforms use 32b python, its python itself that is
> > > running
> > > > out of memory it is allowed to use (IIRC) when it is processing the
python
> > > > (dumping the json to be more specific). If the platform is using 64b
> python
> > I
> > > do
> > > > not think there is a problem.
> > > 
> > > Sorry for the delay in the review.
> > > 
> > > As Randy said, I think that it's possible we'll run into a situation where
> the
> > > low overhead filter, and maybe even just "toplevel", will contain too many
> > > events for a 30s trace to handle. If the first fails, try the second, and
if
> > > that fails, we'll work with you to find something else that will work.
> > > 
> > > This is a general problem that we've just started running up against: the
> > > concept of TBMv2 metrics is really powerful, but we lack a great way to
tell
> > > Chrome exactly which trace events we want recorded.
> > 
> > charliea: Based on your most recent response, can you give this CL a
L.G.T.M?
> 
> In
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf...,
> http://abcnews.go.com/ is failing because the trace too big :-(
> 
> INFO:root:Trace (TraceDataPart("tabIds")) of size 40 bytes saved.
> INFO:root:Trace (TraceDataPart("telemetry")) of size 1082 bytes saved.
> INFO:root:Trace (TraceDataPart("traceEvents")) of size 336850828 bytes saved.
> INFO:root:Trace (TraceDataPart("cpuSnapshots")) of size 25702 bytes saved.
> [ RUN      ] /tmp/tmpRLRPC0.html
> TraceImportError: Invalid string length

Hi Eric,

Could you try this again with just the "toplevel" category enabled?

[erikchen, 2016-09-13 17:35:45]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 17:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[benjhayden, 2016-09-13 18:37:07]

benjhayden@chromium.org changed reviewers:
- benjhayden@chromium.org

[commit-bot: I haz the power, 2016-09-13 18:37:35]

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

[commit-bot: I haz the power, 2016-09-13 18:37:36]

Dry run: Try jobs failed on following builders:
  winx64_10_perf_cq on master.tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_c...)

[erikchen, 2016-09-13 19:12:52]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 19:13:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 20:15:01]

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

[commit-bot: I haz the power, 2016-09-13 20:15:02]

Dry run: Try jobs failed on following builders:
  winx64_10_perf_cq on master.tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_c...)

[erikchen, 2016-09-15 00:20:27]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 00:21:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 07:00:38]

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

[commit-bot: I haz the power, 2016-09-15 07:00:39]

Dry run: Try jobs failed on following builders:
  mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_...)

[erikchen, 2016-09-15 16:59:24]

On 2016/09/13 16:47:00, charliea wrote:
> On 2016/09/13 01:40:30, nednguyen wrote:
> > On 2016/09/12 21:26:29, erikchen wrote:
> > > On 2016/09/12 20:51:16, charliea wrote:
> > > > On 2016/09/12 20:39:25, rnephew (Reviews Here) wrote:
> > > > > On 2016/09/12 20:31:32, erikchen wrote:
> > > > > > On 2016/09/12 20:31:24, erikchen wrote:
> > > > > > > On 2016/09/12 20:28:45, rnephew (Reviews Here) wrote:
> > > > > > > > I know for battor tests we turned 'rails' tracing only so that
we
> > > would
> > > > > stop
> > > > > > > > having OOM problems while processing the traces in python. If we
> > turn
> > > on
> > > > > > more
> > > > > > > > categories there is a chance we will start having OOM problems
> > again.
> > > > > > > 
> > > > > > > OOO issues on Androids, Macs, or both? I imagine that OOO
shouldn't
> be
> > a
> > > > > real
> > > > > > > problem for Macs?
> > > > > > 
> > > > > > s/OOO/OOM
> > > > > 
> > > > > Some of our testing platforms use 32b python, its python itself that
is
> > > > running
> > > > > out of memory it is allowed to use (IIRC) when it is processing the
> python
> > > > > (dumping the json to be more specific). If the platform is using 64b
> > python
> > > I
> > > > do
> > > > > not think there is a problem.
> > > > 
> > > > Sorry for the delay in the review.
> > > > 
> > > > As Randy said, I think that it's possible we'll run into a situation
where
> > the
> > > > low overhead filter, and maybe even just "toplevel", will contain too
many
> > > > events for a 30s trace to handle. If the first fails, try the second,
and
> if
> > > > that fails, we'll work with you to find something else that will work.
> > > > 
> > > > This is a general problem that we've just started running up against:
the
> > > > concept of TBMv2 metrics is really powerful, but we lack a great way to
> tell
> > > > Chrome exactly which trace events we want recorded.
> > > 
> > > charliea: Based on your most recent response, can you give this CL a
> L.G.T.M?
> > 
> > In
> >
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf...,
> > http://abcnews.go.com/ is failing because the trace too big :-(
> > 
> > INFO:root:Trace (TraceDataPart("tabIds")) of size 40 bytes saved.
> > INFO:root:Trace (TraceDataPart("telemetry")) of size 1082 bytes saved.
> > INFO:root:Trace (TraceDataPart("traceEvents")) of size 336850828 bytes
saved.
> > INFO:root:Trace (TraceDataPart("cpuSnapshots")) of size 25702 bytes saved.
> > [ RUN      ] /tmp/tmpRLRPC0.html
> > TraceImportError: Invalid string length
> 
> Hi Eric,
> 
> Could you try this again with just the "toplevel" category enabled?

Same issue:
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf...
"""
INFO:root:Trace (TraceDataPart("cpuSnapshots")) of size 25539 bytes saved.
INFO:root:Trace (TraceDataPart("tabIds")) of size 40 bytes saved.
INFO:root:Trace (TraceDataPart("telemetry")) of size 22057 bytes saved.
INFO:root:Trace (TraceDataPart("traceEvents")) of size 330205233 bytes saved.
[ RUN      ] /tmp/tmp7WE6SW.html
TraceImportError: Invalid string length
"""

[charliea (OOO until 10-5), 2016-09-16 15:27:23]

Hey Erik,

After investigating a bunch more, I think that a 30s trace on abcnews.go.com
might just be too much data. It ends up being >240MB unzipped, even when we
implement the simple fix (of stripping MessageLoop trace event args) that I
tried. This page is doing some *crazy* inefficient stuff, or is at least causing
Chrome to.

Is there a possibility that we could reduce the story length to 20s, or even
15s? I know that you wanted 30s in order to match the existing benchmarks, but
I'm not sure that's feasible given the current level of granularity we have in
trace categories / the given amount of data required to store trace events
(we're trying to move to protobuf in tracing v2 to make traces smaller) / the
given import performance of trace viewer.

[charliea (OOO until 10-5), 2016-09-16 15:39:48]

On 2016/09/16 at 15:27:23, charliea wrote:
> Hey Erik,
> 
> After investigating a bunch more, I think that a 30s trace on abcnews.go.com
might just be too much data. It ends up being >240MB unzipped, even when we
implement the simple fix (of stripping MessageLoop trace event args) that I
tried. This page is doing some *crazy* inefficient stuff, or is at least causing
Chrome to.
> 
> Is there a possibility that we could reduce the story length to 20s, or even
15s? I know that you wanted 30s in order to match the existing benchmarks, but
I'm not sure that's feasible given the current level of granularity we have in
trace categories / the given amount of data required to store trace events
(we're trying to move to protobuf in tracing v2 to make traces smaller) / the
given import performance of trace viewer.

Also, I think that you can just do this for abcnews. You can add something here
(https://cs.chromium.org/chromium/src/tools/perf/page_sets/idle_after_loading_...)
that enforces a max time of 15s for abcnews, and allow all of the other sites to
continue for 30s.

[erikchen, 2016-09-16 17:12:38]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 17:13:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2016-09-16 17:13:41]

> Also, I think that you can just do this for abcnews. You can add something
here
>
(https://cs.chromium.org/chromium/src/tools/perf/page_sets/idle_after_loading_...)
> that enforces a max time of 15s for abcnews, and allow all of the other sites
to
> continue for 30s.

Inconsistency leads to future issues. I've changed the time for all pages in
battor.steady_state to run for 15s.

[erikchen, 2016-09-16 17:17:01]

On 2016/09/16 17:13:41, erikchen wrote:
> > Also, I think that you can just do this for abcnews. You can add something
> here
> >
>
(https://cs.chromium.org/chromium/src/tools/perf/page_sets/idle_after_loading_...)
> > that enforces a max time of 15s for abcnews, and allow all of the other
sites
> to
> > continue for 30s.
> 
> Inconsistency leads to future issues. I've changed the time for all pages in
> battor.steady_state to run for 15s.

I wonder if we wait for the page to quiesce while running battor + tracing, and
if abcnews takes forever to quiesce, so maybe this number won't matter at all.

[nednguyen, 2016-09-16 17:20:25]

On 2016/09/16 17:17:01, erikchen wrote:
> On 2016/09/16 17:13:41, erikchen wrote:
> > > Also, I think that you can just do this for abcnews. You can add something
> > here
> > >
> >
>
(https://cs.chromium.org/chromium/src/tools/perf/page_sets/idle_after_loading_...)
> > > that enforces a max time of 15s for abcnews, and allow all of the other
> sites
> > to
> > > continue for 30s.
> > 
> > Inconsistency leads to future issues. I've changed the time for all pages in
> > battor.steady_state to run for 15s.
> 
> I wonder if we wait for the page to quiesce while running battor + tracing,
and
> if abcnews takes forever to quiesce, so maybe this number won't matter at all.

I don't recall we wait for the page to quiesce by default. But it's worth
checking by running the benchmark locally & looking at the telemetry trace.

[commit-bot: I haz the power, 2016-09-16 23:28:21]

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

[commit-bot: I haz the power, 2016-09-16 23:28:22]

Dry run: Try jobs failed on following builders:
  mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_...)

[charliea (OOO until 10-5), 2016-09-19 16:56:49]

On 2016/09/16 at 17:20:25, nednguyen wrote:
> On 2016/09/16 17:17:01, erikchen wrote:
> > On 2016/09/16 17:13:41, erikchen wrote:
> > > > Also, I think that you can just do this for abcnews. You can add
something
> > > here
> > > >
> > >
> >
(https://cs.chromium.org/chromium/src/tools/perf/page_sets/idle_after_loading_...)
> > > > that enforces a max time of 15s for abcnews, and allow all of the other
> > sites
> > > to
> > > > continue for 30s.
> > > 
> > > Inconsistency leads to future issues. I've changed the time for all pages
in
> > > battor.steady_state to run for 15s.
> > 
> > I wonder if we wait for the page to quiesce while running battor + tracing,
and
> > if abcnews takes forever to quiesce, so maybe this number won't matter at
all.
> 
> I don't recall we wait for the page to quiesce by default. But it's worth
checking by running the benchmark locally & looking at the telemetry trace.

Hey Erik,

We're working hard on our end to come up with a solution to this, but it looks
like ABC news is just such a crazy site that it's pushing our infrastructure to
the limits, even at 15s. I think that, for now, the best solution is to just
leave ABC news out of the page set. Hopefully that's okay with you.

We could do this in a really simple way: just add an exclude_abcnews parameter
to the idle after loading stories and then, inside the AddStory() loop of that
constructor if the parameter is true and the current story is ABC news, move on
to the next iteration. I apologize that we can't find a better solution now, but
I think you'd probably prefer for most of these benchmarks to be up today than
for all of them to be up a week from now.

[charliea (OOO until 10-5), 2016-09-19 16:57:08]

(Also, I talked with Ned, and he's okay with this solution too)

[nednguyen, 2016-09-19 16:58:15]

On 2016/09/19 16:57:08, charliea wrote:
> (Also, I talked with Ned, and he's okay with this solution too)

Yeah, if we go this route, let change the timing back to 30s to keep it
consistent with msr based benchmark

[erikchen, 2016-09-19 16:59:14]

On 2016/09/19 16:57:08, charliea wrote:
> (Also, I talked with Ned, and he's okay with this solution too)

That sounds reasonable to me.

[erikchen, 2016-09-19 17:06:35]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 17:07:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[charliea (OOO until 10-5), 2016-09-19 17:09:50]

lgtm, as long as you're okay with the fact that ABC will no longer be run with
your old MSR-based steady state pages either

https://codereview.chromium.org/2338433002/diff/100001/tools/perf/benchmarks/...
File tools/perf/benchmarks/battor.py (right):

https://codereview.chromium.org/2338433002/diff/100001/tools/perf/benchmarks/...
tools/perf/benchmarks/battor.py:141: # but that produces too much tracing data.
Instead wait for 15 seconds.
nit: please eliminate the second line now that we just got rid of ABC news

[erikchen, 2016-09-19 17:19:38]

https://codereview.chromium.org/2338433002/diff/100001/tools/perf/benchmarks/...
File tools/perf/benchmarks/battor.py (right):

https://codereview.chromium.org/2338433002/diff/100001/tools/perf/benchmarks/...
tools/perf/benchmarks/battor.py:141: # but that produces too much tracing data.
Instead wait for 15 seconds.
On 2016/09/19 17:09:50, charliea wrote:
> nit: please eliminate the second line now that we just got rid of ABC news

Done.

[erikchen, 2016-09-19 17:20:35]

On 2016/09/19 17:09:50, charliea wrote:
> lgtm, as long as you're okay with the fact that ABC will no longer be run with
> your old MSR-based steady state pages either

yup.

[erikchen, 2016-09-19 17:21:17]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-09-19 17:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2338433002/#ps140001
(title: "Add back initial comment.")

[commit-bot: I haz the power, 2016-09-19 17:21:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 17:30:31]

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

[commit-bot: I haz the power, 2016-09-19 17:30:33]

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

[nednguyen, 2016-09-19 17:53:03]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2016-09-19 17:53:04]

lgtm

[commit-bot: I haz the power, 2016-09-19 17:53:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 19:23:54]

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

[commit-bot: I haz the power, 2016-09-19 19:23:55]

Try jobs failed on following builders:
  android_s5_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build
URL)
  mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build
URL)

[erikchen, 2016-09-19 21:53:45]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-09-19 21:54:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 01:23:35]

Description was changed from

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[commit-bot: I haz the power, 2016-09-20 01:23:37]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-20 01:26:57]

Description was changed from

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
Add cpuTimeMetric to BattOr benchmark.

BUG=640312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/68b995c4d3eb37217ce143c225a7d2eed81d320a
Cr-Commit-Position: refs/heads/master@{#419624}
==========

[commit-bot: I haz the power, 2016-09-20 01:26:59]

Patchset 8 (id:??) landed as
https://crrev.com/68b995c4d3eb37217ce143c225a7d2eed81d320a
Cr-Commit-Position: refs/heads/master@{#419624}