Make TracedValue lower overhead.

base/trace_event_value.cc

Index: base/trace_event_value.cc
diff --git a/base/trace_event_value.cc b/base/trace_event_value.cc
new file mode 100644
index 0000000000000000000000000000000000000000..656d61d568089db6914ce4b0827c8526855c7133
--- /dev/null
+++ b/base/trace_event_value.cc
@@ -0,0 +1,404 @@
+// 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/strings/stringprintf.h"
+#include "base/trace_event_value.h"
+
+namespace base {
+
+// Public

[dsinclair, 2013/07/23 14:56:29]

nit: remove comment.

[rterrazas, 2013/08/01 08:01:26]

Done.

+TracedValue::TracedValue() {
+}
+
+TracedValue::~TracedValue() {
+}
+
+void TracedValue::AppendAsTraceFormat(std::string* out) const {
+  // Used to keep track of the opening opcode.
+  std::stack<Opcode> op_stack;

[dsinclair, 2013/07/23 14:56:29]

I think this needs a different name, it's the current array or dictionary. I was
originally thinking it contained all operations.

[rterrazas, 2013/08/01 08:01:26]

Done.

+  // This is needed in case of nested structures.
+  std::stack<int> element_count_stack;
+  PickleIterator iter(pickle_);
+
+  int op_code;
+  bool read_succeeded = pickle_.ReadInt(&iter, &op_code);
+  while (read_succeeded) {
+    switch (op_code) {
+      case BEGIN_ARRAY:
+      case BEGIN_DICTIONARY:
+      {
+        // If this is a nested structure and not the first element, we need
+        // to prepend a comma.

[dsinclair, 2013/07/23 14:56:29]

This is only for the array case? If so, can we move it up to just the array case
statement, then let it fall through?

(Put a comment // Fall-through required.)

[rterrazas, 2013/08/01 08:01:26]

This is really for the case when we are within an array, so if within the array,
we have a value, which is an array/dictionary, we preppend a comma.

On 2013/07/23 14:56:29, dsinclair wrote:

+        if (!op_stack.empty() && op_stack.top() == BEGIN_ARRAY &&
+            element_count_stack.top() > 0)
+          StringAppendF(out, ", ");

[dsinclair, 2013/07/23 14:56:29]

Isn't this, essentially AppendCommaInsideArray(out, op_stack.top(),
element_count_stack.top()?

[rterrazas, 2013/08/01 08:01:26]

Done.

+
+        // Initialize the count for the current datastructure.
+        element_count_stack.push(0);
+        op_stack.push(static_cast<Opcode>(op_code));
+        StringAppendF(out, "{");

[dsinclair, 2013/07/23 14:56:29]

An array in JSON is [] not {}. This would start a dictionary.

[rterrazas, 2013/08/01 08:01:26]

Done.

+        break;
+      }
+      case END_DICTIONARY:
+      case END_ARRAY:
+      {
+        op_stack.pop();

[dsinclair, 2013/07/23 14:56:29]

nit: Move this down one line to keep with the element stack pop.

[rterrazas, 2013/08/01 08:01:26]

Done.

+        StringAppendF(out, "}");
+        element_count_stack.pop();
+        break;
+      }
+      case KEY_AS_RAW_POINTER:
+      {
+        AppendCommaInsideDictionary(out, element_count_stack.top()++);

[dsinclair, 2013/07/23 14:56:29]

Can we pull the AppendCommand methods outside of the switch statement, and just
have one method? Something like the following:

AppendComma(out, op_code, element_count) {
  if (op_code == END_DICTIONARY || op_code == END_ARRAY)
    return;

  if (element_count == 0)
    return;

  if (op_code == KEY_AS_RAW_POINTER || VALUE_AS_RAW_POINTER) {
     append_comma
     return;
  }

  if (op_code != BEGIN_ARRAY)
    return;

  append_comma
}

[rterrazas, 2013/08/01 08:01:26]

Done.

+        AppendKeyAsRawPointer(out, &iter);
+        break;
+      }
+      case VALUE_AS_RAW_POINTER:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsRawPointer(out, &iter);
+        break;
+      }
+      case VALUE_AS_STRING:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsString(out, &iter);
+        break;
+      }
+      case INT32_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsInt32(out, &iter);
+        break;
+      }
+      case INT64_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsInt64(out, &iter);
+        break;
+      }
+      case UINT32_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsUInt32(out, &iter);
+        break;
+      }
+      case UINT64_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsUInt64(out, &iter);
+        break;
+      }
+      case FLOAT_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsFloat(out, &iter);
+        break;
+      }
+      case BOOL_VALUE:
+      {
+        AppendCommaInsideArray(out, op_stack.top(),
+            element_count_stack.top()++);
+        AppendValueAsBool(out, &iter);
+        break;
+      }
+      default:
+        NOTREACHED() << "Don't know what to read, opcode=" << op_code;
+    }
+    read_succeeded = pickle_.ReadInt(&iter, &op_code);
+  }
+  DCHECK(op_stack.empty());
+}
+
+void TracedValue::BeginDictionary(const char* key_in_parent_dict,
+    int size_of_key) {
+  // Named dictionaries are properties of dictionaries, the name is the key.
+  DCHECK_EQ(BEGIN_DICTIONARY, op_codes_.top());
+  PushKey(key_in_parent_dict, size_of_key);
+  op_codes_.push(BEGIN_DICTIONARY);
+  pickle_.WriteInt(BEGIN_DICTIONARY);
+}
+
+void TracedValue::BeginDictionary() {
+  op_codes_.push(BEGIN_DICTIONARY);
+  pickle_.WriteInt(BEGIN_DICTIONARY);
+}
+
+void TracedValue::EndDictionary() {
+  DCHECK_EQ(BEGIN_DICTIONARY, op_codes_.top());
+  op_codes_.pop();
+  pickle_.WriteInt(END_DICTIONARY);
+}
+
+void TracedValue::BeginArray(const char* key_in_parent_dict, int size_of_key) {
+  // Named arrays are properties of dictionaries, and the key of such property
+  // is key_in_parent_dict.
+  DCHECK_EQ(BEGIN_DICTIONARY, op_codes_.top());
+  PushKey(key_in_parent_dict, size_of_key);
+  op_codes_.push(BEGIN_ARRAY);
+  pickle_.WriteInt(BEGIN_ARRAY);
+}
+
+void TracedValue::BeginArray() {
+  op_codes_.push(BEGIN_ARRAY);
+  pickle_.WriteInt(BEGIN_ARRAY);
+}
+
+void TracedValue::EndArray() {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  op_codes_.pop();
+
+  pickle_.WriteInt(END_ARRAY);
+}
+
+// Dictionary push methods.
+void TracedValue::Push(const char* key, int size_of_key,
+    const char* value, int size_of_value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value, size_of_value);
+}

