Closed Bug 831193 Opened 7 years ago Closed 7 years ago

Remove NS_MEMORY_REPORTER_IMPLEMENT

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(20 files, 23 obsolete files)

25.99 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
4.75 KB, patch
njn
: review+
Details | Diff | Splinter Review
7.70 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.77 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.58 KB, patch
njn
: review+
Details | Diff | Splinter Review
12.20 KB, patch
njn
: review+
Details | Diff | Splinter Review
10.05 KB, patch
njn
: review+
Details | Diff | Splinter Review
5.71 KB, patch
njn
: review+
Details | Diff | Splinter Review
11.81 KB, patch
njn
: review+
Details | Diff | Splinter Review
8.22 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
6.20 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.17 KB, patch
njn
: review+
Details | Diff | Splinter Review
13.09 KB, patch
njn
: review+
Details | Diff | Splinter Review
4.53 KB, patch
njn
: review+
Details | Diff | Splinter Review
7.48 KB, patch
njn
: review+
Details | Diff | Splinter Review
28.85 KB, patch
njn
: review+
Details | Diff | Splinter Review
4.70 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
4.44 KB, patch
njn
: review+
Details | Diff | Splinter Review
6.45 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.80 KB, patch
njn
: review+
Details | Diff | Splinter Review
Bug 829439 added the new mozilla::MemoryReporterBase type, which supersedes NS_MEMORY_REPORTER_IMPLEMENT.  This bug is about removing the vestiges of it.
This patch removes the uses of NS_MEMORY_REPORTER_IMPLEMENT in
nsMemoryReporterManager.cpp.

jlebar, the formatting style I've used for the descriptions is a little
unconventional, but I wanted to get something close to the density of the
macro.  Hopefully you don't mind it.
Attachment #702718 - Flags: review?(justin.lebar+bug)
Comment on attachment 702718 [details] [diff] [review]
(part 1) - Remove uses of NS_MEMORY_REPORTER_IMPLEMENT from nsMemoryReporterManager.cpp.

>-NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT(Private,
>-    "private",
>-    KIND_OTHER,
>-    UNITS_BYTES,
>-    GetPrivate,
>-    "Memory that cannot be shared with other processes, including memory that "
>-    "is committed and marked MEM_PRIVATE, data that is not mapped, and "
>-    "executable pages that have been written to.")
>+class PrivateReporter MOZ_FINAL : public MemoryReporterBase
>+{
>+public:
>+    PrivateReporter()
>+      : MemoryReporterBase("private", KIND_OTHER, UNITS_BYTES,
>+"Memory that cannot be shared with other processes, including memory that is "
>+"committed and marked MEM_PRIVATE, data that is not mapped, and executable "
>+"pages that have been written to.")
>+    {}
>+private:
>+    NS_IMETHOD GetAmount(int64_t *aAmount) { return GetPrivate(aAmount); }
>+    int64_t Amount() { MOZ_ASSERT(false); return 0; }
>+};

What would you think about improving MemoryReporterBase so it has a better
interface for writing fallible memory reporters?

One simple option would be to add a default implementation of Amount(), which
MOZ_ASSERTs as you have here.

Another option would be to add FallibleMemoryReporterBase, which implements
Amount() with a MOZ_ASSERT() and exposes a pure virtual GetFallibleAmount (or
something) method.

>+class PageFaultsHardReporter MOZ_FINAL : public MemoryReporterBase
>+{
>+public:
>+    PageFaultsHardReporter()
>+      : MemoryReporterBase("page-faults-hard", KIND_OTHER,
>+                           UNITS_COUNT_CUMULATIVE,
>+"The number of hard page faults (also known as 'major page faults') that have "
>+"occurred since the process started.  A hard page fault occurs when a process "
>+"tries to access a page which is not present in physical memory. The "
>+"operating system must access the disk in order to fulfill a hard page fault. "
>+"When memory is plentiful, you should see very few hard page faults. But if "
>+"the process tries to use more memory than your machine has available, you "
>+"may see many thousands of hard page faults. Because accessing the disk is up "
>+"to a million times slower than accessing RAM, the program may run very "
>+"slowly when it is experiencing more than 100 or so hard page faults a second.")
>+    {}
>+private:
>+    NS_IMETHOD GetAmount(int64_t *aAmount) { return GetPageFaultsHard(aAmount); }
>+    int64_t Amount() { MOZ_ASSERT(false); return 0; }
>+};

