Bug 1014341 (remove-trace-malloc)

Remove trace-malloc

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Alias: remove-trace-malloc
Assignee

Updated

5 years ago
Depends on: 1014343
Assignee

Updated

5 years ago
Depends on: 1014346
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?
Assignee

Comment 2

5 years ago
> Where's the documentation for DMD?

https://wiki.mozilla.org/Performance/MemShrink/DMD
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.
Blocks: 1057134
Assignee

Comment 4

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

Comment 5

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

Fully aware, now.
Depends on: 708831
Assignee

Updated

5 years ago
Blocks: 1080290
Assignee

Comment 6

5 years ago
DMD can do diffs now.
Assignee

Comment 7

5 years ago
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.
Assignee

Comment 8

5 years ago
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.
Assignee

Comment 9

5 years ago
Posted patch Remove trace-malloc (obsolete) — Splinter Review
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.
Attachment #8543198 - Flags: review?(mh+mozilla)
Attachment #8543198 - Flags: review?(dbaron)
I want to look into a few things in order to review this -- hopefully on Tuesday (January 6).
FWIW, DMD doesn't quite do conservative heap scanning yet, but I have a patch basically done, so I should work on landing that...
Assignee

Comment 12

5 years ago
(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 :)
(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 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.
Attachment #8543198 - Flags: review?(mh+mozilla) → review+
(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.
Assignee

Comment 16

5 years ago
> ::: 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.
(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.
Assignee

Comment 18

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

Really? I don't see why...
In the hypothetical case someone would want to restore trace malloc, the option changes should be unrelated to restoring trace malloc.
Assignee

Comment 20

5 years ago
Ok. I can split those changes into a preliminary patch.
Assignee

Comment 22

5 years ago
Attachment #8544861 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8543198 - Attachment is obsolete: true
Attachment #8543198 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8544860 - Flags: review+
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.
Attachment #8544861 - Flags: review?(dbaron) → review+
Assignee

Comment 24

5 years ago
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.
Assignee

Comment 25

5 years ago
Joel, you already said this was ok via email but let's make it official.
Attachment #8545530 - Flags: review?(jmaher)
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!
Attachment #8545530 - Flags: review?(jmaher) → review+
Assignee

Comment 30

5 years ago
> > 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
Blocks: 1126269
You need to log in before you can comment on or make changes to this bug.