[tracing] Turn StackFrame into struct.

base/trace_event/heap_profiler_allocation_context.h

Index: base/trace_event/heap_profiler_allocation_context.h
diff --git a/base/trace_event/heap_profiler_allocation_context.h b/base/trace_event/heap_profiler_allocation_context.h
index 8544c78eb2cd34c3f7693fa4063ccdcf85f837a1..3708c907e5903a958ebdf2dcf668f19f8bb76fbb 100644
--- a/base/trace_event/heap_profiler_allocation_context.h
+++ b/base/trace_event/heap_profiler_allocation_context.h
@@ -35,14 +35,39 @@ namespace trace_event {
 //
 // See the design doc (https://goo.gl/4s7v7b) for more details.
 
-using StackFrame = const char*;
+// Represents (pseudo) stack frame. Used in Backtrace class below.
+//
+// For performance reasons this struct is treated as bytes when comparing
+// and hashing, so all members must be pointer-sized to make sure they are
+// not padded.
+struct BASE_EXPORT StackFrame {
+  enum class Type: uintptr_t {

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

out of curiosity why this needs to be a uintptr_t? isn't the default (int IIRC)
enough?

[Dmitry Skiba, 2016/04/15 07:01:39]

Because else there will be padding on 64-bit platforms, which will cause
comparison and hash functions, which work on struct's bytes, to fail. 

[Primiano Tucci (use gerrit), 2016/04/15 13:47:47]

Yeah but see my comment. You shouldn't rely on these detail. Just compare the
.value and that will be enough. Don't rely on the memory layout of c++ objects.

+    TRACE_EVENT_NAME,   // const char* string
+    THREAD_NAME,        // const char* thread name
+  };
+
+  static StackFrame FromTraceEventName(const char* name) {
+    return {Type::TRACE_EVENT_NAME, name};
+  }
+  static StackFrame FromThreadName(const char* name) {
+    return {Type::THREAD_NAME, name};
+  }
+
+  Type type;
+  const void* value;
+};
+
+bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs);

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

should we turn that map in the deduplicator into an unordered_map, so we just
need hash and operator== ? (ok to do that in a separate cl)

[Dmitry Skiba, 2016/04/15 07:01:39]

Why minimizing number of operators? Yes, we could do that, and remove operator<,
but why?

[Primiano Tucci (use gerrit), 2016/04/15 13:47:47]

Oh is not for minimizing operators, I just realized that that thing should not
have been a map in the first place.
ANyways, this is not a priority, we can do in a separate CL. these are fine

+bool BASE_EXPORT operator == (const StackFrame& lhs, const StackFrame& rhs);
+bool BASE_EXPORT operator != (const StackFrame& lhs, const StackFrame& rhs);
 
 struct BASE_EXPORT Backtrace {
-  // Unused backtrace frames are filled with nullptr frames. If the stack is
-  // higher than what can be stored here, the bottom frames are stored. Based
-  // on the data above, a depth of 12 captures the full stack in the vast
-  // majority of the cases.
-  StackFrame frames[12];
+  // If the stack is higher than what can be stored here, the bottom frames
+  // are stored. Based on the data above, a depth of 12 captures the full
+  // stack in the vast majority of the cases.
+  enum { kMaxFrameCount = 12 };
+  StackFrame frames[kMaxFrameCount];
+  size_t frame_count;

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

not sure I understand from the code below why do we need to keep track of the
frame count? Isn't just simpler to stop at the first nullptr?
This feels to introduce another thing to think about.

 };
 
 bool BASE_EXPORT operator==(const Backtrace& lhs, const Backtrace& rhs);
@@ -83,6 +108,11 @@ bool BASE_EXPORT operator==(const AllocationContext& lhs,
 namespace BASE_HASH_NAMESPACE {
 
 template <>
+struct BASE_EXPORT hash<base::trace_event::StackFrame> {
+  size_t operator()(const base::trace_event::StackFrame& frame) const;
+};
+
+template <>
 struct BASE_EXPORT hash<base::trace_event::Backtrace> {
   size_t operator()(const base::trace_event::Backtrace& backtrace) const;
 };