Allow tracing in third_party libraries

base/debug/trace_event.h

Index: base/debug/trace_event.h
diff --git a/base/debug/trace_event.h b/base/debug/trace_event.h
index 444c97514dc925ed31f46bbd3f7fdfb04399333b..6383e93d70d70649b58b0ed30ba3a880a65a2bed 100644
--- a/base/debug/trace_event.h
+++ b/base/debug/trace_event.h
@@ -160,9 +160,54 @@
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/timer.h"
 
+////////////////////////////////////////////////////////////////////////////////
+/// Customize these defines to call through to platform tracing APIs.

[nduca, 2012/01/11 21:49:11]

You know, I'm starting to think we should make trace_event.h be a stub code that
#defines the TRACE_EVENT_API macros
#includes trace_event_macros.h
#includes trace_event_impl.h

Sorry it took me this long to figure out what you meant.

This would allow you to put nice documentation on the trace_event_macros.h file
that explains what it does. Something like "This header is designed to be give
you trace_event macros without specifying how the events actually get collected
and stored. If you need to expose trace event to some other universe, you can
copy-and-paste this file and just implement API* macros."

I'm not sure the TRACE_EVENT_NAMESPACE adds a lot of value here. That seems
specific to the binding of the macros to the base implementation, no? That is,
webkit wont even need that, right? There, we'll just point the macros at
something completely whacko.

[jbates, 2012/01/11 22:44:35]

I think this sounds good, but this is a step-2 feedback, right? I'm deferring
anything related to splitting up files to a follow-on CL so that we can clearly
see the refactoring changes first.
There are some local types that get copied to other libraries such as
TraceStringWithCopy, which undoubtedly would be put inside a namespace in
third_party libraries. To reduce the hand-editing required to fix up the _export
header, I tried to put everything that will need hand-editing in this section.
Alternatively I could just choose a namespace like "trace_event_internal::" and
throw all this stuff in there -- come to think of it that is a lot cleaner.
Sound good?

[jbates, 2012/01/11 23:52:44]

Removed TRACE_EVENT_NAMESPACE.

+
+#define TRACE_EVENT_NAMESPACE base::debug

[jar (doing other things), 2012/01/12 00:15:36]

What does this macro help you with?

[jbates, 2012/01/12 00:52:20]

Done. (removed)

+
+// volatile bool*
+//     TRACE_EVENT_API_GET_CATEGORY_ENABLED(const char* category_name)
+#define TRACE_EVENT_API_GET_CATEGORY_ENABLED \
+    TRACE_EVENT_NAMESPACE::TraceLog::GetCategoryEnabled
+
+// Returns the threshold_begin_id used by TRACE_IF_LONGER_THAN macros.
+// int TRACE_EVENT_API_ADD_TRACE_EVENT(
+//                    char phase,
+//                    const volatile bool* category_enabled,
+//                    const char* name,
+//                    uint64 id,
+//                    int num_args,
+//                    const char** arg_names,
+//                    const uint8* arg_types,
+//                    const uint64* arg_values,
+//                    int threshold_begin_id,
+//                    int64 threshold,
+//                    uint8 flags)
+#define TRACE_EVENT_API_ADD_TRACE_EVENT \
+    TRACE_EVENT_NAMESPACE::TraceLog::GetInstance()->AddTraceEvent
+
+// void TRACE_EVENT_API_ADD_COUNTER_EVENT(
+//                    const volatile bool* category_enabled,
+//                    const char* name,
+//                    uint64 id,
+//                    const char* arg1_name, int32 arg1_val,
+//                    const char* arg2_name, int32 arg2_val,
+//                    uint8 flags)
+#define TRACE_EVENT_API_ADD_COUNTER_EVENT \
+    TRACE_EVENT_NAMESPACE::TraceLog::GetInstance()->AddCounterEvent
+
+// Mangle |pointer| with a process ID hash so that if |pointer| occurs on more
+// than one process, it will not collide in the trace data.
+// uint64 TRACE_EVENT_API_GET_ID_FROM_POINTER(void* pointer)
+#define TRACE_EVENT_API_GET_ID_FROM_POINTER \
+    TRACE_EVENT_NAMESPACE::TraceLog::GetInstance()->GetInterProcessID
+
+////////////////////////////////////////////////////////////////////////////////
+
 // By default, const char* argument values are assumed to have long-lived scope
 // and will not be copied. Use this macro to force a const char* to be copied.
-#define TRACE_STR_COPY(str) base::debug::TraceValue::StringWithCopy(str)
+#define TRACE_STR_COPY(str) \
+    TRACE_EVENT_NAMESPACE::internal::TraceStringWithCopy(str)
 
 // Older style trace macros with explicit id and extra data
 // Only these macros result in publishing data to ETW as currently implemented.
@@ -187,12 +232,12 @@
 // - category and name strings must have application lifetime (statics or
 //   literals). They may not include " chars.
 #define TRACE_EVENT0(category, name) \
-    TRACE_EVENT1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_SCOPED(category, name)
 #define TRACE_EVENT1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_SCOPED(category, name, arg1_name, arg1_val)
 #define TRACE_EVENT2(category, name, arg1_name, arg1_val, arg2_name, arg2_val) \
-    INTERNAL_TRACE_EVENT_ADD_SCOPED( \
-        category, name, arg1_name, arg1_val, arg2_name, arg2_val)
+    INTERNAL_TRACE_EVENT_ADD_SCOPED(category, name, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 
 // Same as TRACE_EVENT except that they are not included in official builds.
 #ifdef OFFICIAL_BUILD
@@ -229,25 +274,27 @@
 // - category and name strings must have application lifetime (statics or
 //   literals). They may not include " chars.
 #define TRACE_EVENT_INSTANT0(category, name) \
-    TRACE_EVENT_INSTANT1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
+        category, name, TRACE_EVENT_FLAG_NONE)
 #define TRACE_EVENT_INSTANT1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_INSTANT2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val)
 #define TRACE_EVENT_INSTANT2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
