tracing: add dump provider for malloc heap profiler

base/trace_event/malloc_dump_provider.cc

Index: base/trace_event/malloc_dump_provider.cc
diff --git a/base/trace_event/malloc_dump_provider.cc b/base/trace_event/malloc_dump_provider.cc
index 6f9aa9602f7334840489e17c1ca15c7c13198c74..314af42e94e5dc7cb00dbee55b33bf88d118e558 100644
--- a/base/trace_event/malloc_dump_provider.cc
+++ b/base/trace_event/malloc_dump_provider.cc
@@ -7,7 +7,14 @@
 #include <stddef.h>
 
 #include "base/allocator/allocator_extension.h"
+#include "base/allocator/allocator_shim.h"
+#include "base/allocator/features.h"
+#include "base/trace_event/heap_profiler_allocation_context.h"
+#include "base/trace_event/heap_profiler_allocation_context_tracker.h"
+#include "base/trace_event/heap_profiler_allocation_register.h"
+#include "base/trace_event/heap_profiler_heap_dump_writer.h"
 #include "base/trace_event/process_memory_dump.h"
+#include "base/trace_event/trace_event_argument.h"
 #include "build/build_config.h"
 
 #if defined(OS_MACOSX)
@@ -19,6 +26,65 @@
 namespace base {
 namespace trace_event {
 
+#if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
+namespace {
+
+using allocator::AllocatorDispatch;
+
+void* HookAlloc(const AllocatorDispatch* self, size_t size) {
+  const AllocatorDispatch* const next = self->next;
+  void* ptr = next->alloc_function(next, size);
+  if (ptr)
+    MallocDumpProvider::GetInstance()->InsertAllocation(ptr, size);
+  return ptr;
+}
+
+void* HookZeroInitAlloc(const AllocatorDispatch* self, size_t n, size_t size) {
+  const AllocatorDispatch* const next = self->next;
+  void* ptr = next->alloc_zero_initialized_function(next, n, size);
+  if (ptr)
+    MallocDumpProvider::GetInstance()->InsertAllocation(ptr, n * size);
+  return ptr;
+}
+
+void* HookllocAligned(const AllocatorDispatch* self,
+                      size_t alignment,
+                      size_t size) {
+  const AllocatorDispatch* const next = self->next;
+  void* ptr = next->alloc_aligned_function(next, alignment, size);
+  if (ptr)
+    MallocDumpProvider::GetInstance()->InsertAllocation(ptr, size);
+  return ptr;
+}
+
+void* HookRealloc(const AllocatorDispatch* self, void* address, size_t size) {
+  const AllocatorDispatch* const next = self->next;
+  void* ptr = next->realloc_function(next, address, size);
+  MallocDumpProvider::GetInstance()->RemoveAllocation(address);
+  if (size > 0)  // realloc(size == 0) means free().
+    MallocDumpProvider::GetInstance()->InsertAllocation(ptr, size);
+  return ptr;
+}
+
+void HookFree(const AllocatorDispatch* self, void* address) {
+  if (address)
+    MallocDumpProvider::GetInstance()->RemoveAllocation(address);
+  const AllocatorDispatch* const next = self->next;
+  next->free_function(next, address);
+}
+
+AllocatorDispatch g_allocator_hooks = {
+    &HookAlloc,         /* alloc_function */
+    &HookZeroInitAlloc, /* alloc_zero_initialized_function */
+    &HookllocAligned,   /* alloc_aligned_function */
+    &HookRealloc,       /* realloc_function */
+    &HookFree,          /* free_function */
+    nullptr,            /* next */
+};
+
+}  // namespace
+#endif  // BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
+
 // static
 const char MallocDumpProvider::kAllocatedObjects[] = "malloc/allocated_objects";
 
@@ -28,7 +94,8 @@ MallocDumpProvider* MallocDumpProvider::GetInstance() {
                    LeakySingletonTraits<MallocDumpProvider>>::get();
 }
 
-MallocDumpProvider::MallocDumpProvider() {}
+MallocDumpProvider::MallocDumpProvider()
+    : heap_profiler_enabled_(false), tid_dumping_heap_(kInvalidThreadId) {}
 
 MallocDumpProvider::~MallocDumpProvider() {}
 
@@ -96,8 +163,98 @@ bool MallocDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
                           resident_size - allocated_objects_size);
   }
 
+  // Heap profiler dumps.
+  if (!heap_profiler_enabled_)
+    return true;
+
+  // The dumps of the heap profiler should be created only when heap profiling
+  // was enabled (--enable-heap-profiling) AND a DETAILED dump is requested.
+  // However, when enabled, the overhead of the heap profiler should be always
+  // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc()

[petrcermak, 2016/03/11 11:09:23]

nit: s/w.r.t/w.r.t./ and s/vs/vs./

[petrcermak, 2016/03/11 11:09:24]

supernit: I don't think there should be a comma before "to" here.

[petrcermak, 2016/03/11 11:09:24]

supernit: I think you should s/malloc()/malloc/ or even s/malloc()/'malloc'/
because 'malloc' is then shown in the tracing UI.

[petrcermak, 2016/03/11 11:09:24]

What do you mean by "w.r.t LIGHT vs DETAILED"? Don't you mean "even in LIGHT
dumps"?

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Done.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Ok reworded this.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Done.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Done.

