Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1239593002:
Implement a new flow event API that allows binding flow events and regular events. (Closed)
|
Created:
5 years, 5 months ago by yuhaoz Modified:
5 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement a new flow event API that allows binding flow events and regular events.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/21c55cb65ef294af3e5479c34473786281168aef
Cr-Commit-Position: refs/heads/master@{#341023}
Committed: https://crrev.com/565896aeb84ba2188a725b4ffc13bbbdb8d1ceea
Cr-Commit-Position: refs/heads/master@{#341154}
Patch Set 1 #Patch Set 2 : Remove a redundant macro. #
Total comments: 5
Patch Set 3 : Implement TRACE_EVENT_WITH_FLOW #Patch Set 4 : Remove unnecessary comments. #
Total comments: 3
Patch Set 5 : Implement binding flow events to slices for PostTasks. #
Total comments: 14
Patch Set 6 : Add support for flow steps. #
Total comments: 1
Patch Set 7 : The patch that only implements new APIs without effecting the rest of Chrome. #Patch Set 8 : The patch that only implements new APIs without effecting the rest of Chrome. #
Total comments: 1
Patch Set 9 : Always mangle id when the flag is set. #
Total comments: 1
Patch Set 10 : Fix compile bug by creating an overload function to add events. #Patch Set 11 : Modify unittests to compile with new APIs. #Patch Set 12 : Mainly incorporates context_id into the new flow event APIs. #
Total comments: 18
Patch Set 13 : NULL to nullptr and better commenting. #Patch Set 14 : Use overloaded function calls. #Patch Set 15 : Manual merge with ToT. #Patch Set 16 : Add a flag for FLOW_OPTIONAL. #
Total comments: 15
Patch Set 17 : Improve format. #
Total comments: 3
Patch Set 18 : Fix nits. #Patch Set 19 : Manual merge #Patch Set 20 : Fix style. #Patch Set 21 : Style revert. #Patch Set 22 : bind_id_ should be unsigned long long instead of unsigned int. #Patch Set 23 : Merge. #Messages
Total messages: 107 (35 generated)
yuhaoz@google.com changed reviewers: + dsinclair@chromium.org, nduca@chromium.org, vmpstr@chromium.org
It's a new API that allows creating an event and its bundled flow event at the same time. Right now I just implemented it for flow event. This patch is just for review purpose and to get everybody on the page in terms of the interface specification.
Remove a redundant macro.
https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:231: #define TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ These should be TRACE_EVENT_WITH_FLOW1 We put the numbers at the end of all the macros. https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ Why no TRACE_EVENT_WITH_FLOW0? https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:1221: #define FLOW_OUT (static_cast<unsigned char>(1)) These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend the unsigned char to a unsigned int. https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.cc File ipc/ipc_channel_reader.cc (right): https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.... ipc/ipc_channel_reader.cc:86: "name", name).APPEND_TRACE_EVENT_FLOW_END0( Why do we have to l.APPEND a flow event? Why isn't it just part of the WITH_FLOW API above? This syntax isn't great it's hard to read and is going to be hard to use. Why not do what was described in the doc and pass the id into WITH_FLOW?
On 2015/07/14 at 13:52:10, dsinclair wrote: > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event.h:231: #define TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > These should be TRACE_EVENT_WITH_FLOW1 > > We put the numbers at the end of all the macros. > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > Why no TRACE_EVENT_WITH_FLOW0? > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event.h:1221: #define FLOW_OUT (static_cast<unsigned char>(1)) > These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend the unsigned char to a unsigned int. > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.cc > File ipc/ipc_channel_reader.cc (right): > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.... > ipc/ipc_channel_reader.cc:86: "name", name).APPEND_TRACE_EVENT_FLOW_END0( > Why do we have to l.APPEND a flow event? Why isn't it just part of the WITH_FLOW API above? > > This syntax isn't great it's hard to read and is going to be hard to use. Why not do what was described in the doc and pass the id into WITH_FLOW? Passing id around sometimes is very hard because TRACE_EVENT and flow event are called from different functions, and passing an id around would require changing interfaces of too many function calls. I am essentially implementing the unified API that vmpstr@ suggested: TRACE_EVENT0_FLOW("cc", "stuff", flow_id, FLOW_IN); See the Implementation Details section of the design doc for more discussions.
On 2015/07/14 at 13:52:10, dsinclair wrote: > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event.h:231: #define TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > These should be TRACE_EVENT_WITH_FLOW1 > > We put the numbers at the end of all the macros. > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > Why no TRACE_EVENT_WITH_FLOW0? TRACE_EVENT2_WITH_FLOW means we are tracing an event with 2 arguments, but this event will have a flow event appended to it. If we want to have one API that combines regular events and flow events, the API needs to be able to tell the number of arguments of both regular events and flow events, something like: TRACE_EVENT2_WITH_FLOW_END1, and then we are mixing the arguments of both regular events and flow events in one MACRO, which might look a little bit weird: TRACE_EVENT2_WITH_FLOW_END0("ipc,toplevel", "ChannelReader::DispatchInputData", "class", IPC_MESSAGE_ID_CLASS(m.type()), "line", IPC_MESSAGE_ID_LINE(m.type()), TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", m.flags()); But maybe this is acceptable?
On 2015/07/14 16:38:37, yuhaoz wrote: > On 2015/07/14 at 13:52:10, dsinclair wrote: > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > File base/trace_event/trace_event.h (right): > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > base/trace_event/trace_event.h:231: #define > TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > > These should be TRACE_EVENT_WITH_FLOW1 > > > > We put the numbers at the end of all the macros. > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > > Why no TRACE_EVENT_WITH_FLOW0? > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > base/trace_event/trace_event.h:1221: #define FLOW_OUT (static_cast<unsigned > char>(1)) > > These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend the > unsigned char to a unsigned int. > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.cc > > File ipc/ipc_channel_reader.cc (right): > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.... > > ipc/ipc_channel_reader.cc:86: "name", name).APPEND_TRACE_EVENT_FLOW_END0( > > Why do we have to l.APPEND a flow event? Why isn't it just part of the > WITH_FLOW API above? > > > > This syntax isn't great it's hard to read and is going to be hard to use. Why > not do what was described in the doc and pass the id into WITH_FLOW? > > Passing id around sometimes is very hard because TRACE_EVENT and flow event are > called from different functions, and passing an id around would require changing > interfaces of too many function calls. I am essentially implementing the unified > API that vmpstr@ suggested: TRACE_EVENT0_FLOW("cc", "stuff", flow_id, FLOW_IN); > See the Implementation Details section of the design doc for more discussions. I don't like the .APPEND syntax being used here, I think it's a lot more confusing then just passing in an id. Can you give examples of where this will be hard to do? This doesn't look like the API vmpstr@ suggested as there is no flow_id or FLOW_IN provided in the macro call.
On 2015/07/14 16:46:28, yuhaoz wrote: > On 2015/07/14 at 13:52:10, dsinclair wrote: > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > File base/trace_event/trace_event.h (right): > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > base/trace_event/trace_event.h:231: #define > TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > > These should be TRACE_EVENT_WITH_FLOW1 > > > > We put the numbers at the end of all the macros. > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > > Why no TRACE_EVENT_WITH_FLOW0? > > TRACE_EVENT2_WITH_FLOW means we are tracing an event with 2 arguments, but this > event will have a flow event appended to it. If we want to have one API that > combines regular events and flow events, the API needs to be able to tell the > number of arguments of both regular events and flow events, something like: > TRACE_EVENT2_WITH_FLOW_END1, and then we are mixing the arguments of both > regular events and flow events in one MACRO, which might look a little bit > weird: > > TRACE_EVENT2_WITH_FLOW_END0("ipc,toplevel", "ChannelReader::DispatchInputData", > "class", IPC_MESSAGE_ID_CLASS(m.type()), > "line", IPC_MESSAGE_ID_LINE(m.type()), > TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", > m.flags()); > > But maybe this is acceptable? Why is flow an event? That doesn't make sense to me. It should just be ID's that are connected in the viewer. Why is there a separate disabled by default in there? TRACE_EVENT2_WITH_FLOW1 is going to be very confusing for people using the API. We shouldn't be passing in two different sets of arguments.
On 2015/07/14 at 17:00:02, dsinclair wrote: > On 2015/07/14 16:38:37, yuhaoz wrote: > > On 2015/07/14 at 13:52:10, dsinclair wrote: > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > File base/trace_event/trace_event.h (right): > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > base/trace_event/trace_event.h:231: #define > > TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > > > These should be TRACE_EVENT_WITH_FLOW1 > > > > > > We put the numbers at the end of all the macros. > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > > > Why no TRACE_EVENT_WITH_FLOW0? > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > base/trace_event/trace_event.h:1221: #define FLOW_OUT (static_cast<unsigned > > char>(1)) > > > These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend the > > unsigned char to a unsigned int. > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.cc > > > File ipc/ipc_channel_reader.cc (right): > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.... > > > ipc/ipc_channel_reader.cc:86: "name", name).APPEND_TRACE_EVENT_FLOW_END0( > > > Why do we have to l.APPEND a flow event? Why isn't it just part of the > > WITH_FLOW API above? > > > > > > This syntax isn't great it's hard to read and is going to be hard to use. Why > > not do what was described in the doc and pass the id into WITH_FLOW? > > > > Passing id around sometimes is very hard because TRACE_EVENT and flow event are > > called from different functions, and passing an id around would require changing > > interfaces of too many function calls. I am essentially implementing the unified > > API that vmpstr@ suggested: TRACE_EVENT0_FLOW("cc", "stuff", flow_id, FLOW_IN); > > See the Implementation Details section of the design doc for more discussions. > > > I don't like the .APPEND syntax being used here, I think it's a lot more confusing then just passing in an id. Can you give examples of where this will be hard to do? > > This doesn't look like the API vmpstr@ suggested as there is no flow_id or FLOW_IN provided in the macro call. Yeah I was going to implement something like: TRACE_EVENT2_WITH_FLOW_END0("ipc,toplevel", "ChannelReader::DispatchInputData", "class", IPC_MESSAGE_ID_CLASS(m.type()), "line", IPC_MESSAGE_ID_LINE(m.type()), TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", m.flags()); But it seems to me that's mixing two sets of arguments. So I went to decouple the two and use the APPEND one. We can totally revert to the TRACE_EVENT2_WITH_FLOW_END0 format.
On 2015/07/14 17:03:10, yuhaoz wrote: > On 2015/07/14 at 17:00:02, dsinclair wrote: > > On 2015/07/14 16:38:37, yuhaoz wrote: > > > On 2015/07/14 at 13:52:10, dsinclair wrote: > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > > File base/trace_event/trace_event.h (right): > > > > > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > > base/trace_event/trace_event.h:231: #define > > > TRACE_EVENT1_WITH_FLOW(category_group, name, arg1_name, arg1_val) \ > > > > These should be TRACE_EVENT_WITH_FLOW1 > > > > > > > > We put the numbers at the end of all the macros. > > > > > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > > base/trace_event/trace_event.h:239: #define TRACE_EVENT2_WITH_FLOW( \ > > > > Why no TRACE_EVENT_WITH_FLOW0? > > > > > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... > > > > base/trace_event/trace_event.h:1221: #define FLOW_OUT > (static_cast<unsigned > > > char>(1)) > > > > These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend > the > > > unsigned char to a unsigned int. > > > > > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.cc > > > > File ipc/ipc_channel_reader.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1239593002/diff/20001/ipc/ipc_channel_reader.... > > > > ipc/ipc_channel_reader.cc:86: "name", name).APPEND_TRACE_EVENT_FLOW_END0( > > > > Why do we have to l.APPEND a flow event? Why isn't it just part of the > > > WITH_FLOW API above? > > > > > > > > This syntax isn't great it's hard to read and is going to be hard to use. > Why > > > not do what was described in the doc and pass the id into WITH_FLOW? > > > > > > Passing id around sometimes is very hard because TRACE_EVENT and flow event > are > > > called from different functions, and passing an id around would require > changing > > > interfaces of too many function calls. I am essentially implementing the > unified > > > API that vmpstr@ suggested: TRACE_EVENT0_FLOW("cc", "stuff", flow_id, > FLOW_IN); > > > See the Implementation Details section of the design doc for more > discussions. > > > > > > I don't like the .APPEND syntax being used here, I think it's a lot more > confusing then just passing in an id. Can you give examples of where this will > be hard to do? > > > > This doesn't look like the API vmpstr@ suggested as there is no flow_id or > FLOW_IN provided in the macro call. > > Yeah I was going to implement something like: > > TRACE_EVENT2_WITH_FLOW_END0("ipc,toplevel", "ChannelReader::DispatchInputData", > "class", IPC_MESSAGE_ID_CLASS(m.type()), > "line", IPC_MESSAGE_ID_LINE(m.type()), > TRACE_DISABLED_BY_DEFAULT("ipc.flow"), "IPC", > m.flags()); > > But it seems to me that's mixing two sets of arguments. So I went to decouple > the two and use the APPEND one. We can totally revert to the > TRACE_EVENT2_WITH_FLOW_END0 format. The flow data should not be a second set of categories and names. It should just attach to the original slice. There should be no 'flow event'. The id's are what bind things together and the IN/OUT flag tell you where it attaches on the slice. Having two sets of categories on a macro is going to cause confusion.
> The flow data should not be a second set of categories and names. It should just attach to the original slice. There should be no 'flow event'. The id's are what bind things together and the IN/OUT flag tell you where it attaches on the slice. > > Having two sets of categories on a macro is going to cause confusion. Then how about when multiple FLOW_STEP share the same ID? In this case, even if we specify the IN/OUT flag, which FLOW_STEP do we bind to?
On 2015/07/14 17:11:02, yuhaoz wrote: > > The flow data should not be a second set of categories and names. It should > just attach to the original slice. There should be no 'flow event'. The id's are > what bind things together and the IN/OUT flag tell you where it attaches on the > slice. > > > > Having two sets of categories on a macro is going to cause confusion. > > Then how about when multiple FLOW_STEP share the same ID? In this case, even if > we specify the IN/OUT flag, which FLOW_STEP do we bind to? I don't understand what you mean. You pass in the flow_id and bind to FLOW_IN | FLOW_OUT. That would, effectively, be a flow step. When you link them up in the timeline, we can link them by timestamp as, the steps have to progress in linear order, no?
On 2015/07/14 at 17:12:24, dsinclair wrote: > On 2015/07/14 17:11:02, yuhaoz wrote: > > > The flow data should not be a second set of categories and names. It should > > just attach to the original slice. There should be no 'flow event'. The id's are > > what bind things together and the IN/OUT flag tell you where it attaches on the > > slice. > > > > > > Having two sets of categories on a macro is going to cause confusion. > > > > Then how about when multiple FLOW_STEP share the same ID? In this case, even if > > we specify the IN/OUT flag, which FLOW_STEP do we bind to? > > > I don't understand what you mean. You pass in the flow_id and bind to FLOW_IN | FLOW_OUT. That would, effectively, be a flow step. When you link them up in the timeline, we can link them by timestamp as, the steps have to progress in linear order, no? Oh I see. Yes we then in trace-viewer have to deal with timestamps, which are floating point numbers and might run into some of the issues we have. I thought the whole point of the new flow event API was to not have to worry about timestamps at all, i.e., we can always definitely tell which slice a flow event should bind to by having Chrome developers do more work and link flow events with regular events.
On 2015/07/14 17:15:40, yuhaoz wrote: > On 2015/07/14 at 17:12:24, dsinclair wrote: > > On 2015/07/14 17:11:02, yuhaoz wrote: > > > > The flow data should not be a second set of categories and names. It > should > > > just attach to the original slice. There should be no 'flow event'. The id's > are > > > what bind things together and the IN/OUT flag tell you where it attaches on > the > > > slice. > > > > > > > > Having two sets of categories on a macro is going to cause confusion. > > > > > > Then how about when multiple FLOW_STEP share the same ID? In this case, even > if > > > we specify the IN/OUT flag, which FLOW_STEP do we bind to? > > > > > > I don't understand what you mean. You pass in the flow_id and bind to FLOW_IN > | FLOW_OUT. That would, effectively, be a flow step. When you link them up in > the timeline, we can link them by timestamp as, the steps have to progress in > linear order, no? > > Oh I see. Yes we then in trace-viewer have to deal with timestamps, which are > floating point numbers and might run into some of the issues we have. I thought > the whole point of the new flow event API was to not have to worry about > timestamps at all, i.e., we can always definitely tell which slice a flow event > should bind to by having Chrome developers do more work and link flow events > with regular events. We do know exactly what slices the developers asked us to bind too. The only thing we're using the timestamps for is the ordering of the slices. We'd have this issue either way as we sort the slices to display them. Everything is already bound.
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:1221: #define FLOW_OUT (static_cast<unsigned char>(1)) On 2015/07/14 13:52:10, dsinclair wrote: > These should be TRACE_EVENT_FLAG_FLOW_IN/OUT. fmeawad@ has a Cl to extend the > unsigned char to a unsigned int. The CL has landed.
This CL creates a separate trace event when we are tracing a flow event. It is currently implemented against IPC, not PostTask yet.
yuhaoz@google.com changed reviewers: + beaudoin@chromium.org
This lgtm, as long as dsinclair and others are good with it. This makes flow events strictly bound to the slice, which is nice. Next step is to make post tasks use this as well, I'm guessing? https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:228: #define TRACE_EVENT_WITH_FLOW0(category_group, name, bind_id, flow_direction) \ nit: I think bind_id should be named flow_id here https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:230: INTERNAL_TRACE_EVENT_ADD_SCOPED_WITH_FLOW(category_group, name, bind_id, flow_direction) nit: run this through clang-format please (git cl format) https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:854: StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"", static_cast<uint64>(bind_id_)); why not just make the type uint64?
On 2015/07/16 at 21:30:11, vmpstr wrote: > This lgtm, as long as dsinclair and others are good with it. > > This makes flow events strictly bound to the slice, which is nice. Next step is to make post tasks use this as well, I'm guessing? > > https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... > base/trace_event/trace_event.h:228: #define TRACE_EVENT_WITH_FLOW0(category_group, name, bind_id, flow_direction) \ > nit: I think bind_id should be named flow_id here I would prefer bind_id because that indicates the functionality of this id, even though it is technically a flow id. It might be possible that in the future we decide to use values other than the flow_id as the bind_id. Also, if we go with the name flow_id, then flow events will have both a flow_id and an id ;)) > > https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/1239593002/diff/60001/base/trace_event/trace_... > base/trace_event/trace_event_impl.cc:854: StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"", static_cast<uint64>(bind_id_)); > why not just make the type uint64? I actually have no idea why we have to do this static cast? bind_id_ is defined as unsigned long long, and uint64 should also be unsigned long long. One more confusion about this line, if we didn't do the static_cast, I got error like: error: format specifies type 'unsigned long' but the argument has type 'unsigned long long' [-Werror,-Wformat] StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"", bind_id_); This seems to indicate that PRIx64 is unsigned long. However, it is a macro for "I64x" (https://code.google.com/p/chromium/codesearch#chromium/usr/include/brlapi.h&q...), which as I understand is an unsigned 64 bit hex number, which is interpreted as unsigned long instead of unsigned long long??
This patch makes PostTask to use the new Flow Event API.
Awesome, thanks a lot for iterating on this. Mostly just nit's for comments. The one big thing is, can you split this into two CLs. One to add the macros and tracing machinery and a second CL that adds the actual FLOW_EVENTs into the code? Will make it easier to review and keeps the bits separated (the first CL should have no code impact outside tracing) and in case the macros need to be reverted for whatever reason makes things easier. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:1076: #define INTERNAL_TRACE_EVENT_ADD(phase, category_group, name, flags, ...) \ Can you undo the formatting that puts the \'s lined up? It makes it harder to see what actually changed. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:1197: #define TRACE_EVENT_FLAG_FLOW_OUT (static_cast<unsigned int>(1 << 9)) nit: Can you line the ('s up on these ones with ASYNC_TTS (static above. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:1228: const unsigned long long kNoBindId = 0; Let's just rename kNoEventId to kNoId = 0 and use it for both of them. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:1486: template <class ARG2_TYPE> nit: remove space. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:1555: unsigned long long bind_id) { Let's move bind_id up below int thread_id so we keep the various id's together. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:599: unsigned long long bind_id) { Move up below id. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:851: if (flags_ & TRACE_EVENT_FLAG_FLOW_OUT) { I think this needs to be a bit more complicated. What happens if I do flag == TRACE_EVENT_FLAG_FLOW_OUT | TRACE_EVENT_FLAG_FLOW_IN; (So, this is a step in the flow). if (flags_ & TRACE_EVENT_FLAG_FLOW_OUT || flags_ & TRACE_EVNET_FLAG_FLOW_IN) StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"", static_cast<uint64>(bind_id_)); if (flags & TRACE_EVENT_FLAG_FLOW_OUT) StringAppendF(out, ","bo:true"); if (flags & TRACE_EVENT_FLAG_FLOW_IN) StringAppendF(out, ","bi:true"); (Not sure of the bi,bo as the attributes, what do you think?) https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:1226: "overhead", 0, 0, NULL, NULL, NULL, NULL, 0, 0); If this 0 is id use the kNoId please. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:2043: unsigned long long bind_id) { Move up below thread_id https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:2529: TRACE_EVENT_FLAG_NONE, 0); Use kNoId. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event_impl.h (right): https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.h:125: unsigned long long bind_id); Move up below id. https://codereview.chromium.org/1239593002/diff/80001/base/trace_event/trace_... base/trace_event/trace_event_impl.h:472: unsigned long long bind_id); Move up below thread_id https://codereview.chromium.org/1239593002/diff/80001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/80001/content/child/blink_pla... content/child/blink_platform_impl.cc:643: arg_types, arg_values, NULL, flags, 0); kNoId https://codereview.chromium.org/1239593002/diff/80001/content/child/blink_pla... content/child/blink_platform_impl.cc:679: arg_types, arg_values, convertable_wrappers, flags, 0); kNoId
This CL mainly adds support for flow steps, and incorporates Dan's comments. After everything looks good on this, I will split it into two CLs and commit separately.
On 2015/07/17 22:03:10, yuhaoz wrote: > This CL mainly adds support for flow steps, and incorporates Dan's comments. > After everything looks good on this, I will split it into two CLs and commit > separately. Just voicing my approval for this CL. LMK as soon as it's split and I'll do the two reviews thoroughly.
Just one comment on the CL. Seems good so far. Can you split it into few CLs. It looks like you added a bunch of new trace event calls into here, those should go into a number of CLs which make logical sense for them. Each grouping of events should be it's own cl. If we need to revert, it's nicer to just revert one flow chain instead of all of them. https://codereview.chromium.org/1239593002/diff/100001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/100001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2059: else Do you want the else here? If we have an id we should always mangle it.
First step of splitting into multiple patches. This patch only implements new APIs without effecting the rest of Chrome.
https://codereview.chromium.org/1239593002/diff/140001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/140001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2060: id = MangleEventId(id); I think id needs to always be manged if MANGLE_ID is set.
Always mangle id when the flag is set.Forgot to fix this in the previous patch...
lgtm
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps160001 (title: "Always mangle id when the flag is set.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:856: StringAppendF(out, ",\"flow_in\":\"true\""); Can this just be ",\"flow_in\":true"); and false below so we send bools instead of strings?
On 2015/07/20 18:37:32, dsinclair wrote: > https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:856: StringAppendF(out, > ",\"flow_in\":\"true\""); > Can this just be ",\"flow_in\":true"); and false below so we send bools instead > of strings? You also need to update ppapi/shared_impl/ppb_trace_event_impl.cc ui/gl/angle_platform_impl.cc for the CL to compile (angle only fails on windows)
The CQ bit was checked by yuhaoz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yuhaoz@google.com to run a CQ dry run
The CQ bit was unchecked by yuhaoz@google.com
The CQ bit was checked by yuhaoz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/07/20 20:59:58, fmeawad wrote: > On 2015/07/20 18:37:32, dsinclair wrote: > > > https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... > > File base/trace_event/trace_event_impl.cc (right): > > > > > https://codereview.chromium.org/1239593002/diff/160001/base/trace_event/trace... > > base/trace_event/trace_event_impl.cc:856: StringAppendF(out, > > ",\"flow_in\":\"true\""); > > Can this just be ",\"flow_in\":true"); and false below so we send bools > instead > > of strings? > > You also need to update > ppapi/shared_impl/ppb_trace_event_impl.cc > ui/gl/angle_platform_impl.cc > > for the CL to compile (angle only fails on windows) I think it's better to put the original API back and overload it then call the new one from the old one. Then we can update PPAPI and Angle in two CLs and when there are no callers remove the old API.
This patch incorporates the new context_id into the new flow event APIs. I also implemented the overloaded function to handle legacy calls to AddTraceEventWithThreadIdAndTimestamp, as Dan suggested.
From the API perspective, I think this looks fine. Let's document everything in the doc so that it's clear what the intent is here and also where we're headed. Also, just a bunch of minor nits below. https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:417: trace_event_internal::kNoId, nit: Can you put some inline comments with what the var is, something along the lines of: ..., trace_event_internal::kNoId, // id trace_event_internal::kNoId, // context_id trace_event_internal::kNoId, // bind_id Same comment for the nulls below https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:867: StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"", nit: can you put braces around the if (usually we do that for multi-line statement, or maybe it's just me) https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:1254: NULL, nit: For the functions that you're updating, can you also change NULL to nullptr, while in the area? https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... File ppapi/shared_impl/ppb_trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... ppapi/shared_impl/ppb_trace_event_impl.cc:86: // This cast is necessary for LP64 systems, where uint64_t is defined as You'll probably need to rewrap the comment to be 80-col https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... File ui/gl/angle_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... ui/gl/angle_platform_impl.cc:57: NULL, nit: keep nullptr, please.
https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2250: bind_id, Why did you unwrap this call? https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2423: trace_event_internal::kNoId, ditto https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2655: trace_event_internal::kNoId, ditto https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:1457: "arg1", "argval", "arg2", "argval"); this would probably fit on the line above https://codereview.chromium.org/1239593002/diff/220001/content/child/blink_pl... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/content/child/blink_pl... content/child/blink_platform_impl.cc:647: num_args, Why unwrapped? https://codereview.chromium.org/1239593002/diff/220001/content/child/blink_pl... content/child/blink_platform_impl.cc:693: arg_names, ditto https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... File ppapi/shared_impl/ppb_trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... ppapi/shared_impl/ppb_trace_event_impl.cc:81: thread_id, This file shouldn't need to change yet right? We added the overloads for this reason. https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... File ui/gl/angle_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... ui/gl/angle_platform_impl.cc:50: id, This shouldn't need to change because we added the overloads, right?
Incorporating comments. https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2250: bind_id, On 2015/07/21 at 18:11:49, dsinclair wrote: > Why did you unwrap this call? Simply because it looks clearer, especially with many arguments. There was a bug (and subsequent CL) related to the order of these arguments. I was just trying to make it clearer. Do you prefer to blend them into fewer lines? https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... File ppapi/shared_impl/ppb_trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... ppapi/shared_impl/ppb_trace_event_impl.cc:81: thread_id, On 2015/07/21 at 18:11:49, dsinclair wrote: > This file shouldn't need to change yet right? We added the overloads for this reason. Right that's why I removed the kNoId. fmeawad@ for context_id. I removed it so that it uses the overloaded version. https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... File ui/gl/angle_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ui/gl/angle_platform_i... ui/gl/angle_platform_impl.cc:50: id, On 2015/07/21 at 18:11:49, dsinclair wrote: > This shouldn't need to change because we added the overloads, right? Same as the ppapi case. I removed the kNoId that fmeawad@ added for context_id.
https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2250: bind_id, On 2015/07/21 18:22:34, yuhaoz wrote: > On 2015/07/21 at 18:11:49, dsinclair wrote: > > Why did you unwrap this call? > > Simply because it looks clearer, especially with many arguments. There was a bug > (and subsequent CL) related to the order of these arguments. I was just trying > to make it clearer. Do you prefer to blend them into fewer lines? I prefer the old way, personally. It also makes it a lot easier to change since it will highlight just the difference. https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... File ppapi/shared_impl/ppb_trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/220001/ppapi/shared_impl/ppb_... ppapi/shared_impl/ppb_trace_event_impl.cc:81: thread_id, On 2015/07/21 18:22:34, yuhaoz wrote: > On 2015/07/21 at 18:11:49, dsinclair wrote: > > This file shouldn't need to change yet right? We added the overloads for this > reason. > > Right that's why I removed the kNoId. fmeawad@ for context_id. I removed it so > that it uses the overloaded version. > Just add the context_id into the overloaded version. We do want to move off the old API onto the new one, I just think they should be separate CLs. So, this CL shouldn't change Angle or PPAPI and we update each of them later then remove the old API.
Use overloaded functions when possible. Switching to using new APIs should go into a separate CL. Also I advocate for unwrapping the arguments of calling Initialize and AddTraceEventWithThreadIdAndTimestamp just because there are too many arguments, and their order could be confusing. Also that allows us to comment at each line about what each argument is about. This is helpful especially when there are multiple kNoIds in a function call, and there could be different things. I would argue that if we do this in this CL, then in future CLs, the difference of changes will be clearer also since each argument is one line. Comments from others about this?
The CQ bit was checked by yuhaoz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yuhaoz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
So with the discussion this morning, does this patch look fine?
PTAL. Add a flag for FLOW_OPTIONAL.
LGTM with nits. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:231: flow_direction) s/flow_direction/flow_flags ? Since it includes priority. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:1123: category_group, name, bind_id, flow_direction, ...) \ s/flow_direction/flow_flags ? Since it includes priority. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:1218: #define TRACE_EVENT_FLAG_FLOW_OPTIONAL (static_cast<unsigned int>(1 << 12)) Why not group the _FLOW_ flags together? Why change _HAS_CONTECT_ID from 10 to 11? (Why was it 10 in the first place?) https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:1250: trace_event_internal::kNoId, // bind_id NIT: 2 spaces before EOL comment. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2060: trace_event_internal::kNoId, // bind_id NIT: 2 spaces before EOL comment. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2091: trace_event_internal::kNoId, // bind_id NIT: 2 spaces before EOL comment. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2124: trace_event_internal::kNoId, // bind_id NIT: 2 spaces before EOL comment. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2426: trace_event_internal::kNoId, EOL comments? https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2657: trace_event_internal::kNoId, // context_id NIT: 2 spaces before EOL comment. https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... content/child/blink_platform_impl.cc:643: base::PlatformThread::CurrentId(), EOL comments? https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... content/child/blink_platform_impl.cc:689: base::PlatformThread::CurrentId(), EOL comments? https://codereview.chromium.org/1239593002/diff/300001/ui/gl/angle_platform_i... File ui/gl/angle_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/300001/ui/gl/angle_platform_i... ui/gl/angle_platform_impl.cc:52: base::PlatformThread::CurrentId(), EOL comments?
On 2015/07/27 18:34:14, beaudoin wrote: > LGTM with nits. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event.h:231: flow_direction) > s/flow_direction/flow_flags ? Since it includes priority. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event.h:1123: category_group, name, bind_id, > flow_direction, ...) \ > s/flow_direction/flow_flags ? Since it includes priority. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event.h:1218: #define TRACE_EVENT_FLAG_FLOW_OPTIONAL > (static_cast<unsigned int>(1 << 12)) > Why not group the _FLOW_ flags together? Why change _HAS_CONTECT_ID from 10 to > 11? (Why was it 10 in the first place?) > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:1250: trace_event_internal::kNoId, // > bind_id > NIT: 2 spaces before EOL comment. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2060: trace_event_internal::kNoId, // > bind_id > NIT: 2 spaces before EOL comment. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2091: trace_event_internal::kNoId, // > bind_id > NIT: 2 spaces before EOL comment. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2124: trace_event_internal::kNoId, // > bind_id > NIT: 2 spaces before EOL comment. > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2426: trace_event_internal::kNoId, > EOL comments? > > https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:2657: trace_event_internal::kNoId, // > context_id > NIT: 2 spaces before EOL comment. > > https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... > File content/child/blink_platform_impl.cc (right): > > https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... > content/child/blink_platform_impl.cc:643: base::PlatformThread::CurrentId(), > EOL comments? > > https://codereview.chromium.org/1239593002/diff/300001/content/child/blink_pl... > content/child/blink_platform_impl.cc:689: base::PlatformThread::CurrentId(), > EOL comments? > > https://codereview.chromium.org/1239593002/diff/300001/ui/gl/angle_platform_i... > File ui/gl/angle_platform_impl.cc (right): > > https://codereview.chromium.org/1239593002/diff/300001/ui/gl/angle_platform_i... > ui/gl/angle_platform_impl.cc:52: base::PlatformThread::CurrentId(), > EOL comments? Hmmm... I'm rescinding my LGTM based on the hangout discussion. I believe we should kill IN|OUT flows.
On 2015/07/27 at 19:02:55, beaudoin wrote: > Hmmm... I'm rescinding my LGTM based on the hangout discussion. I believe we should kill IN|OUT flows. This CL doesn't insert IN|OUT to the code right? It's just the APIs.
Fixed nits. This whole CL defines and implements the new TRACE_EVENT_WITH_FLOW API. As per our discussion, we will kill flow steps. But that doesn't effect this CL.
LGTM with some more nits. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:1218: #define TRACE_EVENT_FLAG_FLOW_OPTIONAL (static_cast<unsigned int>(1 << 12)) On 2015/07/27 18:34:13, beaudoin wrote: > Why not group the _FLOW_ flags together? Why change _HAS_CONTECT_ID from 10 to > 11? (Why was it 10 in the first place?) Same question here: why change to 12? https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2426: trace_event_internal::kNoId, On 2015/07/27 18:34:13, beaudoin wrote: > EOL comments? Not done? https://codereview.chromium.org/1239593002/diff/320001/content/child/blink_pl... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/320001/content/child/blink_pl... content/child/blink_platform_impl.cc:643: base::PlatformThread::CurrentId(), Should you have one for CurrentId()? It is it's unclead what it's for from the variable name. https://codereview.chromium.org/1239593002/diff/320001/content/child/blink_pl... content/child/blink_platform_impl.cc:689: base::PlatformThread::CurrentId(), Ditto. https://codereview.chromium.org/1239593002/diff/320001/ui/gl/angle_platform_i... File ui/gl/angle_platform_impl.cc (right): https://codereview.chromium.org/1239593002/diff/320001/ui/gl/angle_platform_i... ui/gl/angle_platform_impl.cc:52: base::PlatformThread::CurrentId(), Ditto.
Fix nits. https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/1239593002/diff/300001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:2426: trace_event_internal::kNoId, On 2015/07/29 at 18:55:43, beaudoin wrote: > On 2015/07/27 18:34:13, beaudoin wrote: > > EOL comments? > > Not done? Done. Following the format of the code at line 2294.
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, vmpstr@chromium.org, beaudoin@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps340001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...) android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, dsinclair@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps360001 (title: "Manual merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, dsinclair@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps380001 (title: "Fix style.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, dsinclair@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps400001 (title: "Style revert.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/21c55cb65ef294af3e5479c34473786281168aef Cr-Commit-Position: refs/heads/master@{#341023}
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, dsinclair@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps420001 (title: "bind_id_ should be unsigned long long instead of unsigned int.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yuhaoz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, dsinclair@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1239593002/#ps440001 (title: "Merge.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yuhaoz@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239593002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1239593002/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/565896aeb84ba2188a725b4ffc13bbbdb8d1ceea Cr-Commit-Position: refs/heads/master@{#341154}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Drive by: Patch set 23 is rather confusing. It's a 1 line change. It looks like the CL was used twice. Once to land patch set 22 and again for patch set 23. Patch set 23 committed as https://chromium.googlesource.com/chromium/src/+/565896aeb84ba2188a725b4ffc13... but the commit message is out of context. Please don't do this. Use a new CL for new changes. |
