Normalize thread_times metric against the bounds of the browser process.

Normalize thread_times metric against the bounds of the browser process.

This relies on the assumption that all relevant slices are contained in the
bounds of the browser process. This prevents us from normalizing against parts
of the trace where the there are non-browser related traces.

BUG=chromium:640312
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/b28a7dab04342d2fc249162e031102983b260942

[erikchen, 2016-10-29 01:01:59]

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

[erikchen, 2016-10-29 01:02:00]

nednguyen: Please review.

[erikchen, 2016-10-29 01:02:05]

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

[commit-bot: I haz the power, 2016-10-29 01:02:08]

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-10-29 01:23:21]

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

[commit-bot: I haz the power, 2016-10-29 01:23:22]

Dry run: This issue passed the CQ dry run.

[benjhayden, 2016-10-29 02:59:42]

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

[benjhayden, 2016-10-29 02:59:43]

drive-by :-)

https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):

https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/cpu_time_metric.html:60: rangeOfInterest =
opt_options.rangeOfInterest;
Can you only compute chromeBounds if !opt_options.rangeOfInterest in order to
avoid wasting time?

[nednguyen, 2016-10-31 13:17:42]

On 2016/10/29 02:59:43, benjhayden wrote:
> drive-by :-)
> 
>
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):
> 
>
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> tracing/tracing/metrics/system_health/cpu_time_metric.html:60: rangeOfInterest
=
> opt_options.rangeOfInterest;
> Can you only compute chromeBounds if !opt_options.rangeOfInterest in order to
> avoid wasting time?

Incidentally, we also have computeChromeBounds(model) method in
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

I think it's probably best to move this to chromeHelper.getChromeBounds()

[nednguyen, 2016-10-31 13:17:59]

Description was changed from

==========
Normalize thread_times metric against the bounds of the browser process.

This relies on the assumption that all relevant slices are contained in the
bounds of the browser process. This prevents us from normalizing against parts
of the trace where the there are non-browser related traces.

BUG=chromium:640312
==========

to

==========
Normalize thread_times metric against the bounds of the browser process.

This relies on the assumption that all relevant slices are contained in the
bounds of the browser process. This prevents us from normalizing against parts
of the trace where the there are non-browser related traces.

BUG=chromium:640312
==========

[nednguyen, 2016-10-31 13:17:59]

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

[benjhayden, 2016-10-31 16:31:46]

On 2016/10/31 at 13:17:42, nednguyen wrote:
> On 2016/10/29 02:59:43, benjhayden wrote:
> > drive-by :-)
> > 
> >
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> > File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):
> > 
> >
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> > tracing/tracing/metrics/system_health/cpu_time_metric.html:60:
rangeOfInterest =
> > opt_options.rangeOfInterest;
> > Can you only compute chromeBounds if !opt_options.rangeOfInterest in order
to
> > avoid wasting time?
> 
> Incidentally, we also have computeChromeBounds(model) method in
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...
> 
> I think it's probably best to move this to chromeHelper.getChromeBounds()

+1 please :-)

[erikchen, 2016-10-31 19:50:20]

I created the method chromeBounds on ChromeModelHelper, but did not change the
implementation of power_metric.html. computeChromeBounds checks only the main
thread bounds, but it does so for the browser and renderer processes. This is
quite different from my implementation, which checks the bounds of all threads,
but only on the browser process. 

I've got a sample trace in which these two methods produce different results. If
we do choose to change the implementation of power_metric.html, we should do so
in a separate CL.

https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):

https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/cpu_time_metric.html:60: rangeOfInterest =
opt_options.rangeOfInterest;
On 2016/10/29 02:59:43, benjhayden wrote:
> Can you only compute chromeBounds if !opt_options.rangeOfInterest in order to
> avoid wasting time?

Done.

[erikchen, 2016-10-31 19:50:27]

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

[commit-bot: I haz the power, 2016-10-31 19:50:29]

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

[nednguyen, 2016-10-31 19:55:26]

On 2016/10/31 19:50:20, erikchen wrote:
> I created the method chromeBounds on ChromeModelHelper, but did not change the
> implementation of power_metric.html. computeChromeBounds checks only the main
> thread bounds, but it does so for the browser and renderer processes. This is
> quite different from my implementation, which checks the bounds of all
threads,
> but only on the browser process. 
> 
> I've got a sample trace in which these two methods produce different results.
If
> we do choose to change the implementation of power_metric.html, we should do
so
> in a separate CL.

+1

> 
>
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):
> 
>
https://codereview.chromium.org/2446423009/diff/1/tracing/tracing/metrics/sys...
> tracing/tracing/metrics/system_health/cpu_time_metric.html:60: rangeOfInterest
=
> opt_options.rangeOfInterest;
> On 2016/10/29 02:59:43, benjhayden wrote:
> > Can you only compute chromeBounds if !opt_options.rangeOfInterest in order
to
> > avoid wasting time?
> 
> Done.

lgtm

[nednguyen, 2016-10-31 19:55:33]

https://codereview.chromium.org/2446423009/diff/20001/tracing/tracing/model/h...
File tracing/tracing/model/helpers/chrome_model_helper.html (right):

https://codereview.chromium.org/2446423009/diff/20001/tracing/tracing/model/h...
tracing/tracing/model/helpers/chrome_model_helper.html:147: get chromeBounds() {
nits: memorize the bounds since this could be expensive.

[commit-bot: I haz the power, 2016-10-31 20:09:40]

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

[commit-bot: I haz the power, 2016-10-31 20:09:41]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-10-31 21:14:46]

https://codereview.chromium.org/2446423009/diff/20001/tracing/tracing/model/h...
File tracing/tracing/model/helpers/chrome_model_helper.html (right):

https://codereview.chromium.org/2446423009/diff/20001/tracing/tracing/model/h...
tracing/tracing/model/helpers/chrome_model_helper.html:147: get chromeBounds() {
On 2016/10/31 19:55:33, nednguyen wrote:
> nits: memorize the bounds since this could be expensive.

Done.

[erikchen, 2016-10-31 21:15:02]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-10-31 21:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2446423009/#ps40001
(title: "Comments from nednguyen.")

[commit-bot: I haz the power, 2016-10-31 21:15:06]

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-10-31 21:38:14]

Description was changed from

==========
Normalize thread_times metric against the bounds of the browser process.

This relies on the assumption that all relevant slices are contained in the
bounds of the browser process. This prevents us from normalizing against parts
of the trace where the there are non-browser related traces.

BUG=chromium:640312
==========

to

==========
Normalize thread_times metric against the bounds of the browser process.

This relies on the assumption that all relevant slices are contained in the
bounds of the browser process. This prevents us from normalizing against parts
of the trace where the there are non-browser related traces.

BUG=chromium:640312

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-10-31 21:38:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[nednguyen, 2016-11-07 16:04:22]

On 2016/10/31 21:38:15, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
>
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Charlie: shouldn't we also update the current implementation of power_metric to
use this for Chrome bound?