The default bug view has changed. See this FAQ.

Status

()

Core
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla20
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
As soon as I'm confident that new-DMD has superseded DMDV, I'll remove DMDV.
(Assignee)

Comment 1

4 years ago
Note to self:  I will be able to remove the |name| parameter from NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN once DMDV has been removed.
(Assignee)

Comment 2

4 years ago
Created attachment 694716 [details] [diff] [review]
Remove DMDV.

Nothing surprising here.
Attachment #694716 - Flags: review?(khuey)
Attachment #694716 - Flags: review?(khuey) → review+
(Assignee)

Comment 3

4 years ago
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".
Attachment #695412 - Flags: review?(justin.lebar+bug)
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?
(Assignee)

Comment 5

4 years ago
> 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.
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?
(Assignee)

Comment 7

4 years ago
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.
> 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?
(Assignee)

Comment 9

4 years ago
> 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.
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85464d8b30ff
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/85464d8b30ff
> 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 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.)
Attachment #695412 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 15

4 years ago
Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef8548a7ab6
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/0ef8548a7ab6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.