-        category, name, arg1_name, arg1_val, arg2_name, arg2_val, \
-        TRACE_EVENT_FLAG_NONE)
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 #define TRACE_EVENT_COPY_INSTANT0(category, name) \
-    TRACE_EVENT_COPY_INSTANT1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
+        category, name, TRACE_EVENT_FLAG_COPY)
 #define TRACE_EVENT_COPY_INSTANT1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_COPY_INSTANT2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val)
 #define TRACE_EVENT_COPY_INSTANT2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_INSTANT, \
-        category, name, \
-        arg1_name, base::debug::TraceValue::ForceCopy(arg1_val), \
-        arg2_name, base::debug::TraceValue::ForceCopy(arg2_val), \
-        TRACE_EVENT_FLAG_COPY)
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 
 // Records a single BEGIN event called "name" immediately, with 0, 1 or 2
 // associated arguments. If the category is not enabled, then this
@@ -255,50 +302,54 @@
 // - category and name strings must have application lifetime (statics or
 //   literals). They may not include " chars.
 #define TRACE_EVENT_BEGIN0(category, name) \
-    TRACE_EVENT_BEGIN1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
+        category, name, TRACE_EVENT_FLAG_NONE)
 #define TRACE_EVENT_BEGIN1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_BEGIN2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val)
 #define TRACE_EVENT_BEGIN2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
-        category, name, arg1_name, arg1_val, arg2_name, arg2_val, \
-        TRACE_EVENT_FLAG_NONE)
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 #define TRACE_EVENT_COPY_BEGIN0(category, name) \
-    TRACE_EVENT_COPY_BEGIN1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
+        category, name, TRACE_EVENT_FLAG_COPY)
 #define TRACE_EVENT_COPY_BEGIN1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_COPY_BEGIN2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val)
 #define TRACE_EVENT_COPY_BEGIN2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_BEGIN, \
-        category, name, \
-        arg1_name, base::debug::TraceValue::ForceCopy(arg1_val), \
-        arg2_name, base::debug::TraceValue::ForceCopy(arg2_val), \
-        TRACE_EVENT_FLAG_COPY)
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 
 // Records a single END event for "name" immediately. If the category
 // is not enabled, then this does nothing.
 // - category and name strings must have application lifetime (statics or
 //   literals). They may not include " chars.
 #define TRACE_EVENT_END0(category, name) \
-    TRACE_EVENT_END1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
+        category, name, TRACE_EVENT_FLAG_NONE)
 #define TRACE_EVENT_END1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_END2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val)
 #define TRACE_EVENT_END2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
-        category, name, arg1_name, arg1_val, arg2_name, arg2_val, \
-        TRACE_EVENT_FLAG_NONE)
+        category, name, TRACE_EVENT_FLAG_NONE, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 #define TRACE_EVENT_COPY_END0(category, name) \
-    TRACE_EVENT_COPY_END1(category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
+        category, name, TRACE_EVENT_FLAG_COPY)
 #define TRACE_EVENT_COPY_END1(category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_COPY_END2(category, name, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val)
 #define TRACE_EVENT_COPY_END2(category, name, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD(TRACE_EVENT_PHASE_END, \
-        category, name, \
-        arg1_name, base::debug::TraceValue::ForceCopy(arg1_val), \
-        arg2_name, base::debug::TraceValue::ForceCopy(arg2_val), \
-        TRACE_EVENT_FLAG_COPY)
+        category, name, TRACE_EVENT_FLAG_COPY, arg1_name, arg1_val, \
+        arg2_name, arg2_val)
 
 // Time threshold event:
 // Only record the event if the duration is greater than the specified
@@ -309,15 +360,15 @@
 // - category and name strings must have application lifetime (statics or
 //   literals). They may not include " chars.
 #define TRACE_EVENT_IF_LONGER_THAN0(threshold_us, category, name) \
-    TRACE_EVENT_IF_LONGER_THAN1(threshold_us, category, name, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_SCOPED_IF_LONGER_THAN(threshold_us, category, name)
 #define TRACE_EVENT_IF_LONGER_THAN1( \
-        threshold_us, category, name, arg1_name, arg1_val) \
-    TRACE_EVENT_IF_LONGER_THAN2(threshold_us, category, name, \
-                           arg1_name, arg1_val, NULL, 0)
+    threshold_us, category, name, arg1_name, arg1_val) \
+    INTERNAL_TRACE_EVENT_ADD_SCOPED_IF_LONGER_THAN( \
+        threshold_us, category, name, arg1_name, arg1_val)
 #define TRACE_EVENT_IF_LONGER_THAN2( \
     threshold_us, category, name, arg1_name, arg1_val, arg2_name, arg2_val) \
-    INTERNAL_TRACE_EVENT_ADD_SCOPED_IF_LONGER_THAN(threshold_us, \
-        category, name, arg1_name, arg1_val, arg2_name, arg2_val)
+    INTERNAL_TRACE_EVENT_ADD_SCOPED_IF_LONGER_THAN( \
+        threshold_us, category, name, arg1_name, arg1_val, arg2_name, arg2_val)
 
 // Records the value of a counter called "name" immediately. Value
 // must be representable as a 32 bit integer.
@@ -392,48 +443,54 @@
 //   will be xored with a hash of the process ID so that the same pointer on
 //   two different processes will not collide.
 #define TRACE_EVENT_START0(category, name, id) \
-    TRACE_EVENT_START1(category, name, id, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID)
 #define TRACE_EVENT_START1(category, name, id, arg1_name, arg1_val) \
