[Tracing] Add hook to PartitionAlloc for heap profiling

content/child/web_memory_dump_provider_adapter.cc

Index: content/child/web_memory_dump_provider_adapter.cc
diff --git a/content/child/web_memory_dump_provider_adapter.cc b/content/child/web_memory_dump_provider_adapter.cc
index ef2cab810f0ab628f41333285b72440cd991f46b..4e6e54b79e73721caad8c496ee0c47a5a91ff810 100644
--- a/content/child/web_memory_dump_provider_adapter.cc
+++ b/content/child/web_memory_dump_provider_adapter.cc
@@ -4,9 +4,36 @@
 
 #include "content/child/web_memory_dump_provider_adapter.h"
 
+#include "base/synchronization/lock.h"
+#include "base/trace_event/memory_profiler_allocation_context.h"
+#include "base/trace_event/memory_profiler_allocation_register.h"
 #include "content/child/web_process_memory_dump_impl.h"
 #include "third_party/WebKit/public/platform/WebMemoryDumpProvider.h"
 
+using namespace base;
+using namespace base::trace_event;
+
+namespace {
+AllocationRegister* gAllocationRegister;

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

This is still chrome-land, this should be named g_allocation_register

[Ruud van Asseldonk, 2015/10/13 10:42:18]

Done.

+Lock* gAllocationRegisterLock;
+
+void reportAllocation(void* address, size_t size) {

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

I would probably rename these with a locked suffix and move the locking
reponsibility to the caller.
Rationale: some partition alloc partitions are single threaded and there is no
need to take an extra lock there.

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

Also, IIRC this should start with a capital R

[Ruud van Asseldonk, 2015/10/13 10:42:18]

No, the caller should not have to deal with locking (or reentrancy, though that
is not an issue with PartitionAlloc yet). The reason is that when the memory
dump manager calls |OnMemoryDump|, the register needs to be snapshotted and it
must not be modified when it is being iterated. Furthermore, the partitions
being single-threaded does not help at all, unless you keep a dedicated
allocation register per partition.
Done.

[Primiano Tucci (use gerrit), 2015/10/14 10:10:12]

Ah, you are definitely right. Ignore my comment about locking.

+  // Calling |GetContextSnapshot| is only valid when |capture_enabled| is true.
+  AllocationContext context =
+      AllocationContextTracker::capture_enabled()
+          ? AllocationContextTracker::GetContextSnapshot()
+          : AllocationContext::Empty();
+
+  AutoLock guard(*gAllocationRegisterLock);
+  gAllocationRegister->Insert(address, size, context);
+}
+
+void reportFree(void* address) {

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

ditto s/report/Report/

[Ruud van Asseldonk, 2015/10/13 10:42:18]

Done.

+  AutoLock guard(*gAllocationRegisterLock);
+  gAllocationRegister->Remove(address);
+}
+}
+
 namespace content {
 
 WebMemoryDumpProviderAdapter::WebMemoryDumpProviderAdapter(
@@ -35,7 +62,36 @@ bool WebMemoryDumpProviderAdapter::OnMemoryDump(
   }
   WebProcessMemoryDumpImpl web_pmd_impl(args.level_of_detail, pmd);
 
+  if (web_memory_dump_provider_->supportsHeapProfiling()) {

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

Just to avoid forgetting this in future, here you will want also a check that
args.level_of_detail == HUUUUUUGE 

+    // TODO(ruuda): Dump |gAllocationRegister| into the |ProcessMemoryDump|.
+  }
+
   return web_memory_dump_provider_->onMemoryDump(level, &web_pmd_impl);
 }
 
+void WebMemoryDumpProviderAdapter::OnHeapProfilingEnabled(bool enabled) {

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

I'd probably move the supportsHeapProfiling() on the top and early return:
if (!web_memory_dump_provider_->supportsHeapProfiling())
  return

if (enabled)
 ...
else
...

this will remove one level of indentation and make your code below more readable

[Ruud van Asseldonk, 2015/10/13 10:42:18]

As you wish.

+  if (enabled) {
+    // Lazily initialize the global Blink allocation register.
+    if (gAllocationRegister == nullptr) {
+      gAllocationRegister = new AllocationRegister();

[Primiano Tucci (use gerrit), 2015/10/12 16:36:45]

these will make LeaskSanitizer barf.
Either you need a suppression or you want to use LazyInstance (but that willl
add an interlocked operation in the slow-path)

[Ruud van Asseldonk, 2015/10/13 10:42:18]

The proper way of doing this is to delete when heap profiling is disabled (or in
the dump provider destructor once it can live in Blink), but the hook checks are
nobarrier loads so then potentially a hook can be called after the register has
been deleted. So then the hooks would need to do a barriered check for the
register and there will be |base::subtle| all over the place ... What do you
think, just use |LazyInstance| then?

+      gAllocationRegisterLock = new Lock();
+    }
+
+    // Make this dump provider call the global hooks on every allocation / free.
+    // TODO(ruuda): Because bookkeeping is done here in the adapter, and not in
+    // the dump providers themselves, all dump providers in Blink share the
+    // same global allocation register. At the moment this is not a problem,
+    // because the only dump provider that supports heap profiling is the
+    // PartitionAlloc dump provider. When Blink can depend on base and this
+    // glue layer is removed, dump providers can have their own instance of the
+    // allocation register.
+    if (web_memory_dump_provider_->supportsHeapProfiling())
+      web_memory_dump_provider_->onHeapProfilingEnabled(reportAllocation,
+                                                        reportFree);
+  } else {
+    if (web_memory_dump_provider_->supportsHeapProfiling())
+      web_memory_dump_provider_->onHeapProfilingEnabled(nullptr, nullptr);
+  }
+}
+
 }  // namespace content