What would you think about inlining GetPageFaultsHard() into GetAmount() here?
And similarly for the jemalloc and atoms table reporters.

Being able to inline the Amount() function is the main advantage I see to
moving away from these macros.
> What would you think about improving MemoryReporterBase so it has a better
> interface for writing fallible memory reporters?
> 
> One simple option would be to add a default implementation of Amount(), which
> MOZ_ASSERTs as you have here.

Good idea.  I'll do that.


> What would you think about inlining GetPageFaultsHard() into GetAmount() here?
> And similarly for the jemalloc and atoms table reporters.

Sure.
I was going to attach a patch that applied on top of the previous one, but then
I typed |hg qref| instead of |hg qnew|.  Bah.

Anyway, as suggested:

- I changed it so you only have to provide one of GetAmount() and Amount().

- I inlined:  GetPageFaults{Hard,Soft}, GetPrivate, GetHeap*, GetExplicit,
  GetAtomTableSize.

Also:

- I fixed some |#if| conditions to |#ifdef| -- the behaviour is unchanged
  because the values are #defined to 1, but it's now consistent.

- I renamed AtomTableReporter to AtomsTableReporter to match its path.
Attachment #703661 - Flags: review?(justin.lebar+bug)
Attachment #702718 - Attachment is obsolete: true
Attachment #702718 - Flags: review?(justin.lebar+bug)
Comment on attachment 703661 [details] [diff] [review]
(part 1) - Remove uses of NS_MEMORY_REPORTER_IMPLEMENT from nsMemoryReporterManager.cpp.

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

>@@ -425,17 +425,20 @@ 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 values: path, kind, units, and description.  They are passed
> //   to the MemoryReporterBase constructor.
>-// - An Amount() method.  It can use the MallocSizeOf method if necessary.
>+// - An Amount() or GetAmount() method.  It can use the MallocSizeOf method if
>+//   necessary.  Use Amount() if the reporter is infallible, and GetAmount()
>+//   otherwise.  (You'll get assertion failures if you fail to provide one or
>+//   the other.)

Nit: assertion failures /when the memory reporter is run/

