1014341 - (remove-trace-malloc) Remove trace-malloc

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Nicholas Nethercote [inactive]]

trace-malloc is old C code that is barely used and is incompatible with jemalloc. DMD is modern C++ code that is widely used and is compatible with jemalloc. The two tools have a lot of overlap, because they both track heap allocations.

I'd like to remove trace-malloc. This will require extending DMD to cover some of trace-malloc's use cases.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is trace-malloc actually in anyone's way?

(Also, I think it's relatively straightforward to make it work with jemalloc; it just hasn't seemed worth doing.)

Where's the documentation for DMD?

Comment 2

[Nicholas Nethercote [inactive]]

> Where's the documentation for DMD?

https://wiki.mozilla.org/Performance/MemShrink/DMD

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I support removing trace-malloc if only for the sake of removing duplication between tools. 

Supporting jemalloc is very important, as that's what we ship; while it could be done for trace-malloc, doing that work would be duplicating work that is already done in memory/replace, the infra that DMD rests on.

Comment 4

[Nicholas Nethercote [inactive]]

Another thing to note is that trace-malloc is entirely e10s-unaware. In contrast, DMD is partially aware (see bug 1061066).

Comment 5

[Nicholas Nethercote [inactive]]

> Another thing to note is that trace-malloc is entirely e10s-unaware. In
> contrast, DMD is partially aware (see bug 1061066).

Fully aware, now.

Comment 6

[Nicholas Nethercote [inactive]]

DMD can do diffs now.

Comment 7

[Nicholas Nethercote [inactive]]

I tried to use all of trace-malloc's functionality, following the documentation
in tools/trace-malloc/README and at
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/TraceMalloc.

I ran a --enable-trace-malloc build with |--trace-malloc=tm.log
--shutdown-leaks=sl.log|.

I did the following with the "trace malloc log" (tm.log):

- Nothing! I couldn't work out what could be done with this log.

I did the following with the "live object log" produced at shutdown:

- Running |bloatblame| and |leaksoup| and |spacetrace| and |leakstats| and
  |tmstats| I get this:

> o64t/dist/bin/bloatblame: error while loading shared libraries:
> libmozalloc.so: cannot open shared object file: No such file or directory

  I guess some build system changes broke these.

- Running |histogram.pl| and |uncategorized.pl| I get this:

> Hexadecimal number > 0xffffffff non-portable at
> /home/njn/moz/mi5/tools/trace-malloc/TraceMalloc.pm line 102, <> line 9.

  This repeats a bazillion times. I guess it doesn't handle 64-bit addresses? I
  was unable to run histogram-diff.sh and histogram-pretty.sh, because they
  post-process histogram.pl's output.

- Running diffbloatdump.pl I get output like this:

> 31128312 malloc
>   21238664 malloc
>     5432495 moz_xmalloc
>       468864 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1CD477A
>         455400 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0B64E
>           268704 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0B4C8
>             262368 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0E416
>             6192 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0C64D
>             144 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1CD98CC
>           186696 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0B51A
>             171216 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0E416
>             15480 /home/njn/moz/mi5/o64t/dist/bin/libxul.so+1D0C64D
> [...]

Running the output through fix_linux_stack.py (required for it to be useful)
currently doesn't work because we missed this case in bug 1062709.

Comment 8

[Nicholas Nethercote [inactive]]

I think we're at a point where trace-malloc can be removed. With that in mind, here's a 
comparison of DMD and trace-malloc...

- DMD has continuous integration: compiling and testing (for debug builds on
  Windows, Mac and Linux). trace-malloc does not, and most of its functionality
  is currently broken.

- DMD is modern C++ and uses one Python script for post-processing.
  trace-malloc is mostly older-style C, with some C++, and uses a mixture of
  Perl and shell scripts for post-processing.

- DMD is 5,500 lines of code and text; almost half of this is test inputs and
  expected outputs. trace-malloc is 25,500 lines of code and text.

- DMD works on Win32, Mac, Linux, Fennec and B2G. trace-malloc definitely works
  on Linux (modulo the post-processing bustage mentioned in comment 7), and I'm
  not sure about other platforms.

- DMD's output files are typically 10--100x smaller, because they avoid
  repeating stack trace data and they are auto-gzipped.

- DMD has the following functions:

  - Takes snapshots of the live heap

  - Takes snapshots of the live heap, with additional data about which blocks
    have been reported by a memory reporter

  - Takes snapshots of the cumulative heap (i.e. all allocations up to this
    point)

  - It can do diffs of snapshots

  And it can do all the above with sampling, which speeds things up a lot.

  trace-malloc can also do diffs of snapshot pairs, though it's partly broken
  at the moment due to the fix*.py scripts not working with it. It can
  apparently also do other stuff though I couldn't get anything else to work.

- DMD's docs are up-to-date. trace-malloc's are better than they were (I
  recently combined most of the web ones into a single page on MDN) but still
  not great; in particular, not all the post-processing scripts and programs
  are documented (e.g. spacetrace, tmfrags, leakstats).

- DMD is e10s-aware; it produces a separate file for each process with the pid
  in the filename. trace-malloc is not, as far as I can tell.

- DMD works with (indeed, requires) jemalloc. trace-malloc requires disabling
  jemalloc.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Remove trace-malloc

dbaron, r?ing you for the general idea of removing trace-malloc -- are there
any obstacles remaining?

glandium, r?ing you for the actual changes. Apart from the obvious removal of
the tools/trace-malloc/ directory and its contents, most of the other changes
are build system things.

Diff stats:

 64 files changed, 20 insertions(+), 25859 deletions(-)

