Add an explicit way of making IDs local or global

base/trace_event/common/trace_event_common.h

Index: base/trace_event/common/trace_event_common.h
diff --git a/base/trace_event/common/trace_event_common.h b/base/trace_event/common/trace_event_common.h
index 0c16e7b72382d3d56b17df4761798cc6f6bc202a..11b8c249cefb854bc8974e61ac8f33545456723c 100644
--- a/base/trace_event/common/trace_event_common.h
+++ b/base/trace_event/common/trace_event_common.h
@@ -965,41 +965,40 @@
 // See also TraceTrackableObject.
 #define TRACE_EVENT_OBJECT_CREATED_WITH_ID(category_group, name, id) \
   INTERNAL_TRACE_EVENT_ADD_WITH_ID(                                  \
-      TRACE_EVENT_PHASE_CREATE_OBJECT, category_group, name,         \
-      TRACE_ID_DONT_MANGLE(id), TRACE_EVENT_FLAG_NONE)
+      TRACE_EVENT_PHASE_CREATE_OBJECT, category_group, name, id,     \
+      TRACE_EVENT_FLAG_NONE)
 
 #define TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(category_group, name, id, \
                                             snapshot)                 \
   INTERNAL_TRACE_EVENT_ADD_WITH_ID(                                   \
       TRACE_EVENT_PHASE_SNAPSHOT_OBJECT, category_group, name,        \
-      TRACE_ID_DONT_MANGLE(id), TRACE_EVENT_FLAG_NONE, "snapshot", snapshot)
+      id, TRACE_EVENT_FLAG_NONE, "snapshot", snapshot)
 
-#define TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID_AND_TIMESTAMP(                    \
-    category_group, name, id, timestamp, snapshot)                            \
-  INTERNAL_TRACE_EVENT_ADD_WITH_ID_TID_AND_TIMESTAMP(                         \
-      TRACE_EVENT_PHASE_SNAPSHOT_OBJECT, category_group, name,                \
-      TRACE_ID_DONT_MANGLE(id), TRACE_EVENT_API_CURRENT_THREAD_ID, timestamp, \
-      TRACE_EVENT_FLAG_NONE, "snapshot", snapshot)
+#define TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID_AND_TIMESTAMP(                     \
+    category_group, name, id, timestamp, snapshot)                             \
+  INTERNAL_TRACE_EVENT_ADD_WITH_ID_TID_AND_TIMESTAMP(                          \
+      TRACE_EVENT_PHASE_SNAPSHOT_OBJECT, category_group, name,                 \
+      id, TRACE_EVENT_API_CURRENT_THREAD_ID, timestamp, TRACE_EVENT_FLAG_NONE, \
+      "snapshot", snapshot)
 
 #define TRACE_EVENT_OBJECT_DELETED_WITH_ID(category_group, name, id) \
   INTERNAL_TRACE_EVENT_ADD_WITH_ID(                                  \
-      TRACE_EVENT_PHASE_DELETE_OBJECT, category_group, name,         \
-      TRACE_ID_DONT_MANGLE(id), TRACE_EVENT_FLAG_NONE)
+      TRACE_EVENT_PHASE_DELETE_OBJECT, category_group, name, id,     \
+      TRACE_EVENT_FLAG_NONE)
 
 // Records entering and leaving trace event contexts. |category_group| and
 // |name| specify the context category and type. |context| is a
 // snapshotted context object id.
-#define TRACE_EVENT_ENTER_CONTEXT(category_group, name, context) \
-  INTERNAL_TRACE_EVENT_ADD_WITH_ID(                              \
-      TRACE_EVENT_PHASE_ENTER_CONTEXT, category_group, name,     \
-      TRACE_ID_DONT_MANGLE(context), TRACE_EVENT_FLAG_NONE)
-#define TRACE_EVENT_LEAVE_CONTEXT(category_group, name, context) \
-  INTERNAL_TRACE_EVENT_ADD_WITH_ID(                              \
-      TRACE_EVENT_PHASE_LEAVE_CONTEXT, category_group, name,     \
-      TRACE_ID_DONT_MANGLE(context), TRACE_EVENT_FLAG_NONE)
+#define TRACE_EVENT_ENTER_CONTEXT(category_group, name, context)      \
+  INTERNAL_TRACE_EVENT_ADD_WITH_ID(                                   \
+      TRACE_EVENT_PHASE_ENTER_CONTEXT, category_group, name, context, \
+      TRACE_EVENT_FLAG_NONE)
+#define TRACE_EVENT_LEAVE_CONTEXT(category_group, name, context)      \
+  INTERNAL_TRACE_EVENT_ADD_WITH_ID(                                   \
+      TRACE_EVENT_PHASE_LEAVE_CONTEXT, category_group, name, context, \
+      TRACE_EVENT_FLAG_NONE)
 #define TRACE_EVENT_SCOPED_CONTEXT(category_group, name, context) \
-  INTERNAL_TRACE_EVENT_SCOPED_CONTEXT(category_group, name,       \
-                                      TRACE_ID_DONT_MANGLE(context))
+  INTERNAL_TRACE_EVENT_SCOPED_CONTEXT(category_group, name, context)
 
 // Macro to specify that two trace IDs are identical. For example,
 // TRACE_BIND_IDS(
@@ -1093,7 +1092,12 @@
 #define TRACE_EVENT_FLAG_FLOW_OUT (static_cast<unsigned int>(1 << 9))
 #define TRACE_EVENT_FLAG_HAS_CONTEXT_ID (static_cast<unsigned int>(1 << 10))
 #define TRACE_EVENT_FLAG_HAS_PROCESS_ID (static_cast<unsigned int>(1 << 11))
+#define TRACE_EVENT_FLAG_HAS_LOCAL_ID (static_cast<unsigned int>(1 << 12))
+#define TRACE_EVENT_FLAG_HAS_GLOBAL_ID (static_cast<unsigned int>(1 << 1 | \

[Sami, 2016/08/18 17:37:32]

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.

[chiniforooshan, 2016/08/18 18:09:41]

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.

[Sami, 2016/08/19 16:47:19]

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.

[chiniforooshan, 2016/08/19 17:35:18]

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?

[Sami, 2016/08/22 14:46:41]

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 :)

[chiniforooshan, 2016/08/23 19:59:08]

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.

+                                                                  1 << 12))
+#define TRACE_EVENT_FLAG_ALL (static_cast<unsigned int>((1 << 13) - 1))
 
+#define TRACE_EVENT_FLAG_ID_MASK (static_cast<unsigned int>(1 << 1 | 1 << 12))
 #define TRACE_EVENT_FLAG_SCOPE_MASK                          \
   (static_cast<unsigned int>(TRACE_EVENT_FLAG_SCOPE_OFFSET | \
                              TRACE_EVENT_FLAG_SCOPE_EXTRA))