DMD: add "num" field to blocks in the output

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The DMD output can have *many* duplicate block entries, especially in cumulative mode. (1,000,000+ for the more common ones in longer-running sessions.)

Adding a "n" field to the block output that allows these to be aggregated will allow the output file size to shrink dramatically.
(Assignee)

Comment 1

4 years ago
Created attachment 8550023 [details] [diff] [review]
DMD: add "n" property to blocks in the output

This patch only uses the "n" property for dead blocks, because that's where the
greatest potential benefit lies, but it could be used for live blocks as well.

On one test case (a complex PDF file) running with --mode=cumulative
--sample-below=1 this patch had the following effects.

- Change in running speed was negligible.

- Compressed output file size dropped from 8.8 to 5.0 MB.

- Compressed output file size dropped from 297 to 50 MB.

- dmd.py runtime (without stack fixing) dropped from 30 to 8 seconds.
Attachment #8550023 - Flags: review?(continuation)
(Assignee)

Updated

4 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8550023 [details] [diff] [review]
DMD: add "n" property to blocks in the output

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

Nice improvement.

::: memory/replace/dmd/DMD.cpp
@@ +1067,5 @@
> +void MaybeAddToDeadBlockTable(const DeadBlock& aDb)
> +{
> +  if (gOptions->IsCumulativeMode() && aDb.AllocStackTrace()) {
> +    AutoLockState lock;
> +    if (DeadBlockTable::Ptr p = gDeadBlockTable->lookup(aDb)) {

It would be more efficient to use lookupForAdd() here.

::: memory/replace/dmd/DMD.h
@@ +150,5 @@
>  //   // - 1: The original format. Implemented in bug 1044709.
>  //   // - 2: Added the "mode" property under "invocation". Added in bug 1094552.
>  //   // - 3: The "dmdEnvVar" property under "invocation" can now be |null| if
>  //   //      the |DMD| environment variable is not defined. Done in bug 1100851.
> +//   // - 4: Added the "n" property in "blockList" object. Done in bug 1121830.

nit: these are inconsistent about "implemented in" vs "added in" vs "done in".

@@ +191,5 @@
> +//       // The number of heap blocks with exactly the above properties. This
> +//       // is mandatory if it is greater than one, but omitted otherwise.
> +//       // (Blocks with identical properties don't have to be aggregated via
> +//       // this property, but it can greatly reduce output file size.)
> +//       "n": 5

"n" seems overly terse. Why not "num" or "copies" or something?  I had no idea what this bug was about from its name. ;)

::: memory/replace/dmd/dmd.py
@@ +419,5 @@
>  
> +        record.numBlocks  += n
> +        record.reqSize    += n * reqSize
> +        record.slopSize   += n * slopSize
> +        record.usableSize += n * usableSize

You should implement vector multiplication for records!  Just kidding.
Attachment #8550023 - Flags: review?(continuation) → review+
(Assignee)

Comment 3

4 years ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f168b7c2a2e3
Summary: DMD: add "n" field to blocks in the output → DMD: add "num" field to blocks in the output
https://hg.mozilla.org/mozilla-central/rev/3bf791ce96e8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.