DMD: add a new heap snapshot operation

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
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.
(Assignee)

Comment 2

4 years ago
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|.
Attachment #8452073 - Flags: review?(erahm)
(Assignee)

Comment 3

4 years ago
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.
Attachment #8452074 - Flags: review?(erahm)
(Assignee)

Comment 4

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

I fixed the patch title.
Attachment #8452075 - Flags: review?(erahm)
(Assignee)

Updated

4 years ago
Attachment #8452074 - Attachment is obsolete: true
Attachment #8452074 - Flags: review?(erahm)
(Assignee)

Comment 5

4 years ago
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.
Attachment #8452076 - Flags: review?(erahm)
Attachment #8452076 - Flags: review?(continuation)
(Assignee)

Comment 6

4 years ago
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.
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

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

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

lgtm.
Attachment #8452073 - Flags: review?(erahm) → review+
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.
Attachment #8452076 - Flags: review?(continuation) → review+
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]

Comment 10

4 years ago
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.
Attachment #8452075 - Flags: review?(erahm) → review+
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

4 years ago
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.
Attachment #8452076 - Flags: review?(erahm) → review+
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.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(n.nethercote)

Comment 15

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

Updated

4 years ago
Blocks: 1044709
(Assignee)

Comment 16

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

I'll be lazy and defer that to bug 1044709.
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. ;)
Flags: needinfo?(n.nethercote)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Comment 21

4 years ago
A tiny follow-up fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a42b68a5db
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.