Make TracedValue lower overhead.

base/debug/trace_event_value.cc

Index: base/debug/trace_event_value.cc
diff --git a/base/debug/trace_event_value.cc b/base/debug/trace_event_value.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1986318579293b39619bf56715f6b01e55f26863
--- /dev/null
+++ b/base/debug/trace_event_value.cc
@@ -0,0 +1,301 @@
+// Copyright (c) 2013 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/debug/trace_event_value.h"
+#include "base/json/json_writer.h"
+#include "base/json/string_escape.h"
+#include "base/strings/stringprintf.h"
+
+namespace base {
+namespace debug {
+
+TracedValue::TracedValue() {
+}
+
+TracedValue::~TracedValue() {
+}
+
+void TracedValue::AppendAsTraceFormat(std::string* out) const {
+  // Used to keep track of the opening opcode.
+  std::stack<Opcode> begin_opcode_stack;
+  // This is needed in case of nested structures.
+  std::stack<int> element_count_stack;
+  PickleIterator iter(pickle_);
+
+  int opcode;
+  bool read_succeeded = pickle_.ReadInt(&iter, &opcode);
+  while (read_succeeded) {
+    // Append comma if necessary.
+    if (!begin_opcode_stack.empty()) {
+      DCHECK(!element_count_stack.empty());
+      AppendComma(out, begin_opcode_stack.top(),
+          static_cast<Opcode>(opcode), &element_count_stack.top());
+    }
+
+    switch (opcode) {
+      case BEGIN_ARRAY:
+      case BEGIN_DICTIONARY:
+      {
+        // Initialize the count for the current datastructure.
+        element_count_stack.push(0);
+        begin_opcode_stack.push(static_cast<Opcode>(opcode));
+        StringAppendF(out, opcode == BEGIN_DICTIONARY ? "{" : "[");
+        break;
+      }
+      case END_DICTIONARY:
+      case END_ARRAY:
+      {
+        begin_opcode_stack.pop();
+        element_count_stack.pop();
+        StringAppendF(out, opcode == END_DICTIONARY ? "}" : "]");
+        break;
+      }
+      case KEY_AS_RAW_POINTER:
+      {
+        AppendKeyAsRawPointer(out, &iter);
+        break;
+      }
+      case VALUE_AS_RAW_POINTER:
+      {
+        AppendValueAsRawPointer(out, &iter);
+        break;
+      }
+      case VALUE_AS_STRING:
+      {
+        AppendValueAsString(out, &iter);
+        break;
+      }
+      case INT32_VALUE:
+      {
+        AppendValueAsInt32(out, &iter);
+        break;
+      }
+      case INT64_VALUE:
+      {
+        AppendValueAsInt64(out, &iter);
+        break;
+      }
+      case UINT32_VALUE:
+      {
+        AppendValueAsUInt32(out, &iter);
+        break;
+      }
+      case UINT64_VALUE:
+      {
+        AppendValueAsUInt64(out, &iter);
+        break;
+      }
+      case FLOAT_VALUE:
+      {
+        AppendValueAsFloat(out, &iter);
+        break;
+      }
+      case BOOL_VALUE:
+      {
+        AppendValueAsBool(out, &iter);
+        break;
+      }
+      default:
+        NOTREACHED() << "Don't know what to read, opcode=" << opcode;

[dsinclair, 2013/08/07 15:40:20]

nit: s/read/write

[rterrazas, 2013/08/08 06:21:30]

Done.

+    }
+    read_succeeded = pickle_.ReadInt(&iter, &opcode);
+  }
+  DCHECK(begin_opcode_stack.empty());
+}
+
+void TracedValue::BeginDictionary(const char* key_in_parent_dict) {
+  // Named dictionaries are properties of dictionaries, the name is the key.
+  DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top());
+  PushKeyToPickle(key_in_parent_dict);
+  begin_opcodes_.push(BEGIN_DICTIONARY);
+  pickle_.WriteInt(BEGIN_DICTIONARY);
+}
+
+void TracedValue::BeginDictionary() {
+  begin_opcodes_.push(BEGIN_DICTIONARY);

[dsinclair, 2013/08/07 15:40:20]

DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top())

Should we verify we aren't currently in a dictionary before doing this?

[rterrazas, 2013/08/08 06:21:30]

This would harm implementations of TracedObject, for example, if we had this
check, and an implementation of TracedObject along these lines:

PushInto(traced_obj) {
  traced_obj.BeginDictionary();
  traced_obj.Push("Foo", 12);
  traced_obj.EndDictionary();
}

We'd have a problem.

[dsinclair, 2013/08/08 14:07:28]

I think you're going to need a third type on your begin list. I should not be
able to do:

BeginDictionary()
BeginDictionary()
EndDictionary()
EndDictionary()

Without it raising some kind of DCHECK. In order to handle the PushInto case I
think you should probably push the PUSH_INTO onto your begin_opcodes_ list. When
you do a PushInto you don't have anything started, you're essentially starting
fresh.

We also don't want a user to accidentally do:

BeginDictionary()
PushInto() {
  EndDictionary)
}
EndDictionary()

