Support new flow API in the importer.

Support new flow event API in the importer.

The Chrome side CL is tracked at: https://codereview.chromium.org/1239593002

The design doc is at: https://docs.google.com/document/d/1La_0PPfsTqHJihazYhff96thhjPtvq1KjAUOJu0dvEg/edit?pli=1#

BUG=#1079

Migrated to catapult and landed. https://codereview.chromium.org/1276223005

[dsinclair, 2015-07-20 18:36:00]

I think for the examples, the last step in each one needs a flow direction and
bind_id, yea?

e.g.

{"pid":20966,"tid":20966,"ts":909332655151,"ph":"s","cat":"input,benchmark","name":"LatencyInfo.Flow","args":{"trace_id":12884902037},"tts":4032046,"id":"0x300000095"},

This needs some kind of flow_in or flow_out so we know how to hook the flow up
to it, yea?

[yuhaoz, 2015-07-20 18:38:42]

On 2015/07/20 18:36:00, dsinclair wrote:
> I think for the examples, the last step in each one needs a flow direction and
> bind_id, yea?
> 
> e.g.
> 
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"s","cat":"input,benchmark","name":"LatencyInfo.Flow","args":{"trace_id":12884902037},"tts":4032046,"id":"0x300000095"},
> 
> This needs some kind of flow_in or flow_out so we know how to hook the flow up
> to it, yea?

Not really. It's ph is "s", so it has to be hooked to a flow_out slice. Same
with ph === "f". The only exception is ph === "t", in which case we have to rely
on the order.

[dsinclair, 2015-07-20 18:40:11]

On 2015/07/20 18:38:42, yuhaoz wrote:
> On 2015/07/20 18:36:00, dsinclair wrote:
> > I think for the examples, the last step in each one needs a flow direction
and
> > bind_id, yea?
> > 
> > e.g.
> > 
> >
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"s","cat":"input,benchmark","name":"LatencyInfo.Flow","args":{"trace_id":12884902037},"tts":4032046,"id":"0x300000095"},
> > 
> > This needs some kind of flow_in or flow_out so we know how to hook the flow
up
> > to it, yea?
> 
> Not really. It's ph is "s", so it has to be hooked to a flow_out slice. Same
> with ph === "f". The only exception is ph === "t", in which case we have to
rely
> on the order.


I guess my question is, why would you have the 's' event? You don't need to emit
the individual flow events anymore, you'd want it hooked up to a real slice yea?
So, you'd get an X event with FLOW_IN there instead of 's'?

[yuhaoz, 2015-07-20 19:13:11]

On 2015/07/20 18:40:11, dsinclair wrote:
> On 2015/07/20 18:38:42, yuhaoz wrote:
> > On 2015/07/20 18:36:00, dsinclair wrote:
> > > I think for the examples, the last step in each one needs a flow direction
> and
> > > bind_id, yea?
> > > 
> > > e.g.
> > > 
> > >
> >
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"s","cat":"input,benchmark","name":"LatencyInfo.Flow","args":{"trace_id":12884902037},"tts":4032046,"id":"0x300000095"},
> > > 
> > > This needs some kind of flow_in or flow_out so we know how to hook the
flow
> up
> > > to it, yea?
> > 
> > Not really. It's ph is "s", so it has to be hooked to a flow_out slice. Same
> > with ph === "f". The only exception is ph === "t", in which case we have to
> rely
> > on the order.
> 
> 
> I guess my question is, why would you have the 's' event? You don't need to
emit
> the individual flow events anymore, you'd want it hooked up to a real slice
yea?
> So, you'd get an X event with FLOW_IN there instead of 's'?

Can you give an example trace explaining the trace format you have in your mind?

[dsinclair, 2015-07-20 19:15:18]

On 2015/07/20 19:13:11, yuhaoz wrote:
> On 2015/07/20 18:40:11, dsinclair wrote:
> > On 2015/07/20 18:38:42, yuhaoz wrote:
> > > On 2015/07/20 18:36:00, dsinclair wrote:
> > > > I think for the examples, the last step in each one needs a flow
direction
> > and
> > > > bind_id, yea?
> > > > 
> > > > e.g.
> > > > 
> > > >
> > >
> >
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"s","cat":"input,benchmark","name":"LatencyInfo.Flow","args":{"trace_id":12884902037},"tts":4032046,"id":"0x300000095"},
> > > > 
> > > > This needs some kind of flow_in or flow_out so we know how to hook the
> flow
> > up
> > > > to it, yea?
> > > 
> > > Not really. It's ph is "s", so it has to be hooked to a flow_out slice.
Same
> > > with ph === "f". The only exception is ph === "t", in which case we have
to
> > rely
> > > on the order.
> > 
> > 
> > I guess my question is, why would you have the 's' event? You don't need to
> emit
> > the individual flow events anymore, you'd want it hooked up to a real slice
> yea?
> > So, you'd get an X event with FLOW_IN there instead of 's'?
> 
> Can you give an example trace explaining the trace format you have in your
mind?


