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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

({memory-footprint, mobile})

unspecified
memory-footprint, mobile
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

(Whiteboard: [has-patch])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
tracking-fennec: --- → ?
(Assignee)

Comment 2

8 years ago
Created attachment 511494 [details] [diff] [review]
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?
Assignee: nobody → doug.turner
Attachment #511494 - Flags: review?(jones.chris.g)

Updated

8 years ago
Duplicate of this bug: 630755
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.
Attachment #511494 - Flags: review?(jones.chris.g) → feedback+
(Assignee)

Comment 5

8 years ago
Created attachment 511818 [details] [diff] [review]
patch v.2
Attachment #511818 - Flags: review?(jones.chris.g)
(Assignee)

Updated

8 years ago
Attachment #511818 - Attachment is patch: true
Attachment #511818 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 6

8 years ago
(ignore the change to gfx/thebes/gfxBlur.cpp)
tracking-fennec: ? → 2.0+
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.
Attachment #511818 - Flags: review?(jones.chris.g)
(Assignee)

Comment 8

7 years ago
Created attachment 512844 [details] [diff] [review]
patch v.3
Attachment #511494 - Attachment is obsolete: true
Attachment #511818 - Attachment is obsolete: true
Attachment #512844 - Flags: review?(jones.chris.g)
(Assignee)

Comment 9

7 years ago
Created attachment 512847 [details] [diff] [review]
patch v.4

lets cleanup.
Attachment #512844 - Attachment is obsolete: true
Attachment #512844 - Flags: review?(jones.chris.g)
Oh, forgot to mention you need to clear the registered reporters when the ContentParent goes away.
(Assignee)

Comment 11

7 years ago
why?
Does it make sense for them to stick around forever, possibly after we've created a new content process after a crash, say?
Just need SetChildMemoryReporters(AutoInfallibleTArray()) in ActorDestroy(), I think.
(Assignee)

Comment 14

7 years ago
makes perfect sense.  also clear the observer.
(Assignee)

Comment 15

7 years ago
Created attachment 513612 [details] [diff] [review]
patch v.5
Attachment #512847 - Attachment is obsolete: true
Attachment #513612 - Flags: review?(jones.chris.g)
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.
Attachment #513612 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 17

7 years ago
cjones, it was a suggestion.  I think holding the strong ref in this specific cause is fine.
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.
is this ready to land?
Keywords: footprint, mobile
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [has-patch]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

7 years ago
bounced.  getpid() isn't defined on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
pushed http://hg.mozilla.org/mozilla-central/rev/410519307e63
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
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!
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.
Depends on: 653564
Depends on: 655583
> 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?
You need to log in before you can comment on or make changes to this bug.