DevTools: move flow events tracking to TracingModel, support cross-threads case

DevTools: move flow events tracking to TracingModel, support cross-threads case

This implements support for tracking flow events in TracingModel and correctly supports the spec in terms of id matching for flow events.

Review-Url: https://codereview.chromium.org/2746153005
Cr-Commit-Position: refs/heads/master@{#457245}
Committed: https://chromium.googlesource.com/chromium/src/+/56024c61ffdabb977c7645098cb4654c6a1fdcc1

[caseq, 2017-03-15 19:06:01]

caseq@chromium.org changed reviewers:
+ alph@chromium.org, pfeldman@chromium.org

[caseq, 2017-03-15 19:07:16]

Description was changed from

==========
Move flow events tracking to TracingModel, support cross-threads case
==========

to

==========
DevTools: move flow events tracking to TracingModel, support cross-threads case

This implements support for tracking flow events in TracingModel and correctly
supports the spec in terms of id matching for flow events.
==========

[alph, 2017-03-15 19:31:48]

Thanks! lgtm

https://codereview.chromium.org/2746153005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746153005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:455:
for (; flowEvent; flowEvent = flowEvent.nextFlow) {
I bet merging these two loops together will make it simpler.

[caseq, 2017-03-15 20:47:55]

The CQ bit was checked by caseq@chromium.org

[caseq, 2017-03-15 20:47:55]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2746153005/#ps20001
(title: "review comments addressed")

[commit-bot: I haz the power, 2017-03-15 20:48:27]

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-03-15 22:46:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489610875052650,
"parent_rev": "25df70294a0c70bb8b39a30ea69ef0be072c4097", "commit_rev":
"56024c61ffdabb977c7645098cb4654c6a1fdcc1"}

[commit-bot: I haz the power, 2017-03-15 22:47:05]

Description was changed from

==========
DevTools: move flow events tracking to TracingModel, support cross-threads case

This implements support for tracking flow events in TracingModel and correctly
supports the spec in terms of id matching for flow events.
==========

to

==========
DevTools: move flow events tracking to TracingModel, support cross-threads case

This implements support for tracking flow events in TracingModel and correctly
supports the spec in terms of id matching for flow events.

Review-Url: https://codereview.chromium.org/2746153005
Cr-Commit-Position: refs/heads/master@{#457245}
Committed:
https://chromium.googlesource.com/chromium/src/+/56024c61ffdabb977c7645098cb4...
==========

[commit-bot: I haz the power, 2017-03-15 22:47:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/56024c61ffdabb977c7645098cb4...

[pfeldman, 2017-03-16 01:13:55]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2750133003/ by pfeldman@chromium.org.

The reason for reverting is: Breaks flow events completely..

[pfeldman, 2017-03-16 02:00:22]

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js (right):

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:363:
this._flowHeads.push(e);
So if there is no begin, you still consider it a head?

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:364:
lastInChain = e.phase !== phases.FlowEnd ? e : null;
why do you need all this logic? it is either begin-step-step-step-end or there
is a bug. if there is a bug, but consider the first one begin and the last one
an end.

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:467:
flowPairs.sort((a, b) => a.startTime - b.startTime);
Sort again?!?

[caseq, 2017-03-16 02:05:00]

On 2017/03/16 02:00:22, pfeldman_slow wrote:
>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js (right):
> 
>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:363:
> this._flowHeads.push(e);
> So if there is no begin, you still consider it a head?

Right, why not? We presume start could be before tracing started and still want
to show the remaining part.

>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:364:
> lastInChain = e.phase !== phases.FlowEnd ? e : null;
> why do you need all this logic? it is either begin-step-step-step-end or there
> is a bug. if there is a bug, but consider the first one begin and the last one
> an end.

If I'm parsing you right, it's just as above -- to correctly handle partial
chains.

>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
> (right):
> 
>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:467:
> flowPairs.sort((a, b) => a.startTime - b.startTime);
> Sort again?!?

This is just because we assume sorted list in FlameChart, apparently only for a
bail out upon startTime > windowRight -- we can rather nuke the latter.

[pfeldman, 2017-03-16 07:03:50]

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js (right):

https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:249: var key =
`${event.categoriesString}-${event.name}-${event.id}`;
I don't think categoriesString and event name should be a part of the key.

Overall, I'm looking at this +172 lines, -55 lines patch and I wonder why we are
adding all this complexity. The code was very readable before the change and now
it somewhat cryptic. I don't think we need this sorting, complex keys and model
api for our scenarios. I want to implement the tree shaking and I don't think
I'm going to use this API. At the same time, rendering code is now more complex.
All it all it seems complexity for the sake of complexity, so I wonder, why?

[caseq, 2017-03-16 16:59:19]

On 2017/03/16 07:03:50, pfeldman_slow wrote:
>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js (right):
> 
>
https://codereview.chromium.org/2746153005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:249: var key
=
> `${event.categoriesString}-${event.name}-${event.id}`;
> I don't think categoriesString and event name should be a part of the key.
> 
> Overall, I'm looking at this +172 lines, -55 lines patch and I wonder why we
are
> adding all this complexity. The code was very readable before the change and
now
> it somewhat cryptic. I don't think we need this sorting, complex keys and
model
> api for our scenarios. I want to implement the tree shaking and I don't think
> I'm going to use this API. At the same time, rendering code is now more
complex.
> All it all it seems complexity for the sake of complexity, so I wonder, why?

+172 -55 is partially due to added tests (83 lines) that previous implementation
lacks. We actually need even more tests as the regression of ps2 shows.

The remaining added complexity is to support the flow events in a generic way in
compliance to Trace Event format spec -- your implementation does not handle
cross-thread events properly and does not follow the spec in terms of handling
ids. We generally want Timeline to be able to show regular traces in a
reasonable way, as we are getting more and more requests to be able to display
all events. The rendering implementation actually lost a handful of lines as a
result of this change, and the overall increase in code w/o tests is 37 lines. I
think this is a reasonable price to pay for compatible implementation.

If you have any API suggestions, I'm all ears. I'm not sure I understand what
tree shaking is with regards to flow events.

[pfeldman, 2017-03-16 21:04:11]

> The remaining added complexity is to support the flow events in a generic way
in
> compliance to Trace Event format spec

I don't think the spec is practically useful here - we need our own treatment
for flows. I'd like to see flows as an aspect of trace events and tracing is
currently confusing ids, bind_ids and phases with that regard. It also does not
support certain capabilities such as 'canceling' an async event. Maybe that is
why trace viewer clients I spoke so far with avoid using it.

> -- your implementation does not handle cross-thread events properly and does
not follow the spec in terms of handling ids.

We don't yet have cross-thread async traceability and have no design for its
instrumentation so far.

> We generally want Timeline to be able to show regular traces in a
> reasonable way, as we are getting more and more requests to be able to display
> all events.

This does not include flows - seems like they are not useful /used by tracing /
trace viewer clients.

> If you have any API suggestions, I'm all ears. I'm not sure I understand what
> tree shaking is with regards to flow events.

The strength of the tracing is being a collection of primitive events that are
easy to iterate. The flow analysis capabilities that we will need in the future
include tree shaking - figuring out flow implications of a particular node in
the trace. We will need a separate model for that on top of trace events that
would perceive them as a graph and allow efficient flow analysis. When we have a
good idea on what we need from it _and_ we know whether we are using flow events
or any other markup for our flow, we can start doing something like the patch
you suggest.

[caseq, 2017-03-16 21:17:40]

On 2017/03/16 21:04:11, pfeldman_slow wrote:
> > The remaining added complexity is to support the flow events in a generic
way
> in
> > compliance to Trace Event format spec
> 
> I don't think the spec is practically useful here - we need our own treatment
> for flows. I'd like to see flows as an aspect of trace events and tracing is
> currently confusing ids, bind_ids and phases with that regard. It also does
not
> support certain capabilities such as 'canceling' an async event. Maybe that is
> why trace viewer clients I spoke so far with avoid using it.

We shouldn't use flow events for the purpose of async instrumentation then.
Abusing existent events with well-specified handling rules by introducing
incompatible rules does not make sense.

> > -- your implementation does not handle cross-thread events properly and does
> not follow the spec in terms of handling ids.
> 
> We don't yet have cross-thread async traceability and have no design for its
> instrumentation so far.

Right, but we do have cross-thread flow events.

> The strength of the tracing is being a collection of primitive events that are
> easy to iterate. The flow analysis capabilities that we will need in the
future
> include tree shaking - figuring out flow implications of a particular node in
> the trace. We will need a separate model for that on top of trace events that
> would perceive them as a graph and allow efficient flow analysis. When we have
a
> good idea on what we need from it _and_ we know whether we are using flow
events
> or any other markup for our flow, we can start doing something like the patch
> you suggest.

There are well defined rules how flow events "bind" to other trace events and
from what I can see this could work reasonably well for our use cases. My next
step was actually to implement these bindings in the tracing model and return
"bound" event for each flow event, which would make flamechart visualization
make more sense (compared to having arrows handing in the air).