1061066 - Make DMD work properly with e10s

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Nicholas Nethercote [inactive]]

DMD currently sort of works with e10s.

If you trigger DMD output via the signal or fifo mechanism (this includes via get_about_memory.py on B2G) then DMD dumps one output file for the signalled process and all its child processes into the temp directory. This is fine. (If you signal a child process you'll just get output for that process, but that seems reasonable.)

However, if you trigger DMD output via the "Analyze Reports" or "Analyze Heap" buttons in about:memory, it just dumps output for the parent process, and it writes it to reports.dmd or heap.dmd in the current working directory. This needs to be fixed.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make about:memory's "Analyze Reports" button for DMD work properly with e10s

This changes the "Analyze reports" button to use the same codepaths as the
signal/fifo mechanism. As a result, it (a) produces output for all processes,
which is good, and (b) write that output to the temp directory, which is
acceptable.

This patch doesn't do anything for the "Analyze heap" button, though. That'll
need a different mechanism, and so I'm not entirely sure if this patch is the
right way forward.

Comment 2

[Nicholas Nethercote [inactive]]

> This patch doesn't do anything for the "Analyze heap" button, though. That'll
> need a different mechanism, and so I'm not entirely sure if this patch is the
> right way forward.

My best idea is this: DMD's AnalyzeReports functionality is so closely tied to the memory reporters that it makes sense for it to piggyback onto the mechanism for telling child processes that they need to get memory reports (as it currently does).

But AnalyzeHeap, and mccr8's new SCC stuff, isn't tied to memory reporting. So they should have a separate mechanism. Basically we'd pass a message to the child process(es) and the name of the operation they should run.

Comment 3

[Andrew McCreight [:mccr8]]

FWIW, right now for heap scanning, I'm interested in the state of the heap late in shutdown, so I am putting some code into XPCOM shutdown that triggers DMD for that process.  Eventually we'll want to have that work via environment variable, and I'm not sure how that works with e10s.

Comment 4

[Andrew McCreight [:mccr8]]

Though it isn't sandboxing-safe to just start writing a file.  We're also going to be deep enough in shutdown that maybe IPC won't work.  But anyways, we can deal with that use case elsewhere.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Make DMD work properly with e10s

This patch doubles-down on the previous patch's approach.

- It removes the "Analyze Heap" button from about:memory. It's barely used and
  not e10s-aware. This functionality will be able to be re-implemented via
  post-processing once machine-readable DMD output is in place (bug 1044709).

- It renames the "Analyze Reports" button in about:memory (going back to the
  pre-bug 1035570 name) and changes it to dump memory reports and DMD output to
  $TMPDIR. So you can't get DMD output dumped without also getting memory
  reports dumped, but that's not a big deal, IMO.

- It removes the ability to trigger DMD from JS, because it's no longer used.

Net effect:

 2 files changed, 18 insertions(+), 143 deletions(-)

Comment 6

[Nicholas Nethercote [inactive]]

> FWIW, right now for heap scanning, I'm interested in the state of the heap
> late in shutdown, so I am putting some code into XPCOM shutdown that
> triggers DMD for that process.  Eventually we'll want to have that work via
> environment variable, and I'm not sure how that works with e10s.
>
> Though it isn't sandboxing-safe to just start writing a file.  We're also
> going to be deep enough in shutdown that maybe IPC won't work.  But anyways,
> we can deal with that use case elsewhere.

Mmm, yes, that sounds like it'll need a different mechanism.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8482448 [details] [diff] [review]
Make DMD work properly with e10s

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

It seems reasonable to me to cut out Analyze Heap from about:memory for now.  It is still on the experimental side of things, and anybody who really needs it can hack up a build themselves.

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8482448 [details] [diff] [review]
Make DMD work properly with e10s

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

I figured one review was enough for this, esp. since erahm is on vacation until next week.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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