Which, currently is possible.

+  pickle_.WriteInt(BEGIN_DICTIONARY);
+}
+
+void TracedValue::EndDictionary() {
+  DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top());
+  begin_opcodes_.pop();
+  pickle_.WriteInt(END_DICTIONARY);
+}
+
+void TracedValue::BeginArray(const char* key_in_parent_dict) {
+  // Named arrays are properties of dictionaries, and the key of such property
+  // is key_in_parent_dict.
+  DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top());
+  PushKeyToPickle(key_in_parent_dict);
+  begin_opcodes_.push(BEGIN_ARRAY);
+  pickle_.WriteInt(BEGIN_ARRAY);
+}
+
+void TracedValue::BeginArray() {
+  begin_opcodes_.push(BEGIN_ARRAY);

[dsinclair, 2013/08/07 15:40:20]

DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()?

[rterrazas, 2013/08/08 06:21:30]

Similarly to the dictionary case above, except the problem would arise at:
PushInto(traced_obj) {
  traced_obj.BeginArray();
...
}

+  pickle_.WriteInt(BEGIN_ARRAY);
+}
+
+void TracedValue::EndArray() {
+  DCHECK_EQ(BEGIN_ARRAY, begin_opcodes_.top());
+  begin_opcodes_.pop();
+
+  pickle_.WriteInt(END_ARRAY);
+}
+
+// These methods push stuff in the pickle either in dictionary or array
+// blocks, it's the up to the publicly exposed caller to check the state.

[dsinclair, 2013/08/07 15:40:20]

nit: s/publicly/publically

[rterrazas, 2013/08/08 06:21:30]

Done.

+void TracedValue::PushKeyToPickle(const char* key) {
+  // Check that we're writing a dictionary.
+  DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top());

[dsinclair, 2013/08/07 15:40:20]

The comment above says the caller checks the state, if so, why do we check the
state here?

[rterrazas, 2013/08/08 06:21:30]

Done.

