Introduce base::trace_event::TraceArguments helper class.

NOTE: This a reland of [1] which got reverted in [2].

This CL introduces the TraceArguments helper class to simplify
the way TRACE_EVENTXXX macro named arguments are passed to the
trace log. By using a small dedicated data structure, it is
possible to generate less code at each trace call site,
compared to the old way that relies on passing 5 specially
crafted arguments to TraceLog methods.

For example, this optimization can save about 12 kiB in
libmonochrome.so for the 32-bit ARM build of Chrome on Android.
For more details, see corresponding bug entry.

This CL does the following:

- Introduce the new class in base/trace_event/trace_arguments.h

- Move the definition of TraceValue to trace_arguments.h and
  augment the union with templated methods to support easy
  initialization from a large set of C++ value types.

- Add new TraceLog methods to add trace events using an
  optional TraceArguments parameter, instead of a bag of
  5 ones (count + names + types + values + convertables).

- Simplify internal templates using C++11 universal references
  and std::forward<> to remove special-casing convertable
  arguments in trace event calls.

- Remove the now obsolete TraceValueUnion type from trace_event.h

- Update base/trace_event/ code to use TraceArguments and the
  new TraceEvent move operations.

- Note that this keeps a few forwarding data types, values and
  methods to avoid other direct callers from the Chromium tree,
  these will be updated in future CLs. One Blink header needs
  to be modified though because it specializes a template
  defined in trace_arguments.h, to support sending WTF::CString
  references as trace arguments.

BUG=898794
R=oysteine@chromium.org,chiniforooshan@chromium.org,alexiln@chromium.org,primiano@chromium.org,torne@chromium.org
TBR=torne@chromium.org,primiano@chromium.org

[1] https://chromium-review.googlesource.com/c/1298997
[2] https://chromium-review.googlesource.com/c/1305250

Change-Id: I67fa434ab1c29b65abd0f327d1c9d27973f54f6a
Reviewed-on: https://chromium-review.googlesource.com/c/1318919
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615099}

base/BUILD.gn

@@ -938,6 +938,8 @@ jumbo_component("base") {
     "trace_event/memory_usage_estimator.h",
     "trace_event/process_memory_dump.cc",
     "trace_event/process_memory_dump.h",
+    "trace_event/trace_arguments.cc",
+    "trace_event/trace_arguments.h",
     "trace_event/trace_buffer.cc",
     "trace_event/trace_buffer.h",
     "trace_event/trace_category.h",
@@ -2530,6 +2532,7 @@ test("base_unittests") {
     "trace_event/memory_infra_background_whitelist_unittest.cc",
     "trace_event/memory_usage_estimator_unittest.cc",
     "trace_event/process_memory_dump_unittest.cc",
+    "trace_event/trace_arguments_unittest.cc",
     "trace_event/trace_category_unittest.cc",
     "trace_event/trace_config_unittest.cc",
     "trace_event/trace_event_filter_test_utils.cc",

base/trace_event/blame_context.cc

@@ -33,9 +33,9 @@ BlameContext::BlameContext(const char* category,
 BlameContext::~BlameContext() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(WasInitialized());
-  TRACE_EVENT_API_ADD_TRACE_EVENT(
-      TRACE_EVENT_PHASE_DELETE_OBJECT, category_group_enabled_, type_, scope_,
-      id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID);
+  TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_DELETE_OBJECT,
+                                  category_group_enabled_, type_, scope_, id_,
+                                  nullptr, TRACE_EVENT_FLAG_HAS_ID);
   trace_event::TraceLog::GetInstance()->RemoveAsyncEnabledStateObserver(this);
 }
 
@@ -43,7 +43,6 @@ void BlameContext::Enter() {
   DCHECK(WasInitialized());
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_ENTER_CONTEXT,
                                   category_group_enabled_, name_, scope_, id_,
-                                  0 /* num_args */, nullptr, nullptr, nullptr,
                                   nullptr, TRACE_EVENT_FLAG_HAS_ID);
 }
 
@@ -51,7 +50,6 @@ void BlameContext::Leave() {
   DCHECK(WasInitialized());
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_LEAVE_CONTEXT,
                                   category_group_enabled_, name_, scope_, id_,
-                                  0 /* num_args */, nullptr, nullptr, nullptr,
                                   nullptr, TRACE_EVENT_FLAG_HAS_ID);
 }
 
