Add some way to trigger a DMD run late in shutdown

RESOLVED FIXED in Firefox 43

Status

()

Core
DMD
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({dev-doc-complete})

Trunk
mozilla43
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

3 years ago
Assignee: nobody → continuation
(Assignee)

Comment 1

3 years ago
Created attachment 8647643 [details] [diff] [review]
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.
Attachment #8647643 - Flags: review?(nfroyd)
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

This should be documented on the DMD page.
Keywords: dev-doc-needed
Attachment #8647643 - Flags: review?(nfroyd) → review?(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.
Attachment #8647643 - Flags: review?(erahm) → review+
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.)
(Assignee)

Comment 7

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

Comment 9

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

Comment 11

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

Comment 14

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

Comment 15

3 years ago
I'll update the documentation at some point after it merges.
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/4c245c476379
https://hg.mozilla.org/mozilla-central/rev/a8a8b1313b52
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 17

3 years ago
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
Flags: needinfo?(continuation)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.