Construct Cpu Time MultidimensionalView

Construct Cpu Time MultidimensionalView

This CL adds a function to construct a multidimensional tree view for cpu time
metrics. The tree holds both cpuUage (cpu time per second) and cpuTotal (sum of
cpu time) information. The structure of the multidimensional view is explained
in more detail in the cpu_time_multidimensional_view.md doc.

BUG=catapult:#3325

Review-Url: https://codereview.chromium.org/2807243002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ff3b45d456482f2ac0282d5532a9d38fe3d031d8

[dproy, 2017-04-10 18:33:38]

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

[dproy, 2017-04-10 18:33:39]

PTAL. 

If you think some of the tests are redundant and have ideas about how to reduce
the volume let me know - I had a hard time leaving out any of the scenarios
without assuming too much about the implementation.

I will change the code to use a static class (as discussed here:
https://github.com/catapult-project/catapult/issues/3475) in a separate CL.

[tdresser, 2017-04-10 20:48:29]

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

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:113: * during that range.
Use jsdoc.

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time_multidimensional_view.md (right):

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_multidimensional_view.md:10: The returned
MultiDimensionalView in TopDownTreeView mode. It is three
I think this sentence missing a word?

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

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:176: * Given the root node of a
top down multidimensional tree view, and returns
and returns -> returns

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:181: for (let i = 0; i <= 2;
i++) {
Why 2? (Maybe deserves a comment?)

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:240:
assert.closeTo(getCpuUsage_(values), (30 + 20) / 200, 1e-7);
It might be worth adding comments indicating where these values are coming from,
for cases where it isn't 100% obvious.

(e.g., why are we dividing by 200?)

[dproy, 2017-05-11 00:50:47]

Addressed comments. PTAL.

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

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time.html:113: * during that range.
On 2017/04/10 20:48:29, tdresser wrote:
> Use jsdoc.

Done.

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
File tracing/tracing/extras/chrome/cpu_time_multidimensional_view.md (right):

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_multidimensional_view.md:10: The returned
MultiDimensionalView in TopDownTreeView mode. It is three
On 2017/04/10 20:48:29, tdresser wrote:
> I think this sentence missing a word?

Was missing 'is'. Fixed.

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

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:176: * Given the root node of a
top down multidimensional tree view, and returns
On 2017/04/10 20:48:29, tdresser wrote:
> and returns -> returns

Done.

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:181: for (let i = 0; i <= 2;
i++) {
On 2017/04/10 20:48:29, tdresser wrote:
> Why 2? (Maybe deserves a comment?)

Changed to path.length.

https://codereview.chromium.org/2807243002/diff/1/tracing/tracing/extras/chro...
tracing/tracing/extras/chrome/cpu_time_test.html:240:
assert.closeTo(getCpuUsage_(values), (30 + 20) / 200, 1e-7);
On 2017/04/10 20:48:29, tdresser wrote:
> It might be worth adding comments indicating where these values are coming
from,
> for cases where it isn't 100% obvious.
> 
> (e.g., why are we dividing by 200?)

I added a few comments about the calculations, especially in the first few tests
of constructMultiDimensionalView. Hopefully that should explain the general
approach of how the calculations are done. Adding detailed explanation of the
calculations for every single test is a lot of work, and I don't know if that
will be worth it.

[benjhayden, 2017-05-11 17:20:09]

Just a few nits then lgtm

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:124: * Returns a map of range in
|ranges| to total cpu time used by |thread|
Can you add a comment like this?
Ranges are not interned like Strings, so if you call get() on this Map, you must
use the Range objects from (where are these Range objects from?).

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:169: for (const pid in
model.processes) {
Please use Object.entries() instead of for..in.

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:173: for (const tid in
process.threads) {
Object.entries() please

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:175: const threadType = thread.type;
This alias doesn't look useful.

[dproy, 2017-05-11 20:23:57]

Addressed nits. I'll wait for one more round by tdresser@ before landing.

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:124: * Returns a map of range in
|ranges| to total cpu time used by |thread|
On 2017/05/11 17:20:08, benjhayden wrote:
> Can you add a comment like this?
> Ranges are not interned like Strings, so if you call get() on this Map, you
must
> use the Range objects from (where are these Range objects from?).

Done.

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:169: for (const pid in
model.processes) {
On 2017/05/11 17:20:09, benjhayden wrote:
> Please use Object.entries() instead of for..in.

Done.

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:173: for (const tid in
process.threads) {
On 2017/05/11 17:20:08, benjhayden wrote:
> Object.entries() please

Done.

https://codereview.chromium.org/2807243002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:175: const threadType = thread.type;
On 2017/05/11 17:20:08, benjhayden wrote:
> This alias doesn't look useful.

Removed.

[tdresser, 2017-05-11 20:33:08]

LGTM

https://codereview.chromium.org/2807243002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/cpu_time.html (right):

https://codereview.chromium.org/2807243002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:155: * See
cpu_time_multidimensinoal_view.md for more details about the returned
dimensional

[dproy, 2017-05-12 18:36:47]

The CQ bit was checked by dproy@chromium.org

[dproy, 2017-05-12 18:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/2807243002/#ps80001
(title: "Address benjhayden@ comments and fix test bug")

[commit-bot: I haz the power, 2017-05-12 18:36:51]

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-05-12 19:01:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494614207538730,
"parent_rev": "1c3f75a194ef9ebb3f6ddec4e47ca51521618f68", "commit_rev":
"ff3b45d456482f2ac0282d5532a9d38fe3d031d8"}

[commit-bot: I haz the power, 2017-05-12 19:01:08]

Description was changed from

==========
Construct Cpu Time MultidimensionalView

This CL adds a function to construct a multidimensional tree view for cpu time
metrics. The tree holds both cpuUage (cpu time per second) and cpuTotal (sum of
cpu time) information. The structure of the multidimensional view is explained
in more detail in the cpu_time_multidimensional_view.md doc.

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

to

==========
Construct Cpu Time MultidimensionalView

This CL adds a function to construct a multidimensional tree view for cpu time
metrics. The tree holds both cpuUage (cpu time per second) and cpuTotal (sum of
cpu time) information. The structure of the multidimensional view is explained
in more detail in the cpu_time_multidimensional_view.md doc.

BUG=catapult:#3325

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

[commit-bot: I haz the power, 2017-05-12 19:01:08]

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