Last Comment Bug 698326 - Add memory reporter for the url-classifier
: Add memory reporter for the url-classifier
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: Firefox 11
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on: DMD 701210
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-10-30 19:42 PDT by Nicholas Nethercote [:njn]
Modified: 2014-05-27 12:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1. v1 Make PrefixSet an iMemoryReporter (7.39 KB, patch)
2011-11-09 06:39 PST, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Review
Patch 1. v2 Add Memory Reporter for PrefixSet (7.24 KB, patch)
2011-11-09 11:47 PST, Gian-Carlo Pascutto [:gcp]
justin.lebar+bug: review-
Details | Diff | Review
Patch 1. v3 Add Memory Reporter for PrefixSet (7.13 KB, patch)
2011-11-10 02:21 PST, Gian-Carlo Pascutto [:gcp]
justin.lebar+bug: review+
n.nethercote: review+
Details | Diff | Review
Patch 1. Fix SizeOf naming (4.93 KB, patch)
2011-11-15 00:38 PST, Gian-Carlo Pascutto [:gcp]
n.nethercote: review-
Details | Diff | Review
Patch 2. v2 Remove countMe argument (5.22 KB, patch)
2011-11-15 01:26 PST, Gian-Carlo Pascutto [:gcp]
n.nethercote: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2011-10-30 19:42:01 PDT
I just saw this report from DMD:

==25366== Unreported: 2,097,152 (cumulative: 571,507,048) bytes in 1 heap block(s) in record 30
of 16289:
==25366==  Requested bytes unreported: 1,068,866 / 1,068,866
==25366==  Slop      bytes unreported: 1,028,286 / 1,028,286
==25366==    at 0x402A15B: realloc (vg_replace_malloc.c:632)
==25366==    by 0x403C0EC: moz_xrealloc (mozalloc.cpp:135)
==25366==    by 0x66DFEEB: nsTArrayInfallibleAllocator::Realloc(void*, unsigned long) (nsTArray.h:90)
==25366==    by 0x66E6231: nsTArray_base<nsTArrayDefaultAllocator>::ShrinkCapacity(unsigned int,
 unsigned long) (nsTArray-inl.h:244)
==25366==    by 0x7597C77: nsTArray<unsigned short, nsTArrayDefaultAllocator>::Compact() (nsTArray.h:1099)
==25366==    by 0x7596CF7: nsUrlClassifierPrefixSet::AddPrefixes(unsigned int const*, unsigned int) (nsUrlClassifierPrefixSet.cpp:163)
==25366==    by 0x7596B15: nsUrlClassifierPrefixSet::SetPrefixes(unsigned int const*, unsigned int) (nsUrlClassifierPrefixSet.cpp:125)
==25366==    by 0x758D02C: nsUrlClassifierDBServiceWorker::ConstructPrefixSet() (nsUrlClassifierDBService.cpp:3611)

2MB?  Definitely worth adding a memory reporterer for this.  I didn't see anything else relating to the UrlClassifier, just this stack trace.
Comment 1 Gian-Carlo Pascutto [:gcp] 2011-11-09 06:39:24 PST
Created attachment 573182 [details] [diff] [review]
Patch 1. v1 Make PrefixSet an iMemoryReporter

I made nsUrlClassifierPrefixSet and IMemoryReporter and register itself on construction.
Comment 2 Josh Matthews [:jdm] 2011-11-09 06:44:39 PST
Comment on attachment 573182 [details] [diff] [review]
Patch 1. v1 Make PrefixSet an iMemoryReporter

Unregistering in the destructor will cause the object to leak for the lifecycle of the app, I believe. NS_RegisterMemoryReporter stores reporters in an nsCOMArray which holds strong references to objects, so the destructor will never run until the reporter is removed from the array.
Comment 3 Josh Matthews [:jdm] 2011-11-09 06:48:34 PST
Then again, as long as the memory reporters are designed to live forever, I guess it's not a problem? It looks like every NS_UnregisterMemoryReporter is doing the same (incorrect) thing.
Comment 4 Gian-Carlo Pascutto [:gcp] 2011-11-09 07:22:11 PST
>NS_RegisterMemoryReporter stores reporters in an nsCOMArray which holds strong 
>references to objects

Should that be considered a bug in the users or in NS_RegisterMemoryReporter? 

