829439 - Add new macros that promote better encapsulation within nsIMemoryReporter sub-classes.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

NS_MEMORY_REPORTER_IMPLEMENT doesn't allow nsIMemoryReporter sub-classes to
have state, which promotes an ungainly style of memory reporter for singleton
objects -- we have to use a static measurement function that accesses a static
singleton pointer to the object.  This also encourages static pointers to
memory reporter objects, which are either |nsCOMPtr<nsIMemoryReporter>| (and
thus involve a static constructor) or |nsIMemoryReporter*| (which are a bit
iffy, as per bug 803785).

(An example:  CycleCollectorMultiReporter measures and reports memory used by
the singleton nsCycleCollector object.  Ideally the collector object would have
an nsCOMPtr to the reporter, and the reporter would have a raw pointer back to
the collector object.)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add new macros that promote better encapsulation within nsIMemoryReporter sub-classes.

(Requesting review from mccr8 for the cycle collector changes, hurley for the
netwerk/ changes, and jlebar for the rest.)

This patch:

- Adds some new macros for creating nsIMemoryReporter sub-classes, similar to
  NS_MEMORY_REPORTER_IMPLEMENT, but that allow extra members in the memory
  reporter class.  This allows memory reporters to be more self-contained,
  avoiding the need for static functions and static pointers.

- Uses the macros to remove all the existing static pointers to memory
  reporters.

  - sEventListenerManagersHashReporter.  The reporter for this no longer gets
    unregistered;  instead it can tell if it shouldn't measure any more.

  - gCanvasAzureMemoryReporter.  This pointer wasn't even needed, because the
    reporter is never freed.

  - nsJSEnvironment.cpp:gReporter.  This is now held within
    nsScriptNameSpaceManager.

  - {Memory,Disk}CacheReporter.  These are now held within
    nsCache{Memory,Disk}Device.

  - sCollectorReporter.  Now held in nsCycleCollector.

- Fixed some mode lines.

Comment 2

[Justin Lebar (not reading bugmail)]

I have to admit, I don't find the cycle collector macros to be an example worth
emulating in this case.  I'm all for reducing boilerplate, but are you open to
considering ways to reduce the number of macros here?

My thought is that we'd create a class mozilla::MemoryReporter (or
nsMemoryReporter, if you prefer) defined like

> class MemoryReporter : public nsIMemoryReporter
> {
> public:
>   MemoryReporter(const char* aPath, int32_t aKind,
>                  int32_t aUnits, const char* aDescription)
>     : mPath(aPath)
>     , mKind(aKind)
>     , mUnits(aUnits)
>     , mDescription(aDescription)
>   {}
> 
>   NS_IMETHOD GetProcess(nsACString& aProcess)
>   {
>     Process(aProcess);
>     return NS_OK;
>   }
> 
>   NS_IMETHOD GetKind(int32_t* aKind)
>   {
>     *aKind = Kind();
>     return NS_OK;
>   }
> 
>   NS_IMETHOD GetUnits(int32_t* aUnits) {...}
>   NS_IMETHOD GetDescription(int32_t* aDescription) {...}
> 
>   static MallocSizeOf(const void* aPtr) {...}
> 
> protected:
>   virtual int64_t Amount() = 0; 
> 
>   virtual void Process(nsACString& aProcess) { aProcess.Truncate(); }
>   virtual int32_t Kind() { return mKind; }
>   virtual int32_t Units() { return mUnits; }
>   virtual void Description(nsACString& aDescription) { ... }
> };
> 
> // (In a cpp file)
> NS_IMPL_THREADSAFE_ISUPPORTS1(MemoryReporter, nsIMemoryReporter)

