Report utility process JS memory in task manager.

chrome/browser/task_manager/child_process_resource_provider.cc

Index: chrome/browser/task_manager/child_process_resource_provider.cc
diff --git a/chrome/browser/task_manager/child_process_resource_provider.cc b/chrome/browser/task_manager/child_process_resource_provider.cc
index bf74138859e5f6f83281fd1ff163b236542c0d3f..2273305436f4358c4561ed9c932c597fd5fa408d 100644
--- a/chrome/browser/task_manager/child_process_resource_provider.cc
+++ b/chrome/browser/task_manager/child_process_resource_provider.cc
@@ -7,14 +7,18 @@
 #include <vector>
 
 #include "base/i18n/rtl.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/task_manager/resource_provider.h"
 #include "chrome/browser/task_manager/task_manager.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/nacl/common/nacl_process_type.h"
+#include "content/public/browser/browser_child_process_host.h"
 #include "content/public/browser/browser_child_process_host_iterator.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_data.h"
+#include "content/public/browser/process_resource_usage.h"
+#include "content/public/common/service_registry.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -22,6 +26,7 @@
 
 using content::BrowserChildProcessHostIterator;
 using content::BrowserThread;
+using content::ProcessResourceUsage;
 using content::WebContents;
 
 namespace task_manager {
@@ -43,6 +48,10 @@ class ChildProcessResource : public Resource {
   Type GetType() const override;
   bool SupportNetworkUsage() const override;
   void SetSupportNetworkUsage() override;
+  void Refresh() override;
+  bool ReportsV8MemoryStats() const override;
+  size_t GetV8MemoryAllocated() const override;
+  size_t GetV8MemoryUsed() const override;
 
   // Returns the pid of the child process.
   int process_id() const { return pid_; }
@@ -52,6 +61,12 @@ class ChildProcessResource : public Resource {
   // process would be "Plugin: Flash" when name is "Flash".
   base::string16 GetLocalizedTitle() const;
 
+  static void GetProcessUsageOnIOThread(
+      int id, base::WeakPtr<ChildProcessResource> weak_ptr);
+
+  void OnGetProcessUsageDone(
+      mojo::InterfacePtrInfo<content::ResourceUsageReporter> info);
+
   int process_type_;
   base::string16 name_;
   base::ProcessHandle handle_;
@@ -59,17 +74,43 @@ class ChildProcessResource : public Resource {
   int unique_process_id_;
   mutable base::string16 title_;
   bool network_usage_support_;
+  scoped_ptr<ProcessResourceUsage> resource_usage_;
 
   // The icon painted for the child processs.
   // TODO(jcampan): we should have plugin specific icons for well-known
   // plugins.
   static gfx::ImageSkia* default_icon_;
 
+  base::WeakPtrFactory<ChildProcessResource> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ChildProcessResource);
 };
 
 gfx::ImageSkia* ChildProcessResource::default_icon_ = NULL;
 
+// static
+void ChildProcessResource::GetProcessUsageOnIOThread(

[afakhry, 2015/05/06 17:16:27]

What do you think of moving the functionality of:

- ChildProcessResource::GetProcessUsageOnIOThread(),
- ChildProcessResource::OnGetProcessUsageDone()

to the factory of the ProcessResourceUsage?

Somehow I feel like this code doesn't belong here, but rather the factory should
take care of this complexity and the threading model.
I think for the ChildProcessResource, all it cares about is maybe something like
this:

resource_usage_ = content::ProcessResourceUsage::Create(unique_process_id_);

and then the ProcessResourceUsage objects tracks whether it's ready or not.

That's just my opinion. Nick has the final say in this.

[ncarter (slow), 2015/05/07 20:12:44]

GetProcessUsageInfo probably can move into ProcessResourceUsage or similar;
especially if you switch to using PostTaskAndReply [see next comment...]

Some callback point like OnGetProcessUsageDone needs to live in
ChildProcessResource; while I see afakhry's point that the
InterfacePtrInfo-to-ProcessResourceUsage conversion might be better if hidden in
the ProcessResourceUsage class, we risk losing the elegance of
WeakPtr-cancellation if the reply callback isn't a member of
ChildProcessResource.

Here's the best solution I came up with:
 - use PostTaskAndReplyWithResult
 - Move the static GetProcessUsageInfo() function into ProcessResourceInfo
 - Leave OnGetProcessUsageDone as a method of ChildProcessResource.
 - Change ProcessResourceUsage::Create() so that it accepts a
mojo::InterfacePtrInfo instead of a content::ResourceUsageReporterPtr (thereby
moving a few lines of code from OnGetProcessUsageDone to
ProcessResourceUsage::Create)

[Anand Mistry (off Chromium), 2015/05/11 05:41:31]

Apart from PostTaskAndReplyWithResult, I'm going to push-back on these
suggestions. My reasoning:
1. I don't think the logic for resolving a process id to a Mojo service belongs
in ProcessResourceUsage. Right now, that class only knows about the Mojo service
and nothing else. The service could be running in a different process, or the
same process, it doesn't care. If the constructor was changes to take a process
id or ChildProcessData, then it would be different. But...
2. This specific logic only applies to non-renderer processes. There would need
to be something different for handling renderer processes (which I plan to do
next).

As for taking a mojo::InterfacePtrInfo instead of a ResourceUsageReporterPtr,
this doesn't really fit into the Mojo way. InterfacePtrInfo should be a rarely
used detail, and a connection to the service is canonically represented by a
mojo::InterfacePtr<> (which ResourceUsageReporterPtr is a typedef of). In fact,
none of the other Mojo services in Chromium use it. That said, there's a better
way, but it requires changes that's outside the scope of this CL.

[ncarter (slow), 2015/05/11 20:35:39]

I'm fine with the compromise you've struck. My initial comments were not
informed by any understanding of the difference between InterfacePtrInfo and
ResourceUsageReporterPtr; I was more just reasoning about which lines of code
would need to be duplicated when we hook this up to WebContentsResourceProvider,
and organizing the functions accordingly.

+    int id, base::WeakPtr<ChildProcessResource> weak_ptr) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  content::BrowserChildProcessHost* host =
+      content::BrowserChildProcessHost::Get(id);
+  if (!host)
+    return;
+
+  content::ServiceRegistry* registry = host->GetServiceRegistry();
+  if (!registry)
+    return;
+
+  content::ResourceUsageReporterPtr service;
+  registry->ConnectToRemoteService(&service);
+  auto interface_info = service.PassInterface();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &ChildProcessResource::OnGetProcessUsageDone,
+          weak_ptr, base::Passed(&interface_info)));
+}
+
 ChildProcessResource::ChildProcessResource(
     int process_type,
     const base::string16& name,
@@ -79,7 +120,8 @@ ChildProcessResource::ChildProcessResource(
       name_(name),
       handle_(handle),
       unique_process_id_(unique_process_id),
-      network_usage_support_(false) {
+      network_usage_support_(false),
+      weak_factory_(this) {
   // We cache the process id because it's not cheap to calculate, and it won't
   // be available when we get the plugin disconnected notification.
   pid_ = base::GetProcId(handle);
@@ -88,11 +130,26 @@ ChildProcessResource::ChildProcessResource(
     default_icon_ = rb.GetImageSkiaNamed(IDR_PLUGINS_FAVICON);
     // TODO(jabdelmalek): use different icon for web workers.
   }
+  BrowserThread::PostTask(

[ncarter (slow), 2015/05/07 20:12:43]

It might work well to structure this as a PostTaskAndReplyWithResult, channging
the return type of GetProcessUsageOnIOThread from void to
mojo::InterfacePtrInfo<content::ResourceUsageReporter> or a scoped_ptr thereof.

I see two advantages to that: one, the IO thread doesn't get to see the WeakPtr
at all (so it's easier to comprehend the threading model), and two, the purpose
of GetProcessUsageOnIOThread becomes better reflected in type signature.

[Anand Mistry (off Chromium), 2015/05/11 05:41:31]

Done.

Aside: Many of Mojo's types are move-only, so no need to scoped_ptr<> in many
places. Of course, the exception is if you want to avoid a compile-time
dependency on the type definition.

[ncarter (slow), 2015/05/11 20:35:40]

Great to know, thanks!

+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ChildProcessResource::GetProcessUsageOnIOThread,
+          unique_process_id, weak_factory_.GetWeakPtr()));
 }
 
 ChildProcessResource::~ChildProcessResource() {
 }
 
+void ChildProcessResource::OnGetProcessUsageDone(
+    mojo::InterfacePtrInfo<content::ResourceUsageReporter> info) {

[ncarter (slow), 2015/05/07 20:12:43]

Just curious -- do we expect that this approach will work fine if we try create
two ProcessResourceUsages for the same process (I think this is less of an issue
with utility/child processes, but will certainly come up in the context of
renderer processes, where it's common to have two task manager rows reporting
stats for the same process.

[Anand Mistry (off Chromium), 2015/05/11 05:41:31]

Yes. I see no reason why this wouldn't work. Creating two Mojo connections would
result in two separate Mojo service implementations being instantiated in the
child process. However, as long as those are independent, which they should be,
there won't be a problem.

[ncarter (slow), 2015/05/11 20:35:40]

Excellent.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (info.is_valid()) {
+    content::ResourceUsageReporterPtr service;
+    service.Bind(info.Pass());
+    resource_usage_ = content::ProcessResourceUsage::Create(service.Pass());
+  }
+}
+
 // Resource methods:
 base::string16 ChildProcessResource::GetTitle() const {
   if (title_.empty())
@@ -200,6 +257,29 @@ base::string16 ChildProcessResource::GetLocalizedTitle() const {
   return title;
 }
 
+void ChildProcessResource::Refresh() {
+  if (resource_usage_)
+    resource_usage_->Refresh();

[afakhry, 2015/05/06 17:16:27]

Is it possible if you add a callback of type:
void(size_t, size_t) to ProcessResourceUsage::Refresh()? You Run() this callback
when the refresh is complete which will in turn set two member variables inside
ChildProcessResource. Those two member variables will be used in
ChildProcessResource::GetV8MemoryAllocated() and
ChildProcessResource::GetV8MemoryUsed() instead of querying the resource_usage_
every time. What do you think?

[ncarter (slow), 2015/05/07 20:12:43]

amistry has addressed this to my satisfaction in another comment: the existing
implementaiton is polling-based and he's only seeking parity with that. afakhry
has a good suggestion here and we're all on the same page that it's a good idea,
but it will be best to implement it as a follow-on change; this CL already has
plenty going on.

+}
+
+bool ChildProcessResource::ReportsV8MemoryStats() const {
+  if (resource_usage_)

[ncarter (slow), 2015/05/07 20:12:43]

It's not clear if a |resource_usage_| will be successfully created for every
child process, or just the ones that actually have spun up a V8 instance.

Will e.g. the GPU process continue to show "N/A" in the Javascript Memory
column, or will it show start showing "0KB"?

[Anand Mistry (off Chromium), 2015/05/11 05:41:31]

If the process host has a ServiceRegistry, then yes, it will be created.
However, there's no ability to guarantee that the Mojo service is provided by
the child process at construction time. So instead, the |resource_usage_| is
created and has a default state, which in this case is false for
ReportsV8MemoryStats. That'll be changed on the first IPC response from the
child process.
It shows N/A. Basically, everything using this will show N/A until the first IPC
response from the child process.

[ncarter (slow), 2015/05/11 20:35:40]

Acknowledged.

+    return resource_usage_->ReportsV8MemoryStats();
+  return false;
+}
+
+size_t ChildProcessResource::GetV8MemoryAllocated() const {
+  if (resource_usage_)
+    return resource_usage_->GetV8MemoryAllocated();
+  return 0;
+}
+
+size_t ChildProcessResource::GetV8MemoryUsed() const {
+  if (resource_usage_)
+    return resource_usage_->GetV8MemoryUsed();
+  return 0;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ChildProcessResourceProvider class
 ////////////////////////////////////////////////////////////////////////////////