What's the Alternate Best Way? Do the unregistration in whatever "owns" the PrefixSet? That can get tricky for things with refcounted pointers, though this code isn't one of them, so it should be doable.
Comment 5 Justin Lebar (not reading bugmail) 2011-11-09 07:27:40 PST
Most memory reporter objects are singletons which stay around for the lifetime of the process.  It's not wrong to register and unregister reporters, but the API isn't designed well for that use-case.

Do you have PrefixSet objects coming and going?  If so, you could make a separate reporter object which tracks them.  At worst, you could use an array of weak references.

But if you only have one PrefixSet object, I wouldn't worry about un-registering it.
Comment 6 Gian-Carlo Pascutto [:gcp] 2011-11-09 11:47:18 PST
Created attachment 573268 [details] [diff] [review]
Patch 1. v2 Add Memory Reporter for PrefixSet

PrefixSet's are fairly static for the application lifetime, and only really destroyed when for example the user toggles the prefs. So I think registering and unregistering on creation/destruction should be fine. But it shouldn't leak; that's not cool to do when the user turns off a feature.

I reworked this a bit and split the reporters out of the class itself. That gets rid of the multiple inheritance, and allows the class destructor to unregister the reporter, which can then get destructed when the class destructor finishes. There's also a possibility to name the PrefixSet, which isn't relevant for current m-c but will get relevant when some of my other patches land.

The behavior now more or less matches that of storage, which is also what this is closest too, so I hope that's OK.

https://tbpl.mozilla.org/?tree=Try&rev=4ac7722e4e10
Comment 7 Justin Lebar (not reading bugmail) 2011-11-09 11:56:45 PST
Comment on attachment 573268 [details] [diff] [review]
Patch 1. v2 Add Memory Reporter for PrefixSet

Stealing njn's review.

I think splitting the reporter out into a separate class is fine.  I'll write up comments in a moment.
Comment 8 Justin Lebar (not reading bugmail) 2011-11-09 12:06:23 PST
>+  mReporter = new nsPrefixSetReporter(nsnull);

Why this argument to the constructor?  Doesn't look like you ever use it.

>+  PRUint32 memoryUsage;
>+  EstimateSize(&memoryUsage);
>+  mReporter->SetAmount(memoryUsage);

Instead of updating the reporter's size each time the size of the object
changes, how about the reporter calls a method on its prefixset object when the
reporter's size is read?

> NS_IMETHODIMP
> nsUrlClassifierPrefixSet::EstimateSize(PRUint32 * aSize)
> {
>   MutexAutoLock lock(mPrefixSetLock);
>-  *aSize = sizeof(bool);
>+  *aSize = sizeof(bool) + sizeof(PRUint32);
>+  *aSize += sizeof(mozilla::Mutex) + sizeof(mozilla::CondVar);
>   if (mHasPrefixes) {
>     *aSize += sizeof(PRUint16) * mDeltas.Length();
>     *aSize += sizeof(PRUint32) * mIndexPrefixes.Length();
>     *aSize += sizeof(PRUint32) * mIndexStarts.Length();
>   }
>   return NS_OK;
> }

I think you want sizeof(*this) instead of this hackery.  (Note that this currently misses the overhead of holding an nsTArray.)  You should also use nsTArray::SizeOf().

(Just to check, these TArrays are usually large, right?  If they usually have just a few elements, we should consider using nsAutoTArray.)

