Adding blink gc memory dump infrastructure for thread specific dumps.

Source/platform/heap/BlinkGCMemoryDumpProvider.cpp

 // 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 "config.h"
 #include "Source/platform/heap/BlinkGCMemoryDumpProvider.h"
 
 #include "platform/heap/Handle.h"
+#include "public/platform/Platform.h"
 #include "public/platform/WebMemoryAllocatorDump.h"
 #include "public/platform/WebProcessMemoryDump.h"
 #include "wtf/StdLibExtras.h"
 
 namespace blink {
 
 BlinkGCMemoryDumpProvider* BlinkGCMemoryDumpProvider::instance()
 {
     DEFINE_STATIC_LOCAL(BlinkGCMemoryDumpProvider, instance, ());
     return &instance;
 }
 
-bool BlinkGCMemoryDumpProvider::onMemoryDump(blink::WebProcessMemoryDump* memoryDump)
-{
-    WebMemoryAllocatorDump* allocatorDump = memoryDump->createMemoryAllocatorDump("blink_gc");
-    allocatorDump->AddScalar("inner_size", "bytes", Heap::allocatedObjectSize());
-    allocatorDump->AddScalar("outer_size", "bytes", Heap::allocatedSpace());
-    allocatorDump->AddScalar("estimated_live_object_size", "bytes", Heap::estimatedLiveObjectSize());
-    return true;
-}
-
-BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider()
-{
-}
-
 BlinkGCMemoryDumpProvider::~BlinkGCMemoryDumpProvider()
 {
 }
 
+bool BlinkGCMemoryDumpProvider::onMemoryDump(WebProcessMemoryDump* memoryDump)
+{
+    // The current heap statistics are added to the memory dump.
+    // TODO(ssid): For consistency these values should be dumped at the time of
+    // GC (when statistics are collected). crbug.com/491248.
+    WebMemoryAllocatorDump* allocatorDump = memoryDump->createMemoryAllocatorDump("blink_gc");
+    allocatorDump->AddScalar("inner_size", "bytes", Heap::allocatedObjectSize());
+    allocatorDump->AddScalar("outer_size", "bytes", Heap::allocatedSpace());
+    allocatorDump->AddScalar("estimated_live_object_size", "bytes", Heap::estimatedLiveObjectSize());
+
+    // If available, the last GC memory statistics are added.

[haraken, 2015/05/26 12:21:59]

I'm not sure if this is a good design. I think we have two options:

Option 1: Take the memory dump at each GC and return the latest dump when
onMemoryDump is requested.

Option 2: When onMemoryDump is requested, force a GC (which disables sweeping)
and get the dump.

I guess Option 2 would be more straightforward. As you pointed out in the above
comment, the Option 1 will end up with the inconsistency between "inner_size",
"outer_size" etc and the dump taken in the latest GC.

[Primiano Tucci (use gerrit), 2015/05/26 14:18:03]

See my previous comment on this [1]. Regardless of what option we choose, we
should dump all these stats at the same time.

[1]
https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B...
This is a good option. I think we can smooth this a bit and say: request a dump
when a GC happens. The dump might happen or not (the manager might decide to
rate-limit), and if it happens we know that all the oilpan numbers are taken at
the same time.
This is sub-optimal (because the oilpan numbers might be earlier than other
dumpers' numbers) but probably easier to get in the short term.
No idea of how much complex would be to implement this. What happens if the
other threads are active? Does it mean that the thread on which onMemoryDump is
invoked (the main thread) is going to wait for the other thread to enter a safe
point and proceed?
If this is not too complex to implement we can go straight to option 2.
Otherwise we could get started with Option 1 (% fixing the consistency issue)
and then migrate to the on-demand GC when available.
Yeah, I agree with this. Regardless of the option, let's dump everything
together.

+    if (m_lastProcessMemoryDump)
+        memoryDump->takeAllDumpsFrom(m_lastProcessMemoryDump.get());
+
+    // TODO(ssid): This should return false in the unlikely case that we didn't
+    // see a GC yet, which in turn will cause the dump to be invalidated. But,
+    // right now returning false will stop the manager from getting more dumps
+    // from this provider. So, always returns true while crbug.com/490783 gets
+    // fixed.
+
+    return true;
+}
+
+BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider()
+    : m_lastProcessMemoryDump(adoptPtr(Platform::current()->createProcessMemoryDump()))
+{
+}
+
+WebMemoryAllocatorDump* BlinkGCMemoryDumpProvider::createMemoryAllocatorDumpForLastGC(const String& absoluteName)

[Primiano Tucci (use gerrit), 2015/05/26 11:24:06]

I think this should be PassOwnPtr<WebMemoryAllocatorDump> to make explicit the
fact that you want the caller to take the ownership of the WMAD

[ssid, 2015/05/27 13:15:06]

No, the caller can't take the ownership of the ptr because it can delete the ptr
which will result in the memoryDump not being able to use the dump.

[Primiano Tucci (use gerrit), 2015/05/27 14:19:30]

Oh right, I didn't realize this comes from createMemoryAllocatorDump, which owns
the dump. A raw pointer it's fine then.

+{
+    return m_lastProcessMemoryDump->createMemoryAllocatorDump(absoluteName);
+}
+
+void BlinkGCMemoryDumpProvider::clearLastGCProcessMemoryDump()

[Primiano Tucci (use gerrit), 2015/05/26 11:24:06]

And since you are here, I just realized that a better name for this could be
clearAllMemoryAllocatorDumpsForLastGC(), for naming consistency with the above

[ssid, 2015/05/27 13:15:06]

removed method.

+{
+    // TODO(ssid): Once the api for clearing dump is available, use that
+    // instead of this method here (crbug.com/491248).
+    m_lastProcessMemoryDump = adoptPtr(Platform::current()->createProcessMemoryDump());
+}
+
 } // namespace blink