Add a WinHeap dump provider to the memory profiler.

base/trace_event/winheap_dump_provider_win.cc

Index: base/trace_event/winheap_dump_provider_win.cc
diff --git a/base/trace_event/winheap_dump_provider_win.cc b/base/trace_event/winheap_dump_provider_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ab0f62a0539ee62fcd867804dedd02f18b47ce58
--- /dev/null
+++ b/base/trace_event/winheap_dump_provider_win.cc
@@ -0,0 +1,102 @@
+// 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.
+
+#include "base/trace_event/winheap_dump_provider.h"
+
+#include <windows.h>
+
+#include "base/trace_event/process_memory_dump.h"
+
+namespace base {
+namespace trace_event {
+
+namespace {
+
+const char kDumperFriendlyName[] = "winheap";
+
+// Report a heap dump to a process memory dump. The |heap_info| structure
+// contains the information about this heap, and |heap_name| will be used to
+// represent it in the report.
+bool ReportHeapDump(ProcessMemoryDump* pmd, const WinHeapInfo& heap_info,
+                    const std::string& heap_name) {
+  DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd);

[Primiano Tucci (use gerrit), 2015/04/22 23:48:32]

This would never happen, and even if it happens, AFAIK the pattern in chromium
is to not DCHECK for null pointers: they will just cause a segfault the first
time you try to use them, so DCHECK is not going to add any particular extra
value.

[Sébastien Marchand, 2015/04/23 10:52:03]

One more difference between my codebase (Syzygy) and Chrome ! Removed.

+  MemoryAllocatorDump* dump =
+      pmd->CreateAllocatorDump(kDumperFriendlyName, heap_name);
+  if (!dump)
+    return false;
+  dump->set_physical_size_in_bytes(heap_info.committed_size);
+  dump->set_allocated_objects_count(heap_info.block_count);
+  dump->set_allocated_objects_size_in_bytes(heap_info.allocated_size);
+  return true;
+}
+
+}  // namespace
+

[chrisha, 2015/04/23 09:23:28]

Remove extra blank line.

[Sébastien Marchand, 2015/04/23 10:52:03]

Done.

+
+WinHeapDumpProvider* WinHeapDumpProvider::GetInstance() {
+  return Singleton<WinHeapDumpProvider,
+                   LeakySingletonTraits<WinHeapDumpProvider>>::get();
+}
+
+bool WinHeapDumpProvider::DumpInto(ProcessMemoryDump* pmd) {
+  DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd);

[Primiano Tucci (use gerrit), 2015/04/22 23:48:32]

ditto

[Sébastien Marchand, 2015/04/23 10:52:04]

Done.

+
+  // Retrieves the number of heaps in the current process.
+  DWORD number_of_heaps = ::GetProcessHeaps(0, NULL);
+  WinHeapInfo all_heap_info = { 0 };
+
+  // Try to retrieve a handle to all the heaps owned by this process. Returns
+  // false if the number of heaps has changed.
+  scoped_ptr<HANDLE[]> all_heaps(new HANDLE[number_of_heaps]);
+  if (::GetProcessHeaps(number_of_heaps, all_heaps.get()) != number_of_heaps)
+    return false;

[chrisha, 2015/04/23 09:23:28]

A very minor thing, but this kind of racy 'get count, then get objects' code can
be made slightly more robust by running in a loop a fixed number of iterations
(a common pattern seen elsewhere in Chrome for these types of accesses).

Although I think the benefit is very small here, so probably safe this way. Just
document the fact that this query is inherently racy?

[Sébastien Marchand, 2015/04/23 10:52:03]

Yeah, in Chrome all the heaps get created at startup, added a comment about
that.

+
+  // Skip the pointer to the heap array.
+  std::set<void*> block_to_skip;
+  block_to_skip.insert(all_heaps.get());

[Primiano Tucci (use gerrit), 2015/04/22 23:48:32]

Can you add just a comment to explain why you are skipping the all_heaps struct?
I do trust that there is a good reason for doing that, but by reading the code,
it reads like "let's skip all heaps" :)

[chrisha, 2015/04/23 09:23:28]

From my reading it looks like Seb is trying not to dump information about heap
structures used temporarily by the heap dumping code. Is this something that you
guys actually care about? If so, do you provide an alternative heap to use for
all memory dumping related code?

