1035570 - DMD: add a new heap snapshot operation

Status: RESOLVED FIXED

Description

[Nicholas Nethercote [:njn]]

DMD is currently tied completely to memory reports, i.e. it always divides heap blocks into twice-reported/once-reported/unreported. But it can be useful for more general heap profiling.

Comment 1

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

This is basically what I'm doing in bug 1014346, though that's more of a heap dump, like we can do for the GC heap, than a "snapshot", maybe, so we should compare notes.

Comment 2

[Nicholas Nethercote [:njn]]

Created attachment 8452073 [details] [diff] [review]
(part 1) - DMD: Rename TraceRecord as Record

DMD used to have "stack trace records" and "stack frame records". Bug 1013011
removed the latter, and now "stack trace record" and |TraceRecord| are a bit
unwieldy. This patch changes them to "heap block record" and |Record|.

Comment 3

[Nicholas Nethercote [:njn]]

Created attachment 8452074 [details] [diff] [review]
(part 2) - DMD: Rename TraceRecord as Record

The next patch in the series will add a new script analyzing DMD output files,
and there are likely to be more post-processing scripts like this added in the
future. In anticipation, this patch tweaks DMD's output to be a little more
conducive to machine parsing.

The basic idea is this:

- Lines beginning with '#' are comments and can be ignored, as can blank lines.

- All top level blocks consist of a string ending with '{', and then one or
  more indented lines, and then a closing '}' on its own line. Any multi-line
  things within a block are themselves enclosed in braces.

The diff for memory/replace/dmd/test-expected.dmd shows what this looks like in
practice.

It's a long way from a formal grammar or anything like that, but that would be
overkill. In this form it's quite easy to parse with simple scripts that just
do line-based regexp matching, rather than proper parsing. And it's still very
readable to humans, so I think it's a reasonable balance overall.

Comment 4

[Nicholas Nethercote [:njn]]

Created attachment 8452075 [details] [diff] [review]
(part 2) - DMD: make output easier for machines to parse

I fixed the patch title.

Comment 5

[Nicholas Nethercote [:njn]]

Created attachment 8452076 [details] [diff] [review]
(part 3) - DMD: Add DMDAnalyzeHeap(), a heap snapshot function

The patch also adds DMDAnalyzeReports() as a synonym for DMDReportAndDump(),
and deprecates the latter.

Comment 6

[Nicholas Nethercote [:njn]]

mccr8, I think part 3 is complementary to bug 1014346, because both patches are extending DMD to add a new kind of report. The contents of those reports are fairly different, though.

Comment 7

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

I think the liveTraceRecords thing I added to dump all live records is the same thing you are implementing here, but mine is no doubt way hackier.

Comment 8

[Eric Rahm [:erahm] (ni? for review in phab)]

Comment on attachment 8452073 [details] [diff] [review]
(part 1) - DMD: Rename TraceRecord as Record

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

lgtm.

Comment 9

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

Comment on attachment 8452076 [details] [diff] [review]
(part 3) - DMD: Add DMDAnalyzeHeap(), a heap snapshot function

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

Nice cleanup here.