+  // total.
+
+  tid_dumping_heap_ = PlatformThread::CurrentId();
+  // At this point the Insert/RemoveAllocation hooks will ignore this thread.
+  // Enclosing all the temporariy data structures in a scope, so that the heap
+  // profiler does not see unabalanced malloc/free calls from these containers.
+  {
+    TraceEventMemoryOverhead overhead;
+    hash_map<AllocationContext, size_t> bytes_by_context;
+    {
+      AutoLock lock(allocation_register_lock_);
+      if (allocation_register_) {
+        if (args.level_of_detail == MemoryDumpLevelOfDetail::DETAILED) {
+          for (const auto& alloc_size : *allocation_register_)
+            bytes_by_context[alloc_size.context] += alloc_size.size;
+        }
+        allocation_register_->EstimateTraceMemoryOverhead(&overhead);
+      }
+    }  // lock(allocation_register_lock_)
+
+    if (!bytes_by_context.empty()) {
+      scoped_ptr<TracedValue> heap_dump = ExportHeapDump(
+          bytes_by_context, pmd->session_state()->stack_frame_deduplicator(),
+          pmd->session_state()->type_name_deduplicator());
+      pmd->AddHeapDump("malloc", std::move(heap_dump));
+    }
+    overhead.DumpInto("tracing/heap_profiler_malloc", pmd);

[petrcermak, 2016/03/11 11:09:24]

thought: Wouldn't "tracing/malloc_heap_profiler" be better (easier to find in
the UI)?

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

In the beginning it was like that, but I made it consistent with the
partition_alloc and blink_gc, which today already use heap_profiler_XXX

[petrcermak, 2016/03/11 14:26:31]

Acknowledged.

+  }
+  tid_dumping_heap_ = kInvalidThreadId;
+
   return true;
 }
 
+void MallocDumpProvider::OnHeapProfilingEnabled(bool enabled) {
+#if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
+  if (enabled) {
+    {
+      AutoLock lock(allocation_register_lock_);
+      allocation_register_.reset(new AllocationRegister());
+    }
+    allocator::InsertAllocatorDispatch(&g_allocator_hooks);
+  } else {
+    AutoLock lock(allocation_register_lock_);
+    allocation_register_.reset();
+    // Inser/RemoveAllocation below will no-op if the register is torn down.

[petrcermak, 2016/03/11 11:09:24]

s/Inser/Insert/

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Done.

+  }
+#endif
+  heap_profiler_enabled_ = enabled;
+}
+
+void MallocDumpProvider::InsertAllocation(void* address, size_t size) {
+  if (tid_dumping_heap_ != kInvalidThreadId &&

[petrcermak, 2016/03/11 11:09:24]

Can it ever happen that PlatformThread::CurrentId() == kInvalidThreadID? If not,
the first case is unnecessary. Also applies to line 250.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Heh I knew that you would have commented this. You are conceptually right, but
there is a perf reason for doing this.
CurrentId() is slow on some platforms, as it might end up invoking a syscall (if
the libc implementation is dumb).
In most of the cases, I expect to not be in the reentrant situation here, in
which case the first check will short-circuit the if and prevent the CurrentID()
call at all.
Good spot btw :)

[petrcermak, 2016/03/11 14:26:31]

Acknowledged.

+      tid_dumping_heap_ == PlatformThread::CurrentId())
+    return;
+  // AllocationContextTracker initialization is the only thing here that can
+  // cause re-entrancy.
+  switch (AllocationContextTracker::GetStateForCurrentThread()) {
+    case AllocationContextTracker::kInitialized:
+      break;
+    case AllocationContextTracker::kNotInitialized:
+      AllocationContextTracker::InitializeForCurrentThread();
+      return;
+    case AllocationContextTracker::kInitializing:
+      return;
+  }
+
+  AllocationContext context = AllocationContextTracker::GetContextSnapshot();
+
+  AutoLock lock(allocation_register_lock_);

[petrcermak, 2016/03/11 11:09:24]

Can't the compiler move this definition to the top of the method (i.e. shouldn't
lines 242-246 be in a block)? Also applies to lines 253-256.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

nope. The compiler is not allowed to reorder instructions unless it is sure that
there is no side effect, which is not the case for Lock (because uses atomics,
and more in general is not inlined and defined into another translation unit).
The reason why sometimes you see Lock in a scope, is if you want the effect of
the lock to end before the end of the function, e.g.,

{
   Lock;
   do_something_locked();
}
do_something_unlocked();

+  if (!allocation_register_)
+    return;
+
+  allocation_register_->Insert(address, size, context);
+}
+
+void MallocDumpProvider::RemoveAllocation(void* address) {
+  if (tid_dumping_heap_ != kInvalidThreadId &&
+      tid_dumping_heap_ == PlatformThread::CurrentId())
+    return;
+  AutoLock lock(allocation_register_lock_);

[petrcermak, 2016/03/11 11:09:24]

Is it definitely the case that AllocationContextTracker will be initialized at
this point? If yes, you should probably add a comment explaining why.

[Primiano Tucci (use gerrit), 2016/03/11 13:57:57]

Good spot. It's definitely NOT the case (I mean, it can be non-initialized), for
instance, if the very first thing you intercept after installing the hooks is a
free (very likely can happen).
But the deal here is that we don't need the tracker in the RemoveAllocation()
function, so we don't care if that's initialized or not.
The reason why I bother initializing that in the malloc() is because I need it
few statements below when I do AllocationContextTracker::GetContextSnapshot()
and at that point it must have been initialized or a reentrancy will happen.

But yet again, this was another excellent spot, could have been a nasty bug,
thanks for the -pedantic mode.

[petrcermak, 2016/03/11 14:26:31]

Thanks for the detailed explanation. It might be worth adding a short comment
explaining this in here.

[Primiano Tucci (use gerrit), 2016/03/11 16:57:21]

Done

+  if (!allocation_register_)
+    return;
+  allocation_register_->Remove(address);
+}
+
 }  // namespace trace_event
 }  // namespace base