Last Comment Bug 819819 - Remove DMDV
: Remove DMDV
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: NewDMD
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-09 17:39 PST by Nicholas Nethercote [:njn]
Modified: 2013-01-02 18:12 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove DMDV. (83.36 KB, patch)
2012-12-20 23:03 PST, Nicholas Nethercote [:njn]
khuey: review+
Details | Diff | Splinter Review
(part 2) - Remove |name| from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN. (30.60 KB, patch)
2012-12-23 23:24 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-12-09 17:39:48 PST
As soon as I'm confident that new-DMD has superseded DMDV, I'll remove DMDV.
Comment 1 Nicholas Nethercote [:njn] 2012-12-18 14:09:28 PST
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 [:njn] 2012-12-20 23:03:00 PST
Created attachment 694716 [details] [diff] [review]
Remove DMDV.

Nothing surprising here.
Comment 3 Nicholas Nethercote [:njn] 2012-12-23 23:24:58 PST
Created attachment 695412 [details] [diff] [review]
(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) 2012-12-24 09:20:33 PST
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 [:njn] 2012-12-24 14:06:52 PST
> 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) 2012-12-24 14:32:25 PST
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 [:njn] 2012-12-26 14:06:51 PST
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) 2012-12-27 08:41:36 PST
> 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 [:njn] 2012-12-27 14:19:15 PST
> 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 [:njn] 2012-12-27 14:26:39 PST
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 [:njn] 2012-12-27 15:29:29 PST
Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85464d8b30ff
Comment 12 Graeme McCutcheon [:graememcc] 2012-12-29 04:33:31 PST
https://hg.mozilla.org/mozilla-central/rev/85464d8b30ff
Comment 13 Justin Lebar (not reading bugmail) 2012-12-29 15:33:21 PST
> 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) 2012-12-29 15:45:36 PST
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 [:njn] 2013-01-01 15:21:24 PST
Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef8548a7ab6
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-01-02 18:12:32 PST
https://hg.mozilla.org/mozilla-central/rev/0ef8548a7ab6

Note You need to log in before you can comment on or make changes to this bug.