::: memory/replace/dmd/DMD.cpp
@@ +1465,5 @@
>  
>    void Print(const Writer& aWriter, LocationService* aLocService,
>               uint32_t aM, uint32_t aN, const char* aStr, const char* astr,
>               size_t aCategoryUsableSize, size_t aCumulativeUsableSize,
> +             size_t aTotalUsableSize, bool aShowCategoryPercentage,

This giant grab bag of arguments is pretty bad, and repeated down below for Record::Print.  I guess it can stay for now, but we should think of doing something nicer.

@@ +1998,5 @@
> +  {
> +    RecordTable* table;
> +    uint32_t numReports = aBlock.NumReports();
> +    if (numReports == 0) {
> +      mUnreported.mUsableSize += aBlock.UsableSize();

You could define some kind of ProcessBlock method to RecordKindData that just does the += usable size and numBlocks++ thing, as you seem to do that in a bunch of places.

@@ +2006,5 @@
> +      mOnceReported.mUsableSize += aBlock.UsableSize();
> +      mOnceReported.mNumBlocks++;
> +      table = &mOnceReported.mRecordTable;
> +    } else {
> +      MOZ_ASSERT(numReports == 2);

It looks like this is just existing code, so no need to change it, but in theory something could be reported three times or more, right?  Maybe generalizing mTwiceReported is not worth the effort if it is rare.

@@ +2041,5 @@
> +                       /* showCategoryPercentage = */ true,
> +                       /* showReportedAt = */ true);
> +  }
> +
> +  void PrintSummary(const Writer& aWriter, bool showTilde) const

nit: aShowTilde

@@ +2049,5 @@
> +      100.0,
> +      Show(mTotalNumBlocks,  gBuf2, kBufLen, showTilde),
> +      100.0);
> +
> +    W("  Unreported:     %12s bytes (%6.2f%%) in %7s blocks (%6.2f%%)\n",

These blocks of code are another obvious candidate for a RecordKindData method.

@@ +2071,5 @@
> +
> +  void PrintStats(const Writer& aWriter) const
> +  {
> +    size_t unreportedSize =
> +      mUnreported.mRecordTable.sizeOfIncludingThis(MallocSizeOf);

more boilerplatey code.

::: memory/replace/dmd/DMD.h
@@ +52,1 @@
>  // summary (via |aWrite|).  If |aWrite| is nullptr it will dump to stderr.

"summary of anomalies" maybe?  I think the report produces information about double reporting and missing things, but not about everything that was reported.

::: testing/mochitest/tests/SimpleTest/MemoryStats.js
@@ +98,5 @@
>          var basename = "dmd-" + testNumber + ".txt";
>          var dumpfile = MemoryStats.constructPathname(dumpOutputDirectory,
>                                                       basename);
>          dumpFn("TEST-INFO | " + testURL + " | DMD-DUMP " + dumpfile);
> +        DMDAnalyzeReports(dumpfile);

Hmm... as long as we're still actually supporting DMDReportAndDump(), maybe we should still test it, even though it is deprecated?  My main concern here is that our magic B2G scripts are likely going to use it for a little bit, and we don't want to break those.  On the other hand, it would be pretty amazing to break one but not the other, so up to you.

Comment 10

[Eric Rahm [:erahm] (ni? for review in phab)]

Comment on attachment 8452075 [details] [diff] [review]
(part 2) - DMD: make output easier for machines to parse

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

lgtm, can we add some sort of version header as well? As this output evolves it would be nice if our scripts can maintain backwards compatibility.

Nick and I discussed the possibility of having DMD output something somewhat more machine-readable (and compact) and providing a script to convert that to human-readable output, but I think that's more of a future change.

Comment 11

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

I've written the beginnings of a DMD log parser, though it is obviously not up-to-date with the latest format, and it only bothers to read what I need to correlate objects to their allocation stacks.
  https://github.com/amccreight/heapgraph/tree/master/dmd

Comment 12

[Eric Rahm [:erahm] (ni? for review in phab)]

Comment on attachment 8452076 [details] [diff] [review]
(part 3) - DMD: Add DMDAnalyzeHeap(), a heap snapshot function

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

The only thing that might be a real issue is the removal of the call to ClearReportsInternal, otherwise the rest is super minor.

::: dom/base/nsJSEnvironment.cpp
@@ +1494,5 @@
> +  return AnalyzeReports(cx, argc, vp);
> +}
> +
> +static bool
> +AnalyzeHeap(JSContext *cx, unsigned argc, JS::Value *vp)

--min: AnalyzeHeap and AnalyzeReports are pretty much the same, they could probably share the same base function with a small if else in it.

::: memory/replace/dmd/DMD.cpp
@@ +1815,5 @@
>  //---------------------------------------------------------------------------
>  
>  static void
>  PrintSortedRecords(const Writer& aWriter, LocationService* aLocService,
> +                   int (*aCmp)(const void*, const void*),

--min: perhaps declare a comparator typedef?

@@ +1954,5 @@
> +  virtual RecordTable* ProcessBlock(const Block& aBlock) = 0;
> +
> +  virtual void PrintRecords(const Writer& aWriter,
> +                            LocationService* aLocService) const = 0;
> +  virtual void PrintSummary(const Writer& aWriter, bool showTilde) const = 0;

Maybe change showTilde to isSampled or something like that. or as mccr8 would say "aIsSampled"

@@ +1993,5 @@
> +  }
> +
> +  const char* AnalyzeFunctionName() const { return "AnalyzeReports"; }
> +
> +  RecordTable* ProcessBlock(const Block& aBlock)

--min: style, virtual methods should probably be declared virtual

@@ +2172,5 @@
>  
>    WriteSeparator();
>    W("Invocation {\n");
>    W("  $DMD = '%s'\n", gOptions->DMDEnvVar());
> +  W("  Function = %s\n", aAnalyzer->AnalyzeFunctionName());

--min: there might be another name than function (function makes me think of a stack trace). Maybe just "type," or maybe it doesn't matter.

@@ +2185,5 @@
>    WriteSeparator();
>    W("Summary {\n");
>  
> +  bool showTilde = anyBlocksSampled;
> +  aAnalyzer->PrintSummary(aWriter, showTilde);

showTilde is an internal detail, maybe just pass anyBlockSampled

@@ -2134,5 @@
>    }
>  
>    InfallibleAllocPolicy::delete_(locService);
>  
> -  ClearReportsInternal(); // Use internal version, we already have the lock.