{"pid":20966,"tid":20966,"ts":90933265999,"ph":"X","cat":"toplevel","name":"LatencyInfo::AddLatencyNumberWithTimestampImpl","args":{},"dur":30,"tdur":30,"tts":4032046,"bind_id":"0x300000095","flow_out":"true"},
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"X","cat":"toplevel","name":"Some
other thing
....","args":{},"dur":30,"tdur":30,"tts":409996,"bind_id":"0x300000095","flow_in":"true"},

[yuhaoz, 2015-07-20 22:04:39]

> > Can you give an example trace explaining the trace format you have in your
> mind?
> 
> 
>
{"pid":20966,"tid":20966,"ts":90933265999,"ph":"X","cat":"toplevel","name":"LatencyInfo::AddLatencyNumberWithTimestampImpl","args":{},"dur":30,"tdur":30,"tts":4032046,"bind_id":"0x300000095","flow_out":"true"},
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"X","cat":"toplevel","name":"Some
other thing
....","args":{},"dur":30,"tdur":30,"tts":409996,"bind_id":"0x300000095","flow_in":"true"},

So if we do this, my concern is that we will lose information specific to flow
events, such as their category type, name, etc. For example, right now if we
click a flow event, trace-viewer shows its Title (e.g., MessageLoop::PostTask)
and Category (e.g., disabled-by-default-toplevel.flow).

With the new flow API and the trace format above, we do not have these
information. So we could either:
1) create separate fields in a trace entry such as "flow_cat":
"MessageLoop::PostTask" and "flow_name": "disabled-by-default-toplevel.flow"; 
2) or make flow specific information part of the arguments, such as "args":
{"flow_cat": "MessageLoop::PostTask", "flow_name":
"disabled-by-default-toplevel.flow"}.

Both approaches will require some heavy-duty change to the flow event macro. In
the first approach, the API would look something like:
TRACE_EVENT1(category_group, name, flow_category_group, flow_name, arg1_name,
arg1_val). In the second approach the API would look something like:
TRACE_EVENT3(category_group, name, arg1_name, arg1_val, arg2_name, arg2_val,
arg3_name, arg3_val).

What do you all think?

[nduca(google), 2015-07-20 23:28:30]

Dumb question and maybe you've already done this, but how about before we review
the importer, we pause and have the flow events v2 design doc be something we
all agree on, and then we review this patch once we like the doc. The doc should
provide the c++ api, and the encoding in json.

Maybe you should make sure that doc is up to date, remove the old ideas to a
backup doc [per my notes on design doc writing in
https://docs.google.com/document/d/1mN31mSx-41znNeBQER5VL9YoTlKHaS8c97_hXhb0k...,
then share that doc with tracing@chromium.org, then to be doubly sure, organize
a vc with obvious stakeholders to discuss status. then when you've got all that
sorted, you can then start landing stuff. :)

[dsinclair, 2015-07-21 14:24:24]