>+nsPrefixSetReporter::nsPrefixSetReporter(nsCString * aName)
>+{
>+  mProcess.Truncate();
>+  mPath.Assign(NS_LITERAL_CSTRING("explicit/storage/prefixset"));
>+  if (aName && !aName->IsEmpty()) {
>+    mPath.Append("/");
>+    mPath.Append(*aName);
>+  }
>+  mKind = nsIMemoryReporter::KIND_HEAP;
>+  mUnits = nsIMemoryReporter::UNITS_BYTES;
>+  mAmount = 0;
>+  mDesc.Assign(NS_LITERAL_CSTRING("Memory used by a PrefixSet for "
>+                                  "UrlClassifier, in bytes."));

Instead of storing kind, units, and description as member variables, we usually
just hardcode this into GetKind and GetUnits.  Same for the path, if it's
always constant.

>diff --git a/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
>--- a/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
>+++ b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
>@@ -40,26 +40,46 @@

>+class nsPrefixSetReporter : public nsIMemoryReporter
>+{
>+public:
>+  nsPrefixSetReporter(nsCString * aName);
>+  virtual ~nsPrefixSetReporter() {};
>+  void SetAmount(PRUint32 aAmount);
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIMEMORYREPORTER
>+
>+protected:
>+  nsCString mProcess;
>+  nsCString mPath;
>+  PRInt32   mKind;
>+  PRInt32   mUnits;
>+  PRUint32  mAmount;
>+  nsCString mDesc;
>+};

I think you can forward-declare nsPrefixSetReporter in the header and define it
in the cpp file.  No use exposing it here if you don't need to.
Comment 9 Justin Lebar (not reading bugmail) 2011-11-09 12:07:19 PST
Comment on attachment 573268 [details] [diff] [review]
Patch 1. v2 Add Memory Reporter for PrefixSet

er, not sure how that became an r+.  I'd like to have another look at it.
Comment 10 Nicholas Nethercote [:njn] 2011-11-09 15:04:41 PST
As would I. 

The single most important thing:  please use moz_malloc_usable_size whenever possible to get the size of heap-allocated blocks.  It includes slop bytes caused by the heap allocator rounding up allocation requests, and it makes things much easier for DMD.

The current idiom (which bug 698968 will simplify) is to do something like this:

  size_t usable = moz_malloc_usable_size(p);
  size_t size = usable ? usable : computeSize(p);

For an example, see http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#1578

This idiom makes sense when you understand that moz_malloc_usable_size always returns 0 on some platforms, so you have to fallback to computing the size yourself.
Comment 11 Justin Lebar (not reading bugmail) 2011-11-09 15:15:55 PST
I don't think he needs to use malloc_usable_size anywhere.  The only relevant place is the TArrays, but they should handle themselves properly.

(nsTArray::SizeOf should probably use malloc_usable_size, though.)
Comment 12 Nicholas Nethercote [:njn] 2011-11-09 16:00:36 PST
(In reply to Justin Lebar [:jlebar] from comment #11)
> I don't think he needs to use malloc_usable_size anywhere.

You said "I think you want sizeof(*this) instead of this hackery", shouldn't it be used there, i.e. |moz_malloc_usable_size(this)|?
Comment 13 Justin Lebar (not reading bugmail) 2011-11-09 16:06:54 PST
Hm.  I guess so, assuming that this is allocated with operator new and not as part of a larger structure.  If it's the latter, calling malloc_usable_size may cause us to crash.  :)
Comment 14 Nicholas Nethercote [:njn] 2011-11-09 16:55:43 PST
Comment on attachment 573268 [details] [diff] [review]
Patch 1. v2 Add Memory Reporter for PrefixSet

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

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
@@ +134,5 @@
>      AddPrefixes(prefixes.Elements(), prefixes.Length());
>    }
>  
> +  PRUint32 memoryUsage;
> +  EstimateSize(&memoryUsage);

Why not just return the size instead of using an outparam?

@@ +265,5 @@
>    if (mHasPrefixes) {
>      *aSize += sizeof(PRUint16) * mDeltas.Length();
>      *aSize += sizeof(PRUint32) * mIndexPrefixes.Length();
>      *aSize += sizeof(PRUint32) * mIndexStarts.Length();
>    }

Just to echo Justin:  you need to count the nsTArrays, esp. given that comment 0 shows us that's where most of the memory usage is.

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
@@ +60,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIMEMORYREPORTER
> +
> +protected:

Why protected?  Why not private?
Comment 15 Nicholas Nethercote [:njn] 2011-11-09 17:01:27 PST
(In reply to Josh Matthews [:jdm] from comment #3)
> Then again, as long as the memory reporters are designed to live forever, I
> guess it's not a problem? It looks like every NS_UnregisterMemoryReporter is
> doing the same (incorrect) thing.

(In reply to Justin Lebar [:jlebar] from comment #5)
> Most memory reporter objects are singletons which stay around for the
> lifetime of the process.  It's not wrong to register and unregister
> reporters, but the API isn't designed well for that use-case.

Can either of you explain more?  I don't understand the problem with NS_UnregisterMemoryReporter.
Comment 16 Gian-Carlo Pascutto [:gcp] 2011-11-09 22:23:22 PST
>Why this argument to the constructor?  Doesn't look like you ever use it.

As hinted in comment 6, I have patches pending that introduces more PrefixSets, at which point giving them separate names is useful. I've made this more explicit now by naming the current one "all".

>Why not just return the size instead of using an outparam?

Because IDL/XPCOM doesn't allow that? The function returns an nsresult but can't actually fail.
Comment 17 Gian-Carlo Pascutto [:gcp] 2011-11-10 02:21:31 PST
Created attachment 573465 [details] [diff] [review]
Patch 1. v3 Add Memory Reporter for PrefixSet
Comment 18 Justin Lebar (not reading bugmail) 2011-11-10 07:16:07 PST
r=me with just a few changes.

> As hinted in comment 6, I have patches pending that introduces more PrefixSets, at which point 
> giving them separate names is useful.

Sorry I missed that.

> +private:
> +  nsCString mProcess;
> +  nsCString mPath;
> +  PRUint32  mAmount;
> +  nsUrlClassifierPrefixSet * mParent;
> +};

Is there a similar story for mProcess?  In general, we hardcode "" into GetProcess, meaning that the reporter is for the current process.  If you're not going to use the reporter to hold data which you got from another process, please get rid of this member.

Also, don't need mAmount.

> +nsPrefixSetReporter::nsPrefixSetReporter(nsUrlClassifierPrefixSet * aParent,
> +                                         nsCString & aName)

This should take an nsACString&.  Then...

> +  nsCString name(NS_LITERAL_CSTRING("all"));
> +  mReporter = new nsPrefixSetReporter(this, name);

...instead of the first line, you can do

  NS_NAMED_LITERAL_CSTRING(name, "all");

But I think you actually want to replace both lines with

   mReporter = new nsPrefixSetReporter(this, NS_LITERAL_CSTRING("all"));

> +  size_t usable = moz_malloc_usable_size(this);

Nick, now that I've had a chance to sleep on it, this scares the bejeezus out of me.  This will break spectacularly if anyone allocates a PrefixSet on the stack or within another object (e.g. nsTArray<nsPrefixSet>).  It will also break if another class inherits from PrefixSet.

Our automated tests may not catch this breakage; do we even instantiate the URL classifier in the chrome tests when we call up about:memory?  If we do, can we rely on this not changing in the future?

We're talking about just a few bytes difference between sizeof(*this) and malloc_usable_size(this); I don't think it's worth the danger.
Comment 19 Nicholas Nethercote [:njn] 2011-11-10 14:03:36 PST
> > +  size_t usable = moz_malloc_usable_size(this);
> 
> Nick, now that I've had a chance to sleep on it, this scares the bejeezus
> out of me.  This will break spectacularly if anyone allocates a PrefixSet on
> the stack or within another object (e.g. nsTArray<nsPrefixSet>).  It will
> also break if another class inherits from PrefixSet.
> 
> Our automated tests may not catch this breakage; do we even instantiate the
> URL classifier in the chrome tests when we call up about:memory?  If we do,
> can we rely on this not changing in the future?

Then you'll be horrified to hear that we already have several examples like this in our code :)  I've been using a |bool countMe| parameter to SizeOf()-style functions to indicate if the object itself should be counted.  That parameter also allows SizeOf() to work with inheritance -- when sizing a sub-class, it can do malloc_usable_size(this) and then call |SizeOf(/*countMe*/false)| on the base object.  (I've done exactly this in a not-yet-posted patch for bug 698968.)

(I told you about this once before and you said you didn't like the fact that |countMe| was a bool, you'd prefer an enum.  But I like that it's a bool because it works with both the stack vs. heap case, and the inheritance case.)

The potential for error doesn't worry me because if you do call malloc_usable_size on the wrong thing it crashes immediately and is easy to debug.  I've handled several such cases while working on patches.

> We're talking about just a few bytes difference between sizeof(*this) and
> malloc_usable_size(this); I don't think it's worth the danger.

I view crashiness in the case of error as a virtue.  We don't want to be counting stack-allocated memory in memory reporters for the heap.
Comment 20 Nicholas Nethercote [:njn] 2011-11-10 14:14:52 PST
Comment on attachment 573465 [details] [diff] [review]
Patch 1. v3 Add Memory Reporter for PrefixSet

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

Can you give some example output that you are seeing with this patch applied?  With DMD I regularly see that 2MB allocation in comment 0, hopefully you are capturing that.

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
@@ +70,5 @@
>  
> +class nsPrefixSetReporter : public nsIMemoryReporter
> +{
> +public:
> +  nsPrefixSetReporter(nsUrlClassifierPrefixSet * aParent, nsCString & aName);

Yikes.  |T * p| doesn't match any of Mozilla's coding standards, AFAIK, but it appears to be the going style in this file.  Sigh.  My brain really wants to read that |*| as a multiplication.

@@ +90,5 @@
> +                                         nsCString & aName)
> +: mParent(aParent)
> +{
> +  mProcess.Truncate();
> +  mPath.Assign(NS_LITERAL_CSTRING("explicit/storage/prefixset"));

Is "prefix-set" nicer than "prefixset"?  I'll let you decide.

@@ +160,5 @@
>    if (NS_FAILED(rv)) {
>      LOG(("Failed to initialize PrefixSet"));
>    }
> +
> +  nsCString name(NS_LITERAL_CSTRING("all"));

You can use NS_NAMED_LITERAL_CSTRING(name, "all") here.

@@ +162,5 @@
>    }
> +
> +  nsCString name(NS_LITERAL_CSTRING("all"));
> +  mReporter = new nsPrefixSetReporter(this, name);
> +  NS_RegisterMemoryReporter(mReporter);

How many nsPrefixSetReporters do you typically have?  Hopefully it's just a handful.  If it's a lot, a nsIMemoryMultiReporter would be more appropriate.

@@ +330,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  nsUrlClassifierPrefixSet::EstimateSize(PRUint32 * aSize)

Please call this SizeOf(), it's a standard name used for this type of function.

@@ +334,5 @@
>  nsUrlClassifierPrefixSet::EstimateSize(PRUint32 * aSize)
>  {
>    MutexAutoLock lock(mPrefixSetLock);
> +  size_t usable = moz_malloc_usable_size(this);
> +  *aSize = (PRUint32)(usable ? usable : sizeof(*this));

As discussed in comment 19, you could add a |bool countMe| argument to this function that dictates whether |this| is measured.  

(In the JS engine, I haven't done that in all cases, just for the types where some are heap-allocated and some are stack-allocated, so I'm fine if you leave it as is.  jlebar might be happier if you add it, though :)
Comment 21 Justin Lebar (not reading bugmail) 2011-11-10 14:31:41 PST
> (I told you about this once before and you said you didn't like the fact that |countMe| was a bool, 
> you'd prefer an enum.  But I like that it's a bool because it works with both the stack vs. heap 
> case, and the inheritance case.)

I don't get it; why is a bool better than an enum here?

Two separate functions would also work great.

Quick, does AddObserver(callback, true); keep a weak or a strong ref?  What's the boolean argument at the end of addEventListener() for again?  This bool thing is evil.
Comment 22 Justin Lebar (not reading bugmail) 2011-11-10 14:34:51 PST
> The potential for error doesn't worry me because if you do call malloc_usable_size on the wrong 
> thing it crashes immediately and is easy to debug.

We *hope* it crashes immediately, but that's not a guarantee, right?  In reality, it could return a random value and only crash sometimes.

Anyway, it sounds like we have a convention for this, so you don't need to change anything here.  :)
Comment 23 Nicholas Nethercote [:njn] 2011-11-10 15:05:30 PST
jlebar and I agreed on IRC that the function should be called SizeOfWithThis(), which makes things clear.
Comment 24 Nicholas Nethercote [:njn] 2011-11-10 15:42:12 PST
Actually, I've started using SizeOfIncludingThis() elsewhere, so can you use it here too?  Thanks!
Comment 25 Gian-Carlo Pascutto [:gcp] 2011-11-11 06:59:33 PST
>Can you give some example output that you are seeing with this patch applied?  
>With DMD I regularly see that 2MB allocation in comment 0, hopefully you are 
>capturing that.

1.15M, which is what it's expected to use. I think the 2M is either slop due to 2^x rounding, or because your report is just when the nsTArray's are being Compact-ed.

>Is "prefix-set" nicer than "prefixset"?  I'll let you decide.

It's really a Prefix Trie now, but lets not go down that hole...
Comment 27 Marco Bonardo [::mak] 2011-11-11 11:13:55 PST
backed out per xpcshell tests failure

https://tbpl.mozilla.org/php/getParsedLog.php?id=7355170&tree=Mozilla-Inbound

TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/url-classifier/tests/unit/test_prefixset.js | running test ...
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/url-classifier/tests/unit/test_prefixset.js | test failed (with xpcshell return code: -6), see following log:
>>>>>>>
Comment 28 Nicholas Nethercote [:njn] 2011-11-11 13:17:54 PST
(In reply to Gian-Carlo Pascutto (:gcp) from comment #25)
> 
> 1.15M, which is what it's expected to use. I think the 2M is either slop due
> to 2^x rounding, or because your report is just when the nsTArray's are
> being Compact-ed.

Oh, right;  DMD is still assuming that 1MB+epsilon allocations are rounded up to 2MB, which is no longer true.
Comment 30 Ed Morley [:emorley] 2011-11-14 19:36:17 PST
https://hg.mozilla.org/mozilla-central/rev/45e2baac1fca
Comment 31 Nicholas Nethercote [:njn] 2011-11-14 21:45:49 PST
   4.162  NS_IMETHODIMP
   4.163 -nsUrlClassifierPrefixSet::EstimateSize(PRUint32 * aSize)
   4.164 +nsUrlClassifierPrefixSet::SizeOfIncludingThis(bool aCountMe, PRUint32 * aSize)
   4.165  {
   4.166    MutexAutoLock lock(mPrefixSetLock);
   4.167 -  *aSize = sizeof(bool);
   4.168 -  if (mHasPrefixes) {
   4.169 -    *aSize += sizeof(PRUint16) * mDeltas.Length();
   4.170 -    *aSize += sizeof(PRUint32) * mIndexPrefixes.Length();
   4.171 -    *aSize += sizeof(PRUint32) * mIndexStarts.Length();
   4.172 +  if (aCountMe) {
   4.173 +    size_t usable = moz_malloc_usable_size(this);
   4.174 +    *aSize = (PRUint32)(usable ? usable : sizeof(*this));
   4.175 +  } else {
   4.176 +    *aSize = 0;
   4.177    }
   4.178 +  *aSize += mDeltas.SizeOf();
   4.179 +  *aSize += mIndexPrefixes.SizeOf();
   4.180 +  *aSize += mIndexStarts.SizeOf();
   4.181    return NS_OK;
   4.182  }

The |aCountMe| parameter isn't necessary here, it's implied by the name |SizeOfIncludingThis| that it includes |moz_malloc_usable_size(this)|.  If you wanted to sometimes not include it, you'd have a version called |SizeOfExcludingThis|, and |SizeOfIncludingThis| would call |SizeOfExcludingThis|.

Does that make sense?  Sorry for the confusing discussion above.
Comment 32 Gian-Carlo Pascutto [:gcp] 2011-11-15 00:38:57 PST
Created attachment 574548 [details] [diff] [review]
Patch 1. Fix SizeOf naming

If I understood the previous comments correctly, the function with the countMe argument should just be called "SizeOf".
Comment 33 Nicholas Nethercote [:njn] 2011-11-15 00:53:56 PST
Comment on attachment 574548 [details] [diff] [review]
Patch 1. Fix SizeOf naming

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

After discussing this with jlebar, I prefer using the function name (and possibly two functions) to indicate whether |this| is counted rather than using a bool parameter -- it makes for much clearer code.  That's what I've started doing in other memory reporters (see the patch in bug 698968) and plan to continue doing, so I'd like you to do the same here, please :)
Comment 34 Gian-Carlo Pascutto [:gcp] 2011-11-15 01:26:20 PST
Created attachment 574555 [details] [diff] [review]
Patch 2. v2 Remove countMe argument

No need for SizeOfExcluding here, so just remove the countMe argument.
Comment 35 Nicholas Nethercote [:njn] 2011-11-15 04:55:40 PST
Comment on attachment 574555 [details] [diff] [review]
Patch 2. v2 Remove countMe argument

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

Great, thanks!
Comment 36 Gian-Carlo Pascutto [:gcp] 2011-11-15 10:53:48 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7197122209fa
Comment 37 Ed Morley [:emorley] 2011-11-16 03:15:53 PST
https://hg.mozilla.org/mozilla-central/rev/7197122209fa
Comment 38 Gian-Carlo Pascutto [:gcp] 2011-11-18 02:41:27 PST
So, question here. I have patches which, among many other things, move nsUrlClassifierPrefixSet to be allocated on the stack. Most of the real memory usage will still be on the heap due to the nsTArrays, though. How do I deal with that? 

Most reasonable seems to be to move everything to SizeOfExcludingThis instead, remove malloc_usable_size *and* sizeof(*this), and ignore the (small) non-heap allocation.
Comment 39 Justin Lebar (not reading bugmail) 2011-11-18 07:29:58 PST
> Most reasonable seems to be to move everything to SizeOfExcludingThis instead, remove 
> malloc_usable_size *and* sizeof(*this), and ignore the (small) non-heap allocation.

Sounds reasonable!
Comment 40 Nicholas Nethercote [:njn] 2011-11-18 11:49:35 PST
Yep, there's no need to report stack usage with a memory reporter.

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