+
+  pickle_.WriteInt(KEY_AS_RAW_POINTER);
+  uintptr_t key_ptr = reinterpret_cast<uintptr_t>(key);
+  pickle_.WriteUIntPtr(key_ptr);
+}
+
+void TracedValue::PushValueToPickle(const char* value) {
+  pickle_.WriteInt(VALUE_AS_RAW_POINTER);
+  uintptr_t value_ptr = reinterpret_cast<uintptr_t>(value);
+  pickle_.WriteUIntPtr(value_ptr);
+}
+
+void TracedValue::PushValueToPickle(const std::string& value) {
+  pickle_.WriteInt(VALUE_AS_STRING);
+  pickle_.WriteString(value);
+}
+
+void TracedValue::PushValueToPickle(int value) {
+  pickle_.WriteInt(INT32_VALUE);
+  pickle_.WriteInt(value);
+}
+
+void TracedValue::PushValueToPickle(int64 value) {
+  pickle_.WriteInt(INT64_VALUE);
+  pickle_.WriteInt64(value);
+}
+
+void TracedValue::PushValueToPickle(uint32 value) {
+  pickle_.WriteInt(UINT32_VALUE);
+  pickle_.WriteUInt32(value);
+}
+
+void TracedValue::PushValueToPickle(uint64 value) {
+  pickle_.WriteInt(UINT64_VALUE);
+  pickle_.WriteUInt64(value);
+}
+
+void TracedValue::PushValueToPickle(float value) {
+  pickle_.WriteInt(FLOAT_VALUE);
+  pickle_.WriteFloat(value);
+}
+
+void TracedValue::PushValueToPickle(bool value) {
+  pickle_.WriteInt(BOOL_VALUE);
+  pickle_.WriteBool(value);
+}
+
+void TracedValue::PushValueToPickle(const TracedObject& value) {
+  // The definition of what gets pushed is in the implementation of
+  // TracedObject::PushInto().
+  value.PushInto(this);
+}
+
+// Helper methods used in AppendAsTraceFormat()
+void TracedValue::AppendKeyAsRawPointer(std::string* out,
+    PickleIterator* iter) const {
+  uintptr_t out_ptr;
+  pickle_.ReadUIntPtr(iter, &out_ptr);
+  const char* key = reinterpret_cast<char*>(out_ptr);
+  StringAppendF(out, "\"%s\":", key);

[dsinclair, 2013/08/07 15:40:20]

Is it possible for key to have a " in it? If so, it needs to be escaped.

[rterrazas, 2013/08/08 06:21:30]

Done.

+}
+
+void TracedValue::AppendKeyAsString(std::string* out,
+    PickleIterator* iter) const {
+  std::string out_string;
+  pickle_.ReadString(iter, &out_string);
+  StringAppendF(out, "\"%s\":", out_string.c_str());

[dsinclair, 2013/08/07 15:40:20]

out_string.c_str() needs to escape any " characters.

[rterrazas, 2013/08/08 06:21:30]

Done, actually, I removed this one, wasn't being used, not sure why I added it
in the first place, we're heavily preferring statically allocated strings.

+}
+
+void TracedValue::AppendValueAsRawPointer(std::string* out,
+    PickleIterator* iter) const {
+  uintptr_t out_ptr;
+  pickle_.ReadUIntPtr(iter, &out_ptr);
+  const char* value = reinterpret_cast<char*>(out_ptr);
+  JsonDoubleQuote(std::string(value), true, out);
+}
+
+void TracedValue::AppendValueAsString(std::string* out,
+    PickleIterator* iter) const {
+  std::string out_string;
+  pickle_.ReadString(iter, &out_string);
+  JsonDoubleQuote(out_string, true, out);
+}
+
+void TracedValue::AppendValueAsInt32(std::string* out,
+    PickleIterator* iter) const {
+  int out_int;
+  pickle_.ReadInt(iter, &out_int);
+  StringAppendF(out, "%d", out_int);
+}
+
+void TracedValue::AppendValueAsInt64(std::string* out,
+    PickleIterator* iter) const {
+  int64 out_int64;
+  pickle_.ReadInt64(iter, &out_int64);
+  StringAppendF(out, "%" PRId64, out_int64);
+}
+
+
+void TracedValue::AppendValueAsUInt32(std::string* out,
+    PickleIterator* iter) const {
+  uint32 out_uint32;
+  pickle_.ReadUInt32(iter, &out_uint32);
+  StringAppendF(out, "%u", out_uint32);
+}
+
+void TracedValue::AppendValueAsUInt64(std::string* out,
+    PickleIterator* iter) const {
+  uint64 out_uint64;
+  pickle_.ReadUInt64(iter, &out_uint64);
+  StringAppendF(out, "%" PRIu64, out_uint64);
+}
+
+void TracedValue::AppendValueAsFloat(std::string* out,
+    PickleIterator* iter) const {
+  float out_float;
+  pickle_.ReadFloat(iter, &out_float);
+  out->append(base::JSONWriter::DoubleToFormattedString(out_float,
+      false));

[dsinclair, 2013/08/07 15:40:20]

nit: fits above.

[rterrazas, 2013/08/08 06:21:30]

Done.

+}
+
+void TracedValue::AppendValueAsBool(std::string* out,
+    PickleIterator* iter) const {
+  bool out_bool;
+  pickle_.ReadBool(iter, &out_bool);
+  StringAppendF(out, "%s", out_bool ? "true" : "false");
+}
+
+// Static.
+void TracedValue::AppendComma(std::string* out, Opcode begin_opcode,
+    Opcode current_opcode, int* element_count) {
+  // For arrays, we want to append a comma when the element count within
+  // the array is greater than zero and we are not about to close the array.
+  if (begin_opcode == BEGIN_ARRAY && current_opcode != END_ARRAY &&
+      (*element_count)++ > 0) {
+    StringAppendF(out, ",");
+    return;
+  }
+  // For dictionaries, we want to append a comma when the element count within
+  // the dictionary is greater than zero AND we're about to append is a key.
+  if (begin_opcode == BEGIN_DICTIONARY &&
+      current_opcode == KEY_AS_RAW_POINTER &&
+      (*element_count)++ > 0) {
+    StringAppendF(out, ",");
+  }
+}
+
+}  // namespace debug
+}  // namespace base
+