On 2015/07/20 22:04:39, yuhaoz wrote:
> > > Can you give an example trace explaining the trace format you have in your
> > mind?
> > 
> > 
> >
>
{"pid":20966,"tid":20966,"ts":90933265999,"ph":"X","cat":"toplevel","name":"LatencyInfo::AddLatencyNumberWithTimestampImpl","args":{},"dur":30,"tdur":30,"tts":4032046,"bind_id":"0x300000095","flow_out":"true"},
> >
>
{"pid":20966,"tid":20966,"ts":909332655151,"ph":"X","cat":"toplevel","name":"Some
> other thing
>
....","args":{},"dur":30,"tdur":30,"tts":409996,"bind_id":"0x300000095","flow_in":"true"},
> 
> So if we do this, my concern is that we will lose information specific to flow
> events, such as their category type, name, etc. For example, right now if we
> click a flow event, trace-viewer shows its Title (e.g., MessageLoop::PostTask)
> and Category (e.g., disabled-by-default-toplevel.flow).
> 
> With the new flow API and the trace format above, we do not have these
> information. So we could either:
> 1) create separate fields in a trace entry such as "flow_cat":
> "MessageLoop::PostTask" and "flow_name": "disabled-by-default-toplevel.flow"; 
> 2) or make flow specific information part of the arguments, such as "args":
> {"flow_cat": "MessageLoop::PostTask", "flow_name":
> "disabled-by-default-toplevel.flow"}.
> 
> Both approaches will require some heavy-duty change to the flow event macro.
In
> the first approach, the API would look something like:
> TRACE_EVENT1(category_group, name, flow_category_group, flow_name, arg1_name,
> arg1_val). In the second approach the API would look something like:
> TRACE_EVENT3(category_group, name, arg1_name, arg1_val, arg2_name, arg2_val,
> arg3_name, arg3_val).
> 
> What do you all think?



As we discussed in chat, the category and name for the flow event would be the
same as the slice.
You verified there were no flow events that took args different from the binding
slice so
that isn't an issue. So, you don't need to add anything to the API as the
category and name
are already there.

The idea being, there is no flow event. It's data attached to a slice. The slice
just tells you
a thing comes out or goes into it.

Though, one issue with the current API is you can't have one bind_id flow in and
a second
one flow out. If you have both in/out it has to be a step which we may not want,
but we'd need
to verify. (It also means you can't easily have multiple flow's out of a single
slice as they'd
get mixed up.)

[yuhaoz, 2015-07-21 17:14:16]

On 2015/07/21 14:24:24, dsinclair wrote:
> As we discussed in chat, the category and name for the flow event would be the
> same as the slice.
> You verified there were no flow events that took args different from the
binding
> slice so
> that isn't an issue. So, you don't need to add anything to the API as the
> category and name
> are already there.
> 
> The idea being, there is no flow event. It's data attached to a slice. The
slice
> just tells you
> a thing comes out or goes into it.
> 
> Though, one issue with the current API is you can't have one bind_id flow in
and
> a second
> one flow out. If you have both in/out it has to be a step which we may not
want,
> but we'd need
> to verify. (It also means you can't easily have multiple flow's out of a
single
> slice as they'd
> get mixed up.)

So just to be 100% clear, in the following example (IPC using flow events):

The original implementation is:

TRACE_EVENT2("ipc,toplevel", "ChannelReader::DispatchInputData",
                   "class", IPC_MESSAGE_ID_CLASS(m.type()),
                   "line", IPC_MESSAGE_ID_LINE(m.type()));

and then

