[tracing] Turn StackFrame into struct.

base/trace_event/heap_profiler_heap_dump_writer.cc

Index: base/trace_event/heap_profiler_heap_dump_writer.cc
diff --git a/base/trace_event/heap_profiler_heap_dump_writer.cc b/base/trace_event/heap_profiler_heap_dump_writer.cc
index 9a3112eacf3ea508c4b6da1adc54d279280c0ae5..ef33492db79350ba4de6653c92c5f30c3b3d3588 100644
--- a/base/trace_event/heap_profiler_heap_dump_writer.cc
+++ b/base/trace_event/heap_profiler_heap_dump_writer.cc
@@ -74,25 +74,19 @@ bool operator<(const Bucket& lhs, const Bucket& rhs) {
 // returned list will have |backtrace_cursor| advanced or
 // |is_broken_down_by_type_name| set depending on the property to group by.
 std::vector<Bucket> GetSubbuckets(const Bucket& bucket, BreakDownMode breakBy) {
-  base::hash_map<const char*, Bucket> breakdown;
+  base::hash_map<const void*, Bucket> breakdown;
 
   if (breakBy == BreakDownMode::kByBacktrace) {
     for (const auto& context_and_metrics : bucket.metrics_by_context) {
       const Backtrace& backtrace = context_and_metrics.first->backtrace;
-      const char* const* begin = std::begin(backtrace.frames);
-      const char* const* end = std::end(backtrace.frames);
-      const char* const* cursor = begin + bucket.backtrace_cursor;
-
-      // The backtrace in the context is padded with null pointers, but these
-      // should not be considered for breakdown. Adjust end to point past the
-      // last non-null frame.
-      while (begin != end && *(end - 1) == nullptr)
-        end--;
+      const StackFrame* begin = std::begin(backtrace.frames);
+      const StackFrame* end = begin + backtrace.frame_count;

[Primiano Tucci (use gerrit), 2016/04/19 19:45:06]

shouldn't this be begin + frame_count + 1
If I am reading correctly the code below is expecting end to be past the last
element, right?
Here if count == 0 start == end. while in the previous code if the thing is
empty end = start + 1
Or am I misreading something?

[Dmitry Skiba, 2016/04/19 22:14:14]

I think this is the same - previously if all frames were nullptrs, code would
rewind 'end' to 'begin' (last iteration is where end = begin + 1, *(end - 1) is
nullptr, so we go inside and end--, and then we stop looping because begin ==
end.

And if there are N non-nullptr frames, previously we would stop looping when
*(end - 1) != nullptr, i.e. when end == (begin + N).

[Primiano Tucci (use gerrit), 2016/04/20 13:17:59]

Ok you are right. your code is consistent with the previous one.
I just wonder whether even the original one is bugged in the case of an empty
stacktrace.
I think we should hit that DCHECK if begin == end.
Maybe we early out at some earlier point? Mind checking if that is the case?
(not sure if is it covered by some unittest)

+      const StackFrame* cursor = begin + bucket.backtrace_cursor;
 
       DCHECK_LE(cursor, end);
 
       if (cursor != end) {
-        Bucket& subbucket = breakdown[*cursor];
+        Bucket& subbucket = breakdown[cursor->value];
         subbucket.size += context_and_metrics.second.size;
         subbucket.count += context_and_metrics.second.count;
         subbucket.metrics_by_context.push_back(context_and_metrics);
@@ -195,13 +189,13 @@ bool HeapDumpWriter::AddEntryForBucket(const Bucket& bucket) {
 
   const AllocationContext* context = bucket.metrics_by_context.front().first;
 
-  const char* const* backtrace_begin = std::begin(context->backtrace.frames);
-  const char* const* backtrace_end = backtrace_begin + bucket.backtrace_cursor;
+  const StackFrame* backtrace_begin = std::begin(context->backtrace.frames);
+  const StackFrame* backtrace_end = backtrace_begin + bucket.backtrace_cursor;
   DCHECK_LE(bucket.backtrace_cursor, arraysize(context->backtrace.frames));
 
   Entry entry;
-  entry.stack_frame_id =
-      stack_frame_deduplicator_->Insert(backtrace_begin, backtrace_end);
+  entry.stack_frame_id = stack_frame_deduplicator_->Insert(
+      backtrace_begin, backtrace_end);
 
   // Deduplicate the type name, or use ID -1 if type name is not set.
   entry.type_id = bucket.is_broken_down_by_type_name