-    TRACE_EVENT_START2(category, name, id, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID, arg1_name, arg1_val)
 #define TRACE_EVENT_START2(category, name, id, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
-        category, name, id, arg1_name, arg1_val, arg2_name, arg2_val, \
-        TRACE_EVENT_FLAG_HAS_ID)
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID, \
+        arg1_name, arg1_val, arg2_name, arg2_val)
 #define TRACE_EVENT_COPY_START0(category, name, id) \
-    TRACE_EVENT_COPY_START1(category, name, id, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY)
 #define TRACE_EVENT_COPY_START1(category, name, id, arg1_name, arg1_val) \
-    TRACE_EVENT_COPY_START2(category, name, id, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY, \
+        arg1_name, arg1_val)
 #define TRACE_EVENT_COPY_START2(category, name, id, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_START, \
-        category, name, id, \
-        arg1_name, base::debug::TraceValue::ForceCopy(arg1_val), \
-        arg2_name, base::debug::TraceValue::ForceCopy(arg2_val), \
-        TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY)
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY, \
+        arg1_name, arg1_val, arg2_name, arg2_val)
 
 // Records a single FINISH event for "name" immediately. If the category
 // is not enabled, then this does nothing.
 #define TRACE_EVENT_FINISH0(category, name, id) \
-    TRACE_EVENT_FINISH1(category, name, id, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID)
 #define TRACE_EVENT_FINISH1(category, name, id, arg1_name, arg1_val) \
-    TRACE_EVENT_FINISH2(category, name, id, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID, arg1_name, arg1_val)
 #define TRACE_EVENT_FINISH2(category, name, id, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
-        category, name, id, arg1_name, arg1_val, arg2_name, arg2_val, \
-        TRACE_EVENT_FLAG_HAS_ID)
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID, \
+        arg1_name, arg1_val, arg2_name, arg2_val)
 #define TRACE_EVENT_COPY_FINISH0(category, name, id) \
-    TRACE_EVENT_COPY_FINISH1(category, name, id, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY)
 #define TRACE_EVENT_COPY_FINISH1(category, name, id, arg1_name, arg1_val) \
-    TRACE_EVENT_COPY_FINISH2(category, name, id, arg1_name, arg1_val, NULL, 0)
+    INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY, \
+        arg1_name, arg1_val)
 #define TRACE_EVENT_COPY_FINISH2(category, name, id, arg1_name, arg1_val, \
         arg2_name, arg2_val) \
     INTERNAL_TRACE_EVENT_ADD_WITH_ID(TRACE_EVENT_PHASE_FINISH, \
-        category, name, id, \
-        arg1_name, base::debug::TraceValue::ForceCopy(arg1_val), \
-        arg2_name, base::debug::TraceValue::ForceCopy(arg2_val), \
-        TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY)
+        category, name, id, TRACE_EVENT_FLAG_HAS_ID | TRACE_EVENT_FLAG_COPY, \
+        arg1_name, arg1_val, arg2_name, arg2_val)
 
 
 // Implementation detail: trace event macros create temporary variables
@@ -449,23 +506,22 @@
 // Implementation detail: internal macro to create static category.
 // - ANNOTATE_BENIGN_RACE, see Thread Safety above.
 #define INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category) \
-    static const base::debug::TraceCategory* \
+    static const volatile bool* \

[jar (doing other things), 2012/01/12 00:15:36]

What does the use of volatile do for you?  (for most users, not doing hardware
register coding, this keyword is misused).

[jbates, 2012/01/12 00:52:20]

It's useful in this case, because it tells the compiler that the load of this
bool can't be moved from the inside to the outside of a loop.

[jar (doing other things), 2012/01/12 02:20:03]

I don't think volatile has such ordering semantics.  Can you give me a pointer
to where this is stated? 

On 2012/01/12 00:52:20, jbates wrote:

[joth, 2012/01/12 16:14:46]

Not a normative reference, but very informative:
http://kerneltrap.org/Linux/Volatile_Superstition


"""
Volatile doesn't mean it can't be reordered; volatile means the
accesses can't be eliminated.
...

So if "volatile" makes a difference, it is invariably a sign of a bug in 
serialization (the one exception is for IO - we use "volatile" to avoid 
having to use inline asm for IO on x86) - and for "random values" like 
jiffies).
"""

Not eliminating the memory access is key: I think these bools actually do fall
into this "random values" corner case, where reording is fairly harmless, but
you don't want any accesses to be eliminated.
i.e. absolutely nothing 'bad' would occur w.r.t the production code if this code
happens to read the 'wrong' value on every iteration. The goal here is to stop
the compiler eliminating the inner-loop memory accesses. (whether that's
actually a good trade off though I can't call. Linus's comments about volatile
resulting in crap code generation, which will be a cost in *all* the other
non-loop call-sites!, maybe more important than having the category 'enablement'
be observed by any code already in its loop?)

         INTERNAL_TRACE_EVENT_UID(catstatic) = NULL; \
     ANNOTATE_BENIGN_RACE(&INTERNAL_TRACE_EVENT_UID(catstatic), \
                          "trace_event category"); \
     if (!INTERNAL_TRACE_EVENT_UID(catstatic)) \
       INTERNAL_TRACE_EVENT_UID(catstatic) = \
-          base::debug::TraceLog::GetCategory(category);
+          TRACE_EVENT_API_GET_CATEGORY_ENABLED(category);
 
 // Implementation detail: internal macro to create static category and add
 // event if the category is enabled.
-#define INTERNAL_TRACE_EVENT_ADD( \
-      phase, category, name, arg1_name, arg1_val, arg2_name, arg2_val, flags) \
+#define INTERNAL_TRACE_EVENT_ADD(phase, category, name, flags, ...) \

[jar (doing other things), 2012/01/12 00:15:36]