[dsinclair, 2013/07/23 14:56:29]

These are all the same and could be done with a macro inside the header:

#define TRACE_PUSH(type) \
  void TraceValue::Push(const char* key, int size_of_key, ##type value) { \
    PushKey(key, size_of_key); \
    PushValueToPickle(value); \
}

TRACE_PUSH(const std::string&)
TRACE_PUSH(int)
TRACE_PUSH(int64)
etc.
#undef TRACE_PUSH

[rterrazas, 2013/08/01 08:01:26]

Done.

+
+void TracedValue::Push(const char* key, int size_of_key,
+    const std::string& value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, int value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, int64 value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, uint32 value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, uint64 value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, double value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key, bool value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const char* key, int size_of_key,
+    const TracedObject& value) {
+  PushKey(key, size_of_key);
+  PushValueToPickle(value);
+}
+
+// Array Push methods.
+void TracedValue::Push(const char* value, int size_of_value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());

[dsinclair, 2013/07/23 14:56:29]

Can be macro'ized same as dictionary ones.

[rterrazas, 2013/08/01 08:01:26]

Done.

+  PushValueToPickle(value, size_of_value);
+}
+
+void TracedValue::Push(const std::string& value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(int value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(int64 value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(uint32 value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(uint64 value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(double value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(bool value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+void TracedValue::Push(const TracedObject& value) {
+  DCHECK_EQ(BEGIN_ARRAY, op_codes_.top());
+  PushValueToPickle(value);
+}
+
+// Private

[dsinclair, 2013/07/23 14:56:29]

nit: remove.

[rterrazas, 2013/08/01 08:01:26]

Done.

+
+// 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.
+void TracedValue::PushKey(const char* key, int size_of_key) {
+  // Check that we're writing a dictionary.
+  DCHECK_EQ(BEGIN_DICTIONARY, op_codes_.top());
+
+  pickle_.WriteInt(KEY_AS_RAW_POINTER);
+  pickle_.WriteData(key, size_of_key);
+}
+
+void TracedValue::PushValueToPickle(const char* value, int size_of_value) {
+  pickle_.WriteInt(VALUE_AS_RAW_POINTER);
+  pickle_.WriteData(value, size_of_value);
+}
+
+void TracedValue::PushValueToPickle(const std::string& value) {
+  pickle_.WriteInt(VALUE_AS_STRING);
+  pickle_.WriteString(value);

[dsinclair, 2013/07/23 14:56:29]

The single param versions could be macro'ized.

[rterrazas, 2013/08/01 08:01:26]

I actually think doing this would decrease readability, for example, to macroize
PushValueToPickle(int64), I'd need to pass:

MACRO(int64, INT64_VALUE, WriteInt64)

If you feel strongly about it, please let me know, and I'll give it a try.

+}
+
+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(double 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);

[dsinclair, 2013/07/23 14:56:29]

I feel like TracedObject shouldn't be needed.  What about making this so we can
pass a TracedValue in here. We then pull the pickle out of the TracedValue and
stuff it into this pickle as WriteData(pickle->data(), pickle->size());

Then, when we're outputting as trace format, we see it's a pickle and output it
inline?

[rterrazas, 2013/08/01 08:01:26]

I was thinking that this could probably be used to avoid the problem of
unnecessary instances, for example:

RasterWorkerPoolTaskImpl's DataAsValue() which is later used to build a
TracedValue, could change to the following structure:
// RasterWorkerPoolTaskImpl would have to implement TracedObject of course.
virtual void PushInto(base::debug::TracedValue* traced_value) {
  traced_value.BeginDictionary();
  traced_value.Push("tile_id", TracedValueUtil::CreateIDRef(tile_id_)); // and
CreateIDRef would convert to uintptr_t or something like that?
  traced_value.Push("is_tile_in_pending_tree_now_bin",
is_tile_in_pending_tree_now_bin_);                          
  traced_value.Push("resolution", tile_resolution_); // TileResolution would
also need to implement TracedObject.
  traced_value.Push("source_frame_number", source_frame_number_);               
  traced_value.Push("layer_id", layer_id_);
  traced_value.EndDictionary();
}

That way, we don't hammer the new operator, since we only work on data that's
already instantiated.

And since TracedObject does not define any behavior, this is in line with
Chrome's C++ guidelines for multiple inheritance (in the cases where this would
happen).

WDYT?.

+}
+
+// Helper methods used in AppendAsTraceFormat()
+void TracedValue::AppendKeyAsRawPointer(std::string* out,
+    PickleIterator* iter) const {
+  const char* out_data;
+  int out_length;
+  pickle_.ReadData(iter, &out_data, &out_length);
+  StringAppendF(out, "\"%s\": ", out_data);
+}
+
+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());
+}
+
+void TracedValue::AppendValueAsRawPointer(std::string* out,
+    PickleIterator* iter) const {
+  const char* out_data;
+  int out_length;
+  pickle_.ReadData(iter, &out_data, &out_length);
+  StringAppendF(out, "\"%s\"", out_data);

[dsinclair, 2013/07/23 14:56:29]

This needs to handle strings with "'s in them.

[rterrazas, 2013/08/01 08:01:26]

Done.

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

[dsinclair, 2013/07/23 14:56:29]

This needs to deal with strings with "'s in them.

[rterrazas, 2013/08/01 08:01:26]

Done.

+}
+
+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);
+  StringAppendF(out, "%f", out_float);
+}
+
+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::AppendCommaInsideDictionary(std::string* out,
+    int element_index) {
+  if (element_index > 0)
+    StringAppendF(out, ", ");
+}
+
+// Static.
+void TracedValue::AppendCommaInsideArray(std::string* out, Opcode past_opcode,
+    int element_index) {
+  if (past_opcode == BEGIN_ARRAY && element_index > 0)
+    StringAppendF(out, ", ");
+}
+
+}  // namespace

[dsinclair, 2013/07/23 14:56:29]

nit:  // namespace base

[rterrazas, 2013/08/01 08:01:26]

Done.

+