929797 - Implement proper memory reporting for child processes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

about:memory can handle child processes, at least, it works if you press the "Measure" button.  But the way it works is really ugly -- it sends a message to the child processes, then measures and displays results for the parent process.  If a child process reports back, it will re-measure everything (including the parent process) and redisplay.  If another child process reports back, it will redisplay again.

This is stupid and ugly.  It also requires each consumer of memory reports to deal with the IPC itself.  For this reason, all the other consumers of memory reports (e.g. dumping them to file via about:memory's "Measure and save..." button) only handle a single process.  (Indeed, the "maybe you'll get stuff from a child, in which case start again" strategy is clearly incompatible with writing to a file.)

The aim of this bug is to implement a new strategy, one that hides the IPC from the memory report consumers.  It has to be async, to allow for the IPC.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Implement proper memory reporting for child processes.

This patch implements proper child process memory reporting.  It does the
following.

- Adds a new getReports() method to nsIMemoryReporterManager that encapsulates
  all the IPC.  A consumer just passes it a |handleReport| closure (as before)
  and a |finishReporting| closure.

- Converts about:memory to use getReports().

- Modifies dumpMemoryReportsToNamedFile() so it works with child processes (it
  does this by calling getReports()).  The changes aren't as big as they seem;
  it's mostly code rearrangement due to the async-ness.

- Converts the C++ file dumping code used by "Measure and save..." to use
  the new dumpMemoryReportsToNamedFile().

The patch doesn't change the way B2G memory reporting is done -- that still
uses the per-process API (enumerateReporters/collectReports), triggered via the
signal-based mechanism, and controlled by the get_about_memory.py script.  It
works fine, so not much point changing it.

Specific things worth noting:

- There's a fairly detailed description of how getReports() works in a big
  comment in nsMemoryReporterManager.h.  Read that first.

- ContentParent passes child reports directly (in an array) to
  nsMemoryReporterManager, instead of being cute and bundling them up in a
  ChildReporter and then registering that reporter.  This is much simpler, and
  avoids the need for the "child-memory-reporter-update" event.

- aboutMemory.js now shows "Measuring..." while waiting for the child reporters
  to report back.  This is similar to the "Loading..." already used when
  loading from file.

- The test_aboutmemory3.xul changes are simpler than they look.  It's just
  some rearrangement required because dumpMemoryReportsToNamedFile() is now
  async.

- The test_memoryReports2.xul changes just remove all the manual observer crap
  and use getReports().  Much simpler.

- The time-out for child processes is 5 seconds.

I haven't added new tests yet.  Before landing, I will definitely add one for
multi-process file dumping.  I'd also like to add ones for the various failure
paths (e.g.  child processes failing to report back, or reporting back late),
but that's harder.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Implement proper memory reporting for child processes.

This version of the patch adds more testing:

- It tests dumping reports to file in the presence of multiple children.

- It tests that a call to getReports() silently aborts if a prior request has
  not completed.

There are still no tests for when child processes fail to report back before
the timeout hits.  I can't see how to do that reliably, other than having a
second version of getReports() -- for testing purposes only -- that
deliberately doesn't send the IPC messages to the child processes.  Or setting
a special flag in child processes that means "ignore memory reporter requests".
Either of which would be ugly.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 825739 [details] [diff] [review]
Implement proper memory reporting for child processes.

Review of attachment 825739 [details] [diff] [review]:
-----------------------------------------------------------------

So what happens if processes are created/destroyed during collection of memory reports?  It seems like it would be possible for us to quit too early or timeout.  I think you need to copy the number of processes into a separate variable when we start collection that remains constant throughout the collection.

::: dom/ipc/ContentParent.cpp
@@ +1998,5 @@
>                                        nsDependentString(aData)))
>              return NS_ERROR_NOT_AVAILABLE;
>      }
>      else if (!strcmp(aTopic, "child-memory-reporter-request")) {
> +        unused << SendPMemoryReportRequestConstructor((uint32_t)(uintptr_t)aData);

Yuck.

::: xpcom/base/nsIMemoryInfoDumper.idl
@@ +14,1 @@
>  [scriptable, builtinclass, uuid(3FFA5113-2A10-43BB-923B-6A2FAF67BE97)]

this needs an iid change.

::: xpcom/base/nsIMemoryReporter.idl
@@ +193,2 @@
>     */
> +  void init();

Why did you switch this

@@ +220,2 @@
>     */
> +  nsISimpleEnumerator enumerateReporters();

with this?

@@ +226,5 @@
> +   * once all reports have been handled.
> +   *
> +   * |finishReporting| is called even if, for example, some child processes
> +   * fail to report back.  However, calls to this method will silently and
> +   * immediately abort -- and |finishReporting| will not be called -- if a

Why silently?  Why not throw?

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +874,5 @@
>      enumerator.forget(aResult);
>      return NS_OK;
>  }
>  
> +#define DEBUG_CHILD_PROCESS_MEMORY_REPORTING 1  // njn: 0 before landing

