Make TracedValue lower overhead.

base/trace_event_value.h

Index: base/trace_event_value.h
diff --git a/base/trace_event_value.h b/base/trace_event_value.h
new file mode 100644
index 0000000000000000000000000000000000000000..74fbec051b395900fbc3fd5cc55ed36a2c96dee7
--- /dev/null
+++ b/base/trace_event_value.h
@@ -0,0 +1,129 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.

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

These should probably be in base/debug with the other trace_event code.

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TRACE_EVENT_VALUE_H_
+#define BASE_TRACE_EVENT_VALUE_H_
+
+#include <stack>
+#include <string>
+
+#include "base/debug/trace_event.h"
+#include "base/format_macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/pickle.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/traced_object.h"
+
+#define STATIC_STR(value) value, sizeof(value)

[rterrazas, 2013/07/21 22:23:49]

I don't like this (Because I'm hiding the fact that we're sending out two
parameters, but makes the API a tiny bit prettier IMHO), but calling sizeof
inside the function returns the pointer size, and sizeof(key[0]), returns the
size of the character, however, if we get sizeof at the moment of the
definition, we get the right value.

I can expose methods for dictionary keys if needed, but since const char*
keys/values are preferred, I didn't do that.

WDYT?.

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

Let's drop this. It seems like the kind of thing that's going to cause issues
later.

+
+namespace base {
+
+// TracedValue has several functions to push data that will be converted to
+// JSON format without depending on base::Value objects.

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

Remove the JSON and just say trace format. We don't expose outside of our
implementation that it's JSON.

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

Done.

+class BASE_EXPORT TracedValue : public base::debug::ConvertableToTraceFormat,
+                    public base::NonThreadSafe {

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

nit: indenting

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

Done.

+ public:
+  TracedValue();
+  virtual ~TracedValue();
+
+  virtual void AppendAsTraceFormat(std::string* out) const OVERRIDE;
+
+  // Begin a dictionary, that is a value within another dictionary.
+  void BeginDictionary(const char* key_in_parent_dict, int size_of_key);

[nduca, 2013/07/23 04:54:04]

any way we can make this not require key_in_parent_dict? that seems pretty
awkward

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

I thought about this also, and toyed with the idea of exposing the PushKey(const
char*), but it felt wrong, too easy to misuse I think.

Would you prefer to do that?.

+  // Begin a dictionary, it can either be a top-level structure or a value in
+  // an array.
+  void BeginDictionary();
+  // Used for both kinds of BeginDictionary functions.
+  void EndDictionary();
+  // Begin an array, that is a value within a dictonary.
+  void BeginArray(const char* key_in_parent_dict, int size_of_key);
+  // Begin an array, that is either a top-level structure or a value in an
+  // array.
+  void BeginArray();
+  // Used for both kinds of BeginArray functions.
+  void EndArray();
+
+  // Put stuff in dictionary using statically allocated string keys.
+  void Push(const char* key, int size_of_key,  const char* value,

[nduca, 2013/07/23 04:54:04]

i think you can simplify this using the following assertion: if a user passes a
const char* into the api, then they are sayign that the const char* will live
forever. meaning we only have to store the raw pointer in the pickle. if they
have something that may disappear, they would pass in a std::string, which we'd
push by value into the pickle.

want to try making that change?

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

Done.

+      int size_of_value);
+  void Push(const char* key, int size_of_key,  const std::string& value);

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

Why do you need size_of_key? Is this the sizeof(char) or is it the string
length? If it's the string lenght, why not just do strlen(key) inside the
methods?

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

It was more like sizeof buffer, but Nat's hint above allowed me to get rid of
this :)

+  void Push(const char* key, int size_of_key, int value);
+  void Push(const char* key, int size_of_key, int64 value);
+  void Push(const char* key, int size_of_key, uint32 value);
+  void Push(const char* key, int size_of_key, uint64 value);
+  void Push(const char* key, int size_of_key, double value);
+  void Push(const char* key, int size_of_key, bool value);
+  void Push(const char* key, int size_of_key, const TracedObject& value);
+
+  // Put stuff in an array.
+  void Push(const char* value, int size_of_key);
+  void Push(const std::string& value);
+  void Push(int value);
+  void Push(int64 value);
+  void Push(uint32 value);
+  void Push(uint64 value);
+  void Push(double value);
+  void Push(bool value);
+  void Push(const TracedObject& value);
+
+ private:
+  // Push key/value should not be exposed, instead, the Push methods are
+  // exposed, to have them assert that we are in the right state before writing
+  // to the pickle.
+  void PushKey(const char* key, int size_of_key);

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

Any reason why this isn't PushkeyToPickle to be consistent with the Value
versions?

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

Done.

+  void PushValueToPickle(const char* value, int size_of_value);
+  void PushValueToPickle(const std::string& value);
+  void PushValueToPickle(int value);
+  void PushValueToPickle(int64 value);
+  void PushValueToPickle(uint32 value);
+  void PushValueToPickle(uint64 value);
+  void PushValueToPickle(double value);
+  void PushValueToPickle(bool value);
+  void PushValueToPickle(const TracedObject& value);
+
+  // Helper methods used in AppendAsTraceFormat().
+  void AppendKeyAsRawPointer(std::string* out, PickleIterator* iter) const;
+  void AppendKeyAsString(std::string* out,
+      PickleIterator* iter) const;
+  void AppendValueAsRawPointer(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsString(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsInt32(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsInt64(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsUInt32(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsUInt64(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsFloat(std::string* out, PickleIterator* iter) const;
+  void AppendValueAsBool(std::string* out, PickleIterator* iter) const;
+
+  Pickle pickle_;
+  enum Opcode {
+    BEGIN_DICTIONARY,
+    END_DICTIONARY,
+    BEGIN_ARRAY,
+    END_ARRAY,
+    KEY_AS_RAW_POINTER,
+    VALUE_AS_RAW_POINTER,
+    VALUE_AS_STRING,
+    INT32_VALUE,
+    INT64_VALUE,
+    UINT32_VALUE,
+    UINT64_VALUE,
+    FLOAT_VALUE,
+    BOOL_VALUE
+  };
+
+  static void AppendCommaInsideDictionary(std::string* out, int element_index);
+  static void AppendCommaInsideArray(std::string* out,
+      Opcode past_opcode, int element_index);
+
+  // This is used to keep track of the operations that need to be closed, in
+  // in order to enforce concistency in the operations.

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

nit: in in
nit: consistency

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

Done.

+  std::stack<Opcode> op_codes_;

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

From reading the code, this isn't all the op_codes seen, but just the BEGIN
DICTIONARY and BEGIN ARRAY does?

If that's correct, I think we need a better name/comment to go here.

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(TracedValue);
+};
+
+}  // Closing namespace base.

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

nit:  // namespace base

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

Done.

+
+#endif  // Endif for BASE_TRACE_EVENT_VALUE_H_.

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

nit: remove comment.

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

Done.

+