633305 - about:memory should display memory reporters that live in the child process

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

about:memory currently doesn't show us anything about memory in the child process.  We should set it up so that about:memory shows all of the memory reporters in the child process.

Comment 1

[Doug Turner (:dougt)]

the basic idea here is:

1) create a new event from the parent to the child that informs the child that the parent wants update statistics.  We can also have a xpcom observer event that the PContentParent can listen for so that we make it easy for js code to force an update of these reporters.

2) create a new event from the child to the parent that reports on memory usage (a serialization of the nsMemoryReporter's).  When the parent gets this event, it can create and register n number of nsMemoryReporters that correspond the the reporters that live in the child.  (they can be prefixed with pid, or some string "Child Process - <type>".

3) since this is an async process, the code that current reports will have to deal with the race.  this can just be wallpapered over with a setTimeout as it is non-critical and we expect high performance of this event.  A time out of 1s should be more than fine.  If this is a problem we can investigate.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

mostly a work in progress.  i think it is good enough for the bloat testing that needs to happen.  chris, you wanna take a look?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 511494 [details] [diff] [review]
patch v.1

This looks like a good start.  You'd need vlad's review on the about:memory hookup, I don't know how that works.  I don't like relying on a timeout for the results.  This doesn't have to be racy by design.

I like the idea of attaching a PID or something to the output.

For the IPC bits, there are some things I'd like to see different: use the "request protocol" pattern and let IPDL handle the memory reporter pickling.  So you could have something like

 struct MemoryReport {
   nsCString prefix;
   nsCString path;
   nsCString desc;
   PRInt64 memoryUsed;
 };

 protocol PMemoryReportRequest {
 parent:
   recv __delete__(MemoryReport[] report);
 }

Moving the pickling code into IPDL is a no-brainer, no need to write what the compiler can generate for you.  You'd still need a conversion from MemoryReport->something accessible from script, but that's simple.  I think the request pattern would be useful here for two reasons: first so that you can attach an about:memory-scoped listener to PMemoryReportRequest instances.  That way if in PMemoryReportRequest::Recv__delete__() the listener has gone away, the report could just be dropped.  Second, to allow deferred processing of the memory-report request if it's received at an awkward time, like in a nested event loop or something.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Doug Turner (:dougt)]

(ignore the change to gfx/thebes/gfxBlur.cpp)

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 511818 [details] [diff] [review]
patch v.2

>+PMemoryReportRequestChild*
>+ContentChild::AllocPMemoryReportRequest()
>+{
>+    MemoryReportRequestChild* child = new MemoryReportRequestChild();
>+    return child;

|return new MemoryReportRequestChild();| ?

>+    nsRefPtr<ContentParent> mOwner;

Hmmm this is a little scary, but OK in this case.  I'd still recommend
instead using Manager(), with a helper like

  ContentParent* Owner()
  {
    return static_cast<ContentParent*>(Manager());
  }

which is guaranteed to be free of cycles and lifetime management
issues.

>+    nsCOMArray<nsIMemoryReporter> mMemoryReporters;

This is a pretty neat cache.  I'd document that.


>diff --git a/gfx/thebes/gfxBlur.cpp b/gfx/thebes/gfxBlur.cpp

Er oops?

>+function doLoad()
>+{
>+    var os = Components.classes["@mozilla.org/observer-service;1"].
>+        getService(Components.interfaces.nsIObserverService);
>+    os.notifyObservers(null, "child-memory-reporter", null);
>+
>+    setTimeout(delayLoad, 1000);
>+}

I still don't like this at all.  What about instead
 - load the known reporters immediately (which will pick up the cached child values
 - register to listen for a "new-child-memory-reporters" notification or something
 - fire off the child-memory-reporter request
 - after SetChildMemoryReporters(), fire off new-child-memory-reporters
 - when new-child-memory-reporters arrives here, rebuild the memory reports and
   |updateMemoryStatus()| (if that does what it appears and regens the page)

This doesn't seem like a lot more work and would I think give better results.

>diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp

Really vlad ought to be reviewing this stuff, but it looks OK to me.

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.4

lets cleanup.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, forgot to mention you need to clear the registered reporters when the ContentParent goes away.

Comment 11

[Doug Turner (:dougt)]

why?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Does it make sense for them to stick around forever, possibly after we've created a new content process after a crash, say?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Just need SetChildMemoryReporters(AutoInfallibleTArray()) in ActorDestroy(), I think.

Comment 14

[Doug Turner (:dougt)]

makes perfect sense.  also clear the observer.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 513612 [details] [diff] [review]
patch v.5

> Hmmm this is a little scary, but OK in this case.  I'd still recommend
> instead using Manager(), with a helper like
> 
>   ContentParent* Owner()
>   {
>     return static_cast<ContentParent*>(Manager());
>   }
> 
> which is guaranteed to be free of cycles and lifetime management
> issues.

Didn't address this.

r=me with that fixed.

Comment 17

[Doug Turner (:dougt)]

cjones, it was a suggestion.  I think holding the strong ref in this specific cause is fine.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's a bad habit to get into, because that code is duplicating, for no reason, a member IPDL-generated code creates automatically.  It's OK in this case because there will probably only be a handful of memory-report requests at any one time, and there's no reference cycle, but I don't want bad patterns to get copy-paste'd around our codebase.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

is this ready to land?

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/74a64df4f758

Comment 21

[Doug Turner (:dougt)]

bounced.  getpid() isn't defined on windows.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/410519307e63

Comment 23

[Nicholas Nethercote [inactive]]

FWIW, I was wondering why the code looks like this:

      MemoryReport memreport(nsPrintfCString("Content Process - %d - ", getpid()),
                             path,
                             desc,
                             memoryUsed);

And yet the child entries were always of the form "Content Process - malloc/allocated".  Turns out that nsPrintfCString by default has a max length of 15, so the " - %d - " is getting truncated.  You can ask for a longer string with eg. |nsPrintfCString(30, "...")|.

BTW, I think omitting the pid isn't necessarily a bad thing!

Comment 24

[Nicholas Nethercote [inactive]]

Oh, another thing.  AFAICT the memory reporters for the child are determined by iterating over the memory reporters for the parent.  The problem with this is that some reporters are not registered at start-up, but only when certain code is used.  So it's possible that a reporter that isn't registered by the parent should be registered and used by the child, but isn't.

Comment 25

[Nicholas Nethercote [inactive]]

> Oh, another thing.  AFAICT the memory reporters for the child are
> determined by iterating over the memory reporters for the parent.

Oh, I think I'm wrong about that.  At this line:

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#306

the do_GetService() call is getting the memory reporter manager for the
child, right?  In which case it's all ok.  Is that right, dougt?