Reland: TracedValue initializer list - containers

Implement initializer list support for creating arrays and dictionaries
in TracedValue::Build. Thus speed up adding debugging tracing of
multiple parameters, including container types.

Reland with fix the problem of: crrev.com/c/2346286 by keeping a copy of
a std::string instead of just keeping a reference to it.

Bug: 1043616
Change-Id: I731347c24eff278f6e0c47887267de4d280b654a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362657Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Karel Král <karelkral@google.com>
Cr-Commit-Position: refs/heads/master@{#800170}

base/trace_event/traced_value.cc

@@ -597,6 +597,11 @@ void TracedValue::AppendString(base::StringPiece value) {
   writer_->AppendString(value);
 }
 
+void TracedValue::AppendPointer(void* value) {
+  DCHECK_CURRENT_CONTAINER_IS(kStackTypeArray);
+  writer_->AppendString(PointerToString(value));
+}
+
 void TracedValue::BeginArray() {
   DCHECK_CURRENT_CONTAINER_IS(kStackTypeArray);
   DEBUG_PUSH_CONTAINER(kStackTypeArray);
@@ -642,71 +647,218 @@ void TracedValue::EstimateTraceMemoryOverhead(
   writer_->EstimateTraceMemoryOverhead(overhead);
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name, int value) {
-  name_ = name;
+TracedValue::Array::Array(const std::initializer_list<ArrayItem> items) {
+  items_ = std::move(items);
+}
+
+TracedValue::Array::Array(TracedValue::Array&& other) {
+  items_ = std::move(other.items_);
+}
+
+void TracedValue::Array::WriteToValue(TracedValue* value) const {
+  for (const auto& item : items_) {
+    item.WriteToValue(value);
+  }
+}
+
+TracedValue::Dictionary::Dictionary(
+    const std::initializer_list<DictionaryItem> items) {
+  items_ = items;
+}
+
+TracedValue::Dictionary::Dictionary(TracedValue::Dictionary&& other) {
+  items_ = std::move(other.items_);
+}
+
+void TracedValue::Dictionary::WriteToValue(TracedValue* value) const {
+  for (const auto& item : items_) {
+    item.WriteToValue(value);
+  }
+}
+
+TracedValue::ValueHolder::ValueHolder(int value) {
   kept_value_.int_value = value;
   kept_value_type_ = KeptValueType::kIntType;
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name, double value) {
-  name_ = name;
+TracedValue::ValueHolder::ValueHolder(double value) {
   kept_value_.double_value = value;
   kept_value_type_ = KeptValueType::kDoubleType;
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name, bool value) {
-  name_ = name;
+TracedValue::ValueHolder::ValueHolder(bool value) {
   kept_value_.bool_value = value;
   kept_value_type_ = KeptValueType::kBoolType;
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name,
-                                            base::StringPiece value) {
-  name_ = name;
+TracedValue::ValueHolder::ValueHolder(base::StringPiece value) {
   kept_value_.string_piece_value = value;
   kept_value_type_ = KeptValueType::kStringPieceType;
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name, void* value) {
-  name_ = name;
+TracedValue::ValueHolder::ValueHolder(std::string value) {
+  new (&kept_value_.std_string_value) std::string(std::move(value));
+  kept_value_type_ = KeptValueType::kStdStringType;
+}
+
+TracedValue::ValueHolder::ValueHolder(void* value) {
   kept_value_.void_ptr_value = value;
   kept_value_type_ = KeptValueType::kVoidPtrType;
 }
 
-TracedValue::DictionaryItem::DictionaryItem(const char* name,
-                                            const char* value) {
-  name_ = name;
+TracedValue::ValueHolder::ValueHolder(const char* value) {
   kept_value_.string_piece_value = value;
   kept_value_type_ = KeptValueType::kStringPieceType;
 }
 
-void TracedValue::DictionaryItem::WriteToValue(TracedValue* value) const {
+TracedValue::ValueHolder::ValueHolder(TracedValue::Dictionary& value) {
+  new (&kept_value_.dictionary_value) TracedValue::Dictionary(std::move(value));
+  kept_value_type_ = KeptValueType::kDictionaryType;
+}
+
+TracedValue::ValueHolder::ValueHolder(TracedValue::Array& value) {
+  new (&kept_value_.array_value) TracedValue::Array(std::move(value));
+  kept_value_type_ = KeptValueType::kArrayType;
+}
+
+TracedValue::ValueHolder::ValueHolder(TracedValue::ValueHolder&& other) {
+  // Remember to call a destructor if necessary.
+  if (kept_value_type_ == KeptValueType::kStdStringType) {
+    delete (&kept_value_.std_string_value);
+  }
+  switch (other.kept_value_type_) {
+    case KeptValueType::kIntType: {
+      kept_value_.int_value = other.kept_value_.int_value;
+      break;
+    }
+    case KeptValueType::kDoubleType: {
+      kept_value_.double_value = other.kept_value_.double_value;
+      break;
+    }
+    case KeptValueType::kBoolType: {
+      kept_value_.bool_value = other.kept_value_.bool_value;
+      break;
+    }
+    case KeptValueType::kStringPieceType: {
+      kept_value_.string_piece_value = other.kept_value_.string_piece_value;
+      break;
+    }
+    case KeptValueType::kStdStringType: {
+      new (&kept_value_.std_string_value)
+          std::string(std::move(other.kept_value_.std_string_value));
+      break;
+    }
+    case KeptValueType::kVoidPtrType: {
+      kept_value_.void_ptr_value = other.kept_value_.void_ptr_value;
+      break;
+    }
+    case KeptValueType::kArrayType: {
+      new (&kept_value_.array_value)
+          TracedValue::Array(std::move(other.kept_value_.array_value));
+      break;
+    }
+    case KeptValueType::kDictionaryType: {
+      new (&kept_value_.dictionary_value) TracedValue::Dictionary(
+          std::move(other.kept_value_.dictionary_value));
+      break;
+    }
+  }
+  kept_value_type_ = other.kept_value_type_;
+}
+
+void TracedValue::ValueHolder::WriteToValue(TracedValue* value) const {
+  switch (kept_value_type_) {
+    case KeptValueType::kIntType: {
+      value->AppendInteger(kept_value_.int_value);
+      break;
+    }
+    case KeptValueType::kDoubleType: {
+      value->AppendDouble(kept_value_.double_value);
+      break;
+    }
+    case KeptValueType::kBoolType: {
+      value->AppendBoolean(kept_value_.bool_value);
+      break;
+    }
+    case KeptValueType::kStringPieceType: {
+      value->AppendString(kept_value_.string_piece_value);
+      break;
+    }
+    case KeptValueType::kStdStringType: {
+      value->AppendString(kept_value_.std_string_value);
+      break;
+    }
+    case KeptValueType::kVoidPtrType: {
+      value->AppendPointer(kept_value_.void_ptr_value);
+      break;
+    }
+    case KeptValueType::kArrayType: {
+      value->BeginArray();
+      kept_value_.array_value.WriteToValue(value);
+      value->EndArray();
+      break;
+    }
+    case KeptValueType::kDictionaryType: {
+      value->BeginDictionary();
+      kept_value_.dictionary_value.WriteToValue(value);
+      value->EndDictionary();
+      break;
+    }
+  }
+}
+
+void TracedValue::ValueHolder::WriteToValue(const char* name,
+                                            TracedValue* value) const {
   switch (kept_value_type_) {
     case KeptValueType::kIntType: {
-      value->SetInteger(name_, kept_value_.int_value);
+      value->SetInteger(name, kept_value_.int_value);
       break;
     }
     case KeptValueType::kDoubleType: {
-      value->SetDouble(name_, kept_value_.double_value);
+      value->SetDouble(name, kept_value_.double_value);
       break;
     }
     case KeptValueType::kBoolType: {
-      value->SetBoolean(name_, kept_value_.bool_value);
+      value->SetBoolean(name, kept_value_.bool_value);
       break;
     }
     case KeptValueType::kStringPieceType: {
-      value->SetString(name_, kept_value_.string_piece_value);
+      value->SetString(name, kept_value_.string_piece_value);
+      break;
+    }
+    case KeptValueType::kStdStringType: {
+      value->SetString(name, kept_value_.std_string_value);
       break;
     }
     case KeptValueType::kVoidPtrType: {
-      value->SetPointer(name_, kept_value_.void_ptr_value);
+      value->SetPointer(name, kept_value_.void_ptr_value);
+      break;
+    }
+    case KeptValueType::kArrayType: {
+      value->BeginArray(name);
+      kept_value_.array_value.WriteToValue(value);
+      value->EndArray();
+      break;
+    }
+    case KeptValueType::kDictionaryType: {
+      value->BeginDictionary(name);
+      kept_value_.dictionary_value.WriteToValue(value);
+      value->EndDictionary();
       break;
     }
   }
 }
 
+void TracedValue::ArrayItem::WriteToValue(TracedValue* value) const {
+  ValueHolder::WriteToValue(value);
+}
+
+void TracedValue::DictionaryItem::WriteToValue(TracedValue* value) const {
+  ValueHolder::WriteToValue(name_, value);
+}
+
 std::unique_ptr<TracedValue> TracedValue::Build(
-    std::initializer_list<DictionaryItem> items) {
+    const std::initializer_list<DictionaryItem> items) {
   std::unique_ptr<TracedValue> value(new TracedValue());
   for (const auto& item : items) {
     item.WriteToValue(value.get());

base/trace_event/traced_value.h

@@ -57,6 +57,7 @@ class BASE_EXPORT TracedValue : public ConvertableToTraceFormat {
   void AppendDouble(double);
   void AppendBoolean(bool);
   void AppendString(base::StringPiece);
+  void AppendPointer(void*);
   void BeginArray();
   void BeginDictionary();
 
@@ -66,57 +67,131 @@ class BASE_EXPORT TracedValue : public ConvertableToTraceFormat {
 
   void EstimateTraceMemoryOverhead(TraceEventMemoryOverhead* overhead) override;
 
-  // TracedValue::Build is a friend of TracedValue::DictionaryItem.
+  class BASE_EXPORT Array;
+  class BASE_EXPORT Dictionary;
+  class BASE_EXPORT ValueHolder;
+  class BASE_EXPORT ArrayItem;
   class BASE_EXPORT DictionaryItem;
 
-  // Helper to enable easier initialization of TracedValue. This is intended for
-  // quick local debugging as there is overhead of creating
-  // std::initializer_list of name-value objects. This method does not support
-  // creation of dictionaries or arrays.
+  // Helper to enable easier initialization of |TracedValue|. This is intended
+  // for quick local debugging as there is overhead of creating
+  // |std::initializer_list| of name-value objects (in the case of containers
+  // the value is also a |std::initializer_list|). Generally the helper types
+  // |TracedValue::Dictionary|, |TracedValue::Array|,
+  // |TracedValue::DictionaryItem|, |TracedValue::ArrayItem| must be valid as
+  // well as their internals (e.g., |base::StringPiece| data should be valid
+  // when |TracedValue::Build| is called; |TracedValue::Array| or
+  // |TracedValue::Dictionary| holds a |std::initializer_list| whose underlying
+  // array needs to be valid when calling |TracedValue::Build|).
   //
   // Example:
   //    auto value = TracedValue::Build({
   //      {"int_var_name", 42},
   //      {"double_var_name", 3.14},
-  //      {"string_var_name", "hello world"}
+  //      {"string_var_name", "hello world"},
+  //      {"empty_array", TracedValue::Array({})},
+  //      {"dictionary", TracedValue::Dictionary({
+  //        {"my_ptr", static_cast<void*>(my_ptr)},
+  //        {"nested_array", TracedValue::Array({1, false, 0.5})},
+  //      })},
   //    });
-  //
-  // |name| is assumed to be a long lived "quoted" string.
   static std::unique_ptr<TracedValue> Build(
-      std::initializer_list<DictionaryItem> items);
+      const std::initializer_list<DictionaryItem> items);
 
-  // DictionaryItem instance represents a single name-value pair.
-  class DictionaryItem {
+  // An |Array| instance represents an array of |ArrayItem| objects. This is a
+  // helper to allow initializer list like construction of arrays using
+  // |TracedValue::Build|.
+  //
+  // An instance holds an |std::initializer_list<TracedValue::ArrayItem>| and is
+  // cheap to copy (copying the initializer_list does not copy the underlying
+  // objects). The underlying array must exist at the time when
+  // |TracedValue::Build| is called.
+  class Array {
    public:
-    // These constructors assume that |name| is a long lived "quoted" string.
-    DictionaryItem(const char* name, int value);
-    DictionaryItem(const char* name, double value);
-    DictionaryItem(const char* name, bool value);
-    DictionaryItem(const char* name, void* value);
+    // This constructor expects that the initializer_list is valid when
+    // |TracedValue::Build| is called.
+    Array(const std::initializer_list<ArrayItem> items);
+    Array(Array&&);
+    void WriteToValue(TracedValue* value) const;
+
+   private:
+    std::initializer_list<ArrayItem> items_;
+  };
+
+  // A helper to hold a dictionary. Similar to |TracedValue::Array|.
+  class Dictionary {
+   public:
+    // This constructor expects that the initializer_list is valid when
+    // |TracedValue::Build| is called.
+    Dictionary(const std::initializer_list<DictionaryItem> items);
+    Dictionary(Dictionary&&);
+    void WriteToValue(TracedValue* value) const;
+
+   private:
+    std::initializer_list<DictionaryItem> items_;
+  };
+
+  // A |ValueHolder| holds a single value or a container (int, double... or an
+  // |Array| / |Dictionary|). Not to be used outside of the context of
+  // |TracedValue::Build| (has one parameter implicit constructors).
+  //
+  // Base class for |TracedValue::ArrayItem| and |TracedValue::DictionaryItem|.
+  class ValueHolder {
+   public:
+    // Implicit constructors allow constructing |DictionaryItem| without having
+    // to write |{"name", TracedValue::ValueHolder(1)}|.
+    ValueHolder(int value);     // NOLINT(google-explicit-constructor)
+    ValueHolder(double value);  // NOLINT(google-explicit-constructor)
+    ValueHolder(bool value);    // NOLINT(google-explicit-constructor)
+    ValueHolder(void* value);   // NOLINT(google-explicit-constructor)
     // StringPiece's backing storage / const char* pointer needs to remain valid
     // until TracedValue::Build is called.
-    DictionaryItem(const char* name, base::StringPiece value);
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    ValueHolder(base::StringPiece value);
+    // Create a copy to avoid holding a reference to a non-existing string:
+    //
+    // Example:
+    //   TracedValue::Build({{"my_string", std::string("std::string value")}});
+    // Explanation:
+    //   1. std::string temporary is passed to the constructor of |ValueHolder|.
+    //   2. |ValueHolder| is passed to the constructor of |DictionaryItem|.
+    //   3. |Build| iterates initializer_list of |DictionaryItems|.
+    //
+    //   If the original |ValueHolder| kept just a reference to the string (or
+    //   a |base::StringPiece|) then |Build| is undefined behaviour, as it is
+    //   passing a reference to an out-of-scope temporary to
+    //   |TracedValue::SetString|.
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    ValueHolder(std::string value);
     // Define an explicit overload for const char* to resolve the ambiguity
     // between the base::StringPiece, void*, and bool constructors for string
     // literals.
-    DictionaryItem(const char* name, const char* value);
-
-   private:
-    friend std::unique_ptr<TracedValue> TracedValue::Build(
-        std::initializer_list<DictionaryItem> items);
+    ValueHolder(const char* value);  // NOLINT(google-explicit-constructor)
+    ValueHolder(Array& value);       // NOLINT(google-explicit-constructor)
+    ValueHolder(Dictionary& value);  // NOLINT(google-explicit-constructor)
+    ValueHolder(ValueHolder&&);
 
+   protected:
     void WriteToValue(TracedValue* value) const;
+    void WriteToValue(const char* name, TracedValue* value) const;
 
+   private:
     union KeptValue {
+      // Copy is handled by the holder (based on
+      // |TracedValue::ValueHolder::kept_value_type_|).
       int int_value;
       double double_value;
       bool bool_value;
       base::StringPiece string_piece_value;
+      std::string std_string_value;
       void* void_ptr_value;
+      Array array_value;
+      Dictionary dictionary_value;
 
       // Default constructor is implicitly deleted because union field has a
       // non-trivial default constructor.
-      KeptValue() {}
+      KeptValue() {}   // NOLINT(modernize-use-equals-default)
+      ~KeptValue() {}  // NOLINT(modernize-use-equals-default)
     };
 
     // Reimplementing a subset of C++17 std::variant.
@@ -125,14 +200,45 @@ class BASE_EXPORT TracedValue : public ConvertableToTraceFormat {
       kDoubleType,
       kBoolType,
       kStringPieceType,
+      kStdStringType,
       kVoidPtrType,
+      kArrayType,
+      kDictionaryType,
     };
 
     KeptValue kept_value_;
-    const char* name_;
     KeptValueType kept_value_type_;
   };
 
+  // |ArrayItem| is a |ValueHolder| which can be used to construct an |Array|.
+  class ArrayItem : public ValueHolder {
+   public:
+    // Implicit constructors allow calling |TracedValue::Array({1, true, 3.14})|
+    // instead of |TracedValue::Array({TracedValue::ArrayItem(1),
+    // TracedValue::ArrayItem(true), TracedValue::ArrayItem(3.14)})|.
+    template <typename T>
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    ArrayItem(T value) : ValueHolder(value) {}
+
+    void WriteToValue(TracedValue* value) const;
+  };
+
+  // |DictionaryItem| instance represents a single name-value pair.
+  //
+  // |name| is assumed to be a long lived "quoted" string.
+  class DictionaryItem : public ValueHolder {
+   public:
+    // These constructors assume that |name| is a long lived "quoted" string.
+    template <typename T>
+    DictionaryItem(const char* name, T value)
+        : ValueHolder(value), name_(name) {}
+
+    void WriteToValue(TracedValue* value) const;
+
+   private:
+    const char* name_;
+  };
+
   // A custom serialization class can be supplied by implementing the
   // Writer interface and supplying a factory class to SetWriterFactoryCallback.
   // Primarily used by Perfetto to write TracedValues directly into its proto

base/trace_event/traced_value_unittest.cc

@@ -15,6 +15,31 @@
 namespace base {
 namespace trace_event {
 
+TEST(TraceEventArgumentTest, InitializerListCreatedContainers) {
+  std::string json;
+  TracedValue::Build(
+      {
+          {"empty_array", TracedValue::Array({})},
+          {"empty_dictionary", TracedValue::Dictionary({})},
+          {"nested_array", TracedValue::Array({
+                               TracedValue::Array({}),
+                               TracedValue::Dictionary({}),
+                               true,
+                           })},
+          {"nested_dictionary", TracedValue::Dictionary({
+                                    {"d", TracedValue::Dictionary({})},
+                                    {"a", TracedValue::Array({})},
+                                    {"b", true},
+                                })},
+      })
+      ->AppendAsTraceFormat(&json);
+  EXPECT_EQ(
+      "{\"empty_array\":[],\"empty_dictionary\":{},"
+      "\"nested_array\":[[],{},true],"
+      "\"nested_dictionary\":{\"d\":{},\"a\":[],\"b\":true}}",
+      json);
+}
+
 TEST(TraceEventArgumentTest, InitializerListCreatedFlatDictionary) {
   std::string json;
   TracedValue::Build({{"bool_var", true},
@@ -28,23 +53,34 @@ TEST(TraceEventArgumentTest, InitializerListCreatedFlatDictionary) {
       json);
 }
 
+std::string SayHello() {
+  // Create a string by concatenating two strings, so that there is no literal
+  // corresponding to the result.
+  return std::string("hello ") + std::string("world");
+}
+
 TEST(TraceEventArgumentTest, StringAndPointerConstructors) {
   std::string json;
   const char* const_char_ptr_var = "const char* value";
-  TracedValue::Build({
-                         {"literal_var", "literal"},
-                         {"std_string_var", std::string("std::string value")},
-                         {"base_string_piece_var",
-                          base::StringPiece("base::StringPiece value")},
-                         {"const_char_ptr_var", const_char_ptr_var},
-                         {"void_nullptr", static_cast<void*>(nullptr)},
-                         {"int_nullptr", static_cast<int*>(nullptr)},
-                         {"void_1234ptr", reinterpret_cast<void*>(0x1234)},
-                     })
+  TracedValue::Build(
+      {
+          {"literal_var", "literal"},
+          {"std_string_var", std::string("std::string value")},
+          {"string_from_function", SayHello()},
+          {"string_from_lambda", []() { return std::string("hello"); }()},
+          {"base_string_piece_var",
+           base::StringPiece("base::StringPiece value")},
+          {"const_char_ptr_var", const_char_ptr_var},
+          {"void_nullptr", static_cast<void*>(nullptr)},
+          {"int_nullptr", static_cast<int*>(nullptr)},
+          {"void_1234ptr", reinterpret_cast<void*>(0x1234)},
+      })
       ->AppendAsTraceFormat(&json);
   EXPECT_EQ(
       "{\"literal_var\":\"literal\","
       "\"std_string_var\":\"std::string value\","
+      "\"string_from_function\":\"hello world\","
+      "\"string_from_lambda\":\"hello\","
       "\"base_string_piece_var\":\"base::StringPiece value\","
       "\"const_char_ptr_var\":\"const char* value\","
       "\"void_nullptr\":\"0x0\","