Note that because we did NS_IMPL_ISUPPORTS1(MemoryReporter), if you inherit
from MemoryReporter you can't inherit from any other XPIDL interfaces without
some pain (if it's possible at all).  This doesn't bother me, and isn't a
regression wrt the macro approach.

I added the virtual Process/Kind/Units/Description methods to mirror the
NS_IMPL_MEMORY_REPORTER_{PROCESS,PATH,KIND,UNITS,AMOUNT,DESCRIPTION} macros;
the virtual methods let you provide a dynamic process/kind/units/description
method with just one line of impl.

We could have the same sort of helper for multi-reporters, if you wanted.  (We
could do it in a separate bug.)

I looked through all the memory reporters in this patch, and I think they would
all be approximately the same length with this approach and with the macro
approach.

Comment 3

[Justin Lebar (not reading bugmail)]

I meant to add: What do you think?

Comment 4

[u408661]

Comment on attachment 700873 [details] [diff] [review]
Add new macros that promote better encapsulation within nsIMemoryReporter sub-classes.

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

All the netwerk/ stuff looks just fine to me, but since all necko changes require r+ from a necko peer (which I am not), I'm shipping the official review over to Michal.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 700873 [details] [diff] [review]
Add new macros that promote better encapsulation within nsIMemoryReporter sub-classes.

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

Thanks for doing this, it is nice to have this be less weird.

r=me on nsCycleCollector.cpp changes.

::: xpcom/base/nsCycleCollector.cpp
@@ +2447,5 @@
> +
> +class CycleCollectorMultiReporter MOZ_FINAL : public nsIMemoryMultiReporter
> +{
> +  private:
> +    NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(MallocSizeOf)

Move this later... somewhere...

@@ +2449,5 @@
> +{
> +  private:
> +    NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(MallocSizeOf)
> +
> +    nsCycleCollector* mCollector;

Field declarations should be at the very end of the class definition.

@@ +2454,5 @@
> +
> +  public:
> +    NS_DECL_ISUPPORTS
> +
> +    CycleCollectorMultiReporter(nsCycleCollector* aCollector)

Constructor definition should go first, followed by NS_DECL_ISUPPORTS.

@@ +2458,5 @@
> +    CycleCollectorMultiReporter(nsCycleCollector* aCollector)
> +      : mCollector(aCollector)
> +    {}
> +
> +    NS_IMETHOD GetName(nsACString &name)

nit: & on the left

@@ +2465,5 @@
> +        return NS_OK;
> +    }
> +
> +    NS_IMETHOD CollectReports(nsIMemoryMultiReporterCallback *aCb,
> +                              nsISupports *aClosure)

nit: * on the left. This file isn't very consistent, but you are adding an all-new chunk of code, so you might as well do the "right" thing.

@@ +2474,5 @@
> +                                        &objectSize, &graphNodesSize,
> +                                        &graphEdgesSize, &whiteNodesSize,
> +                                        &purpleBufferSize);
> +
> +    #define REPORT(_path, _amount, _desc)                                     \

It seems a little odd you don't centrally define REPORT_HEAP_BYTES or something in the memory reporter header and put a new macro definition here, but no big deal.

@@ +2508,5 @@
> +
> +        return NS_OK;
> +    }
> +
> +    NS_IMETHOD GetExplicitNonHeap(int64_t *n)

nit: * should be on the left

@@ +2555,4 @@
>  }
>  
>  
> +void

The style here is to follow "void" by a space, to represent the void of space... no I kid, thanks for fixing these.