make sure you remember to change this ;-)

::: xpcom/base/nsMemoryReporterManager.h
@@ +30,5 @@
>  
> +  // Gets the memory reporter manager service.
> +  static nsMemoryReporterManager* Get()
> +  {
> +      nsCOMPtr<nsIMemoryReporterManager> imgr =

two space indents for consistency.

Also Gecko style would be to call this GetOrCreate since it can create the singleton in question.

@@ +126,5 @@
>  private:
> +  nsresult RegisterReporterHelper(nsIMemoryReporter* aReporter, bool aForce);
> +
> +  static void TimeoutCallback(nsITimer* aTimer, void* aData);
> +  static const uint32_t TimeoutLengthMS = 5000;

kTimeoutLengthMS

@@ +132,5 @@
>    nsTHashtable<nsISupportsHashKey> mReporters;
>    Mutex mMutex;
>    bool mIsRegistrationBlocked;
> +
> +  int mNumChildProcesses;

Please use a fixed width type.

@@ +144,5 @@
> +    nsCOMPtr<nsISupports>                mHandleReportData;
> +    nsCOMPtr<nsIFinishReportingCallback> mFinishReporting;
> +    nsCOMPtr<nsISupports>                mFinishReportingData;
> +
> +    int                                  mNumChildProcessesCompleted;

Here too.

@@ +162,5 @@
> +    {}
> +  };
> +
> +  // When this is non-null, a request is in flight.
> +  GetReportsState* mGetReportsState;

This should be an nsAutoPtr.

Comment 4

[Nicholas Nethercote [inactive]]

> ::: xpcom/base/nsIMemoryReporter.idl
> @@ +193,2 @@
> >     */
> > +  void init();
> 
> Why did you switch this
> 
> @@ +220,2 @@
> >     */
> > +  nsISimpleEnumerator enumerateReporters();
> 
> with this?

Just because it makes sense to have the init() method first.  (Admittedly, it is orthogonal to the rest of the patch.)


> @@ +226,5 @@
> > +   * once all reports have been handled.
> > +   *
> > +   * |finishReporting| is called even if, for example, some child processes
> > +   * fail to report back.  However, calls to this method will silently and
> > +   * immediately abort -- and |finishReporting| will not be called -- if a
> 
> Why silently?  Why not throw?

From nsMemoryReporterManager.h:

+  // - If a request is made while the previous request is in flight, the new
+  //   request is ignored, as per getReports()'s specification.  No error is
+  //   reported, because the previous request will complete soon enough.

In practice, in about:memory if it threw, we'd end up in the exception-handling case, which causes an error message to be shown.  That message would (I think) immediately be overwritten with the results from the prior request.  In other words, if it did throw an exception, all the callers would have to be aware of it, but they'd almost certainly just ignore it anyway.

I'll clarify this in the comment.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cca1dea21a0d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cca1dea21a0d

Comment 7

[[e-mail]]

Comment on attachment 825739 [details] [diff] [review]
Implement proper memory reporting for child processes.

>-  NS_IMETHOD CollectReports(nsIMemoryReporterCallback* aCallback,
>+  NS_IMETHOD CollectReports(nsIHandleReport* aHandleReport,
Typo?

Comment 8

[Nicholas Nethercote [inactive]]

> >-  NS_IMETHOD CollectReports(nsIMemoryReporterCallback* aCallback,
> >+  NS_IMETHOD CollectReports(nsIHandleReport* aHandleReport,
> Typo?

No.  nsIHandleReport is typedef'd to nsIMemoryReporterCallback.

Comment 9

[Nicholas Nethercote [inactive]]

> > >-  NS_IMETHOD CollectReports(nsIMemoryReporterCallback* aCallback,
> > >+  NS_IMETHOD CollectReports(nsIHandleReport* aHandleReport,
> > Typo?
> 
> No.  nsIHandleReport is typedef'd to nsIMemoryReporterCallback.

Oh, you're right.  It should be nsIHandleReportCallback.  Reported in bug 936074.