Was this incorrect? If we want to push this out the caller, I think we need to update nsMemoryInfoDumper.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +299,5 @@
>                              "collection log.\n" +
>                              "WARNING: These logs may be large (>1GB).";
>  
> +  const AnalyzeReportsDesc = "Analyze memory reports and save the output to '" +
> +                             gAnalyzeReportsFile + "'.\n";

"Analyze memory reports" vs "Analyze heap usage" are a bit ambiguous. Memory reports, to me, in an about:memory context means the thing called memory_report, or the thing displayed when you measure memory. Analyze heap usage, that's reasonably accurate, although perhaps just "measure heap usage" would be clearer.

Comment 13

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

Nick, what is your plan for landing this?  I have some DMD patches I'd like to work on, but I want to wait until the dust settles from this.

Comment 14

[Nicholas Nethercote [:njn]]

erahm suggested that DMD should produce JSON format instead of its own semi-formal text format, and I've been pondering that but haven't got anywhere. If it helps you, I can just land these as is and then think about the JSON later.

Comment 15

[Eric Rahm [:erahm] (ni? for review in phab)]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> erahm suggested that DMD should produce JSON format

Machine-readable, not necessarily JSON. Something like protobufs might be more appropriate. I agree that should probably just be put off for another bug.

Comment 16

[Nicholas Nethercote [:njn]]

> can we add some sort of version header as well?

I'll be lazy and defer that to bug 1044709.

Comment 17

[Nicholas Nethercote [:njn]]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=96e8002e9d64

Comment 18

[Nicholas Nethercote [:njn]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/664fbd3821c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58334ac4362
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b7bbf5c1af

Comment 19

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

I was running a DMD build with this patch locally, and I noticed you introduced a (minor) bug between the reviewed version and the landed version: AnalyzeReportsDesc needs a + between the two segments. Right now, the button description is just "Analyze memory reports coverage and save the ".  r=me to fix that. ;)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This was merged to m-c.

https://hg.mozilla.org/mozilla-central/rev/664fbd3821c7
https://hg.mozilla.org/mozilla-central/rev/c58334ac4362
https://hg.mozilla.org/mozilla-central/rev/92b7bbf5c1af

Comment 21

[Nicholas Nethercote [:njn]]

A tiny follow-up fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a42b68a5db

Comment 22

[Carsten Book [:Tomcat]]

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