Getting segments for rail stage and initiator

Getting segments for rail stage and initiator

This CL adds a function to get a map from rail stage and initiator type
to all the segments associated with a stage and initiator. This
function will be used by the cpu time metric to break down cpu time by
rail stage and initiator types.

This CL depends on https://codereview.chromium.org/2798213002/
BUG=catapult:#3325
R=benjhayden@chromium.org,tdresser@chromium.org

Review-Url: https://codereview.chromium.org/2804693005
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1c3f75a194ef9ebb3f6ddec4e47ca51521618f68

[dproy, 2017-04-06 03:32:18]

Description was changed from

==========
Getting segments for rail stage and initiator

This CL adds a function to get a map from rail stage and initiator type
to all the segments associated with a stage and initiator. This
function will be used by the cpu time metric to break down cpu time by
rail stage and initiator types.

BUG=catapult:#3325
R=benjhayden@chromium.org,tdresser@chromium.org
==========

to

==========
Getting segments for rail stage and initiator

This CL adds a function to get a map from rail stage and initiator type
to all the segments associated with a stage and initiator. This
function will be used by the cpu time metric to break down cpu time by
rail stage and initiator types.

This CL depends on https://codereview.chromium.org/2798213002/
BUG=catapult:#3325
R=benjhayden@chromium.org,tdresser@chromium.org
==========

[dproy, 2017-04-06 03:34:54]

PTAL.

Also, let me know if there's a formal way to indicate CL dependency. Chromium OS
uses CQ-DEPEND in the cl description, but I couldn't find anything similar for
general use.

[benjhayden, 2017-04-06 03:59:58]

A couple questions, feel free to schedule a VC.

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

https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:43: * The returned value is a
Map<string, Map<string, Set>> object. Example
Will this intermediate data structure be used for anything besides building
Histograms?
If not, then maybe go ahead and build them instead of formalizing this
intermediate data structure?