Is ellipsis (vararg) defined for the preprocessor???  I thought this was only
just part of the emerging standard... and I'm a little surprised to see it used
(as it may make porting harder).  

Am I too old school?

[jbates, 2012/01/12 00:52:20]

It's used in base/logging.h so I'm hoping I can use it here to avoid a lot of
macro code duplication.

[jar (doing other things), 2012/01/12 02:20:03]

Interesting.  OK.  Thanks for the example.  It is still surprising to me.

On 2012/01/12 00:52:20, jbates wrote:

     INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category); \
-    if (INTERNAL_TRACE_EVENT_UID(catstatic)->enabled) { \
-      base::debug::TraceLog::GetInstance()->AddTraceEvent( \
-          phase, INTERNAL_TRACE_EVENT_UID(catstatic), \
-          name, 0, arg1_name, arg1_val, arg2_name, arg2_val, -1, 0, flags); \
+    if (*INTERNAL_TRACE_EVENT_UID(catstatic)) { \
+      TRACE_EVENT_NAMESPACE::internal::AddTraceEvent( \
+          phase, INTERNAL_TRACE_EVENT_UID(catstatic), name, 0, flags, \
+          ##__VA_ARGS__); \

[jar (doing other things), 2012/01/12 00:15:36]

... and you're also using the ## gcc compatibility hack.

Again, for portability, I'm surprised we're doing this stuff.  Can you give me
any pointers?

[jbates, 2012/01/12 00:52:20]

I just stole this from base/logging.h, which did not appear to be a platform
dependent implementation (surprising to me as well).

     }
 
 // Implementation detail: internal macro to create static category and
@@ -473,26 +529,25 @@
 #define INTERNAL_TRACE_EVENT_ADD_COUNTER( \
       category, name, id, arg1_name, arg1_val, arg2_name, arg2_val, flags) \
     INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category); \
-    if (INTERNAL_TRACE_EVENT_UID(catstatic)->enabled) { \
-      base::debug::TraceLog::GetInstance()->AddCounterEvent( \
+    if (*INTERNAL_TRACE_EVENT_UID(catstatic)) { \
+      TRACE_EVENT_API_ADD_COUNTER_EVENT( \
           INTERNAL_TRACE_EVENT_UID(catstatic), \
-          name, id, arg1_name, arg1_val, arg2_name, arg2_val, flags); \
+          name, TRACE_EVENT_NAMESPACE::TraceID(id).data(), \
+          arg1_name, arg1_val, arg2_name, arg2_val, flags); \
     }
 
 // Implementation detail: internal macro to create static category and add begin
 // event if the category is enabled. Also adds the end event when the scope
 // ends.
-#define INTERNAL_TRACE_EVENT_ADD_SCOPED( \
-      category, name, arg1_name, arg1_val, arg2_name, arg2_val) \
+#define INTERNAL_TRACE_EVENT_ADD_SCOPED(category, name, ...) \
     INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category); \
-    base::debug::internal::TraceEndOnScopeClose \
+    TRACE_EVENT_NAMESPACE::internal::TraceEndOnScopeClose \
         INTERNAL_TRACE_EVENT_UID(profileScope); \
-    if (INTERNAL_TRACE_EVENT_UID(catstatic)->enabled) { \
-      base::debug::TraceLog::GetInstance()->AddTraceEvent( \
+    if (*INTERNAL_TRACE_EVENT_UID(catstatic)) { \
+      TRACE_EVENT_NAMESPACE::internal::AddTraceEvent( \
           TRACE_EVENT_PHASE_BEGIN, \
           INTERNAL_TRACE_EVENT_UID(catstatic), \
-          name, 0, arg1_name, arg1_val, arg2_name, arg2_val, -1, 0, \
-          TRACE_EVENT_FLAG_NONE); \
+          name, 0, TRACE_EVENT_FLAG_NONE, ##__VA_ARGS__); \
       INTERNAL_TRACE_EVENT_UID(profileScope).Initialize( \
           INTERNAL_TRACE_EVENT_UID(catstatic), name); \
     }
@@ -501,17 +556,16 @@
 // event if the category is enabled. Also adds the end event when the scope
 // ends. If the elapsed time is < threshold time, the begin/end pair is erased.
 #define INTERNAL_TRACE_EVENT_ADD_SCOPED_IF_LONGER_THAN(threshold, \
-      category, name, arg1_name, arg1_val, arg2_name, arg2_val) \
+        category, name, ...) \
     INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category); \
-    base::debug::internal::TraceEndOnScopeCloseThreshold \
+    TRACE_EVENT_NAMESPACE::internal::TraceEndOnScopeCloseThreshold \
         INTERNAL_TRACE_EVENT_UID(profileScope); \
-    if (INTERNAL_TRACE_EVENT_UID(catstatic)->enabled) { \
+    if (*INTERNAL_TRACE_EVENT_UID(catstatic)) { \
       int INTERNAL_TRACE_EVENT_UID(begin_event_id) = \
-        base::debug::TraceLog::GetInstance()->AddTraceEvent( \
+        TRACE_EVENT_NAMESPACE::internal::AddTraceEvent( \
             TRACE_EVENT_PHASE_BEGIN, \
             INTERNAL_TRACE_EVENT_UID(catstatic), \
-            name, 0, arg1_name, arg1_val, arg2_name, arg2_val, -1, 0, \
-            TRACE_EVENT_FLAG_NONE); \
+            name, 0, TRACE_EVENT_FLAG_NONE, ##__VA_ARGS__); \
       INTERNAL_TRACE_EVENT_UID(profileScope).Initialize( \
           INTERNAL_TRACE_EVENT_UID(catstatic), name, \
           INTERNAL_TRACE_EVENT_UID(begin_event_id), threshold); \
@@ -519,13 +573,14 @@
 
 // Implementation detail: internal macro to create static category and add
 // event if the category is enabled.
-#define INTERNAL_TRACE_EVENT_ADD_WITH_ID(phase, category, name, id, \
-      arg1_name, arg1_val, arg2_name, arg2_val, flags) \
+#define INTERNAL_TRACE_EVENT_ADD_WITH_ID(phase, category, name, id, flags, \
+                                         ...) \
     INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category); \
