Add function to get total cpu time in thread

Add function to get total cpu time in thread

This adds the most basic building block of the new cpu time metric - a
function to compute total cpu time consumed by a thread within specified
bounds. If slices straddle bounds, we assume cpu usage was uniformly
distributed within the slice.

The function is added to the new cpu_time.html file in
tracing/extras/chrome. This file will eventually contain several other
key pieces of the cpu time metric, similar to the expected queuing time
metric.

BUG=catapult:#3325

Review-Url: https://codereview.chromium.org/2798213002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/87e8335e7464200d1d500fc5f7ff6ab1f527714c

[dproy, 2017-04-06 00:17:37]

dproy@chromium.org changed reviewers:
+ benjhayden@chromium.org, tdresser@chromium.org

[dproy, 2017-04-06 00:21:43]

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:11:
tr.exportTo('tr.e.chrome.cpuTime', function() {
I added added an extra level of namespacing here (tr.e.chrome.cpuTime as opposed
to just tr.e.chrome). There will eventually be other exported functions in this
module that will be pretty specific to cpu time computations, so this
namespacing makes things cleaner. Let me know if this is ok.

[benjhayden, 2017-04-06 04:14:30]

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:11:
tr.exportTo('tr.e.chrome.cpuTime', function() {
On 2017/04/06 at 00:21:43, dproy wrote:
> I added added an extra level of namespacing here (tr.e.chrome.cpuTime as
opposed to just tr.e.chrome). There will eventually be other exported functions
in this module that will be pretty specific to cpu time computations, so this
namespacing makes things cleaner. Let me know if this is ok.

Hm, doesn't really seem to matter, but it is a novel way of namespacing in
catapult, so let's see if it's a good precedent to set or if there's a better
precedent to follow.

Sometimes we use a static class like tr.b.Statistics or tr.v.ChartJsonConverter.
Would exportTo('tr.e.chrome', ... class CpuTime { static ... }... ) work here?

Sometimes we just export all the functions directly to the directory namespace,
tr.e.chrome in this case, though there are obvious downsides to that solution.

What are all the functions that you're planning on putting here?
Imagine somebody who has a passing familiarity with Chrome and TBMv2 metrics:
would they have a reasonable chance of guessing that the functions would be
defined in "cpu_time.html" as opposed to one of the other files in tr.e.chrome?
If so, then it might be ok to leave the functions in the tr.e.chrome namespace.

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:15: function
getCpuTimeForThread(thread, bounds) {
It looks like this is going to require callers to make several iterations over
topLevelSlices, depending on UEs.
Would it be possible to use an IntervalTree to reduce the number of iterations?

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:21: const sliceRange =
This looks unused.

[dproy, 2017-04-06 14:15:08]

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:11:
tr.exportTo('tr.e.chrome.cpuTime', function() {
On 2017/04/06 04:14:30, benjhayden wrote:
> On 2017/04/06 at 00:21:43, dproy wrote:
> > I added added an extra level of namespacing here (tr.e.chrome.cpuTime as
> opposed to just tr.e.chrome). There will eventually be other exported
functions
> in this module that will be pretty specific to cpu time computations, so this
> namespacing makes things cleaner. Let me know if this is ok.
> 
> Hm, doesn't really seem to matter, but it is a novel way of namespacing in
> catapult, so let's see if it's a good precedent to set or if there's a better
> precedent to follow.
> 
> Sometimes we use a static class like tr.b.Statistics or
tr.v.ChartJsonConverter.
> Would exportTo('tr.e.chrome', ... class CpuTime { static ... }... ) work here?
> 
> Sometimes we just export all the functions directly to the directory
namespace,
> tr.e.chrome in this case, though there are obvious downsides to that solution.
> 
> What are all the functions that you're planning on putting here?
> Imagine somebody who has a passing familiarity with Chrome and TBMv2 metrics:
> would they have a reasonable chance of guessing that the functions would be
> defined in "cpu_time.html" as opposed to one of the other files in
tr.e.chrome?
> If so, then it might be ok to leave the functions in the tr.e.chrome
namespace.

I think I like having the extra tr.e.chrome.cpu_time namespace as opposed to
just dropping the functions in tr.e.chrome. The less time people have to spend
trying to guess where a function is actually defined the better. Let's keep this
extra namespace now, and when we're done adding functions to this file, we can
remove this if the namespace feels redundant.

The static class approach would achieve the same goals but feels more verbose
than it needs to be. Unless I'm using java, I don't see the benefit of using a
class to structure something that cannot be instantiated.

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:15: function
getCpuTimeForThread(thread, bounds) {
On 2017/04/06 04:14:30, benjhayden wrote:
> It looks like this is going to require callers to make several iterations over
> topLevelSlices, depending on UEs.
> Would it be possible to use an IntervalTree to reduce the number of
iterations?

You're right. I was meaning to call this function many times and it is worth
making this more efficient. I read about interval trees, and they seem more
useful when intervals are overlapping. The wikipedia page says "In a simple
case, the intervals do not overlap and they can be inserted into a simple binary
search tree and queried in O(log n) time."
(https://en.wikipedia.org/wiki/Interval_tree).

The top level slices are definitely non overlapping. Can we assume
|topLevelSlices| is sorted? A binary search for the two end points can be a
simple solution, and we won't have to set up more complicated data structures.

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:21: const sliceRange =
On 2017/04/06 04:14:30, benjhayden wrote:
> This looks unused.

Deleted. Thanks for catching this.

[dproy, 2017-04-06 15:05:59]

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2798213002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:15: function
getCpuTimeForThread(thread, bounds) {
On 2017/04/06 14:15:08, dproy wrote:
> On 2017/04/06 04:14:30, benjhayden wrote:
> > It looks like this is going to require callers to make several iterations
over
> > topLevelSlices, depending on UEs.
> > Would it be possible to use an IntervalTree to reduce the number of
> iterations?
> 
> You're right. I was meaning to call this function many times and it is worth
> making this more efficient. I read about interval trees, and they seem more
> useful when intervals are overlapping. The wikipedia page says "In a simple
> case, the intervals do not overlap and they can be inserted into a simple
binary
> search tree and queried in O(log n) time."
> (https://en.wikipedia.org/wiki/Interval_tree).
> 
> The top level slices are definitely non overlapping. Can we assume
> |topLevelSlices| is sorted? A binary search for the two end points can be a
> simple solution, and we won't have to set up more complicated data structures.


Using tr.b.math.iterateOverIntersectingIntervals in the latest patch, which does
this binary search.

[dproy, 2017-04-07 18:01:03]

The CL has now been updated to be in sync with
https://codereview.chromium.org/2804043003/ and is ready for review. PTAL :)

[benjhayden, 2017-04-07 18:10:02]

lgtm

[dproy, 2017-04-07 18:26:59]

The CQ bit was checked by dproy@chromium.org

[commit-bot: I haz the power, 2017-04-07 18:27:06]

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

[commit-bot: I haz the power, 2017-04-07 18:27:07]

This CL has an open dependency (Issue 2800933002 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[dproy, 2017-04-07 21:04:20]

The CQ bit was checked by dproy@chromium.org

[commit-bot: I haz the power, 2017-04-07 21:04:29]

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, 2017-04-07 21:28:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491599060892560,
"parent_rev": "5c399d8801ada5b88f1153fd3fb646bd7eb395e2", "commit_rev":
"87e8335e7464200d1d500fc5f7ff6ab1f527714c"}

[commit-bot: I haz the power, 2017-04-07 21:28:07]

Description was changed from

==========
Add function to get total cpu time in thread

This adds the most basic building block of the new cpu time metric - a
function to compute total cpu time consumed by a thread within specified
bounds. If slices straddle bounds, we assume cpu usage was uniformly
distributed within the slice.

The function is added to the new cpu_time.html file in
tracing/extras/chrome. This file will eventually contain several other
key pieces of the cpu time metric, similar to the expected queuing time
metric.

BUG=catapult:#3325
==========

to

==========
Add function to get total cpu time in thread

This adds the most basic building block of the new cpu time metric - a
function to compute total cpu time consumed by a thread within specified
bounds. If slices straddle bounds, we assume cpu usage was uniformly
distributed within the slice.

The function is added to the new cpu_time.html file in
tracing/extras/chrome. This file will eventually contain several other
key pieces of the cpu time metric, similar to the expected queuing time
metric.

BUG=catapult:#3325

Review-Url: https://codereview.chromium.org/2798213002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-04-07 21:28:08]

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