@@ +2568,5 @@
> +
> +    static bool registered = false;
> +    if (!registered) {
> +        // We can't do this in nsCycleCollector() because that runs before the
> +        // memory reporter manager is initialized.  So we do it here instead.

Thanks for the comment. Maybe it makes more sense to put the comment before the definition of |registered|, because conceptually this entire block has to do with memory reporter registering.

Comment 6

[Nicholas Nethercote [inactive]]

> It seems a little odd you don't centrally define REPORT_HEAP_BYTES or
> something in the memory reporter header and put a new macro definition here,
> but no big deal.

There's enough slight variations in the different REPORT macros around the place that trying to centralize it is doomed to fail, I fear.  See XPCJSRuntime.cpp where there are seven different *REPORT* macros...

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add MemoryReporterBase class that promotes better encapsulation within nsIMemoryReporter sub-classes.

New version that doesn't use macros.  It also addresses mccr8's comments.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Some memory reporter clean-ups.

This patch cleans up some more stuff.

- We have a nsMemoryReporter class that's just used for a fixed report that's
  sent by a child process.  (This is for multi-process reporting.)  This patch
  moves it into ContentParent.cpp, renames it ChildMemoryReporter, and makes it
  a sub-class of MemoryReporter.

- It removes the MemoryReport struct, which is unused.  Who knows what that was
  for.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 701672 [details] [diff] [review]
(part 1) - Add MemoryReporterBase class that promotes better encapsulation within nsIMemoryReporter sub-classes.

>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl
>--- a/xpcom/base/nsIMemoryReporter.idl
>+++ b/xpcom/base/nsIMemoryReporter.idl

>+// The NS_*MEMORY_REPORTER_IMPLEMENT* macros are the deprecated short-cut way
>+// of defining memory reporters.   You should instead subclass the
>+// MemoryReporterBase and ThreadSafeMemoryReporterBase classes below.

What would you think about making memory reporters threadsafe by default?

I'll go out on a limb and say that we don't have enough memory reporter objects
for the threadsafe refcounting to make a perf difference, and if they're safe
by default, it's one less thing to screw up when implementing reporters.

>@@ -416,9 +419,97 @@ void RunReporters();
>+namespace mozilla {
>+
>+// The following base class reduces the amount of boilerplate code required for
>+// memory reporters.  You just need to provide the following.
>+// - The constant path, kind, units, description values.

Please say "to the MemoryReporterBase constructor" or something.

>+// - An Amount() function. 
>+// - Possibly a NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN function.  This function
>+//   isn't provided in this class because we want a more specific subclass
>+//   method to show up in DMD's "reported by" stack traces.

I'm not revisiting the question of whether we want the MALLOC_SIZEOF_FUNs in
general, but do we need to do this?

Presumably any use of FooReporter's MALLOC_SIZEOF_FUN will have a FooReporter
method on its stack.  It might not be at the top, but we can guarantee it'll be
in there somewhere if we make MemoryReporterBase's MallocSizeOf function
protected.

Are you open to a compromise here where we define MallocSizeOf in
MemoryReporterBase, or is the concern that the stacks might be so deep that you
wouldn't be able to see the FooReporter method?

>diff --git a/content/canvas/src/CanvasRenderingContext2D.cpp b/content/canvas/src/CanvasRenderingContext2D.cpp
>--- a/content/canvas/src/CanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/CanvasRenderingContext2D.cpp

>+    static bool registered = false;
>+    if (!registered) {
>+      registered = true;
>+      NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(CanvasAzureMemory));

If we're going to say it's OK to call NS_RegisterMemoryReporter while the
reporter object has a refcount of 0, we should explicitly document that it's
safe and we should explicitly make it safe by acquiring a strong ref at the
beginning of NS_RegisterMemoryReporter and asserting that, if we release that
ref, the object's refcount does not go to 0.

>diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
>--- a/xpcom/base/nsCycleCollector.cpp
>+++ b/xpcom/base/nsCycleCollector.cpp

>+    #define REPORT(_path, _amount, _desc)                                     \

Nit: #undef REPORT?

>diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp
>--- a/xpcom/base/nsMemoryReporterManager.cpp
>+++ b/xpcom/base/nsMemoryReporterManager.cpp

>+NS_IMPL_ISUPPORTS1(MemoryReporterBase, nsIMemoryReporter)
>+NS_IMPL_THREADSAFE_ISUPPORTS1(ThreadSafeMemoryReporterBase, nsIMemoryReporter)

I'm not 100% sure this works.  I guess AddRef, Release, and QI are just virtual
methods, and you're just overriding them here, so it's probably fine?  Anyway
if we don't make all of the memory reporters threadsafe (which is the simplest
thing, I think), I'd like us to check with bsmedberg or bz or someone that it's
safe to do

  class Foo : public nsIFoo {};
  class Bar : public Foo {};

  NS_IMPL_ISUPPORTS1(Foo, nsIFoo);
  NS_IMPL_ISUPPORTS1(Bar, nsIFoo);

Comment 10

[Nicholas Nethercote [inactive]]

> What would you think about making memory reporters threadsafe by default?

Sounds good.

> Presumably any use of FooReporter's MALLOC_SIZEOF_FUN will have a FooReporter
> method on its stack.  It might not be at the top, but we can guarantee it'll
> be in there somewhere if we make MemoryReporterBase's MallocSizeOf function
> protected.

Good point.  I'll look at the DMD stacks.  If things are still clear with this change, I'll do it.

> If we're going to say it's OK to call NS_RegisterMemoryReporter while the
> reporter object has a refcount of 0, we should explicitly document that it's
> safe and we should explicitly make it safe by acquiring a strong ref at the
> beginning of NS_RegisterMemoryReporter and asserting that, if we release that
> ref, the object's refcount does not go to 0.

Yes.  That's bug 803785.  I was working on that when I decided that fixing up this was something of a prerequisite.  I've marked it as depending on this bug, and I should get to it soon.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cf84d5ec1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6925a363cddf

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f6cf84d5ec1a
https://hg.mozilla.org/mozilla-central/rev/6925a363cddf