1190483 - Add some way to trigger a DMD run late in shutdown

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Andrew McCreight [:mccr8]]

When fixing shutdown leaks with DMD, you need to trigger a DMD run late in shutdown. There should be something like this in the tree so it is easy to do with a few environment variables:
  https://bug1058178.bmoattachments.org/attachment.cgi?id=8615571

The DMD run is triggered at that location because it is after nsTraceRefcnt::Shutdown(), which cleans up a lot of the XPCOM leak checking entries which otherwise gum up the log.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Add a way to record a DMD log late in shutdown.

I tested this locally. Feel free to pass this review off to njn or somebody, Nathan, but I thought I should give you the right of first refusal, given that it dumps a chunk of ugly got in nsTraceRefcnt.

Comment 2

[Andrew McCreight [:mccr8]]

You run this with something like:
  MOZ_DMD_LOG_PROCESS=tab MOZ_DMD_SHUTDOWN_LOG=~/logs/dmd/dmd ./mach run --dmd --mode=scan -P debug

That will create a file like ~/logs/dmd/dmd-98629.log.gz on exit. The PROCESS specifier is optional. If not set, it will be recorded for all processes. This is useful for looking at content process leaks, because the main process log is quite large, and not recording it speeds things up.

Comment 3

[Andrew McCreight [:mccr8]]

In practice, it will probably be run something like this:

MOZ_CC_LOG_DIRECTORY=~/logs/ MOZ_CC_LOG_PROCESS=content MOZ_CC_LOG_THREAD=main MOZ_CC_LOG_SHUTDOWN=1 MOZ_DMD_LOG_PROCESS=tab MOZ_DMD_LOG=~/logs/dmd ./mach run --dmd --mode=scan -P debug

That records cycle collector, garbage collector and DMD logs and shutdown, and puts them in the ~/logs/ directory. With the cycle collector log, you can look up which nsGlobalWindow has missing references, and what the known references are, and try to match them up to the references to that block in the DMD log.

Comment 4

[Andrew McCreight [:mccr8]]

> ugly got in nsTraceRefcnt.
That should be "ugly code", not "ugly got".

This should be documented on the DMD page.

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8647643 [details] [diff] [review]
Add a way to record a DMD log late in shutdown.

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

Overall seems like a good addition. I think it might make sense to move the implementation into nsMemoryInfoDumper, but considering this is limited to nsTraceRefcnt's cpp file it's probably not a big deal. r=me either way.

Also please update the DMD docs when this lands (it might be nice to document the possible process types as well).

::: xpcom/base/nsTraceRefcnt.cpp
@@ +973,5 @@
> +    return;
> +  }
> +
> +  nsPrintfCString fileName("%s-%d.log.gz", dmdFilePrefix, base::GetCurrentProcId());
> +  FILE* logFile = fopen(fileName.get(), "w");

We should error check this return value.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8647643 [details] [diff] [review]
Add a way to record a DMD log late in shutdown.

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

::: xpcom/base/nsTraceRefcnt.cpp
@@ +952,5 @@
>  
> +#ifdef MOZ_DMD
> +// If MOZ_DMD_SHUTDOWN_LOG is set, dump a DMD report to a file.
> +// The value of this environment variable is used as the prefix
> +// of the file name, so you probably want something like /tmp/dmd

Would it be simpler to just specify a directory, minus the prefix? Or even, not specify one at all and let DMD put it in the usual spot. (Though that might be tricky given that you're doing this from a non-DMD file.)

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Would it be simpler to just specify a directory, minus the prefix?

I think that code would have to be platform specific ("\" vs "/"). Oh I guess I could make the prefix include the slash. Maybe that's better?

> Or even, not specify one at all and let DMD put it in the usual spot. (Though that
> might be tricky given that you're doing this from a non-DMD file.)

What do you mean by that? (I don't think this is what you mean, but just to be clear, nsMemoryInfoDumper::OpenDMDFile() fails when run this late.)

Comment 8

[Nicholas Nethercote [inactive]]

> What do you mean by that? (I don't think this is what you mean, but just to
> be clear, nsMemoryInfoDumper::OpenDMDFile() fails when run this late.)

My basic question is: how much flexibility is actually required for the file location?

1. User chooses the path and prefix
2. User chooses the path, DMD chooses the prefix
3. DMD chooses the path and prefix

1 is potentially confusing. And 2 might be easier to implement, and 3 even more so. But 2 and 3 are less flexible for the user. I'll let you decide what the right trade-off is.

Comment 9

[Andrew McCreight [:mccr8]]

The prefix doesn't need to be picked by the user, it is always boring. The path is nice to set, but not essential. I have it generated by the user because I don't know how to come up with a path in a cross-platform way that works late in shutdown.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> The prefix doesn't need to be picked by the user, it is always boring. The
> path is nice to set, but not essential. I have it generated by the user
> because I don't know how to come up with a path in a cross-platform way that
> works late in shutdown.

As a hacky workaround, you could check whether we're generating a shutdown log much earlier, when DMD's stuff is still working, and stash the path away somewhere.  I don't know if trace refcnt initialization is too early, but surely we have an initialization function you could hang this off of...maybe even DMD's initialization itself?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #10)
> As a hacky workaround, you could check whether we're generating a shutdown
> log much earlier, when DMD's stuff is still working, and stash the path away
> somewhere.

Yeah, I was thinking about that, but it seems like a lot of work for something that probably only a couple of people are ever going to use.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c245c476379

Comment 13

[Pulsebot]

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

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Eric Rahm [:erahm] from comment #5)
> I think it might make sense to move the
> implementation into nsMemoryInfoDumper, but considering this is limited to
> nsTraceRefcnt's cpp file it's probably not a big deal. r=me either way.

I had it like that in one iteration, but it isn't entirely clear to me how much of it is specific to just shutdown (which should probably be closer to where it is called) and how much is a generic DMD file creator. If it is the latter, how robust should it be? Etc. So I just left it for this special purpose in nsTracerRefCnt. If somebody needs something like this later it can be updated.

I also changed it so the non-directory part of the prefix, "dmd", is included. That seems less weird. So now you would do something like this to have a file named dmd-<pid>.log.gz in your home directory:
MOZ_DMD_SHUTDOWN_LOG=~/

If somebody wants to come up with something fancier, I can review the patch.

> We should error check this return value.

Oops, I forgot to add this. I pushed a followup for it.

Comment 15

[Andrew McCreight [:mccr8]]

I'll update the documentation at some point after it merges.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c245c476379
https://hg.mozilla.org/mozilla-central/rev/a8a8b1313b52

Comment 17

[Andrew McCreight [:mccr8]]

I added a description of these environment variables on the DMD page under "Runtime".
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD#Runtime