[tracing] Turn StackFrame into struct.

base/trace_event/heap_profiler_allocation_context.h

 // Copyright 2015 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.
 
 #ifndef BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_CONTEXT_H_
 #define BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_CONTEXT_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 17 matching lines @@
 // The number of stack frames stored in the backtrace is a trade off between
 // memory used for tracing and accuracy. Measurements done on a prototype
 // revealed that:
 //
 // - In 60 percent of the cases, stack depth <= 7.
 // - In 87 percent of the cases, stack depth <= 9.
 // - In 95 percent of the cases, stack depth <= 11.
 //
 // 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);
 
 // The |AllocationContext| is context metadata that is kept for every allocation
 // when heap profiling is enabled. To simplify memory management for book-
 // keeping, this struct has a fixed size. All |const char*|s here must have
 // static lifetime.
 struct BASE_EXPORT AllocationContext {
  public:
@@ skipping 20 matching lines @@
 
 bool BASE_EXPORT operator==(const AllocationContext& lhs,
                             const AllocationContext& rhs);
 
 }  // namespace trace_event
 }  // namespace base
 
 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;
 };
 
 template <>
 struct BASE_EXPORT hash<base::trace_event::AllocationContext> {
   size_t operator()(const base::trace_event::AllocationContext& context) const;
 };
 
 }  // BASE_HASH_NAMESPACE
 
 #endif  // BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_CONTEXT_H_