Heap Profiler: Add trace category group names as type names for allocations

base/trace_event/heap_profiler_allocation_context_tracker.cc

Index: base/trace_event/heap_profiler_allocation_context_tracker.cc
diff --git a/base/trace_event/heap_profiler_allocation_context_tracker.cc b/base/trace_event/heap_profiler_allocation_context_tracker.cc
index 31f311a918e6d5931addb8aecce5ebd2d6420ef1..48582076fd24b14847ba6feda49ea034e72e53af 100644
--- a/base/trace_event/heap_profiler_allocation_context_tracker.cc
+++ b/base/trace_event/heap_profiler_allocation_context_tracker.cc
@@ -85,6 +85,7 @@ AllocationContextTracker::AllocationContextTracker()
     : thread_name_(nullptr), ignore_scope_depth_(0) {
   pseudo_stack_.reserve(kMaxStackDepth);
   task_contexts_.reserve(kMaxTaskDepth);
+  trace_categories_.reserve(kMaxStackDepth);
 }
 AllocationContextTracker::~AllocationContextTracker() {}
 
@@ -108,6 +109,7 @@ void AllocationContextTracker::SetCaptureMode(CaptureMode mode) {
 }
 
 void AllocationContextTracker::PushPseudoStackFrame(
+    const char* category,

[Primiano Tucci (use gerrit), 2016/08/25 16:31:15]

s/category/trace_event_category/ for consistency with trace_event_name below

[ssid, 2016/08/25 19:05:39]

I just wanted the declaration in header file to fit in one line :P now it is 2.

     const char* trace_event_name) {
   // Impose a limit on the height to verify that every push is popped, because
   // in practice the pseudo stack never grows higher than ~20 frames.
@@ -115,9 +117,18 @@ void AllocationContextTracker::PushPseudoStackFrame(
     pseudo_stack_.push_back(trace_event_name);
   else
     NOTREACHED();
+
+  // Ignore the category name if it has "," since it is confusing in UI.
+  if (strchr(category, ',') == nullptr &&

[Primiano Tucci (use gerrit), 2016/08/25 16:31:15]

Can we do this check in the UI? This is going to slow down every trace event
filtered.
Also, isn't this going to make the trace_categories_ stack out of sync?

[ssid, 2016/08/25 19:05:39]

Yess, that is the reason I have a separate stack for categories :P instead of
making the "pseudo_stack_" a vector of structs.
Ideally the check can actually be done in deduplicator itself instead of UI. The
reason I avoided doing this is because the allocations if they get a category
that has ",", then it is difficult to choose which one to take. If I ignore the
categories that have "," then we would still be left with some [unknown]. This
is better because the category is taken from the previous event in stack.

The other option would be to just take the first category if there are 2. Only
issue is we have both kinds of trace events like ones with "browser,startup" and
"startup,browser". So, strchr seems like a better option.

I have one other complex idea of having an array of boolean and the TraceLog
sends us the category_index aklong with category name. Using this the check for
"," can be done only once and every time we can read the array.

+      trace_categories_.size() < kMaxStackDepth) {
+    trace_categories_.push_back(category);
+  } else {
+    DCHECK_LE(trace_categories_.size(), kMaxStackDepth);
+  }
 }
 
 void AllocationContextTracker::PopPseudoStackFrame(
+    const char* category,
     const char* trace_event_name) {
   // Guard for stack underflow. If tracing was started with a TRACE_EVENT in
   // scope, the frame was never pushed, so it is possible that pop is called
@@ -132,6 +143,11 @@ void AllocationContextTracker::PopPseudoStackFrame(
       << "Encountered an unmatched TRACE_EVENT_END";
 
   pseudo_stack_.pop_back();
+
+  if (!trace_categories_.empty() && trace_categories_.back() == category)
+    trace_categories_.pop_back();
+  else
+    DCHECK(strchr(category, ','));
 }
 
 void AllocationContextTracker::PushCurrentTaskContext(const char* context) {
@@ -193,7 +209,7 @@ AllocationContext AllocationContextTracker::GetContextSnapshot() {
       }
     case CaptureMode::PSEUDO_STACK:
       {
-        for (const char* event_name: pseudo_stack_) {
+        for (const char* event_name : pseudo_stack_) {
           if (backtrace == backtrace_end) {
             break;
           }
@@ -237,7 +253,11 @@ AllocationContext AllocationContextTracker::GetContextSnapshot() {
 
   // TODO(ssid): Fix crbug.com/594803 to add file name as 3rd dimension
   // (component name) in the heap profiler and not piggy back on the type name.
-  ctx.type_name = task_contexts_.empty() ? nullptr : task_contexts_.back();
+  if (!task_contexts_.empty()) {
+    ctx.type_name = task_contexts_.back();
+  } else if (!trace_categories_.empty()) {
+    ctx.type_name = trace_categories_.back();
+  }
 
   return ctx;
 }