TRACE_EVENT_FLOW_END_BIND_TO_ENCLOSING0(
        TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", header()->flags);

Now with the new flow event API, we would simply do:

TRACE_EVENT_WITH_FLOW2("ipc,toplevel", "ChannelReader::DispatchInputData",
                   bind_id, TRACE_EVENT_FLAG_OUT,
                   "class", IPC_MESSAGE_ID_CLASS(m.type()),
                   "line", IPC_MESSAGE_ID_LINE(m.type()));

[yuhaoz, 2015-07-21 17:28:18]

On 2015/07/21 17:14:16, yuhaoz wrote:
> 
> So just to be 100% clear, in the following example (IPC using flow events):
> 
> The original implementation is:
> 
> TRACE_EVENT2("ipc,toplevel", "ChannelReader::DispatchInputData",
>                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
>                    "line", IPC_MESSAGE_ID_LINE(m.type()));
> 
> and then
> 
> TRACE_EVENT_FLOW_END_BIND_TO_ENCLOSING0(
>         TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", header()->flags);
> 
> Now with the new flow event API, we would simply do:
> 
> TRACE_EVENT_WITH_FLOW2("ipc,toplevel", "ChannelReader::DispatchInputData",
>                    bind_id, TRACE_EVENT_FLAG_OUT,
>                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
>                    "line", IPC_MESSAGE_ID_LINE(m.type()));

So thinking about it, I am OK with this API format. The only thing I have is
that I think the category should probably be the original flow event category
name, i.e., TRACE_DISABLED_BY_DEFAULT("ipc.flow") in this case. This will allow
us to enable/disable tracking flow events easily. Using the original flow event
category name actually makes sense because in TRACE_EVENT_WITH_FLOW2, we are
really interested in the flow, rather than the binding slice.

[yuhaoz, 2015-07-21 17:31:06]

On 2015/07/20 23:28:30, nduca(google) wrote:
> Dumb question and maybe you've already done this, but how about before we
review
> the importer, we pause and have the flow events v2 design doc be something we
> all agree on, and then we review this patch once we like the doc. The doc
should
> provide the c++ api, and the encoding in json.
> 
> Maybe you should make sure that doc is up to date, remove the old ideas to a
> backup doc [per my notes on design doc writing in
>
https://docs.google.com/document/d/1mN31mSx-41znNeBQER5VL9YoTlKHaS8c97_hXhb0k...,
> then share that doc with mailto:tracing@chromium.org, then to be doubly sure,
organize
> a vc with obvious stakeholders to discuss status. then when you've got all
that
> sorted, you can then start landing stuff. :)

Yes updating the doc is on my plate!

[dsinclair, 2015-07-21 18:40:08]

On 2015/07/21 17:28:18, yuhaoz wrote:
> On 2015/07/21 17:14:16, yuhaoz wrote:
> > 
> > So just to be 100% clear, in the following example (IPC using flow events):
> > 
> > The original implementation is:
> > 
> > TRACE_EVENT2("ipc,toplevel", "ChannelReader::DispatchInputData",
> >                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
> >                    "line", IPC_MESSAGE_ID_LINE(m.type()));
> > 
> > and then
> > 
> > TRACE_EVENT_FLOW_END_BIND_TO_ENCLOSING0(
> >         TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", header()->flags);
> > 
> > Now with the new flow event API, we would simply do:
> > 
> > TRACE_EVENT_WITH_FLOW2("ipc,toplevel", "ChannelReader::DispatchInputData",
> >                    bind_id, TRACE_EVENT_FLAG_OUT,
> >                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
> >                    "line", IPC_MESSAGE_ID_LINE(m.type()));
> 
> So thinking about it, I am OK with this API format. The only thing I have is
> that I think the category should probably be the original flow event category
> name, i.e., TRACE_DISABLED_BY_DEFAULT("ipc.flow") in this case. This will
allow
> us to enable/disable tracking flow events easily. Using the original flow
event
> category name actually makes sense because in TRACE_EVENT_WITH_FLOW2, we are
> really interested in the flow, rather than the binding slice.


I think we should keep the old names. For your use case the flow event is the
important part. For others, the slice is the important part. The fact that there
is a flow event there becomes an implementation detail is isn't relevant for the
actual trace event. Flow arrows just fall out of the slices.

[yuhaoz, 2015-07-21 18:45:22]

On 2015/07/21 18:40:08, dsinclair wrote:
> On 2015/07/21 17:28:18, yuhaoz wrote:
> > On 2015/07/21 17:14:16, yuhaoz wrote:
> > > 
> > > So just to be 100% clear, in the following example (IPC using flow
events):
> > > 
> > > The original implementation is:
> > > 
> > > TRACE_EVENT2("ipc,toplevel", "ChannelReader::DispatchInputData",
> > >                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
> > >                    "line", IPC_MESSAGE_ID_LINE(m.type()));
> > > 
> > > and then
> > > 
> > > TRACE_EVENT_FLOW_END_BIND_TO_ENCLOSING0(
> > >         TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", header()->flags);
> > > 
> > > Now with the new flow event API, we would simply do:
> > > 
> > > TRACE_EVENT_WITH_FLOW2("ipc,toplevel", "ChannelReader::DispatchInputData",
> > >                    bind_id, TRACE_EVENT_FLAG_OUT,
> > >                    "class", IPC_MESSAGE_ID_CLASS(m.type()),
> > >                    "line", IPC_MESSAGE_ID_LINE(m.type()));
> > 
> > So thinking about it, I am OK with this API format. The only thing I have is
> > that I think the category should probably be the original flow event
category
> > name, i.e., TRACE_DISABLED_BY_DEFAULT("ipc.flow") in this case. This will
> allow
> > us to enable/disable tracking flow events easily. Using the original flow
> event
> > category name actually makes sense because in TRACE_EVENT_WITH_FLOW2, we are
> > really interested in the flow, rather than the binding slice.
> 
> 
> I think we should keep the old names. For your use case the flow event is the
> important part. For others, the slice is the important part. The fact that
there
> is a flow event there becomes an implementation detail is isn't relevant for
the
> actual trace event. Flow arrows just fall out of the slices.

So if we enable ipc/toplevel in tracing, then we automatically always get flow
events?

From looking at the codebase, in all cases where we would use
TRACE_EVENT_WITH_FLOW, we are creating a new slice just for binding the flow
event. Most flow events are trace events are traced in different functions as of
now.

[beaudoin, 2015-07-27 20:01:00]

https://codereview.appspot.com/251300043/diff/1/tracing/tracing/extras/import...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/trace_event_importer.html:365: flow_dir =
'inout';
See discussion on hangout: we should get rid of this.

https://codereview.appspot.com/251300043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/trace_event_importer.html:1172: }
Consistency: no {} for 1-line if.

https://codereview.appspot.com/251300043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/trace_event_importer.html:1201: }
No {}

https://codereview.appspot.com/251300043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/trace_event_importer.html:1278: } else {
I find this 'else' logic a bit brittle. At least add a comment:
// event.ph === 'f'

[yuhaoz, 2015-07-31 01:56:12]

Create flows between flow producer and flow consumers. The new API makes the
importer implementation very clean and easy.

[nduca(google), 2015-07-31 02:28:53]

can you get vmpstr to pre-review this?

[vmpstr, 2015-07-31 17:33:37]

(sorry if this was answered already)

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:404:
this.FlowProducerToSliceMap_[event.bind_id] = slice;
Is there any way to assert here that there isn't already a slice at this bind
id?

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:406:
this.FlowConsumerToSliceMap_[event.bind_id] = slice;
How does this handle multiple consumers?

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1339:
flowIdToEvent[event.bind_id] = undefined;
Again, I'm not clear how this would work with multiple consumers.

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/model/sli...
File tracing/tracing/model/slice.html (right):

https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/model/sli...
tracing/tracing/model/slice.html:46: this.bind_id_ = opt_bind_id;
if it is undefined, then bind_id is also undefined? why not just say
this.bind_id_ = opt_bind_id without the conditional?

[yuhaoz, 2015-08-01 18:57:55]

On 2015/07/31 17:33:37, vmpstr wrote:
> (sorry if this was answered already)
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
> File tracing/tracing/extras/importer/trace_event_importer.html (right):
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
> tracing/tracing/extras/importer/trace_event_importer.html:404:
> this.FlowProducerToSliceMap_[event.bind_id] = slice;
> Is there any way to assert here that there isn't already a slice at this bind
> id?
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
> tracing/tracing/extras/importer/trace_event_importer.html:406:
> this.FlowConsumerToSliceMap_[event.bind_id] = slice;
> How does this handle multiple consumers?
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/extras/im...
> tracing/tracing/extras/importer/trace_event_importer.html:1339:
> flowIdToEvent[event.bind_id] = undefined;
> Again, I'm not clear how this would work with multiple consumers.
We will just have to create a new flow event.
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/model/sli...
> File tracing/tracing/model/slice.html (right):
> 
>
https://codereview.appspot.com/251300043/diff/20001/tracing/tracing/model/sli...
> tracing/tracing/model/slice.html:46: this.bind_id_ = opt_bind_id;
> if it is undefined, then bind_id is also undefined? why not just say
> this.bind_id_ = opt_bind_id without the conditional?
Done

[yuhaoz, 2015-08-01 18:58:18]

PTAL. Add support for multiple flow consumers, and add some unittess for it.

[dsinclair, 2015-08-04 19:54:31]

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:381: if (event.flow_in
=== true)
Shouldn't need === true here. Can just do if (event.flow_in)

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:382: flow_dir = 'in';
nit: indenting

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1183: if (event.ph ===
'X') {
We probably don't want to use 'X' here as we're going to want to bind to other
types as well. This should probably just check for bind_id on the event instead
of a specific ph.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1204: if (event.ph ===
'X') {
This assumption seems invalid.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1233:
startSlice.outFlowEvents.push(flowEvent);
Can we unify the flow event creation with the above? The id and duration are
different but the test of this seems the same.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1240: if (event.ph ===
'X') {
ditto.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1254: // TODO(nduca):
Figure out endSlice from ts and binding point.
I think we should kill this auto-binding code in general.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1291: if (event.ph ===
'X') {
ditto.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1297: // Cannot have
more than one flow producers.
What does this mean? A given slice can't? What can't?

[yuhaoz, 2015-08-04 20:54:57]

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1204: if (event.ph ===
'X') {
On 2015/08/04 19:54:31, dsinclair wrote:
> This assumption seems invalid.

What do you mean invalid?

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1254: // TODO(nduca):
Figure out endSlice from ts and binding point.
On 2015/08/04 19:54:31, dsinclair wrote:
> I think we should kill this auto-binding code in general.

Then we are not going to be able to support old traces, and more importantly,
any other code that use old Flow APIs.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1297: // Cannot have
more than one flow producers.
On 2015/08/04 19:54:31, dsinclair wrote:
> What does this mean? A given slice can't? What can't?

Oh I meant for a given slice.

[dsinclair, 2015-08-04 20:59:01]

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1204: if (event.ph ===
'X') {
On 2015/08/04 20:54:57, yuhaoz wrote:
> On 2015/08/04 19:54:31, dsinclair wrote:
> > This assumption seems invalid.
> 
> What do you mean invalid?

We're going to want to have flows come from things other then X events. So,
assuming the phase is X isn't a valid way to determine if this is flow v2 or
not.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1254: // TODO(nduca):
Figure out endSlice from ts and binding point.
On 2015/08/04 20:54:57, yuhaoz wrote:
> On 2015/08/04 19:54:31, dsinclair wrote:
> > I think we should kill this auto-binding code in general.
> 
> Then we are not going to be able to support old traces, and more importantly,
> any other code that use old Flow APIs.


a) I believe we won't be supporting that many old traces for RAIL so I think
breaking this is fine.

b) This binding was part of the original flow APIs we added it later. It never
worked quite correctly and has issues. I think old flow API should just go back
to, essentially, being instant events linked by arrows and we stop trying to
makeup where they attach.

[yuhaoz, 2015-08-04 21:25:33]

> On 2015/08/04 20:54:57, yuhaoz wrote:
> > On 2015/08/04 19:54:31, dsinclair wrote:
> 
> b) This binding was part of the original flow APIs we added it later. It never
> worked quite correctly and has issues. I think old flow API should just go
back
> to, essentially, being instant events linked by arrows and we stop trying to
> makeup where they attach.

Just to make sure I understand it. We will turn "s", "t", "f" phases events into
"I" events, and then create flow events to link them together?

If that's how we dealt with them before, I guess it by and large a matter of
reverting back. Could you please show me the CL that made this change?

[dsinclair, 2015-08-05 14:57:52]

On 2015/08/04 21:25:33, yuhaoz wrote:
> > On 2015/08/04 20:54:57, yuhaoz wrote:
> > > On 2015/08/04 19:54:31, dsinclair wrote:
> > 
> > b) This binding was part of the original flow APIs we added it later. It
never
> > worked quite correctly and has issues. I think old flow API should just go
> back
> > to, essentially, being instant events linked by arrows and we stop trying to
> > makeup where they attach.
> 
> Just to make sure I understand it. We will turn "s", "t", "f" phases events
into
> "I" events, and then create flow events to link them together?
> 
> If that's how we dealt with them before, I guess it by and large a matter of
> reverting back. Could you please show me the CL that made this change?


No, we create flow events. The just look like instant events in the UI. We don't
turn them into anything. The thing we added was the binding of flow events to
other slices. You'd have to go through the git logs, I'd guess it was several
changes which added the feature, there is no one cl.

[yuhaoz, 2015-08-06 17:43:15]

PTAL. Support flow steps since we decided to bring it back.

[yuhaoz, 2015-08-06 17:55:36]

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:381: if (event.flow_in
=== true)
On 2015/08/04 19:54:31, dsinclair wrote:
> Shouldn't need === true here. Can just do if (event.flow_in)

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:382: flow_dir = 'in';
On 2015/08/04 19:54:31, dsinclair wrote:
> nit: indenting

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1183: if (event.ph ===
'X') {
On 2015/08/04 19:54:31, dsinclair wrote:
> We probably don't want to use 'X' here as we're going to want to bind to other
> types as well. This should probably just check for bind_id on the event
instead
> of a specific ph.

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1204: if (event.ph ===
'X') {
On 2015/08/04 20:59:01, dsinclair wrote:
> On 2015/08/04 20:54:57, yuhaoz wrote:
> > On 2015/08/04 19:54:31, dsinclair wrote:
> > > This assumption seems invalid.
> > 
> > What do you mean invalid?
> 
> We're going to want to have flows come from things other then X events. So,
> assuming the phase is X isn't a valid way to determine if this is flow v2 or
> not.

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1233:
startSlice.outFlowEvents.push(flowEvent);
On 2015/08/04 19:54:31, dsinclair wrote:
> Can we unify the flow event creation with the above? The id and duration are
> different but the test of this seems the same.

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1240: if (event.ph ===
'X') {
On 2015/08/04 19:54:31, dsinclair wrote:
> ditto.

Done.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1254: // TODO(nduca):
Figure out endSlice from ts and binding point.
On 2015/08/04 20:59:01, dsinclair wrote:
> On 2015/08/04 20:54:57, yuhaoz wrote:
> > On 2015/08/04 19:54:31, dsinclair wrote:
> > > I think we should kill this auto-binding code in general.
> > 
> > Then we are not going to be able to support old traces, and more
importantly,
> > any other code that use old Flow APIs.
> 
> 
> a) I believe we won't be supporting that many old traces for RAIL so I think
> breaking this is fine.
> 
> b) This binding was part of the original flow APIs we added it later. It never
> worked quite correctly and has issues. I think old flow API should just go
back
> to, essentially, being instant events linked by arrows and we stop trying to
> makeup where they attach.

I will remove the old binding once I make Chrome completely use Flow v2. For
now, this binding is still useful for working on associated events.

Maybe I should open a bug on trace-viewer to keep track of this.

https://codereview.appspot.com/251300043/diff/40001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:1291: if (event.ph ===
'X') {
On 2015/08/04 19:54:31, dsinclair wrote:
> ditto.

Done.

[yuhaoz, 2015-08-06 22:09:42]

On 2015/08/06 17:55:36, yuhaoz wrote:

Just two screenshots showing the effects of flow steps and multiple consumers.

https://x20web.corp.google.com/~yuhaoz/Screen%20Shot%202015-08-06%20at%203.07...
https://x20web.corp.google.com/~yuhaoz/Screen%20Shot%202015-08-06%20at%203.07...

[dsinclair, 2015-08-07 14:03:48]

On 2015/08/06 22:09:42, yuhaoz wrote:
> On 2015/08/06 17:55:36, yuhaoz wrote:
> 
> Just two screenshots showing the effects of flow steps and multiple consumers.
> 
>
https://x20web.corp.google.com/~yuhaoz/Screen%20Shot%202015-08-06%20at%203.07...
>
https://x20web.corp.google.com/~yuhaoz/Screen%20Shot%202015-08-06%20at%203.07...


Those both look good to me, is there some issue with them?

Can you please move this review over to the catapult repo?

[yuhaoz, 2015-08-07 21:07:16]

On 2015/08/07 14:03:48, dsinclair wrote:
> Those both look good to me, is there some issue with them?
> 
> Can you please move this review over to the catapult repo?

No there is nothing wrong with them. I was just trying to show the end results.
Could you advice how to move the review to catapult? Also seems like I won't be
able to land this CL to trace-viewer, but have to land it to catapult. Do I have
to do anything to make that happen?

[dsinclair, 2015-08-08 12:22:41]

On 2015/08/07 21:07:16, yuhaoz wrote:
> On 2015/08/07 14:03:48, dsinclair wrote:
> > Those both look good to me, is there some issue with them?
> > 
> > Can you please move this review over to the catapult repo?
> 
> No there is nothing wrong with them. I was just trying to show the end
results.
> Could you advice how to move the review to catapult? Also seems like I won't
be
> able to land this CL to trace-viewer, but have to land it to catapult. Do I
have
> to do anything to make that happen?

Yes, you have to checkout github.com/catapult-project/catapult. Then hit the raw
button above to get a raw patch. Checkout a branch in catapult and patch -p1 <
issue251300043_60001.diff to apply the patch then git cl upload which will
create a review on codereview.chromium.org

[vmpstr, 2015-08-10 17:25:54]

https://codereview.appspot.com/251300043/diff/60001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/60001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:386: event.flowPhase =
'consumer';
Are slices that don't have any flow considered consumers? Why?

[yuhaoz, 2015-08-10 17:28:46]

https://codereview.appspot.com/251300043/diff/60001/tracing/tracing/extras/im...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.appspot.com/251300043/diff/60001/tracing/tracing/extras/im...
tracing/tracing/extras/importer/trace_event_importer.html:386: event.flowPhase =
'consumer';
On 2015/08/10 17:25:54, vmpstr wrote:
> Are slices that don't have any flow considered consumers? Why?

Ah my bad ;( Should check flow_in.