[Sébastien Marchand, 2015/04/23 10:52:04]

Yeah, my goal is to doesn't include the memory used by this provider in the
calculation. This way we can get a more accurate number of how much memory
Chrome actually use.

[Sébastien Marchand, 2015/04/23 10:52:04]

Acknowledged.

+
+  // Retrieves some metrics about each heaps.

[chrisha, 2015/04/23 09:23:28]

each heap*.

[Sébastien Marchand, 2015/04/23 10:52:04]

Done.

+  for (size_t i = 0; i < number_of_heaps; ++i) {
+    WinHeapInfo heap_info = { 0 };
+    heap_info.heap_id = all_heaps[i];
+    GetHeapInformation(&heap_info, block_to_skip);
+
+    all_heap_info.allocated_size += heap_info.allocated_size;

[Primiano Tucci (use gerrit), 2015/04/22 23:48:32]

If you want you can ReportHeapDump the heaps invidivually here (using the a
unique string each time) and the UI will do the math to add up (it's not there
yet but will be soon)

[chrisha, 2015/04/23 09:23:28]

Yeah, it would be good to report them using names like we discussed in that
group meeting. Something like winheap/heap-#{HEAPID}, and
winheap/heap-#{HEAPID}-process-heap, or what have you (not sure if there's a
preferred naming style, lowercase, CamelCase, snake_style, dashes or
underscores, what have you).

[Sébastien Marchand, 2015/04/23 10:52:03]

If it's not there yet then I'd prefer to stick with this approach, it's easier
to test as is. And I'll still need this code later (when I'll report the details
about each heap).

[Sébastien Marchand, 2015/04/23 10:52:04]

I'm planning to report all the heaps individually, but currently the UI doesn't
support it really well... There's at least 4-5 heaps per process (most of them
aren't used, except for the process heap) and they all get displayed into a
distinct column, so with 5 process you end up with an array of 5 lines and ~15
columns, so you have to scroll horizontally a lot to find the information you're
looking for... So I'd prefer to make it work first (i.e. report only the
all_heaps data) and I'll try to get more detailed data in a further CL.

[Primiano Tucci (use gerrit), 2015/04/23 11:10:02]

Yeah, makes sense.
Maybe add a todo or file a bug just to make sure we don't lose track of it

+    all_heap_info.committed_size += heap_info.committed_size;
+    all_heap_info.block_count += heap_info.block_count;
+  }
+  // Report the heap dump.
+  if (!ReportHeapDump(pmd, all_heap_info, MemoryAllocatorDump::kRootHeap))
+    return false;
+
+  return true;
+}
+
+const char* WinHeapDumpProvider::GetFriendlyName() const {
+  return kDumperFriendlyName;
+}
+
+bool WinHeapDumpProvider::GetHeapInformation(WinHeapInfo* heap_info,
+    const std::set<void*>& block_to_skip) {
+  DCHECK_NE(reinterpret_cast<WinHeapInfo*>(nullptr), heap_info);
+  CHECK(::HeapLock(heap_info->heap_id) == TRUE);
+  PROCESS_HEAP_ENTRY heap_entry;
+  heap_entry.lpData = nullptr;
+  // Walk over all the entries in this heap.
+  while (::HeapWalk(heap_info->heap_id, &heap_entry) != FALSE) {
+    if (block_to_skip.find(heap_entry.lpData) != block_to_skip.end())

[Primiano Tucci (use gerrit), 2015/04/22 23:48:32]

You can do if (block_to_skip.count(heap_entry.lpData) == 1) and probably save
some cycle per iteration

[Sébastien Marchand, 2015/04/23 10:52:03]

Done.

+      continue;
+    if ((heap_entry.wFlags & PROCESS_HEAP_ENTRY_BUSY) != 0) {
+      heap_info->allocated_size += heap_entry.cbData;
+      heap_info->block_count++;
+    } else if ((heap_entry.wFlags & PROCESS_HEAP_REGION) != 0) {
+      heap_info->committed_size += heap_entry.Region.dwCommittedSize;
+    }
+  }
+  CHECK(::HeapUnlock(heap_info->heap_id) == TRUE);
+  return true;
+}
+
+}  // namespace trace_event
+}  // namespace base