Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253973003:
Add an explicit way of making IDs local or global (Closed)
|
Created:
4 years, 4 months ago by chiniforooshan Modified:
4 years, 3 months ago CC:
chromium-reviews, petrcermak, 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. |
DescriptionAdd an explicit way of making IDs local or global
Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If
these are used in trace events, a boolean "id_is_global" field
will be added to the produced JSON record.
We add the above-mentioned macros only in base. We will add them
to other subsystems (V8, skia, WebKit, ...) separately, when
needed.
This patch also removes the DONT_MANGLE macros inside object and
context events; object and context events are process-local by
default and mangling or not mangling them should not change
anything.
Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s/edit#heading=h.5lebrnsh6g7g
BUG=catapult:#2465
Committed: https://crrev.com/38e3c2977c913973fc7237089560ce7fc0822bc4
Cr-Commit-Position: refs/heads/master@{#419777}
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments #
Total comments: 8
Patch Set 3 : comments #Patch Set 4 : comments #Patch Set 5 : comments #
Total comments: 6
Patch Set 6 : comments #Patch Set 7 : comments #Patch Set 8 : lints #
Total comments: 8
Patch Set 9 : comments #Patch Set 10 : versioning the id field #
Messages
Total messages: 45 (15 generated)
Description was changed from ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=N/A ========== to ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=N/A ==========
chiniforooshan@chromium.org changed reviewers: + caseq@chromium.org, skyostil@chromium.org
As per offline discussions and comments in https://codereview.chromium.org/2162183002, this patch introduces macros for creating local/global IDs. Subsequent patches will deal with binding IDs and making composite IDs. PTAL :) Thanks!
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1085: #define TRACE_EVENT_FLAG_MANGLE_ID (static_cast<unsigned int>(1 << 2)) Could you add a TODO to remove this? https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ Do we need to add both of these? My naive hope was that we could change the macros so that we always know whether the ID is local or global. Also, I think it would be better to call these ID_IS_GLOBAL etc. so we still only refer to one bit.
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1085: #define TRACE_EVENT_FLAG_MANGLE_ID (static_cast<unsigned int>(1 << 2)) On 2016/08/18 17:37:32, Sami wrote: > Could you add a TODO to remove this? Done. https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ On 2016/08/18 17:37:32, Sami wrote: > Do we need to add both of these? My naive hope was that we could change the > macros so that we always know whether the ID is local or global. > > Also, I think it would be better to call these ID_IS_GLOBAL etc. so we still > only refer to one bit. I tried to explain this in the linked doc, too. I agree it may be a little bit confusing. But, at this transitioning stage, we basically have 4 cases: 1- The event does not have an ID. 2- The event has an ID but its process-locality is determined from the event type (i.e. the default behaviour, before this patch). 3- The event has an ID and the trace processor should treat it as a process-local ID. 4- The event has an ID and the trace processor should treat it as a global ID. We cannot always know process-locality unless we change all instrumentation points with IDs in this CL, which would be a huge change I think.
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ Could we change the C++ code which writes out the trace events to patch in the correct locality bit based on the event type? That way all future traces will have deterministic locality information and we don't have to bake in these 4 cases into the format.
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ On 2016/08/19 16:47:19, Sami wrote: > Could we change the C++ code which writes out the trace events to patch in the > correct locality bit based on the event type? That way all future traces will > have deterministic locality information and we don't have to bake in these 4 > cases into the format. We could... but, where? We cannot do it too late (e.g. in AppendAsJSON) because how else, other than using flags, we can pass the 4 cases there? So we have to do this at the call sight. So, we should either pass the phase to the TraceID constructor, wherever TraceID is used, or process the phase wherever TraceID is used and set the process-locality flag accordingly. I think that these alternatives are ugly and don't worth doing just to save a few defines, but that's just my opinion. What do you think? Do you have a different implementation in mind?
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ On 2016/08/19 17:35:18, chiniforooshan wrote: > On 2016/08/19 16:47:19, Sami wrote: > > Could we change the C++ code which writes out the trace events to patch in the > > correct locality bit based on the event type? That way all future traces will > > have deterministic locality information and we don't have to bake in these 4 > > cases into the format. > > We could... but, where? We cannot do it too late (e.g. in AppendAsJSON) because > how else, other than using flags, we can pass the 4 cases there? So we have to > do this at the call sight. So, we should either pass the phase to the TraceID > constructor, wherever TraceID is used, or process the phase wherever TraceID is > used and set the process-locality flag accordingly. > > I think that these alternatives are ugly and don't worth doing just to save a > few defines, but that's just my opinion. What do you think? Do you have a > different implementation in mind? I'm not worried about defines but more about what ends up into the trace event format. I was basically wondering if we could get away with only having two cases instead of four: - We add a ID_IS_GLOBAL flag which can be passed in by the macro user. - When writing out events, ID_IS_GLOBAL is added automatically for flow events and async events. That way the importer can look for the global flag in new traces and keep the per-event type heuristics in place for old traces (which are needed in any case). The downside is that you can't use local flow/async events. Maybe we actually do need to do that, in which I think I agree that we need both bits. WDYT? (Also feel free to tell me I'm bikeshedding this too much :)
https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:81: #define INTERNAL_SET_ID_FLAGS(flags, id_bits) \ INTERNAL_SET_ID_BITS() perhaps? https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:82: flags &= TRACE_EVENT_FLAG_ALL - TRACE_EVENT_FLAG_ID_MASK; \ flags &= ~TRACE_EVENT_FLAG_ID_MASK https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:581: TraceID(LocalId raw_id, unsigned int* flags) : raw_id_(raw_id.raw_id()) { I wonder if it's going to be cleaner to have something like template<typename IdType> unsigned flagsForId() { return TRACE_EVENT_FLAG_HAS_ID; } that would be explicitly specialized for LocalId / GlobalId / Mangle / DontMangle to return proper flag (and would let us move flag modification outside of TraceId constructor, i.e. stop passing &flags here)? https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:367: *out += ",\"id_is_global\":false"; rather than emitting it as a bool flag, can we vary the field name instead? I.e. only emit "id" if the id is global and call it "localId" otherwise for async events?
https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:81: #define INTERNAL_SET_ID_FLAGS(flags, id_bits) \ On 2016/08/22 19:48:51, caseq wrote: > INTERNAL_SET_ID_BITS() perhaps? Done. https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:82: flags &= TRACE_EVENT_FLAG_ALL - TRACE_EVENT_FLAG_ID_MASK; \ On 2016/08/22 19:48:51, caseq wrote: > flags &= ~TRACE_EVENT_FLAG_ID_MASK Done. https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:581: TraceID(LocalId raw_id, unsigned int* flags) : raw_id_(raw_id.raw_id()) { On 2016/08/22 19:48:52, caseq wrote: > I wonder if it's going to be cleaner to have something like > template<typename IdType> unsigned flagsForId() { return > TRACE_EVENT_FLAG_HAS_ID; } > that would be explicitly specialized for LocalId / GlobalId / Mangle / > DontMangle to return proper flag (and would let us move flag modification > outside of TraceId constructor, i.e. stop passing &flags here)? Done (kind of... does the implementation in patch #5 look good? To be able to compose the classes together, I need to add a flags field.) https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:367: *out += ",\"id_is_global\":false"; On 2016/08/22 19:48:52, caseq wrote: > rather than emitting it as a bool flag, can we vary the field name instead? I.e. > only emit "id" if the id is global and call it "localId" otherwise for async > events? Is there anything wrong with using a boolean here? Without the boolean, wouldn't the "id" field be ambiguous for importers? It can mean "global ID" if the trace is new or have a type-related scope if the trace is old (and there is no way of finding out which one it is).
https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/1/base/trace_event/common/tra... base/trace_event/common/trace_event_common.h:1096: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ On 2016/08/22 14:46:41, Sami wrote: > On 2016/08/19 17:35:18, chiniforooshan wrote: > > On 2016/08/19 16:47:19, Sami wrote: > > > Could we change the C++ code which writes out the trace events to patch in > the > > > correct locality bit based on the event type? That way all future traces > will > > > have deterministic locality information and we don't have to bake in these 4 > > > cases into the format. > > > > We could... but, where? We cannot do it too late (e.g. in AppendAsJSON) > because > > how else, other than using flags, we can pass the 4 cases there? So we have to > > do this at the call sight. So, we should either pass the phase to the TraceID > > constructor, wherever TraceID is used, or process the phase wherever TraceID > is > > used and set the process-locality flag accordingly. > > > > I think that these alternatives are ugly and don't worth doing just to save a > > few defines, but that's just my opinion. What do you think? Do you have a > > different implementation in mind? > > I'm not worried about defines but more about what ends up into the trace event > format. I was basically wondering if we could get away with only having two > cases instead of four: > > - We add a ID_IS_GLOBAL flag which can be passed in by the macro user. > - When writing out events, ID_IS_GLOBAL is added automatically for flow events > and async events. > > That way the importer can look for the global flag in new traces and keep the > per-event type heuristics in place for old traces (which are needed in any > case). > > The downside is that you can't use local flow/async events. Maybe we actually do > need to do that, in which I think I agree that we need both bits. WDYT? > > (Also feel free to tell me I'm bikeshedding this too much :) Yeah, that way the ID of flow/async events won't be configurable. Right now most async IDs are practically per process except the ones related to input latency. Ideally, we want to use local IDs for everything and don't worry about ID conflicting and only use globals when needed, for example for network and input latency.
On 2016/08/23 19:59:08, chiniforooshan wrote: > Yeah, that way the ID of flow/async events won't be configurable. Right now most > async IDs are practically per process except the ones related to input latency. > Ideally, we want to use local IDs for everything and don't worry about ID > conflicting and only use globals when needed, for example for network and input > latency. Okay, in that case it sounds like we need full flexibility. Re: adding a new id property vs. using flags, I don't mind either way but please make sure you don't ruin Primiano's day with the protobuffer-based rewrite.
On 2016/08/24 13:56:05, Sami wrote: > On 2016/08/23 19:59:08, chiniforooshan wrote: > > Yeah, that way the ID of flow/async events won't be configurable. Right now > most > > async IDs are practically per process except the ones related to input > latency. > > Ideally, we want to use local IDs for everything and don't worry about ID > > conflicting and only use globals when needed, for example for network and > input > > latency. > > Okay, in that case it sounds like we need full flexibility. > > Re: adding a new id property vs. using flags, I don't mind either way but please > make sure you don't ruin Primiano's day with the protobuffer-based rewrite. Thanks Sami for reviewing! caseq@, ping :)
On 2016/08/23 19:54:53, chiniforooshan wrote:
> Is there anything wrong with using a boolean here? Without the boolean,
wouldn't
> the "id" field be ambiguous for importers? It can mean "global ID" if the
trace
> is new or have a type-related scope if the trace is old (and there is no way
of
> finding out which one it is).
Well, I can live with a bool, I don't want to block you on that. My idea was
that we keep the old name only when we keep semantics. So yes, if the old parser
reads a new trace, if it sees an as event with { "id": 42, "is_local_id": "true"
} it's going to quietly disregard the bool flag and then perhaps subtly fail
because the id semantics is different from what it assume (i.e. global scope).
If, on the other hand, there's no "id" field but just "localId", it will break
early and fairly explicitly.
So lgtm % a couple of nits.
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/blame_...
File base/trace_event/blame_context.cc (right):
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/blame_...
base/trace_event/blame_context.cc:39: TRACE_EVENT_FLAG_HAS_LOCAL_ID);
Oh, so you're also fixing things! :-)
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/common...
File base/trace_event/common/trace_event_common.h (right):
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/common...
base/trace_event/common/trace_event_common.h:1100: #define
TRACE_EVENT_FLAG_ID_MASK (static_cast<unsigned int>(1 << 1 | 1 << 12))
please define in terms of other constants, i.e. something like
#define TRACE_EVENT_FLAG_ID_MASK (TRACE_EVENT_FLAG_HAS_LOCAL_ID |
TRACE_EVENT_FLAG_HAS_GLOBAL_ID)
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_event.h (right):
https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_event.h:565: void update_flags(unsigned int* flags) {
*flags |= flags_; }
perhaps just a getter for flags and then something like
unsigned int trace_event_flags = flags | trace_event_trace_id.flags()?
on the call site.
chiniforooshan@chromium.org changed reviewers: + primiano@chromium.org
+primiano for owners approval. Thanks Andrey. Now, I write the new IDs under "local_id" and "global_id" fields and don't use the boolean flag when writing the events, as you suggested. If the ID locality is not specified at the call sight, the ID is written under "id" as before. https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/blame_... File base/trace_event/blame_context.cc (right): https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/blame_... base/trace_event/blame_context.cc:39: TRACE_EVENT_FLAG_HAS_LOCAL_ID); On 2016/08/29 23:39:26, caseq wrote: > Oh, so you're also fixing things! :-) Reverted these "fixes" since, as you suggested, this will cause the id be logged as "local_id" and will break trace_viewer until it understands local IDs. https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/common... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/common... base/trace_event/common/trace_event_common.h:1100: #define TRACE_EVENT_FLAG_ID_MASK (static_cast<unsigned int>(1 << 1 | 1 << 12)) On 2016/08/29 23:39:26, caseq wrote: > please define in terms of other constants, i.e. something like > #define TRACE_EVENT_FLAG_ID_MASK (TRACE_EVENT_FLAG_HAS_LOCAL_ID | > TRACE_EVENT_FLAG_HAS_GLOBAL_ID) Done. https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2253973003/diff/80001/base/trace_event/trace_... base/trace_event/trace_event.h:565: void update_flags(unsigned int* flags) { *flags |= flags_; } On 2016/08/29 23:39:26, caseq wrote: > perhaps just a getter for flags and then something like > > unsigned int trace_event_flags = flags | trace_event_trace_id.flags()? > > on the call site. Done. Changed the method name to id_flags so that the compiler is not confused when replacing macro parameters.
Damn, apparently I haven't been on vacation long enough :P Joking aside, conceptually this looks good to me, jut have two comments, which should be pretty minor at this point (see inline comments). I don't have bandwidth to look at the TraceId::LocalId / GlobalID classes being introduced. At a quick look they LG, just would appreciate if anybody can cover the implementation there, from what I remember from my last review to WithScope they are quite subtle. Thanks for whoever drove this in these two weeks. The current state feels more sensible to me (or probably is just Stockholm syndrome). As a final thing, would be nice if "somewhere" we had a recap of how this new BIND macro plays with bind_scope and bind_id. At this point my only reaction when I see an event in the JSON which matches .*bind.*, my reaction is "I surrender, you won" :) https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:1085: // TODO(crbug.com/639003): Free this bit after ID mangling is deprecated. Nononono that would break backwards compat if somebody frees this bit and somebody else reuses it. At most this should be just renamed to TRACE_EVENT_FLAG_MANGLE_ID_DEPRECATED. But probably such rename would have disastrous consequences on other projects depending on this file, so I'll just say here: // Deprecated, use HAS_{LOCAL,GLOBAL}_ID below. https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:1097: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ had a chat offline, saving this one bit is making the sources really hard to read. Please just use another bit, the code below trace_event_impl.cc where *almost* every _FLAG_ (but not GLOBAL_ID) is a bit tripped my mind :) I deal daily with multi-MB regressions, I can guarantee we can still afford an extra bit in chrome, nobody will notice :P https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; instead of changing the field name, which feels quite magical, can we just make it so that: the old-style id becomes just id="..." LOCAL_ID turns into id="local:..." GLOBAL_ID turns into id="global:..." so we can use the same pattern consistently for bind_id, without having to create bind_global_id, bind_local_id etc? I guess that would make the logic in the importer more reusable (I can imagine some helper LookupObjectById) The TraceEventFormat[1] doc doesn't seem to mandate that id have to be 0x1234 hex parsable, and they are strings so ... :) [1] https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...
https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:1085: // TODO(crbug.com/639003): Free this bit after ID mangling is deprecated. On 2016/08/31 19:11:22, Primiano Tucci wrote: > Nononono that would break backwards compat if somebody frees this bit and > somebody else reuses it. > At most this should be just renamed to TRACE_EVENT_FLAG_MANGLE_ID_DEPRECATED. > But probably such rename would have disastrous consequences on other projects > depending on this file, so I'll just say here: > // Deprecated, use HAS_{LOCAL,GLOBAL}_ID below. What backward compat are we talking about here? This is not exposed in the trace output and is purely an implementation detail of the tracing subsystem. I reckon we have events from couple of other components, but those should be easy to update.
https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:1097: #define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \ On 2016/08/31 19:11:22, Primiano Tucci wrote: > had a chat offline, saving this one bit is making the sources really hard to > read. > Please just use another bit, the code below trace_event_impl.cc where *almost* > every _FLAG_ (but not GLOBAL_ID) is a bit tripped my mind :) > I deal daily with multi-MB regressions, I can guarantee we can still afford an > extra bit in chrome, nobody will notice :P Done. https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; On 2016/08/31 19:11:22, Primiano Tucci wrote: > instead of changing the field name, which feels quite magical, can we just make > it so that: > > the old-style id becomes just id="..." > LOCAL_ID turns into id="local:..." > GLOBAL_ID turns into id="global:..." > > so we can use the same pattern consistently for bind_id, without having to > create bind_global_id, bind_local_id etc? I guess that would make the logic in > the importer more reusable (I can imagine some helper LookupObjectById) > > The TraceEventFormat[1] doc doesn't seem to mandate that id have to be 0x1234 > hex parsable, and they are strings so ... :) > > [1] > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... Done. Note that this was originally due to Andrey's suggestion that if we use a different field here for the new IDs, then old trace consumers will break explicitly when they read an event with a new ID as opposed to reading the ID field as usual but handling it incorrectly. But, since we are going to change the two most important consumers (trace viewer and devtools) soon, and probably before we actually use these macros, I'm in favour of the cleaner choice :) (i.e. "id": "local:0x1234").
> As a final thing, would be nice if "somewhere" we had a recap of how this new > BIND macro plays with bind_scope and bind_id. At this point my only reaction > when I see an event in the JSON which matches .*bind.*, my reaction is "I > surrender, you won" :) This change doesn't deal with non of those stuff :) It only introduces TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros because reviewers thought (and I agree) that it's a good idea to factor this part out of the binding macros for ease of reviewing. I'll make sure to have a recap of the meaning of BIND macros and their (lack of) relation to flow events (that use bind_id) in the description of the next coming patch that is actually about binding macros. For now, pretend TRACE_EVENT_BIND_IDS does not exist, since it's going to be changed significantly in the next patch.
On 2016/08/31 19:11:22, Primiano Tucci wrote: > https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; > instead of changing the field name, which feels quite magical, can we just make > it so that: > > the old-style id becomes just id="..." > LOCAL_ID turns into id="local:..." > GLOBAL_ID turns into id="global:..." > > so we can use the same pattern consistently for bind_id, without having to > create bind_global_id, bind_local_id etc? I guess that would make the logic in > the importer more reusable (I can imagine some helper LookupObjectById) > > The TraceEventFormat[1] doc doesn't seem to mandate that id have to be 0x1234 > hex parsable, and they are strings so ... :) > > [1] > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... How about cases where we refer to an id in another field (id_ref here's an example where this happens in a more-or-less regular fashion: https://cs.chromium.org/chromium/infra/appengine/third_party/catapult/tracing..., but my guess it's not the only case)? So having "local/global" prefixes will still break things, and it will break them subtly. As I mentioned in #14, the idea of changing the name is to avoid quietly changing the semantics of an existent field. However, if the consensus is that we need to keep the old name, I think Ehsan's original way of adding a bool flag is a better solution than adding a prefix.
On 2016/09/01 05:51:49, caseq wrote: > How about cases where we refer to an id in another field (id_ref here's an > example where this happens in a more-or-less regular fashion: > https://cs.chromium.org/chromium/infra/appengine/third_party/catapult/tracing..., > but my guess it's not the only case)? So having "local/global" prefixes will > still break things, and it will break them subtly. As I mentioned in #14, the > idea of changing the name is to avoid quietly changing the semantics of an > existent field. However, if the consensus is that we need to keep the old name, > I think Ehsan's original way of adding a bool flag is a better solution than > adding a prefix. I was hoping that this would make easier to deal precisely with cases like this. I don't know too much the TraceViewer code but I was imagining that there we could have *one* id lookup function that understands local/global prefixes, instead of having to spread the logic of "look for the old field, if not look the new ones" in the various places. Plz talk with somebody who knows TV better than me (virtually anybody, +petrcermak/nduca might be good candidates) to figure out the best approach. RS-LGTM here regardless on how you encode the local/global bit (prefix, field name, boolean, a pony), as long as: - the code doesn't diverge too much from this (% the encoding strategy mentioned above) - somebody who maintains TV is happy about encoding strategy chosen. - any other reviewer does has a final pass on the final code.
chiniforooshan@chromium.org changed reviewers: + benjhayden@chromium.org, nduca@chromium.org
+Ben +Nat for trace_event_impl.cc as per Primiano's suggestion.
On 2016/09/01 13:57:49, Primiano Tucci wrote: > On 2016/09/01 05:51:49, caseq wrote: > > > How about cases where we refer to an id in another field (id_ref here's an > > example where this happens in a more-or-less regular fashion: > > > https://cs.chromium.org/chromium/infra/appengine/third_party/catapult/tracing..., > > but my guess it's not the only case)? So having "local/global" prefixes will > > still break things, and it will break them subtly. As I mentioned in #14, the > > idea of changing the name is to avoid quietly changing the semantics of an > > existent field. However, if the consensus is that we need to keep the old > name, > > I think Ehsan's original way of adding a bool flag is a better solution than > > adding a prefix. > > I was hoping that this would make easier to deal precisely with cases like this. > I don't know too much the TraceViewer code but I was imagining that there we > could have *one* id lookup function that understands local/global prefixes, > instead of having to spread the logic of "look for the old field, if not look > the new ones" in the various places. > Plz talk with somebody who knows TV better than me (virtually anybody, > +petrcermak/nduca might be good candidates) to figure out the best approach. > > RS-LGTM here regardless on how you encode the local/global bit (prefix, field > name, boolean, a pony), as long as: > - the code doesn't diverge too much from this (% the encoding strategy mentioned > above) > - somebody who maintains TV is happy about encoding strategy chosen. > - any other reviewer does has a final pass on the final code. Here is my opinion on this issue: if we care about old consumers other than TV and devtools (that are going to be updated according to this CL anyway), then I vote for Andrey's suggestion. Otherwise, since we are changing the trace format here (the actual trace string that is publicly advertised in https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU) we should choose whatever we think is cleaner and easier to understand; consumers (trace viewer, devtools, etc) can parse whatever we decide with a little bit more or less work...
Pinging Nat or Ben :) https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; Nat, Ben, can one of you please tell us your preference about the JSON format here. The options discussed so far are: 1- ("id":"0x1000", ...), ("id":"0x1000", "id_is_global":false, ...), and ("id":"0x1000", "id_is_global":true, ...) vs 2- ("id":"0x1000", ...), ("local_id":"0x1000", ...), and ("global_id":"0x1000", ...) vs 3- ("id":"0x1000", ...), ("id":"local:0x1000", ...), and ("id":"global:0x1000", ...) Thanks a lot!
On 2016/09/02 at 19:37:37, chiniforooshan wrote: > Pinging Nat or Ben :) > > https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... > base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; > Nat, Ben, can one of you please tell us your preference about the JSON format here. The options discussed so far are: > > 1- ("id":"0x1000", ...), ("id":"0x1000", "id_is_global":false, ...), and ("id":"0x1000", "id_is_global":true, ...) > > vs > > 2- ("id":"0x1000", ...), ("local_id":"0x1000", ...), and ("global_id":"0x1000", ...) > > vs > > 3- ("id":"0x1000", ...), ("id":"local:0x1000", ...), and ("id":"global:0x1000", ...) > > Thanks a lot! Sorry, I don't think I can help here. @nduca ptal above.
chiniforooshan@chromium.org changed reviewers: + petrcermak@chromium.org
https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2253973003/diff/140001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:367: id_field_name = "local_id"; On 2016/09/02 19:37:37, chiniforooshan wrote: > Nat, Ben, can one of you please tell us your preference about the JSON format > here. The options discussed so far are: > > 1- ("id":"0x1000", ...), ("id":"0x1000", "id_is_global":false, ...), and > ("id":"0x1000", "id_is_global":true, ...) > > vs > > 2- ("id":"0x1000", ...), ("local_id":"0x1000", ...), and ("global_id":"0x1000", > ...) > > vs > > 3- ("id":"0x1000", ...), ("id":"local:0x1000", ...), and ("id":"global:0x1000", > ...) > > Thanks a lot! +petrcermak maybe?
Description was changed from ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=N/A ========== to ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=catapult:#2465 ==========
I like the idea of breaking the ids so that we don't get subtle "looks like its
working but its a lie" bugs that @caseq brings up.
Maybe we can do that just by versioning the id field rather than having the
fieldname convey semantics?
E.g.
id2: ...whatever we want...
Then, the rvalue of id2 could be either {local: "0x1000"} or it could be
"local:0x1000", whichever y'all prefer...
Even the ideas propose LGTM though. Progress here is better than perfection.
The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/12 16:16:36, nduca wrote:
> I like the idea of breaking the ids so that we don't get subtle "looks like
its
> working but its a lie" bugs that @caseq brings up.
>
> Maybe we can do that just by versioning the id field rather than having the
> fieldname convey semantics?
>
> E.g.
>
> id2: ...whatever we want...
>
>
> Then, the rvalue of id2 could be either {local: "0x1000"} or it could be
> "local:0x1000", whichever y'all prefer...
>
> Even the ideas propose LGTM though. Progress here is better than perfection.
Done.
Thanks Nat! I like your suggestion of versioning the ID field.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
piyapongmekpung26@gmail.com changed reviewers: + piyapongmekpung26@gmail.com
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, primiano@chromium.org, nduca@chromium.org Link to the patchset: https://codereview.chromium.org/2253973003/#ps180001 (title: "versioning the id field")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=catapult:#2465 ========== to ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=catapult:#2465 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=catapult:#2465 ========== to ========== Add an explicit way of making IDs local or global Two macros are added: TRACE_GLOBAL_ID and TRACE_LOCAL_ID. If these are used in trace events, a boolean "id_is_global" field will be added to the produced JSON record. We add the above-mentioned macros only in base. We will add them to other subsystems (V8, skia, WebKit, ...) separately, when needed. This patch also removes the DONT_MANGLE macros inside object and context events; object and context events are process-local by default and mangling or not mangling them should not change anything. Design doc: https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw... BUG=catapult:#2465 Committed: https://crrev.com/38e3c2977c913973fc7237089560ce7fc0822bc4 Cr-Commit-Position: refs/heads/master@{#419777} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/38e3c2977c913973fc7237089560ce7fc0822bc4 Cr-Commit-Position: refs/heads/master@{#419777} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