>@@ -484,17 +487,22 @@ public:
> 
>   NS_IMETHOD GetDescription(nsACString& aDescription)
>   {
>     aDescription.Assign(mDescription);
>     return NS_OK;
>   }
> 
> protected:
>-  virtual int64_t Amount() = 0; 
>+  virtual int64_t Amount()
>+  {
>+    // This fails if neither GetAmount() nor Amount() were overridden.

neither was

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

Wow, diff really did a number on this part.
Attachment #703661 - Flags: review?(justin.lebar+bug) → review+
MemoryReporterBase is the new, nicer way of doing things.
Attachment #704366 - Flags: review?(michal.novotny)
This is the new, improved style.
Attachment #704367 - Flags: review?(terrence)
This is the new, improved style.
Attachment #704369 - Flags: review?(benjamin)
This is the new, improved style.
Attachment #704370 - Flags: review?(jmuizelaar)
Marco, this patch is tiny, so I figure you'll be ok to review it.
Attachment #704372 - Flags: review?(mak77)
Attachment #704374 - Attachment description: (part 7) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in StartupCache.cpp. → (part 8) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in StartupCache.cpp.
As well as removing the uses of NS_MEMORY_REPORTER_IMPLEMENT, this patch does
the following.

- Converts the two reporters into a single multi-reporter, which is a bit
  nicer. 

- Renames MediaMemoryReporter as MediaMemoryTracker, which better reflects its
  function.
Attachment #704397 - Flags: review?(cpearce)
We don't need NumLowMemoryEventsReporter as a base class any more.

A question... the paths for the three fields are:

- "low-memory-events/virtual"
- "low-memory-events/physical"
- "low-commit-space-events"

Should the last one be changed to "low-memory-events/commit-space"?
Attachment #704400 - Flags: review?(justin.lebar+bug)
Ah, finally.
Attachment #704407 - Flags: review?(justin.lebar+bug)
Attachment #704410 - Flags: review?(justin.lebar+bug)
Fix a silly compile error.
Attachment #704414 - Flags: review?(justin.lebar+bug)
Attachment #704400 - Attachment is obsolete: true
Attachment #704400 - Flags: review?(justin.lebar+bug)
Comment on attachment 704397 [details] [diff] [review]
(part 16) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in media/.

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

Matthew wrote the original memory reporters, so I think he's a better reviewer.
Attachment #704397 - Flags: review?(cpearce) → review?(kinetik)
Comment on attachment 704394 [details] [diff] [review]
(part 14) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in layout/.

I don't think review from a layout peer is needed for this sort of thing; you want review from somebody know knows the memory reporting infrastructure instead.

Thus punting review to jlebar since he reviewed bug 829439, but feel free to punt further.
Attachment #704394 - Flags: review?(dbaron) → review?(justin.lebar+bug)
Attachment #704369 - Flags: review?(benjamin) → review?(jones.chris.g)
Comment on attachment 704373 [details] [diff] [review]
(part 7) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in toolkit/components/.

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

I like the new style!

::: toolkit/components/telemetry/Telemetry.cpp
@@ +337,5 @@
> +  n += mHistogramMap.SizeOfExcludingThis(nullptr, aMallocSizeOf);
> +  n += mPrivateSQL  .SizeOfExcludingThis(nullptr, aMallocSizeOf);
> +  n += mSanitizedSQL.SizeOfExcludingThis(nullptr, aMallocSizeOf);
> +  n += mTrackedDBs  .SizeOfExcludingThis(nullptr, aMallocSizeOf);
> +  n += mHangReports .SizeOfExcludingThis();

Please don't align these.
Attachment #704373 - Flags: review?(nfroyd) → review+
Comment on attachment 704379 [details] [diff] [review]
(part 9) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in Preferences.cpp.

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

::: modules/libpref/src/Preferences.cpp
@@ +217,5 @@
>  class AddPreferencesMemoryReporterRunnable : public nsRunnable
>  {
>    NS_IMETHOD Run()
>    {
> +    return NS_RegisterMemoryReporter(new PreferencesReporter);

I thought we had decided once upon a time that a ref to the memory reporter should be held over the call registering the reporter.  Is this no longer the case?  I still see a mix of both styles in existing code and the patch series for this bug.
Attachment #704379 - Flags: review?(nfroyd) → review+
Attachment #704374 - Flags: review?(taras.mozilla) → review+
Attachment #704396 - Flags: review?(ehsan) → review+
Attachment #704367 - Flags: review?(terrence) → review+
> I don't think review from a layout peer is needed for this sort of thing;
> you want review from somebody know knows the memory reporting infrastructure
> instead.

I asked you because you reviewed the original memory reporter patches.  But I'm fine with jlebar reviewing if you are.
> I thought we had decided once upon a time that a ref to the memory reporter
> should be held over the call registering the reporter.  Is this no longer
> the case?  I still see a mix of both styles in existing code and the patch
> series for this bug.

Bug 831193 will fix this situation fully.  In the meantime, holding a ref isn't necessary and most of the existing code doesn't do it (excluding the cases where an nsCOMPtr is held by a parent object) so I'm going for consistency.
Attachment #704385 - Flags: review?(jmuizelaar) → review+
Attachment #704391 - Flags: review?(jmuizelaar) → review+
Attachment #704397 - Flags: review?(kinetik) → review+
Comment on attachment 704383 [details] [diff] [review]
(part 10) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in parts of gfx/.

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

::: gfx/gl/GLContext.cpp
@@ +104,2 @@
>      } else {
> +        sAmount += bytes;

Two questions here. I realize that these are questions about the existing code rather than your patch, so I'm not r-'ing for that, but it looks like the original patch could have used some r-'d. git blame says it's from bug 818060.

1. I thought that about:memory's approach to memory accounting was explicitly to re-count everything everytime, and not to use accumulating counters, as you explain on
  https://wiki.mozilla.org/Memory_Reporting
But the counters here are violating this good principle. 

2. sAmount is static, and OpenGL work is done on more than one thread (for now on Android, in the near future on other platforms). So shouldn't these operations be made atomic?

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +191,5 @@
>  
>  GrallocBufferActor::~GrallocBufferActor()
>  {
>    if (mAllocBytes > 0) {
> +    GrallocReporter::sAmount -= mAllocBytes;

Same remarks about this counter.
Attachment #704383 - Flags: review?(bjacob) → review+
Filed bug 833185 to continue that conversation.
I forgot to convert a couple of mReporter fields from raw pointers to nsCOMPtr.
Attachment #704771 - Flags: review?(justin.lebar+bug)
Attachment #704414 - Attachment is obsolete: true
Attachment #704414 - Flags: review?(justin.lebar+bug)
Comment on attachment 704408 [details] [diff] [review]
(part 19) - Make nsPrefixSetReporter a subclass of MemoryReporterBase.

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

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
@@ +33,5 @@
>  #define LOG(args)
>  #define LOG_ENABLED() (false)
>  #endif
>  
> +class PrefixSetReporter MOZ_FINAL : public MemoryReporterBase

Given that the old code used THREADSAFE_ISUPPORTS1, shouldn't this be ThreadSafeMemoryReporterBase?

@@ +68,5 @@
>  
>  NS_IMETHODIMP
>  nsUrlClassifierPrefixSet::Init(const nsACString& aName)
>  {
> +  mReporter = new PrefixSetReporter(this, aName);

Any reasoning for dropping the ns Prefix? It's inconsistent with the rest of this source file.
Attachment #704408 - Flags: review?(gpascutto) → review+
> > +class PrefixSetReporter MOZ_FINAL : public MemoryReporterBase
> 
> Given that the old code used THREADSAFE_ISUPPORTS1, shouldn't this be
> ThreadSafeMemoryReporterBase?

Good question!  MemoryReporterBase is now threadsafe by default;  registration isn't performance critical and it's safer.

> Any reasoning for dropping the ns Prefix? It's inconsistent with the rest of
> this source file.

It's annoying, and the class isn't visible outside the file.
Comment on attachment 704372 [details] [diff] [review]
(part 6) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in mozStorageService.cpp.

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

::: storage/src/mozStorageService.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */

this change clashes with http://mxr.mozilla.org/mozilla-central/source/storage/style.txt#14
and I suppose it's a bit late to change style and have all the module differ from it. please revert for now.

@@ +53,5 @@
> +{
> +public:
> +  StorageSQLiteReporter()
> +    : MemoryReporterBase("storage-sqlite", KIND_OTHER, UNITS_BYTES,
> +"Memory used by SQLite.")

the indentation here is wrong, please align arguments

@@ +56,5 @@
> +    : MemoryReporterBase("storage-sqlite", KIND_OTHER, UNITS_BYTES,
> +"Memory used by SQLite.")
> +  {}
> +private:
> +  int64_t Amount() { return ::sqlite3_memory_used(); }

please annotate with MOZ_OVERRIDE
Attachment #704372 - Flags: review?(mak77) → review+
Attachment #704366 - Flags: review?(michal.novotny) → review+
Comment on attachment 704384 [details] [diff] [review]
(part 11) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in gfxAndroidPlatform.cpp.

>+class FreetypeReporter MOZ_FINAL : public MemoryReporterBase
>+{
>+public:
>+    FreetypeReporter()
>+      : MemoryReporterBase("explicit/freetype", KIND_HEAP, UNITS_BYTES,
>+                           "Memory used by Freetype.")
>+    {}
> 
>+    static void* CountingAlloc(FT_Memory, long size)
> {
>     void *p = malloc(size);
>+        sAmount += MallocSizeOfOnAlloc(p);
>     return p;
> }

Is this a diff -w, so the indentation is screwy?

>+private:
>+    NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_ALLOC_FUN(MallocSizeOfOnAlloc)
>+    NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_FREE_FUN(MallocSizeOfOnFree)

I'd prefer if these lived in MemoryReporterBase.

>+    int64_t Amount() { return sAmount; }
>+
>+    static int64_t sAmount;

Using a static variable as a member variable of a non-singleton class is pretty
scary.  It also sets a bad example for others to cargo-cult off.

Could you do something to ensure that this class is actually a singleton (even
if that's just ensuring that the constructor never runs twice)?

Anoter alternative would be to stick the reporter in a ClearOnShutdown'ed
StaticRefPtr, add a factory method, and make the constructor private.
Attachment #704384 - Flags: review?(justin.lebar+bug) → review-
Comment on attachment 704394 [details] [diff] [review]
(part 14) - Don't use NS_MEMORY_REPORTER_IMPLEMENT in layout/.

>diff --git a/layout/base/nsStyleSheetService.cpp b/layout/base/nsStyleSheetService.cpp
>--- a/layout/base/nsStyleSheetService.cpp
>+++ b/layout/base/nsStyleSheetService.cpp

>@@ -16,52 +17,50 @@
> #include "nsIServiceManager.h"
> #include "nsICategoryManager.h"
> #include "nsISupportsPrimitives.h"
> #include "nsNetUtil.h"
> #include "nsIObserverService.h"
> #include "nsLayoutStatics.h"
> #include "nsIMemoryReporter.h"
> 
>-NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(LayoutStyleSheetServiceMallocSizeOf)
>-
>-static int64_t
>-GetStyleSheetServiceSize()
>+class LayoutStyleSheetServiceReporter MOZ_FINAL
>+  : public mozilla::MemoryReporterBase
> {
>-  return nsStyleSheetService::SizeOfIncludingThis(
>-           LayoutStyleSheetServiceMallocSizeOf);
>-}
>-
>-NS_MEMORY_REPORTER_IMPLEMENT(StyleSheetService,
>-  "explicit/layout/style-sheet-service",
>-  KIND_HEAP,
>-  nsIMemoryReporter::UNITS_BYTES,
>-  GetStyleSheetServiceSize,
>+public:
>+  LayoutStyleSheetServiceReporter(nsStyleSheetService* aService)
>+    : MemoryReporterBase("explicit/layout/style-sheet-service",
>+                         KIND_HEAP, UNITS_BYTES,
>   "Memory used for style sheets held by the style sheet service.")
>+  , mService(aService)
>+  {}
>+private:
>+  int64_t Amount() { return mService->SizeOfIncludingThis( MallocSizeOf); }

"( "

> nsStyleSheetService::nsStyleSheetService()
> {
>   PR_STATIC_ASSERT(0 == AGENT_SHEET && 1 == USER_SHEET && 2 == AUTHOR_SHEET);
>   NS_ASSERTION(!gInstance, "Someone is using CreateInstance instead of GetService");
>   gInstance = this;
>   nsLayoutStatics::AddRef();
> 
>-  mReporter = new NS_MEMORY_REPORTER_NAME(StyleSheetService);
>+  mReporter = new LayoutStyleSheetServiceReporter(this);

>-  (void)::NS_RegisterMemoryReporter(mReporter);
>+  NS_RegisterMemoryReporter(mReporter);

I suspect the (void) was put in to avoid compiler warnings, and the :: was put
in out of someone's understanding of layout style.  Since I'm not a layout
peer, I'm not comfortable approving of a reversion of that style.

>-  (void)::NS_UnregisterMemoryReporter(mReporter);
>-  mReporter = nullptr;
>+  NS_UnregisterMemoryReporter(mReporter);

ibid.

>@@ -221,24 +210,23 @@ nsLayoutStylesheetCache::nsLayoutStylesh
>-  mReporter = new NS_MEMORY_REPORTER_NAME(StyleSheetCache);
>-  (void)::NS_RegisterMemoryReporter(mReporter);
>+  mReporter = new LayoutStyleSheetCacheReporter(this);
>+  NS_RegisterMemoryReporter(mReporter);
> }
> 
> nsLayoutStylesheetCache::~nsLayoutStylesheetCache()
> {
>-  (void)::NS_UnregisterMemoryReporter(mReporter);
>-  mReporter = nullptr;
>+  NS_UnregisterMemoryReporter(mReporter);
> }

ibid.

OT, but this idiom we've developed here of having classes with a
nsCOMPtr<nsIMemoryReporter> member which they register on construction and
unregister on destruction is kind of bad OOP.

ISTM that a better way of doing it would be to inherit from a special class.  E.g.

  class Foo : public MemoryReporterMixin<Foo>
  {
    Foo()
      : MemoryReporterMixin<Foo>(KIND_HEAP, ...)
    {}

    size_t SizeOfIncludingThis(...) const {...};
  };

  class MemoryReporterMixin<T>
  {
    int64_t Amount() { return T::SizeOfIncludingThis(...); }
  };

Then the MemoryReporterMixin constructor would register the memory reporter,
and the MemoryReporterMixin destructor would unregister it.

The main trick would be refcounting.  The memory reporter manager couldn't keep
a strong ref to Foo, because then Foo would never be destructed.  Maybe a weak
reference would do.

Actually, now that I think of it, this is not entirely off-topic. Suppose we
have the following situation.

  1. On the main thread, we take a reference to a Foo memory reporter.
     (For example, we might be running telemetry, which holds refs to
     nsIMemoryReporter objects via JS.)
  
     Our memory reporter has a member |Foo* mFoo|.  When we run the memory
     reporter, we do mFoo->SizeOfIncludingThis().

  2. Object Foo is destroyed off the main thread.  This does
     NS_UnregisterMemoryReporter(Foo), but the memory reporter is not deleted,
     because JS has a ref to it!

  3. We do GetAmount() on the memory reporter.

So this idiom is not thread-safe.  That probably invalidates some of your patches here.  :-/
Attachment #704394 - Flags: review?(justin.lebar+bug) → review+
Attachment #704407 - Flags: review?(justin.lebar+bug) → review+
Attachment #704410 - Flags: review?(justin.lebar+bug) → review+
Attachment #704771 - Flags: review?(justin.lebar+bug) → review+
> this change clashes with
> http://mxr.mozilla.org/mozilla-central/source/storage/style.txt#14
> and I suppose it's a bit late to change style and have all the module differ
> from it. please revert for now.

Sigh.


> >-  (void)::NS_RegisterMemoryReporter(mReporter);
> >+  NS_RegisterMemoryReporter(mReporter);
> 
> I suspect the (void) was put in to avoid compiler warnings, and the :: was
> put
> in out of someone's understanding of layout style.  Since I'm not a layout
> peer, I'm not comfortable approving of a reversion of that style.

It's required for the storage module style, and then it got copied everywhere else.  So removing it here (and everywhere else) is a win.  Storage will continue to live in its own little world, unfortunately.
> It's required for the storage module style, and then it got copied everywhere else.  So 
> removing it here (and everywhere else) is a win.

Okay, if it's not a layout thing, that's fine with me.
> > +  int64_t Amount() { return ::sqlite3_memory_used(); }
> 
> please annotate with MOZ_OVERRIDE

I can do it in this case, but I won't do it in general because if you get the override wrong then MemoryReporterBase::GetAmount() will assert.
> So this idiom is not thread-safe.  That probably invalidates some of your
> patches here.  :-/

Hmm.  It's not the nsCOMPtr<nsIMemoryReporter>, which points from the Foo to the FooReporter, that's the problem.  It's the |nsFoo*|, which points from the FooReporter to the Foo.

Currently we have various Foo/FooReporter pairs, for singleton classes.  Generally the Amount() function equivalent is static and does something like this:

  x = get unique instance of Foo
  if (x)
    measure x

These patches have changed this to:

  x = local pointer to Foo
  measure x        // x may be dangling

Even the first case isn't thread-safe, AFAICT, because the Foo could be destroyed between getting and check |x|.
> Even the first case isn't thread-safe, AFAICT, because the Foo could be destroyed between getting 
> and checking |x|.

Yes, although I'd guess that most of our static objects are main-thread-only, so I'm most concerned with the cases where we have a FooReporter for a multithreaded, non-singleton Foo.
Attachment #704369 - Flags: review?(jones.chris.g) → review+
Attachment #704370 - Flags: review?(jmuizelaar) → review+
> ISTM that a better way of doing it would be to inherit from a special class.
> E.g.
> 
>   class Foo : public MemoryReporterMixin<Foo>
>   {
>     Foo()
>       : MemoryReporterMixin<Foo>(KIND_HEAP, ...)
>     {}
> 
>     size_t SizeOfIncludingThis(...) const {...};
>   };
> 
>   class MemoryReporterMixin<T>
>   {
>     int64_t Amount() { return T::SizeOfIncludingThis(...); }
>   };
> 
> Then the MemoryReporterMixin constructor would register the memory
> reporter, and the MemoryReporterMixin destructor would unregister it.
> 
> The main trick would be refcounting.  The memory reporter manager
> couldn't keep a strong ref to Foo, because then Foo would never be
> destructed.  Maybe a weak reference would do.

I think this will work.  The manager would keep weak references (just raw pointers should suffice, I think).  When enumerating the reporters, we'd use temporary strong pointers to ensure the objects stay alive until after GetAmount et al. are called.

But there's still one problem:  what if a Foo object is destroyed on another thread while we are in the middle of enumerating?  We'll be mixing enumeration with deletion, which sounds bad.  In fact, I think we already have that problem with the existing code!

More specifically, nsMemoryReporterManager has a lock which it takes when adding and removing reporters, and when *creating* an enumerator.  But it doesn't lock when *using* an enumerator.  I guess we should hold the lock the entire time the enumerator is alive?  The obvious way to do this is for EnumerateReporters() to take the lock and then add a FinishEnumeratingReporters() method that releases the lock, though hopefully there's an RAII approach that avoids the need for the latter method.
> what if a Foo object is destroyed on another thread while we are in the middle of 
> enumerating?

While the enumerator object is alive, it holds a strong ref to all of the "enumeratees" (the memory reporters).  So I think this isn't a problem?
> While the enumerator object is alive, it holds a strong ref to all of the
> "enumeratees" (the memory reporters).

That is true, but I was worried that we might e.g. try to enumerate N elements but by the time we get to the end there are only N-1 in the array.  But NeilAway informed me (and I checked) that NS_NewArrayEnumerator makes a temporary copy of the array to avoid problems with this case.  So I think it's safe.
Blocks: 847641
No longer blocks: 847641
After seven months I'm resurrecting this.  jlebar's concerns about races were valid, so I'm going to avoid creating FooReporter classes that contain raw pointers to Foo objects -- instead I'll stick with the existing code that typically goes via a static singleton pointer.

I've rebased the patches and made that change.  Since it brings the patches closer to the current code than all the previous patches, I will carry over the r+'s that are already present.
Attachment #704366 - Attachment is obsolete: true
Attachment #704367 - Attachment is obsolete: true
Attachment #704369 - Attachment is obsolete: true
Attachment #704370 - Attachment is obsolete: true
Attachment #704372 - Attachment is obsolete: true
Attachment #704373 - Attachment is obsolete: true
jlebar, you r-'d this previously because it's a reporter with a static counter
but no protection against multiple instantiation.  I added an assertion to the
constructor, which is the same strategy used in several other reporters.
Attachment #794466 - Flags: review?(justin.lebar+bug)
This reporter was added after the first round of patches was written.
Attachment #794475 - Flags: review?(justin.lebar+bug)
Attachment #704374 - Attachment is obsolete: true
Attachment #704379 - Attachment is obsolete: true
Attachment #704383 - Attachment is obsolete: true
Attachment #704384 - Attachment is obsolete: true
Attachment #704385 - Attachment is obsolete: true
Attachment #704391 - Attachment is obsolete: true
Attachment #704394 - Attachment is obsolete: true
Attachment #704396 - Attachment is obsolete: true
Attachment #704397 - Attachment is obsolete: true
Attachment #704407 - Attachment is obsolete: true
Attachment #704408 - Attachment is obsolete: true
Attachment #704410 - Attachment is obsolete: true
Attachment #704771 - Attachment is obsolete: true
Attachment #794456 - Flags: review+
Attachment #794457 - Flags: review+
Attachment #794459 - Flags: review+
Attachment #794460 - Attachment is obsolete: true
Attachment #794460 - Flags: review+
Attachment #794461 - Flags: review+
Attachment #794462 - Flags: review+
Attachment #794463 - Flags: review+
Attachment #794464 - Flags: review+
Attachment #794465 - Flags: review+
Attachment #794467 - Flags: review+
Attachment #794468 - Flags: review+
Attachment #794469 - Flags: review+
Attachment #794470 - Flags: review+
Attachment #794472 - Flags: review+
Attachment #794473 - Flags: review+
Attachment #794476 - Flags: review+
Attachment #794478 - Flags: review+
Attachment #794479 - Flags: review+
Attachment #794466 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 794475 [details] [diff] [review]
(part 18) - Don't use NS_MEMORY_REPORTER_IMPLEMENT for the ICU reporter.

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp
>--- a/xpcom/build/nsXPComInit.cpp
>+++ b/xpcom/build/nsXPComInit.cpp

>@@ -330,61 +330,66 @@ NS_GetTraceRefcnt(nsITraceRefcnt** resul
> 
> EXPORT_XPCOM_API(nsresult)
> NS_InitXPCOM(nsIServiceManager* *result,
>                              nsIFile* binDirectory)
> {
>     return NS_InitXPCOM2(result, binDirectory, nullptr);
> }
> 
>-// |sSizeOfICU| can be accessed by multiple JSRuntimes, so it must be
>-// thread-safe.
>-static Atomic<size_t> sSizeOfICU;
>+class ICUReporter MOZ_FINAL : public MemoryReporterBase
>+{
>+public:
>+    ICUReporter()
>+      : MemoryReporterBase("explicit/icu", KIND_HEAP, UNITS_BYTES,
>+"Memory used by ICU, a Unicode and globalization support library.")

This patch duplicates a lot of code from the previous patch I reviewed, and you
may have this same code elsewhere.

Is it time to factor it out?

r=me because this patch is a net improvement.
Attachment #794475 - Flags: review?(justin.lebar+bug) → review+
> This patch duplicates a lot of code from the previous patch I reviewed, and
> you
> may have this same code elsewhere.
> 
> Is it time to factor it out?

Yeah, as a follow-up I've been thinking about having multiple variants of MemoryReporterBase, one of which would be for counter-based reporters like this.
You need to log in before you can comment on or make changes to this bug.