-    if (INTERNAL_TRACE_EVENT_UID(catstatic)->enabled) { \
-      base::debug::TraceLog::GetInstance()->AddTraceEvent( \
+    if (*INTERNAL_TRACE_EVENT_UID(catstatic)) { \
+      TRACE_EVENT_NAMESPACE::internal::AddTraceEvent( \
           phase, INTERNAL_TRACE_EVENT_UID(catstatic), \
-          name, id, arg1_name, arg1_val, arg2_name, arg2_val, -1, 0, flags); \
+          name, TRACE_EVENT_NAMESPACE::TraceID(id).data(), flags, \
+          ##__VA_ARGS__); \
     }
 
 template <typename Type>
@@ -537,153 +592,32 @@ class RefCountedString;
 
 namespace debug {
 
-// Categories allow enabling/disabling of streams of trace events
-struct TraceCategory {
-  const char* name;
-  volatile bool enabled;
-};
-
-const size_t kTraceMaxNumArgs = 2;
+const int kTraceMaxNumArgs = 2;
 
 // Phase indicates the nature of an event entry. E.g. part of a begin/end pair.
-typedef char TraceEventPhase;
-#define TRACE_EVENT_PHASE_BEGIN     (base::debug::TraceEventPhase('B'))
-#define TRACE_EVENT_PHASE_END       (base::debug::TraceEventPhase('E'))
-#define TRACE_EVENT_PHASE_INSTANT   (base::debug::TraceEventPhase('I'))
-#define TRACE_EVENT_PHASE_START     (base::debug::TraceEventPhase('S'))
-#define TRACE_EVENT_PHASE_FINISH    (base::debug::TraceEventPhase('F'))
-#define TRACE_EVENT_PHASE_METADATA  (base::debug::TraceEventPhase('M'))
-#define TRACE_EVENT_PHASE_COUNTER   (base::debug::TraceEventPhase('C'))
-
-typedef uint8 TraceEventFlags;
-#define TRACE_EVENT_FLAG_NONE        (base::debug::TraceEventFlags(0))
-#define TRACE_EVENT_FLAG_COPY        (base::debug::TraceEventFlags(1<<0))
-#define TRACE_EVENT_FLAG_HAS_ID      (base::debug::TraceEventFlags(1<<1))
-
-// Simple union of values. This is much lighter weight than base::Value, which
-// requires dynamic allocation and a vtable. To keep the trace runtime overhead
-// low, we want constant size storage here.
-class BASE_EXPORT TraceValue {
- public:
-  enum Type {
-    TRACE_TYPE_UNDEFINED,
-    TRACE_TYPE_BOOL,
-    TRACE_TYPE_UINT,
-    TRACE_TYPE_INT,
-    TRACE_TYPE_DOUBLE,
-    TRACE_TYPE_POINTER,
-    TRACE_TYPE_STRING,
-    TRACE_TYPE_STATIC_STRING
-  };
-
-  TraceValue() : type_(TRACE_TYPE_UNDEFINED) {
-    value_.as_uint = 0ull;
-  }
-  TraceValue(bool rhs) : type_(TRACE_TYPE_BOOL) {
-    value_.as_bool = rhs;
-  }
-  TraceValue(uint64 rhs) : type_(TRACE_TYPE_UINT) {
-    value_.as_uint = rhs;
-  }
-  TraceValue(uint32 rhs) : type_(TRACE_TYPE_UINT) {
-    value_.as_uint = rhs;
-  }
-  TraceValue(uint16 rhs) : type_(TRACE_TYPE_UINT) {
-    value_.as_uint = rhs;
-  }
-  TraceValue(uint8 rhs) : type_(TRACE_TYPE_UINT) {
-    value_.as_uint = rhs;
-  }
-  TraceValue(int64 rhs) : type_(TRACE_TYPE_INT) {
-    value_.as_int = rhs;
-  }
-  TraceValue(int32 rhs) : type_(TRACE_TYPE_INT) {
-    value_.as_int = rhs;
-  }
-  TraceValue(int16 rhs) : type_(TRACE_TYPE_INT) {
-    value_.as_int = rhs;
-  }
-  TraceValue(int8 rhs) : type_(TRACE_TYPE_INT) {
-    value_.as_int = rhs;
-  }
-  TraceValue(double rhs) : type_(TRACE_TYPE_DOUBLE) {
-    value_.as_double = rhs;
-  }
-  TraceValue(const void* rhs) : type_(TRACE_TYPE_POINTER) {
-    value_.as_pointer = rhs;
-  }
-  TraceValue(const std::string& rhs) : type_(TRACE_TYPE_STRING) {
-    value_.as_string = rhs.c_str();
-  }
-  TraceValue(const char* rhs) : type_(TRACE_TYPE_STATIC_STRING) {
-    value_.as_string = rhs;
-  }
-
-  static TraceValue StringWithCopy(const char* rhs) {
-    TraceValue value(rhs);
-    if (rhs)
-      value.type_ = TRACE_TYPE_STRING;
-    return value;
-  }
-
-  static TraceValue ForceCopy(const TraceValue& rhs) {
-    TraceValue value(rhs);
-    if (value.type_ == TRACE_TYPE_STATIC_STRING && value.as_string())
-      value.type_ = TRACE_TYPE_STRING;
-    return value;
-  }
-
-  bool is_string() const {
-    return type_ == TRACE_TYPE_STRING || type_ == TRACE_TYPE_STATIC_STRING;
-  }
-
-  void AppendAsJSON(std::string* out) const;
-
-  Type type() const {
-    return type_;
-  }
-  uint64 as_uint() const {
-    DCHECK_EQ(TRACE_TYPE_UINT, type_);
-    return value_.as_uint;
-  }
-  bool as_bool() const {
-    DCHECK_EQ(TRACE_TYPE_BOOL, type_);
-    return value_.as_bool;
-  }
-  int64 as_int() const {
-    DCHECK_EQ(TRACE_TYPE_INT, type_);
-    return value_.as_int;
-  }
-  double as_double() const {
-    DCHECK_EQ(TRACE_TYPE_DOUBLE, type_);
-    return value_.as_double;
-  }
-  const void* as_pointer() const {
-    DCHECK_EQ(TRACE_TYPE_POINTER, type_);
-    return value_.as_pointer;
-  }
-  const char* as_string() const {
-    DCHECK(is_string());
-    return value_.as_string;
-  }
-  const char** as_assignable_string() {
-    DCHECK_EQ(TRACE_TYPE_STRING, type_);
-    return &value_.as_string;
-  }
-
- private:
-  union Value {
-    bool as_bool;
-    uint64 as_uint;
-    int64 as_int;
-    double as_double;
-    const void* as_pointer;
-    const char* as_string;
-  };
-
-  Type type_;
-  Value value_;
-};
+// New types can be added, but existing values must never change.

[nduca, 2012/01/11 21:49:11]

Might want to clarify them 'must never change'. I think you mean something like
"downstream versions of trace_event reliy on these, so if you change them, you
need to very carefully update downstream as well."

[jbates, 2012/01/11 22:44:35]

I guess I don't see any reason to discuss changing existing values, because if
we've designed this API properly, changing values should never be necessary
(analogous to OpenGL values). Is there a less confusing way to write "existing
values must never change"?

[jbates, 2012/01/11 23:52:44]

Done. Ignore my previous comment, I was confused about whether we were talking
about chromium versions of this comment or third_party versions.

+#define TRACE_EVENT_PHASE_BEGIN    ('B')
+#define TRACE_EVENT_PHASE_END      ('E')
+#define TRACE_EVENT_PHASE_INSTANT  ('I')
+#define TRACE_EVENT_PHASE_START    ('S')
+#define TRACE_EVENT_PHASE_FINISH   ('F')
+#define TRACE_EVENT_PHASE_METADATA ('M')
+#define TRACE_EVENT_PHASE_COUNTER  ('C')
+
+// New types can be added, but existing values must never change.
+#define TRACE_EVENT_FLAG_NONE        (uint8(0))
+#define TRACE_EVENT_FLAG_COPY        (uint8(1<<0))
+#define TRACE_EVENT_FLAG_HAS_ID      (uint8(1<<1))
+
+// Type values for identifying types in the TraceValue union.
+// New types can be added, but existing values must never change.
+#define TRACE_VALUE_TYPE_BOOL         (uint8(1))
+#define TRACE_VALUE_TYPE_UINT         (uint8(2))
+#define TRACE_VALUE_TYPE_INT          (uint8(3))
+#define TRACE_VALUE_TYPE_DOUBLE       (uint8(4))
+#define TRACE_VALUE_TYPE_POINTER      (uint8(5))
+#define TRACE_VALUE_TYPE_STRING       (uint8(6))
+#define TRACE_VALUE_TYPE_COPY_STRING  (uint8(7))
 
 // TraceID encapsulates an ID that can either be an integer or pointer. Pointers
 // are mangled with the Process ID so that they are unlikely to collide when the
@@ -711,26 +645,29 @@ class BASE_EXPORT TraceID {
 // force it to happen.
 class BASE_EXPORT TraceEvent {
  public:
+  union TraceValue {
+    bool as_bool;
+    uint64 as_uint;
+    int64 as_int;
+    double as_double;
+    const void* as_pointer;
+    const char* as_string;

[nduca, 2012/01/11 21:49:11]

No clue what goog style says, so I'll assume as_x is what le-style-gods-decree?

[jbates, 2012/01/11 22:44:35]

The TraceValue union was defined this way already, but let me know if there's a
style problem.

+  };
+
   TraceEvent();
   TraceEvent(int thread_id,
              TimeTicks timestamp,
-             TraceEventPhase phase,
-             const TraceCategory* category,
+             char phase,
+             const volatile bool* category_enabled,

[joth, 2012/01/12 16:14:46]

I think you can achieve your goal by just putting volatile on the pointer
through which the bool accessed (up on line 509).
i.e. No need to permeate it throughout the file. (with reference to the thread I
linked above, this is equivalent to hiding its use inside atomic_read() rather
than putting it on the typedef itself)

[jbates, 2012/01/12 19:41:29]

Just getting rid of volatile completely, as it doesn't seem worth the perf
tradeoff as you mentioned.

              const char* name,
-             TraceID id,
-             const char* arg1_name, const TraceValue& arg1_val,
-             const char* arg2_name, const TraceValue& arg2_val,
-             TraceEventFlags flags);
+             uint64 id,
+             int num_args,
+             const char** arg_names,
+             const uint8* arg_types,
+             const uint64* arg_values,
+             uint8 flags);
   ~TraceEvent();
 
-  static char GetPhaseChar(TraceEventPhase phase) {
-    return static_cast<char>(phase);
-  }
-
-  static TraceEventPhase GetPhase(const char* phase) {
-    return static_cast<TraceEventPhase>(*phase);
-  }
-
   // Serialize event data to JSON
   static void AppendEventsAsJSON(const std::vector<TraceEvent>& events,
                                  size_t start,
@@ -750,17 +687,22 @@ class BASE_EXPORT TraceEvent {
 
  private:
   // Note: these are ordered by size (largest first) for optimal packing.
+  // 64-bit types:

[nduca, 2012/01/11 21:49:11]

not sure we need these comments, but if you feel they're value adds, then at
least make them stentences. // These are all 64 bit types.

[jbates, 2012/01/11 22:44:35]

Does seem redundant with the first comment -- will remove them.

[jbates, 2012/01/11 23:52:44]

Done.

   TimeTicks timestamp_;
   // id_ can be used to store phase-specific data.
-  TraceID id_;
+  uint64 id_;
   TraceValue arg_values_[kTraceMaxNumArgs];
+  // Pointers:
   const char* arg_names_[kTraceMaxNumArgs];
-  const TraceCategory* category_;
+  const volatile bool* category_enabled_;
   const char* name_;
   scoped_refptr<base::RefCountedString> parameter_copy_storage_;
+  // Ints:
   int thread_id_;
-  TraceEventPhase phase_;
-  TraceEventFlags flags_;
+  // Bytes:
+  char phase_;
+  uint8 flags_;
+  uint8 arg_types_[kTraceMaxNumArgs];
 };
 
 
@@ -870,7 +812,8 @@ class BASE_EXPORT TraceLog {
   void Flush();
 
   // Called by TRACE_EVENT* macros, don't call this directly.
-  static const TraceCategory* GetCategory(const char* name);
+  static const volatile bool* GetCategoryEnabled(const char* name);
+  static const char* GetCategoryName(const volatile bool* category_enabled);
 
   // Called by TRACE_EVENT* macros, don't call this directly.
   // Returns the index in the internal vector of the event if it was added, or
@@ -880,36 +823,41 @@ class BASE_EXPORT TraceLog {
   // is less than the threshold, the begin/end event pair is dropped.
   // If |copy| is set, |name|, |arg_name1| and |arg_name2| will be deep copied
   // into the event; see "Memory scoping note" and TRACE_EVENT_COPY_XXX above.
-  int AddTraceEvent(TraceEventPhase phase,
-                    const TraceCategory* category,
+  int AddTraceEvent(char phase,
+                    const volatile bool* category_enabled,
                     const char* name,
-                    TraceID id,
-                    const char* arg1_name, TraceValue arg1_val,
-                    const char* arg2_name, TraceValue arg2_val,
+                    uint64 id,
+                    int num_args,
+                    const char** arg_names,
+                    const uint8* arg_types,
+                    const uint64* arg_values,
                     int threshold_begin_id,
                     int64 threshold,
-                    TraceEventFlags flags);
-  static void AddTraceEventEtw(TraceEventPhase phase,
+                    uint8 flags);
+  static void AddTraceEventEtw(char phase,
                                const char* name,
                                const void* id,
                                const char* extra);
-  static void AddTraceEventEtw(TraceEventPhase phase,
+  static void AddTraceEventEtw(char phase,
                                const char* name,
                                const void* id,
                                const std::string& extra);
 
   // A wrapper around AddTraceEvent used by TRACE_COUNTERx macros
   // that allows only integer values for the counters.
-  int AddCounterEvent(const TraceCategory* category,
-                      const char* name,
-                      TraceID id,
-                      const char* arg1_name, int32 arg1_val,
-                      const char* arg2_name, int32 arg2_val,
-                      TraceEventFlags flags);
-
-  // Mangle |id| with a hash based on the process ID so that if |id| occurs on
+  void AddCounterEvent(const volatile bool* category_enabled,
+                       const char* name,
+                       uint64 id,
+                       const char* arg1_name, int32 arg1_val,
+                       const char* arg2_name, int32 arg2_val,
+                       uint8 flags);
+
+  // Mangle |ptr| with a hash based on the process ID so that if |ptr| occurs on
   // more than one process, it will not collide.
-  uint64 GetIntraProcessID(uint64 id) const { return id ^ process_id_hash_; }
+  uint64 GetInterProcessID(void* ptr) const {
+    return static_cast<uint64>(reinterpret_cast<uintptr_t>(ptr)) ^
+           process_id_hash_;
+  }
 
   int process_id() const { return process_id_; }
 
@@ -937,7 +885,7 @@ class BASE_EXPORT TraceLog {
 
   TraceLog();
   ~TraceLog();
-  const TraceCategory* GetCategoryInternal(const char* name);
+  const volatile bool* GetCategoryEnabledInternal(const char* name);
   void AddThreadNameMetadataEvents();
   void AddClockSyncMetadataEvents();
 
@@ -963,6 +911,124 @@ class BASE_EXPORT TraceLog {
 
 namespace internal {
 
+// Simple union to store various types as uint64.
+union TraceValueUnion {
+  bool as_bool;
+  uint64 as_uint;
+  int64 as_int;
+  double as_double;
+  const void* as_pointer;
+  const char* as_string;
+};
+
+// Simple container for const char* that should be copied instead of retained.
+class TraceStringWithCopy {
+ public:
+  explicit TraceStringWithCopy(const char* str) : str_(str) {}
+  operator const char* () const { return str_; }
+ private:
+  const char* str_;
+};
+
+// Define SetTraceValue for each allowed type. It stores the type and
+// value in the return arguments. This allows this API to avoid declaring any
+// structures so that it is portable to third_party libraries.
+#define INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, \

[jar (doing other things), 2012/01/12 02:20:03]

I'm not a template master at all... but couldn't this be done better with
templates, and avoid macros??  I don't see the magic that macros bring to the
table here.

[jbates, 2012/01/12 19:41:29]

In this case we need full control over the supported types, so templates would
be too flexible. I did try to get rid of the macro anyway, but it results in a
lot of code duplication, so the macros seems to be the lesser of two evils. I'm
fine either way though, let me know if you'd prefer the functions typed out.

+                                         union_member, \
+                                         value_type_id) \
+    static inline void SetTraceValue(actual_type arg, \
+                                     uint8* type, \
+                                     uint64* value) { \
+      TraceValueUnion type_value; \
+      type_value.union_member = arg; \
+      *type = value_type_id; \
+      *value = type_value.as_uint; \
+    }
+
+INTERNAL_DECLARE_SET_TRACE_VALUE(bool, as_bool, TRACE_VALUE_TYPE_BOOL)
+INTERNAL_DECLARE_SET_TRACE_VALUE(uint64, as_uint, TRACE_VALUE_TYPE_UINT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(uint32, as_uint, TRACE_VALUE_TYPE_UINT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(uint16, as_uint, TRACE_VALUE_TYPE_UINT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(uint8, as_uint, TRACE_VALUE_TYPE_UINT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(int64, as_int, TRACE_VALUE_TYPE_INT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(int32, as_int, TRACE_VALUE_TYPE_INT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(int16, as_int, TRACE_VALUE_TYPE_INT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(int8, as_int, TRACE_VALUE_TYPE_INT)
+INTERNAL_DECLARE_SET_TRACE_VALUE(double, as_double, TRACE_VALUE_TYPE_DOUBLE)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const void*, as_pointer,
+                                 TRACE_VALUE_TYPE_POINTER)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const char*, as_string,
+                                 TRACE_VALUE_TYPE_STRING)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWithCopy&, as_string,
+                                 TRACE_VALUE_TYPE_COPY_STRING)
+

[jar (doing other things), 2012/01/12 02:20:03]

I you really want to use a macro... you may as well undef it here.

[jbates, 2012/01/12 19:41:29]

Done.

+// std::string version of SetTraceValue so that trace arguments can be strings.
+static inline void SetTraceValue(const std::string& arg,
+                                 uint8* type,
+                                 uint64* value) {
+  TraceValueUnion type_value;
+  type_value.as_string = arg.c_str();
+  *type = TRACE_VALUE_TYPE_COPY_STRING;
+  *value = type_value.as_uint;
+}
+
+// These AddTraceEvent template functions are defined here instead of in the
+// macro, because the arg_values could be temporary objects, such as
+// std::string. In order to store pointers to the internal c_str and pass
+// through to the tracing API, the arg_values must live throughout
+// these procedures.
+
+static inline int AddTraceEvent(char phase,
+                                const volatile bool* category_enabled,
+                                const char* name,
+                                uint64 id,
+                                uint8 flags) {
+  return TRACE_EVENT_API_ADD_TRACE_EVENT(
+      phase, category_enabled, name, id,
+      0, NULL, NULL, NULL,
+      -1, 0, flags);
+}
+
+template<class ARG1_TYPE>
+static inline int AddTraceEvent(char phase,
+                                const volatile bool* category_enabled,
+                                const char* name,
+                                uint64 id,
+                                uint8 flags,
+                                const char* arg1_name,
+                                const ARG1_TYPE& arg1_val) {
+  const int num_args = 1;
+  uint8 arg_types[1];
+  uint64 arg_values[1];
+  SetTraceValue(arg1_val, &arg_types[0], &arg_values[0]);
+  return TRACE_EVENT_API_ADD_TRACE_EVENT(
+      phase, category_enabled, name, id,
+      num_args, &arg1_name, arg_types, arg_values,
+      -1, 0, flags);
+}
+
+template<class ARG1_TYPE, class ARG2_TYPE>
+static inline int AddTraceEvent(char phase,
+                                const volatile bool* category_enabled,
+                                const char* name,
+                                uint64 id,
+                                uint8 flags,
+                                const char* arg1_name,
+                                const ARG1_TYPE& arg1_val,
+                                const char* arg2_name,
+                                const ARG2_TYPE& arg2_val) {
+  const int num_args = 2;
+  const char* arg_names[2] = { arg1_name, arg2_name };
+  uint8 arg_types[2];
+  uint64 arg_values[2];
+  SetTraceValue(arg1_val, &arg_types[0], &arg_values[0]);
+  SetTraceValue(arg2_val, &arg_types[1], &arg_values[1]);
+  return TRACE_EVENT_API_ADD_TRACE_EVENT(
+      phase, category_enabled, name, id,
+      num_args, arg_names, arg_types, arg_values,
+      -1, 0, flags);
+}
+
 // Used by TRACE_EVENTx macro. Do not use directly.
 class BASE_EXPORT TraceEndOnScopeClose {
  public:
@@ -973,7 +1039,7 @@ class BASE_EXPORT TraceEndOnScopeClose {
       AddEventIfEnabled();
   }
 
-  void Initialize(const TraceCategory* category,
+  void Initialize(const volatile bool* category_enabled,
                   const char* name);
 
  private:
@@ -986,7 +1052,7 @@ class BASE_EXPORT TraceEndOnScopeClose {
   // members of this class instead, compiler warnings occur about potential
   // uninitialized accesses.
   struct Data {
-    const TraceCategory* category;
+    const volatile bool* category_enabled;
     const char* name;
   };
   Data* p_data_;
@@ -1005,7 +1071,7 @@ class BASE_EXPORT TraceEndOnScopeCloseThreshold {
 
   // Called by macros only when tracing is enabled at the point when the begin
   // event is added.
-  void Initialize(const TraceCategory* category,
+  void Initialize(const volatile bool* category_enabled,
                   const char* name,
                   int threshold_begin_id,
                   int64 threshold);
@@ -1021,7 +1087,7 @@ class BASE_EXPORT TraceEndOnScopeCloseThreshold {
   // uninitialized accesses.
   struct Data {
     int64 threshold;
-    const TraceCategory* category;
+    const volatile bool* category_enabled;
     const char* name;
     int threshold_begin_id;
   };