@@ -63,15 +61,10 @@ void BlameContext::TakeSnapshot() {
   std::unique_ptr<trace_event::TracedValue> snapshot(
       new trace_event::TracedValue);
   AsValueInto(snapshot.get());
-  static const char* const kArgName = "snapshot";
-  const int kNumArgs = 1;
-  unsigned char arg_types[1] = {TRACE_VALUE_TYPE_CONVERTABLE};
-  std::unique_ptr<trace_event::ConvertableToTraceFormat> arg_values[1] = {
-      std::move(snapshot)};
+  TraceArguments args("snapshot", std::move(snapshot));
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_SNAPSHOT_OBJECT,
                                   category_group_enabled_, type_, scope_, id_,
-                                  kNumArgs, &kArgName, arg_types, nullptr,
-                                  arg_values, TRACE_EVENT_FLAG_HAS_ID);
+                                  &args, TRACE_EVENT_FLAG_HAS_ID);
 }
 
 void BlameContext::OnTraceLogEnabled() {
@@ -95,9 +88,9 @@ void BlameContext::Initialize() {
   DCHECK(thread_checker_.CalledOnValidThread());
   category_group_enabled_ =
       TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(category_);
-  TRACE_EVENT_API_ADD_TRACE_EVENT(
-      TRACE_EVENT_PHASE_CREATE_OBJECT, category_group_enabled_, type_, scope_,
-      id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID);
+  TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_CREATE_OBJECT,
+                                  category_group_enabled_, type_, scope_, id_,
+                                  nullptr, TRACE_EVENT_FLAG_HAS_ID);
   trace_event::TraceLog::GetInstance()->AddAsyncEnabledStateObserver(
       weak_factory_.GetWeakPtr());
   TakeSnapshot();

base/trace_event/event_name_filter_unittest.cc

@@ -13,8 +13,8 @@ namespace trace_event {
 
 const TraceEvent& MakeTraceEvent(const char* name) {
   static TraceEvent event;
-  event.Reset(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0, 0,
-              nullptr, nullptr, nullptr, nullptr, 0);
+  event.Reset(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0,
+              nullptr, 0);
   return event;
 }
 

base/trace_event/process_memory_dump.cc

@@ -13,6 +13,7 @@
 #include "base/process/process_metrics.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_infra_background_whitelist.h"
+#include "base/trace_event/trace_event_impl.h"
 #include "base/trace_event/traced_value.h"
 #include "base/unguessable_token.h"
 #include "build/build_config.h"

base/trace_event/trace_arguments.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/trace_event/trace_arguments.h"
+
+#include <inttypes.h>
+#include <stddef.h>
+#include <string.h>
+
+#include <stdio.h>
+
+#include "base/json/string_escape.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+
+namespace base {
+namespace trace_event {
+
+namespace {
+
+size_t GetAllocLength(const char* str) {
+  return str ? strlen(str) + 1 : 0;
+}
+
+// Copies |*member| into |*buffer|, sets |*member| to point to this new
+// location, and then advances |*buffer| by the amount written.
+void CopyTraceEventParameter(char** buffer,
+                             const char** member,
+                             const char* end) {
+  if (*member) {
+    size_t written = strlcpy(*buffer, *member, end - *buffer) + 1;
+    DCHECK_LE(static_cast<int>(written), end - *buffer);
+    *member = *buffer;
+    *buffer += written;
+  }
+}
+
+// Append |val| as a JSON output value to |*out|.
+void AppendDoubleAsJSON(double val, std::string* out) {
+  // FIXME: base/json/json_writer.cc is using the same code,
+  //        should be made into a common method.
+  std::string real;
+  if (std::isfinite(val)) {
+    real = NumberToString(val);
+    // Ensure that the number has a .0 if there's no decimal or 'e'.  This
+    // makes sure that when we read the JSON back, it's interpreted as a
+    // real rather than an int.
+    if (real.find('.') == std::string::npos &&
+        real.find('e') == std::string::npos &&
+        real.find('E') == std::string::npos) {
+      real.append(".0");
+    }
+    // The JSON spec requires that non-integer values in the range (-1,1)
+    // have a zero before the decimal point - ".52" is not valid, "0.52" is.
+    if (real[0] == '.') {
+      real.insert(0, "0");
+    } else if (real.length() > 1 && real[0] == '-' && real[1] == '.') {
+      // "-.1" bad "-0.1" good
+      real.insert(1, "0");
+    }
+  } else if (std::isnan(val)) {
+    // The JSON spec doesn't allow NaN and Infinity (since these are
+    // objects in EcmaScript).  Use strings instead.
+    real = "\"NaN\"";
+  } else if (val < 0) {
+    real = "\"-Infinity\"";
+  } else {
+    real = "\"Infinity\"";
+  }
+  StringAppendF(out, "%s", real.c_str());
+}
+
+const char* TypeToString(char arg_type) {
+  switch (arg_type) {
+    case TRACE_VALUE_TYPE_INT:
+      return "int";
+    case TRACE_VALUE_TYPE_UINT:
+      return "uint";
+    case TRACE_VALUE_TYPE_DOUBLE:
+      return "double";
+    case TRACE_VALUE_TYPE_BOOL:
+      return "bool";
+    case TRACE_VALUE_TYPE_POINTER:
+      return "pointer";
+    case TRACE_VALUE_TYPE_STRING:
+      return "string";
+    case TRACE_VALUE_TYPE_COPY_STRING:
+      return "copy_string";
+    case TRACE_VALUE_TYPE_CONVERTABLE:
+      return "convertable";
+    default:
+      NOTREACHED();
+      return "UNKNOWN_TYPE";
+  }
+}
+
+void AppendValueDebugString(const TraceArguments& args,
+                            size_t idx,
+                            std::string* out) {
+  *out += (args.names()[idx] ? args.names()[idx] : "NULL_NAME");
+  *out += "=";
+  *out += TypeToString(args.types()[idx]);
+  *out += "(";
+  args.values()[idx].AppendAsJSON(args.types()[idx], out);
+  *out += ")";
+}
+
+}  // namespace
+
+void StringStorage::Reset(size_t alloc_size) {
+  if (!alloc_size) {
+    if (data_)
+      ::free(data_);
+    data_ = nullptr;
+  } else if (!data_ || alloc_size != data_->size) {
+    data_ = static_cast<Data*>(::realloc(data_, sizeof(size_t) + alloc_size));
+    data_->size = alloc_size;
+  }
+}
+
+bool StringStorage::Contains(const TraceArguments& args) const {
+  for (size_t n = 0; n < args.size(); ++n) {
+    if (args.types()[n] == TRACE_VALUE_TYPE_COPY_STRING &&
+        !Contains(args.values()[n].as_string)) {
+      return false;
+    }
+  }
+  return true;
+}
+
+static_assert(
+    std::is_pod<TraceValue>::value,
+    "TraceValue must be plain-old-data type for performance reasons!");
+
+void TraceValue::AppendAsJSON(unsigned char type, std::string* out) const {
+  switch (type) {
+    case TRACE_VALUE_TYPE_BOOL:
+      *out += this->as_bool ? "true" : "false";
+      break;
+    case TRACE_VALUE_TYPE_UINT:
+      StringAppendF(out, "%" PRIu64, static_cast<uint64_t>(this->as_uint));
+      break;
+    case TRACE_VALUE_TYPE_INT:
+      StringAppendF(out, "%" PRId64, static_cast<int64_t>(this->as_int));
+      break;
+    case TRACE_VALUE_TYPE_DOUBLE:
+      AppendDoubleAsJSON(this->as_double, out);
+      break;
+    case TRACE_VALUE_TYPE_POINTER:
+      // JSON only supports double and int numbers.
+      // So as not to lose bits from a 64-bit pointer, output as a hex string.
+      StringAppendF(
+          out, "\"0x%" PRIx64 "\"",
+          static_cast<uint64_t>(reinterpret_cast<uintptr_t>(this->as_pointer)));
+      break;
+    case TRACE_VALUE_TYPE_STRING:
+    case TRACE_VALUE_TYPE_COPY_STRING:
+      EscapeJSONString(this->as_string ? this->as_string : "NULL", true, out);
+      break;
+    case TRACE_VALUE_TYPE_CONVERTABLE:
+      this->as_convertable->AppendAsTraceFormat(out);
+      break;
+    default:
+      NOTREACHED() << "Don't know how to print this value";
+      break;
+  }
+}
+
+TraceArguments& TraceArguments::operator=(TraceArguments&& other) noexcept {
+  if (this != &other) {
+    this->~TraceArguments();
+    new (this) TraceArguments(std::move(other));
+  }
+  return *this;
+}
+
+TraceArguments::TraceArguments(int num_args,
+                               const char* const* arg_names,
+                               const unsigned char* arg_types,
+                               const unsigned long long* arg_values) {
+  if (num_args > static_cast<int>(kMaxSize))
+    num_args = static_cast<int>(kMaxSize);
+
+  size_ = static_cast<unsigned char>(num_args);
+  for (size_t n = 0; n < size_; ++n) {
+    types_[n] = arg_types[n];
+    names_[n] = arg_names[n];
+    values_[n].as_uint = arg_values[n];
+  }
+}
+
+void TraceArguments::Reset() {
+  for (size_t n = 0; n < size_; ++n) {
+    if (types_[n] == TRACE_VALUE_TYPE_CONVERTABLE)
+      delete values_[n].as_convertable;
+  }
+  size_ = 0;
+}
+
+void TraceArguments::CopyStringsTo(StringStorage* storage,
+                                   bool copy_all_strings,
+                                   const char** extra_string1,
+                                   const char** extra_string2) {
+  // First, compute total allocation size.
+  size_t alloc_size = 0;
+
+  if (copy_all_strings) {
+    alloc_size +=
+        GetAllocLength(*extra_string1) + GetAllocLength(*extra_string2);
+    for (size_t n = 0; n < size_; ++n)
+      alloc_size += GetAllocLength(names_[n]);
+  }
+  for (size_t n = 0; n < size_; ++n) {
+    if (copy_all_strings && types_[n] == TRACE_VALUE_TYPE_STRING)
+      types_[n] = TRACE_VALUE_TYPE_COPY_STRING;
+    if (types_[n] == TRACE_VALUE_TYPE_COPY_STRING)
+      alloc_size += GetAllocLength(values_[n].as_string);
+  }
+
+  if (alloc_size) {
+    storage->Reset(alloc_size);
+    char* ptr = storage->data();
+    const char* end = ptr + alloc_size;
+    if (copy_all_strings) {
+      CopyTraceEventParameter(&ptr, extra_string1, end);
+      CopyTraceEventParameter(&ptr, extra_string2, end);
+      for (size_t n = 0; n < size_; ++n)
+        CopyTraceEventParameter(&ptr, &names_[n], end);
+    }
+    for (size_t n = 0; n < size_; ++n) {
+      if (types_[n] == TRACE_VALUE_TYPE_COPY_STRING)
+        CopyTraceEventParameter(&ptr, &values_[n].as_string, end);
+    }
+#if DCHECK_IS_ON()
+    DCHECK_EQ(end, ptr) << "Overrun by " << ptr - end;
+    if (copy_all_strings) {
+      if (extra_string1 && *extra_string1)
+        DCHECK(storage->Contains(*extra_string1));
+      if (extra_string2 && *extra_string2)
+        DCHECK(storage->Contains(*extra_string2));
+      for (size_t n = 0; n < size_; ++n)
+        DCHECK(storage->Contains(names_[n]));
+    }
+    for (size_t n = 0; n < size_; ++n) {
+      if (types_[n] == TRACE_VALUE_TYPE_COPY_STRING)
+        DCHECK(storage->Contains(values_[n].as_string));
+    }
+#endif  // DCHECK_IS_ON()
+  } else {
+    storage->Reset();
+  }
+}
+
+void TraceArguments::AppendDebugString(std::string* out) {
+  *out += "TraceArguments(";
+  for (size_t n = 0; n < size_; ++n) {
+    if (n > 0)
+      *out += ", ";
+    AppendValueDebugString(*this, n, out);
+  }
+  *out += ")";
+}
+
+}  // namespace trace_event
+}  // namespace base

base/trace_event/trace_event_android.cc

@@ -39,32 +39,25 @@ void WriteToATrace(int fd, const char* buffer, size_t size) {
   }
 }
 
-void WriteEvent(
-    char phase,
-    const char* category_group,
-    const char* name,
-    unsigned long long id,
-    const char** arg_names,
-    const unsigned char* arg_types,
-    const TraceEvent::TraceValue* arg_values,
-    const std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-    unsigned int flags) {
+void WriteEvent(char phase,
+                const char* category_group,
+                const char* name,
+                unsigned long long id,
+                const TraceArguments& args,
+                unsigned int flags) {
   std::string out = StringPrintf("%c|%d|%s", phase, getpid(), name);
   if (flags & TRACE_EVENT_FLAG_HAS_ID)
     StringAppendF(&out, "-%" PRIx64, static_cast<uint64_t>(id));
   out += '|';
 
-  for (int i = 0; i < kTraceMaxNumArgs && arg_names[i];
-       ++i) {
+  const char* const* arg_names = args.names();
+  for (size_t i = 0; i < args.size() && arg_names[i]; ++i) {
     if (i)
       out += ';';
     out += arg_names[i];
     out += '=';
     std::string::size_type value_start = out.length();
-    if (arg_types[i] == TRACE_VALUE_TYPE_CONVERTABLE)
-      convertable_values[i]->AppendAsTraceFormat(&out);
-    else
-      TraceEvent::AppendValueAsJSON(arg_types[i], arg_values[i], &out);
+    args.values()[i].AppendAsJSON(args.types()[i], &out);
 
     // Remove the quotes which may confuse the atrace script.
     ReplaceSubstringsAfterOffset(&out, value_start, "\\\"", "'");
@@ -147,43 +140,35 @@ void TraceEvent::SendToATrace() {
 
   switch (phase_) {
     case TRACE_EVENT_PHASE_BEGIN:
-      WriteEvent('B', category_group, name_, id_,
-                 arg_names_, arg_types_, arg_values_, convertable_values_,
-                 flags_);
+      WriteEvent('B', category_group, name_, id_, args_, flags_);
       break;
 
     case TRACE_EVENT_PHASE_COMPLETE:
-      WriteEvent(duration_.ToInternalValue() == -1 ? 'B' : 'E',
-                 category_group, name_, id_,
-                 arg_names_, arg_types_, arg_values_, convertable_values_,
-                 flags_);
+      WriteEvent(duration_.ToInternalValue() == -1 ? 'B' : 'E', category_group,
+                 name_, id_, args_, flags_);
       break;
 
     case TRACE_EVENT_PHASE_END:
       // Though a single 'E' is enough, here append pid, name and
       // category_group etc. So that unpaired events can be found easily.
-      WriteEvent('E', category_group, name_, id_,
-                 arg_names_, arg_types_, arg_values_, convertable_values_,
-                 flags_);
+      WriteEvent('E', category_group, name_, id_, args_, flags_);
       break;
 
     case TRACE_EVENT_PHASE_INSTANT:
       // Simulate an instance event with a pair of begin/end events.
-      WriteEvent('B', category_group, name_, id_,
-                 arg_names_, arg_types_, arg_values_, convertable_values_,
-                 flags_);
+      WriteEvent('B', category_group, name_, id_, args_, flags_);
       WriteToATrace(g_atrace_fd, "E", 1);
       break;
 
     case TRACE_EVENT_PHASE_COUNTER:
-      for (int i = 0; i < kTraceMaxNumArgs && arg_names_[i]; ++i) {
-        DCHECK(arg_types_[i] == TRACE_VALUE_TYPE_INT);
-        std::string out = base::StringPrintf(
-            "C|%d|%s-%s", getpid(), name_, arg_names_[i]);
+      for (size_t i = 0; i < arg_size() && arg_name(i); ++i) {
+        DCHECK(arg_type(i) == TRACE_VALUE_TYPE_INT);
+        std::string out =
+            base::StringPrintf("C|%d|%s-%s", getpid(), name_, arg_name(i));
         if (flags_ & TRACE_EVENT_FLAG_HAS_ID)
           StringAppendF(&out, "-%" PRIx64, static_cast<uint64_t>(id_));
-        StringAppendF(&out, "|%d|%s",
-                      static_cast<int>(arg_values_[i].as_int), category_group);
+        StringAppendF(&out, "|%d|%s", static_cast<int>(arg_value(i).as_int),
+                      category_group);
         WriteToATrace(g_atrace_fd, out.c_str(), out.size());
       }
       break;

base/trace_event/trace_event_etw_export_win.cc

@@ -168,16 +168,11 @@ bool TraceEventETWExport::IsETWExportEnabled() {
 }
 
 // static
-void TraceEventETWExport::AddEvent(
-    char phase,
-    const unsigned char* category_group_enabled,
-    const char* name,
-    unsigned long long id,
-    int num_args,
-    const char* const* arg_names,
-    const unsigned char* arg_types,
-    const unsigned long long* arg_values,
-    const std::unique_ptr<ConvertableToTraceFormat>* convertable_values) {
+void TraceEventETWExport::AddEvent(char phase,
+                                   const unsigned char* category_group_enabled,
+                                   const char* name,
+                                   unsigned long long id,
+                                   const TraceArguments* args) {
   // We bail early in case exporting is disabled or no consumer is listening.
   auto* instance = GetInstance();
   if (!instance || !instance->etw_export_enabled_ || !EventEnabledChromeEvent())
@@ -255,26 +250,22 @@ void TraceEventETWExport::AddEvent(
   }
 
   std::string arg_values_string[3];
-  for (int i = 0; i < num_args; i++) {
-    if (arg_types[i] == TRACE_VALUE_TYPE_CONVERTABLE) {
+  size_t num_args = args ? args->size() : 0;
+  for (size_t i = 0; i < num_args; i++) {
+    if (args->types()[i] == TRACE_VALUE_TYPE_CONVERTABLE) {
       // Temporarily do nothing here. This function consumes 1/3 to 1/2 of
       // *total* process CPU time when ETW tracing, and many of the strings
       // created exceed WPA's 4094 byte limit and are shown as:
       // "Unable to parse data". See crbug.com/488257
-      // convertable_values[i]->AppendAsTraceFormat(arg_values_string + i);
     } else {
-      TraceEvent::TraceValue trace_event;
-      trace_event.as_uint = arg_values[i];
-      TraceEvent::AppendValueAsJSON(arg_types[i], trace_event,
-                                    arg_values_string + i);
+      args->values()[i].AppendAsJSON(args->types()[i], arg_values_string + i);
     }
   }
 
   EventWriteChromeEvent(
-      name, phase_string, num_args > 0 ? arg_names[0] : "",
-      arg_values_string[0].c_str(), num_args > 1 ? arg_names[1] : "",
-      arg_values_string[1].c_str(), num_args > 2 ? arg_names[2] : "",
-      arg_values_string[2].c_str());
+      name, phase_string, num_args > 0 ? args->names()[0] : "",
+      arg_values_string[0].c_str(), num_args > 1 ? args->names()[1] : "",
+      arg_values_string[1].c_str(), "", "");
 }
 
 // static

base/trace_event/trace_event_etw_export_win.h

@@ -47,16 +47,11 @@ class BASE_EXPORT TraceEventETWExport {
 
   // Exports an event to ETW. This is mainly used in
   // TraceLog::AddTraceEventWithThreadIdAndTimestamp to export internal events.
-  static void AddEvent(
-      char phase,
-      const unsigned char* category_group_enabled,
-      const char* name,
-      unsigned long long id,
-      int num_args,
-      const char* const* arg_names,
-      const unsigned char* arg_types,
-      const unsigned long long* arg_values,
-      const std::unique_ptr<ConvertableToTraceFormat>* convertable_values);
+  static void AddEvent(char phase,
+                       const unsigned char* category_group_enabled,
+                       const char* name,
+                       unsigned long long id,
+                       const TraceArguments* args);
 
   // Exports an ETW event that marks the end of a complete event.
   static void AddCompleteEndEvent(const char* name);

base/trace_event/trace_event_impl.h

@@ -24,6 +24,7 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_local.h"
 #include "base/trace_event/common/trace_event_common.h"
+#include "base/trace_event/trace_arguments.h"
 #include "base/trace_event/trace_event_memory_overhead.h"
 #include "build/build_config.h"
 
@@ -37,48 +38,6 @@ typedef base::Callback<bool(const char* category_group_name,
                             ArgumentNameFilterPredicate*)>
     ArgumentFilterPredicate;
 
-// For any argument of type TRACE_VALUE_TYPE_CONVERTABLE the provided
-// class must implement this interface.
-class BASE_EXPORT ConvertableToTraceFormat {
- public:
-  ConvertableToTraceFormat() = default;
-  virtual ~ConvertableToTraceFormat() = default;
-
-  // Append the class info to the provided |out| string. The appended
-  // data must be a valid JSON object. Strings must be properly quoted, and
-  // escaped. There is no processing applied to the content after it is
-  // appended.
-  virtual void AppendAsTraceFormat(std::string* out) const = 0;
-
-  // Append the class info directly into the Perfetto-defined proto
-  // format; this is attempted first and if this returns true,
-  // AppendAsTraceFormat is not called. The ProtoAppender interface
-  // acts as a bridge to avoid proto/Perfetto dependencies in base.
-  class BASE_EXPORT ProtoAppender {
-   public:
-    virtual ~ProtoAppender() = default;
-
-    virtual void AddBuffer(uint8_t* begin, uint8_t* end) = 0;
-    // Copy all of the previous buffers registered with AddBuffer
-    // into the proto, with the given |field_id|.
-    virtual size_t Finalize(uint32_t field_id) = 0;
-  };
-  virtual bool AppendToProto(ProtoAppender* appender);
-
-  virtual void EstimateTraceMemoryOverhead(TraceEventMemoryOverhead* overhead);
-
-  std::string ToString() const {
-    std::string result;
-    AppendAsTraceFormat(&result);
-    return result;
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ConvertableToTraceFormat);
-};
-
-const int kTraceMaxNumArgs = 2;
-
 struct TraceEventHandle {
   uint32_t chunk_seq;
   // These numbers of bits must be kept consistent with
@@ -90,14 +49,8 @@ struct TraceEventHandle {
 
 class BASE_EXPORT TraceEvent {
  public:
-  union TraceValue {
-    bool as_bool;
-    unsigned long long as_uint;
-    long long as_int;
-    double as_double;
-    const void* as_pointer;
-    const char* as_string;
-  };
+  // TODO(898794): Remove once all users have been updated.
+  using TraceValue = base::trace_event::TraceValue;
 
   TraceEvent();
 
@@ -110,11 +63,7 @@ class BASE_EXPORT TraceEvent {
              const char* scope,
              unsigned long long id,
              unsigned long long bind_id,
-             int num_args,
-             const char* const* arg_names,
-             const unsigned char* arg_types,
-             const unsigned long long* arg_values,
-             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+             TraceArguments* args,
              unsigned int flags);
 
   ~TraceEvent();
@@ -142,11 +91,7 @@ class BASE_EXPORT TraceEvent {
              const char* scope,
              unsigned long long id,
              unsigned long long bind_id,
-             int num_args,
-             const char* const* arg_names,
-             const unsigned char* arg_types,
-             const unsigned long long* arg_values,
-             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+             TraceArguments* args,
              unsigned int flags);
 
   void UpdateDuration(const TimeTicks& now, const ThreadTicks& thread_now);
@@ -159,9 +104,12 @@ class BASE_EXPORT TraceEvent {
       const ArgumentFilterPredicate& argument_filter_predicate) const;
   void AppendPrettyPrinted(std::ostringstream* out) const;
 
+  // TODO(898794): Remove once caller has been updated.
   static void AppendValueAsJSON(unsigned char type,
                                 TraceValue value,
-                                std::string* out);
+                                std::string* out) {
+    value.AppendAsJSON(type, out);
+  }
 
   TimeTicks timestamp() const { return timestamp_; }
   ThreadTicks thread_timestamp() const { return thread_timestamp_; }
@@ -176,8 +124,8 @@ class BASE_EXPORT TraceEvent {
   unsigned long long bind_id() const { return bind_id_; }
   // Exposed for unittesting:
 
-  const std::string* parameter_copy_storage() const {
-    return parameter_copy_storage_.get();
+  const StringStorage& parameter_copy_storage() const {
+    return parameter_copy_storage_;
   }
 
   const unsigned char* category_group_enabled() const {
@@ -186,12 +134,17 @@ class BASE_EXPORT TraceEvent {
 
   const char* name() const { return name_; }
 
-  unsigned char arg_type(size_t index) const { return arg_types_[index]; }
-  const char* arg_name(size_t index) const { return arg_names_[index]; }
-  const TraceValue& arg_value(size_t index) const { return arg_values_[index]; }
+  size_t arg_size() const { return args_.size(); }
+  unsigned char arg_type(size_t index) const { return args_.types()[index]; }
+  const char* arg_name(size_t index) const { return args_.names()[index]; }
+  const TraceValue& arg_value(size_t index) const {
+    return args_.values()[index];
+  }
 
   ConvertableToTraceFormat* arg_convertible_value(size_t index) {
-    return convertable_values_[index].get();
+    return (arg_type(index) == TRACE_VALUE_TYPE_CONVERTABLE)
+               ? arg_value(index).as_convertable
+               : nullptr;
   }
 
 #if defined(OS_ANDROID)
@@ -199,12 +152,7 @@ class BASE_EXPORT TraceEvent {
 #endif
 
  private:
-  void InitArgs(int num_args,
-                const char* const* arg_names,
-                const unsigned char* arg_types,
-                const unsigned long long* arg_values,
-                std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-                unsigned int flags);
+  void InitArgs(TraceArguments* args);
 
   // Note: these are ordered by size (largest first) for optimal packing.
   TimeTicks timestamp_ = TimeTicks();
@@ -218,13 +166,10 @@ class BASE_EXPORT TraceEvent {
   // The equivalence is checked with a static_assert() in trace_event_impl.cc.
   const char* scope_ = nullptr;
   unsigned long long id_ = 0u;
-  TraceValue arg_values_[kTraceMaxNumArgs];
-  const char* arg_names_[kTraceMaxNumArgs];
-  std::unique_ptr<ConvertableToTraceFormat>
-      convertable_values_[kTraceMaxNumArgs];
   const unsigned char* category_group_enabled_ = nullptr;
   const char* name_ = nullptr;
-  std::unique_ptr<std::string> parameter_copy_storage_;
+  StringStorage parameter_copy_storage_;
+  TraceArguments args_;
   // Depending on TRACE_EVENT_FLAG_HAS_PROCESS_ID the event will have either:
   //  tid: thread_id_, pid: current_process_id (default case).
   //  tid: -1, pid: process_id_ (when flags_ & TRACE_EVENT_FLAG_HAS_PROCESS_ID).
@@ -234,7 +179,6 @@ class BASE_EXPORT TraceEvent {
   };
   unsigned int flags_ = 0;
   unsigned long long bind_id_ = 0;
-  unsigned char arg_types_[kTraceMaxNumArgs];
   char phase_ = TRACE_EVENT_PHASE_BEGIN;
 
   DISALLOW_COPY_AND_ASSIGN(TraceEvent);

base/trace_event/trace_event_unittest.cc

@@ -1698,10 +1698,10 @@ TEST_F(TraceEventTestFixture, StaticStringVsString) {
     ASSERT_TRUE(event2);
     EXPECT_STREQ("name1", event1->name());
     EXPECT_STREQ("name2", event2->name());
-    EXPECT_TRUE(event1->parameter_copy_storage() != nullptr);
-    EXPECT_TRUE(event2->parameter_copy_storage() != nullptr);
-    EXPECT_GT(event1->parameter_copy_storage()->size(), 0u);
-    EXPECT_GT(event2->parameter_copy_storage()->size(), 0u);
+    EXPECT_FALSE(event1->parameter_copy_storage().empty());
+    EXPECT_FALSE(event2->parameter_copy_storage().empty());
+    EXPECT_GT(event1->parameter_copy_storage().size(), 0u);
+    EXPECT_GT(event2->parameter_copy_storage().size(), 0u);
     EndTraceAndFlush();
   }
 
@@ -1731,8 +1731,8 @@ TEST_F(TraceEventTestFixture, StaticStringVsString) {
     ASSERT_TRUE(event2);
     EXPECT_STREQ("name1", event1->name());
     EXPECT_STREQ("name2", event2->name());
-    EXPECT_TRUE(event1->parameter_copy_storage() == nullptr);
-    EXPECT_TRUE(event2->parameter_copy_storage() == nullptr);
+    EXPECT_TRUE(event1->parameter_copy_storage().empty());
+    EXPECT_TRUE(event2->parameter_copy_storage().empty());
     EndTraceAndFlush();
   }
 }

base/trace_event/trace_log.h

@@ -220,6 +220,58 @@ class BASE_EXPORT TraceLog : public MemoryDumpProvider {
   // Called by TRACE_EVENT* macros, don't call this directly.
   // 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.
+  TraceEventHandle AddTraceEvent(char phase,
+                                 const unsigned char* category_group_enabled,
+                                 const char* name,
+                                 const char* scope,
+                                 unsigned long long id,
+                                 TraceArguments* args,
+                                 unsigned int flags);
+  TraceEventHandle AddTraceEventWithBindId(
+      char phase,
+      const unsigned char* category_group_enabled,
+      const char* name,
+      const char* scope,
+      unsigned long long id,
+      unsigned long long bind_id,
+      TraceArguments* args,
+      unsigned int flags);
+  TraceEventHandle AddTraceEventWithProcessId(
+      char phase,
+      const unsigned char* category_group_enabled,
+      const char* name,
+      const char* scope,
+      unsigned long long id,
+      int process_id,
+      TraceArguments* args,
+      unsigned int flags);
+  TraceEventHandle AddTraceEventWithThreadIdAndTimestamp(
+      char phase,
+      const unsigned char* category_group_enabled,
+      const char* name,
+      const char* scope,
+      unsigned long long id,
+      int thread_id,
+      const TimeTicks& timestamp,
+      TraceArguments* args,
+      unsigned int flags);
+  TraceEventHandle AddTraceEventWithThreadIdAndTimestamp(
+      char phase,
+      const unsigned char* category_group_enabled,
+      const char* name,
+      const char* scope,
+      unsigned long long id,
+      unsigned long long bind_id,
+      int thread_id,
+      const TimeTicks& timestamp,
+      TraceArguments* args,
+      unsigned int flags);
+
+  // Adds a metadata event that will be written when the trace log is flushed.
+  void AddMetadataEvent(const unsigned char* category_group_enabled,
+                        const char* name,
+                        TraceArguments* args,
+                        unsigned int flags);
 
   // TODO(898794): Remove methods below when all callers have been updated.
   TraceEventHandle AddTraceEvent(

services/tracing/public/cpp/perfetto/trace_event_data_source.cc

@@ -229,9 +229,10 @@ class TraceEventDataSource::ThreadLocalEventSink {
 
     EnsureValidHandles();
 
-    int name_index = 0;
-    int category_name_index = 0;
-    int arg_name_indices[base::trace_event::kTraceMaxNumArgs] = {0};
+    uint32_t name_index = 0;
+    uint32_t category_name_index = 0;
+    const size_t kMaxSize = base::trace_event::TraceArguments::kMaxSize;
+    uint32_t arg_name_indices[kMaxSize] = {0};
 
     // Populate any new string table parts first; has to be done before
     // the add_trace_events() call (as the string table is part of the outer
@@ -247,9 +248,8 @@ class TraceEventDataSource::ThreadLocalEventSink {
           GetStringTableIndexForString(TraceLog::GetCategoryGroupName(
               trace_event->category_group_enabled()));
 
-      for (int i = 0;
-           i < base::trace_event::kTraceMaxNumArgs && trace_event->arg_name(i);
-           ++i) {
+      for (size_t i = 0;
+           i < trace_event->arg_size() && trace_event->arg_name(i); ++i) {
         arg_name_indices[i] =
             GetStringTableIndexForString(trace_event->arg_name(i));
       }
@@ -293,8 +293,7 @@ class TraceEventDataSource::ThreadLocalEventSink {
     char phase = trace_event->phase();
     new_trace_event->set_phase(phase);
 
-    for (int i = 0;
-         i < base::trace_event::kTraceMaxNumArgs && trace_event->arg_name(i);
+    for (size_t i = 0; i < trace_event->arg_size() && trace_event->arg_name(i);
          ++i) {
       auto type = trace_event->arg_type(i);
       auto* new_arg = new_trace_event->add_args();

third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h

@@ -11,19 +11,14 @@
 #include "third_party/blink/renderer/platform/wtf/text/cstring.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
-namespace WTF {
-
-// CString version of SetTraceValue so that trace arguments can be strings.
-static inline void SetTraceValue(const CString& arg,
-                                 unsigned char* type,
-                                 unsigned long long* value) {
-  trace_event_internal::TraceValueUnion type_value;
-  type_value.as_string = arg.data();
-  *type = TRACE_VALUE_TYPE_COPY_STRING;
-  *value = type_value.as_uint;
-}
-
-}  // namespace WTF
+// Conversion from CString to TraceValue so that trace arguments can be strings.
+template <>
+struct base::trace_event::TraceValue::Helper<WTF::CString> {
+  static constexpr unsigned char kType = TRACE_VALUE_TYPE_COPY_STRING;
+  static inline void SetValue(TraceValue* v, const WTF::CString& value) {
+    v->as_string = value.data();
+  }
+};
 
 namespace blink {
 namespace trace_event {