819819 - Remove DMDV

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

As soon as I'm confident that new-DMD has superseded DMDV, I'll remove DMDV.

Comment 1

[Nicholas Nethercote [inactive]]

Note to self:  I will be able to remove the |name| parameter from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN once DMDV has been removed.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Remove DMDV.

Nothing surprising here.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove |name| from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN.

This patch removes the |name| arg from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN and
NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_ALLOC_FUN.

The patch also makes the names given to these functions more consistent, i.e.
always "FooMallocSizeOf", not "FooSizeOf" or "FooMallocSizeOfFun".

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 695412 [details] [diff] [review]
(part 2) - Remove |name| from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN.

Can we make NS_MEMORY_REPORTER_MALLOC_SIZEOF_FN a true function, instead of a macro which defines a function, or is there some reason to define these individual functions still?

Comment 5

[Nicholas Nethercote [inactive]]

> is there some reason to define these individual functions still?

Yes.  They show up in the report stack trace which makes it easier to see which reporter did the reporting.

Comment 6

[Justin Lebar (not reading bugmail)]

But the caller also shows up in the report stack trace, and how many functions do we have in our code which call two different REPORT_FN functions?

Comment 7

[Nicholas Nethercote [inactive]]

We have report stack traces like this:

 Reported at:
   mozilla::dmd::Report(...
   DOMStyleMallocSizeOf(...
   nsAtomList::SizeOfIncludingThis(...
   nsCSSSelector::SizeOfIncludingThis(...
   nsCSSSelectorList::SizeOfIncludingThis(...
   mozilla::css::StyleRule::SizeOfIncludingThis(...
   mozilla::css::Rule::SizeOfCOMArrayElementIncludingThis(...
   SizeOfElementIncludingThisEnumerator(...

I particularly like having an obvious MallocSizeOf function near the top of the stack trace.  Yes, you can work out which reporter is responsible from the entire stack, but it's harder, esp. if you're not familiar with the existing memory reporters.

And something which isn't obvious is that I use this facility while writing new memory reporters.  E.g. if a single memory reporter measures several different things, I'll temporarily create multiple MALLOC_SIZEOF_FUN functions in order to understand the sizes of each of these things, and decide which ones should be reported individually and which should be aggregated.

We used to have a string (so it would be "Reported at dom-style", or similar).  I got rid of that, at a slight usability loss, to reduce memory consumption.  I don't want to remove the MallocSizeOf indicator as well.

Comment 8

[Justin Lebar (not reading bugmail)]

> Yes, you can work out which reporter is responsible from the entire stack, but it's 
> harder, esp. if you're not familiar with the existing memory reporters.

Is this a task that people unfamiliar with memory reporters will be undertaking commonly, compared to the frequency with which those people will be writing memory reporters?

In terms of optimizing for that crowd, I think not having this confusing extra step of declaring a sizeof fn is helpful.

> E.g. if a single memory reporter measures several different things, I'll temporarily 
> create multiple MALLOC_SIZEOF_FUN functions in order to understand the sizes of each of 
> these things, and decide which ones should be reported individually and which should be 
> aggregated.

But you could do this just by creating multiple different memory reporters, right?  my-reporter/foo, my-reporter/bar, etc.

If we were writing this code today, do you think we'd have added these functions?

Comment 9

[Nicholas Nethercote [inactive]]

> If we were writing this code today, do you think we'd have added these
> functions?

Yes.  I don't want to remove these functions.

Comment 10

[Nicholas Nethercote [inactive]]

Just to clarify the most important point:  I know the memory reporters better than anyone.  And yet the FooMallocSizeOf functions are still useful to me for identifying the less prominent memory reporters.  If they are removed I'll frequently have to spend time looking at the code to determine which reporter is responsible for a stack instead of the stack immediately telling me.

Comment 11

[Nicholas Nethercote [inactive]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85464d8b30ff

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/85464d8b30ff

Comment 13

[Justin Lebar (not reading bugmail)]

> Yes.  I don't want to remove these functions.

Okay, sorry, this isn't worth fighting over.  Thanks for hearing my objections; I'll review your patch.

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 695412 [details] [diff] [review]
(part 2) - Remove |name| from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN.

>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl
>--- a/xpcom/base/nsIMemoryReporter.idl
>+++ b/xpcom/base/nsIMemoryReporter.idl
>@@ -371,56 +371,54 @@ void RunReporters();
> #if defined(MOZ_DMD)
> 
> #if !defined(MOZ_MEMORY)
> #error "MOZ_DMD requires MOZ_MEMORY"
> #endif
> 
> #include "DMD.h"
> 
>-#define MOZ_REPORT(ptr, usable, name)          mozilla::dmd::Report(ptr)
>-#define MOZ_REPORT_ON_ALLOC(ptr, usable, name) mozilla::dmd::ReportOnAlloc(ptr)
>+#define MOZ_REPORT(ptr)          mozilla::dmd::Report(ptr)
>+#define MOZ_REPORT_ON_ALLOC(ptr) mozilla::dmd::ReportOnAlloc(ptr)
> 
> #else
> 
>-#define MOZ_REPORT(ptr, usable, name)
>-#define MOZ_REPORT_ON_ALLOC(ptr, usable, name)
>+#define MOZ_REPORT(ptr)
>+#define MOZ_REPORT_ON_ALLOC(ptr)
> 
> #endif  // defined(MOZ_DMD)
> 
> // Functions generated via this macro should be used by all traversal-based
> // memory reporters.  Such functions return |moz_malloc_size_of(ptr)|;  this
> // will always be zero on some obscure platforms.
> //
> // You might be wondering why we have a macro that creates multiple
> // functions distinguished only by |name|, instead of a single
> // MemoryReporterMallocSizeOf function.  It's mostly to help with DMD
> // integration, though it sometimes also helps with debugging and temporary
> // ad hoc profiling.  The |name| chosen doesn't matter greatly, but it's
> // best to make it similar to the path used by the relevant memory
> // reporter(s).
>-#define NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(fn, name)                        \
>+#define NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(fn)                              \

Update this comment, please.  (There's no longer a |name| arg.)

Comment 15

[Nicholas Nethercote [inactive]]

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef8548a7ab6

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ef8548a7ab6