This includes the removal of six Perl scripts.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I want to look into a few things in order to review this -- hopefully on Tuesday (January 6).

Comment 11

[Andrew McCreight [:mccr8] (PTO until 12-2)]

FWIW, DMD doesn't quite do conservative heap scanning yet, but I have a patch basically done, so I should work on landing that...

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #10)
> I want to look into a few things in order to review this -- hopefully on
> Tuesday (January 6).

Sure thing.

(In reply to Andrew McCreight [:mccr8] from comment #11)
> FWIW, DMD doesn't quite do conservative heap scanning yet, but I have a
> patch basically done, so I should work on landing that...

And trace-malloc's has been busted for an unknown length of time, so I figure it's a wash on that front :)

Comment 13

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> - Running |bloatblame| and |leaksoup| and |spacetrace| and |leakstats| and
>   |tmstats| I get this:
> 
> > o64t/dist/bin/bloatblame: error while loading shared libraries:
> > libmozalloc.so: cannot open shared object file: No such file or directory
> 
>   I guess some build system changes broke these.

Likely not. They probably have required setting LD_LIBRARY_PATH or running through run-mozilla.sh all along.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8543198 [details] [diff] [review]
Remove trace-malloc

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

::: js/src/devtools/automation/arm-sim
@@ -1,4 @@
>  --enable-optimize
>  --enable-debug
>  --enable-stdcxx-compat
> ---enable-trace-malloc

I'd separate those out.

::: xpcom/glue/standalone/nsXPCOMGlue.cpp
@@ -217,5 @@
> -{
> -  return _valloc(size);
> -}
> -#endif /* NS_TRACE_MALLOC */
> -

I'm so glad this goes away.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Nicholas Nethercote [:njn] from comment #7)
> > - Running |bloatblame| and |leaksoup| and |spacetrace| and |leakstats| and
> >   |tmstats| I get this:
> > 
> > > o64t/dist/bin/bloatblame: error while loading shared libraries:
> > > libmozalloc.so: cannot open shared object file: No such file or directory
> > 
> >   I guess some build system changes broke these.
> 
> Likely not. They probably have required setting LD_LIBRARY_PATH or running
> through run-mozilla.sh all along.

Yes, they've historically required either of those, and presumably still do.

Comment 16

[Nicholas Nethercote [inactive]]

> ::: js/src/devtools/automation/arm-sim
> @@ -1,4 @@
> >  --enable-optimize
> >  --enable-debug
> >  --enable-stdcxx-compat
> > ---enable-trace-malloc
> 
> I'd separate those out.

Sorry, I don't understand this comment.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> > ::: js/src/devtools/automation/arm-sim
> > @@ -1,4 @@
> > >  --enable-optimize
> > >  --enable-debug
> > >  --enable-stdcxx-compat
> > > ---enable-trace-malloc
> > 
> > I'd separate those out.
> 
> Sorry, I don't understand this comment.

I'd move the removal of --enable-trace-malloc from those three files to a separate patch.

Comment 18

[Nicholas Nethercote [inactive]]

> I'd move the removal of --enable-trace-malloc from those three files to a separate patch.

Really? I don't see why...

Comment 19

[Mike Hommey [:glandium]]

In the hypothetical case someone would want to restore trace malloc, the option changes should be unrelated to restoring trace malloc.

Comment 20

[Nicholas Nethercote [inactive]]

Ok. I can split those changes into a preliminary patch.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 0) - Don't use trace-malloc in devtools automation files

sfink r+'d this on IRC.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove trace-malloc

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8544861 [details] [diff] [review]
(part 1) - Remove trace-malloc

Could you split the changes to:
  testing/talos/diff-talos.py
into a separate patch and get separate review for those?  I'm not sure
if the version maintained on trunk should be able to deal with tests run
on old branches.


At some point I'll hopefully find a chance to ask you about how to use
DMD to dump the state of memory from a point in either C++ or JS code
(as opposed to from about:memory UI).


I'm somewhat sad to see this go, but r=dbaron.

Comment 24

[Nicholas Nethercote [inactive]]

Thank you, dbaron!

> Could you split the changes to:
>   testing/talos/diff-talos.py
> into a separate patch and get separate review for those?  I'm not sure
> if the version maintained on trunk should be able to deal with tests run
> on old branches.

I asked jmaher via email if it's safe to remove these and he said it's fine. But I can split it out anyway.

> At some point I'll hopefully find a chance to ask you about how to use
> DMD to dump the state of memory from a point in either C++ or JS code
> (as opposed to from about:memory UI).

I'll add instructions to the DMD docs about this.

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 0b) - Remove trace-malloc mentions from diff-talos.py

Joel, you already said this was ok via email but let's make it official.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8545530 [details] [diff] [review]
(part 0b) - Remove trace-malloc mentions from diff-talos.py

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

thanks, this looks good!

Comment 27

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06563617ea60

Comment 28

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eff1149c52dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e30ec51f41b
https://hg.mozilla.org/integration/mozilla-inbound/rev/94bd476efa14

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/eff1149c52dc
https://hg.mozilla.org/mozilla-central/rev/8e30ec51f41b
https://hg.mozilla.org/mozilla-central/rev/94bd476efa14

Comment 30

[Nicholas Nethercote [inactive]]

> > At some point I'll hopefully find a chance to ask you about how to use
> > DMD to dump the state of memory from a point in either C++ or JS code
> > (as opposed to from about:memory UI).
> 
> I'll add instructions to the DMD docs about this.

I added a new section: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD#Triggering_dumps_from_code

Comment 31

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f63dc251b2577564acd53ea409ba0e1c1a43a471
Bug 1014341 (part 1) - Remove trace-malloc. r=dbaron,glandium.