https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:67: for (const exp of
segment.expectations) {
This doesn't look quite like how Segments are intended to be used.
If you're going to handle all of the expectations, then why deal with Segments
at all?

[tdresser, 2017-04-06 13:23:20]

On 2017/04/06 03:59:58, benjhayden wrote:
> A couple questions, feel free to schedule a VC.
> 
>
https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
> File tracing/tracing/extras/chrome/cpu_time.html (right):
> 
>
https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
> tracing/tracing/extras/chrome/cpu_time.html:43: * The returned value is a
> Map<string, Map<string, Set>> object. Example
> Will this intermediate data structure be used for anything besides building
> Histograms?
> If not, then maybe go ahead and build them instead of formalizing this
> intermediate data structure?
> 
>
https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
> tracing/tracing/extras/chrome/cpu_time.html:67: for (const exp of
> segment.expectations) {
> This doesn't look quite like how Segments are intended to be used.
> If you're going to handle all of the expectations, then why deal with Segments
> at all?

To manage cl dependencies, either:
* Use `git branch -t`, to set up the dependency in git, which rietveld will
reflect.
* Use `git cl upload $NAME_OF_ANCESTOR_BRANCH`

[dproy, 2017-04-07 19:11:23]

This is now in sync with https://codereview.chromium.org/2804043003/ and ready
for review again. PTAL :)

I tried to encapsulate all use of rangeOfInterest in these three functions. The
rest of the code should not have to meddle around with rangeOfInterest.

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

https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:43: * The returned value is a
Map<string, Map<string, Set>> object. Example
On 2017/04/06 03:59:58, benjhayden wrote:
> Will this intermediate data structure be used for anything besides building
> Histograms?
> If not, then maybe go ahead and build them instead of formalizing this
> intermediate data structure?

Over VC, we talked about having a real javascript class here instead of a plain
map. I'm still leaning towards keeping it a simple map - defining a new class of
words for capturing what this hierarchy is hard, and might actually make the
code harder to understand. Let me know if you strongly feel otherwise about it -
I can reconsider my decision.

https://codereview.chromium.org/2804693005/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:67: for (const exp of
segment.expectations) {
On 2017/04/06 03:59:58, benjhayden wrote:
> This doesn't look quite like how Segments are intended to be used.
> If you're going to handle all of the expectations, then why deal with Segments
> at all?

Segments turned out to be really good as a unit of time I can compute cpu time
for and then aggregate easily for different breakdowns. I could do it with
expectations directly, but it leads to many unnecessary repeated calculations.

[dproy, 2017-04-13 18:20:10]

ping

[tdresser, 2017-04-13 19:41:43]

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

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:52: * The returned value is a
Use an @returns below.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:86:
'all_stages').get('all_initiators').add(segment);
Would it make sense to just keep a reference to this outside the loop, instead
of looking it up every time?

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:88: for (const exp of
segment.expectations) {
I'd use |expectation|.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:120: * |segments| should be filtered
before calling this function.
jsdoc would be nice here.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:139: * the intersection logic does
not have to sprinkled all over the rest of the
"does not have to sprinkled"

Missing a word?

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:140: * code.
jsdoc!

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:143: function
getStageToInitiatorToSegmentBounds(segments, rangeOfInterest) {
I'm not quite following this - could we have getStageToInitiatorToSegments leaf
nodes include both the segment and clamped segment range? Would that be simpler?

[benjhayden, 2017-04-16 23:38:39]

lgtm stamp, don't block on me.

[dproy, 2017-05-10 18:41:19]

PTAL.

benjhayden@: Let me know if you had any more comments.

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

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:52: * The returned value is a
On 2017/04/13 19:41:43, tdresser wrote:
> Use an @returns below.

Done.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:86:
'all_stages').get('all_initiators').add(segment);
Totally. Done.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:88: for (const exp of
segment.expectations) {
On 2017/04/13 19:41:43, tdresser wrote:
> I'd use |expectation|.

Done.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:120: * |segments| should be filtered
before calling this function.
On 2017/04/13 19:41:43, tdresser wrote:
> jsdoc would be nice here.

Deleted function in latest patch.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:139: * the intersection logic does
not have to sprinkled all over the rest of the
On 2017/04/13 19:41:43, tdresser wrote:
> "does not have to sprinkled"
> 
> Missing a word?

Deleted function in latest patch.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:140: * code.
On 2017/04/13 19:41:43, tdresser wrote:
> jsdoc!

Deleted function in latest patch.

https://codereview.chromium.org/2804693005/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/cpu_time.html:143: function
getStageToInitiatorToSegmentBounds(segments, rangeOfInterest) {
On 2017/04/13 19:41:43, tdresser wrote:
> I'm not quite following this - could we have getStageToInitiatorToSegments
leaf
> nodes include both the segment and clamped segment range? Would that be
simpler?

Turns out all the duplication could easily be removed by taking the intersection
at the beginning of the loop. Fixed in latest patch.

[benjhayden, 2017-05-10 20:10:54]

Upon further review, still lgtm. :-)

[dproy, 2017-05-11 20:05:51]

tdresser@ I'll wait for your approval before landing.

[tdresser, 2017-05-11 20:22:53]

LGTM!

[dproy, 2017-05-12 17:55:15]

The CQ bit was checked by dproy@chromium.org

[commit-bot: I haz the power, 2017-05-12 17:55:20]

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 18:19:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494611715855520,
"parent_rev": "fd3eb1dce85a8482e93658fb3fc00eb88c9c6cc0", "commit_rev":
"1c3f75a194ef9ebb3f6ddec4e47ca51521618f68"}

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

Description was changed from

==========
Getting segments for rail stage and initiator

This CL adds a function to get a map from rail stage and initiator type
to all the segments associated with a stage and initiator. This
function will be used by the cpu time metric to break down cpu time by
rail stage and initiator types.

This CL depends on https://codereview.chromium.org/2798213002/
BUG=catapult:#3325
R=benjhayden@chromium.org,tdresser@chromium.org
==========

to

==========
Getting segments for rail stage and initiator

This CL adds a function to get a map from rail stage and initiator type
to all the segments associated with a stage and initiator. This
function will be used by the cpu time metric to break down cpu time by
rail stage and initiator types.

This CL depends on https://codereview.chromium.org/2798213002/
BUG=catapult:#3325
R=benjhayden@chromium.org,tdresser@chromium.org

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

[commit-bot: I haz the power, 2017-05-12 18:19:20]

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