Closed Bug 955942 Opened 6 years ago Closed 6 years ago

Remove MemoryUniReporter

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files)

MemoryUniReporter is a class designed to reduce the amount of boilerplate code needed for memory reporters.  It is moderately successful at that, but introduces a different kind of complexity -- it has its own state, and so we need to use various INHERITED macros.

In this bug I will replace its uses with vanilla nsIMemoryReporter, which is slightly more verbose but less complex and more typical.

(Ehsan, this bug's patches will break your patch in bug 884368 if it lands first.  Fortunately, if this happens, the required change is straightforward.)
We used to have nsIMemoryReporter and nsIMemoryMultiReporter.  The KIND_ and
UNITS_* constants lived in the former, so instances of the latter needed to
qualify uses with |nsIMemoryReporter::|.  But the two classes where merged a
while back, so many of these prefixes are no longer necessary.
Attachment #8355101 - Flags: review?(continuation)
This patch removes MemoryUniReporter.

For each class Foo that inherited from it, here's the recipe I followed.

- Make Foo inherit from nsIMemoryReporter instead of MemoryUniReporter

- Add NS_DECL_ISUPPORTS (or NS_DECL_THREADSAFE_ISUPPORTS) if necessary.

- Add NS_DECL_NSIMEMORYREPORTER if CollectReports() isn't inline.

- Replace Amount()/GetAmount() with CollectReports(), moving the appropriate
  stuff out of Foo's constructor, and adding a MOZ_DEFINE_MALLOC_SIZE_OF() if
  necessary.

- Change NS_IMPL_ISUPPORTS_INHERITED<n> to NS_IMPL_ISUPPORTS<n+1>, or add
  NS_IMPL_ISUPPORTS1 if it doesn't have one already.

I added a MOZ_COLLECT_REPORT macro to simplify the actual reporting step,
which is really common.

I also merged a couple of uni-reporters into single multi-reporters.

Diffstats show that the additional boilerplate is minimal:

 42 files changed, 694 insertions(+), 614 deletions(-)
Attachment #8355106 - Flags: review?(continuation)
This patch merges the various heap reporters into a single reporter.

 1 file changed, 44 insertions(+), 111 deletions(-)
Attachment #8355107 - Flags: review?(continuation)
Comment on attachment 8355101 [details] [diff] [review]
(part 1) - Remove unnecessary nsIMemoryReporter qualifiers from UNITS_* and KIND_* constants.

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

Thanks for the explanation.  I'd noticed you'd been removing these piecemeal in other patches, but I wasn't sure why.
Attachment #8355101 - Flags: review?(continuation) → review+
Comment on attachment 8355106 [details] [diff] [review]
(part 2) - Remove MemoryUniReporter.

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

r=me with the two comments for the macro MOZ_COLLECT_REPORT addressed.

::: content/base/src/nsContentUtils.cpp
@@ +262,5 @@
>  {
>  public:
> +  NS_DECL_ISUPPORTS
> +
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

Make this private?

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +212,5 @@
> +  {
> +    return MOZ_COLLECT_REPORT(
> +      "gralloc", KIND_OTHER, UNITS_BYTES, sAmount,
> +"Special RAM that can be shared between processes and directly accessed by "
> +"both the CPU and GPU.  Gralloc memory is usually a relatively precious "

Not a big deal, but this comment isn't consistent about one space vs two after a period.  You might as well fix it while you are here.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1633,5 @@
>  {
> +  public:
> +    NS_DECL_ISUPPORTS
> +
> +    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,

Should the * be on the right in JS-style for these two arguments?

::: xpcom/base/nsIMemoryReporter.idl
@@ +507,5 @@
>    }
>  
> +// This macro assumes the presence of appropriate |aHandleReport| and |aData|
> +// variables.
> +#define MOZ_COLLECT_REPORT(path, kind, units, amount, description)            \

This macro ignores the kind and units arguments, which is bad.

@@ +510,5 @@
> +// variables.
> +#define MOZ_COLLECT_REPORT(path, kind, units, amount, description)            \
> +  aHandleReport->Callback(EmptyCString(), NS_LITERAL_CSTRING(path),           \
> +                          KIND_HEAP, UNITS_BYTES, amount,                     \
> +                          NS_LITERAL_CSTRING(description), aData);

I think it makes sense to remove the semi-colon from the macro definition. You seemed to add one most of the places you used it (which is extraneous as-is), which makes sense, because this is more of an expression than a statement.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +827,5 @@
>  {
>  public:
> +    NS_DECL_ISUPPORTS
> +
> +    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

private?
Attachment #8355106 - Flags: review?(continuation) → review+
Comment on attachment 8355107 [details] [diff] [review]
(part 3) - Merge the heap reporters.

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

This is a good argument against uni-reporters. ;)

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +667,5 @@
>   **/
>  
>  #ifdef HAVE_JEMALLOC_STATS
>  
> +// This has UNITS_PERCENTAGE, so it is multiplied by 100x.

nit: the "x" seems redundant with "multiplied by".

@@ +678,3 @@
>  }
>  
> +class HeapReporter MOZ_FINAL : public nsIMemoryReporter

"HeapReporter" is a bit generic. Maybe add something about jemalloc to the name?
Attachment #8355107 - Flags: review?(continuation) → review+
> > +    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
> 
> Should the * be on the right in JS-style for these two arguments?

The style of that whole file is a total mess of SM and Gecko styles.  Since |aFoo| arguments are Gecko style, I'll keep the |*| on the right for now.  

More generally, I have a proposal regarding code style that I'm planning to send to dev-platform soon, which will hopefully give us a path forward to fix this kind of style mixings.

> > +#define MOZ_COLLECT_REPORT(path, kind, units, amount, description)            \
> 
> This macro ignores the kind and units arguments, which is bad.

Whoa. Good catch.
> This is a good argument against uni-reporters. ;)

When the measurements are related, definitely.  We originally only had uni-reporters, and it's been a gradual, multi-year evolution to the current state.

> "HeapReporter" is a bit generic. Maybe add something about jemalloc to the
> name?

I contemplated "JemallocHeapReporter" when writing the patch, so I'll go with that.
You need to log in before you can comment on or make changes to this bug.