707865 - Convert nsTArray::SizeOf() to nsTArray::SizeOfExcludingThis()

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch converts nsTArray::SizeOf() to nsTArray::SizeOfExcludingThis(), 
following the new "mallocSizeOf" style.  It's on top of the patch from bug 
705987.

The nsTArray changes were mostly straightforward, because a lot of the 
places where nsTArray::SizeOf() was in use (gfx, layout) were places where 
"mallocSizeOf" was already being threaded through.

The more complicated changes involved nsTHashTable and pldhash/jsdhash, 
which the nsTArray changes necessitated.  pldhash/jsdhash already have
new-style SizeOf{In,Ex}cludingThis functions, which take a function that
measures each entry.  I added nsTHashTable::SizeOfExcludingThis() in the 
same style.  The nsTHashTable function is layered over the pldhash/jsdhash
one, and due to boring annoying reasons involving templates and casting, I
had to add a |void *| closure-style argument to the pldhash/jsdhash ones
(see |s_SizeOfStub|, which I based on |s_EnumStub|, to understand why).  The
similar hash enumeration functions already have such a |void *| argument so
this isn't such a big deal.

As a result of these changes, some of the gfx/textrun and places measuring 
code became simpler, and lots of NULL |void *| args were added to xpconnect
and layout measuring code.

Comment 1

[Justin Lebar (not reading bugmail)]

Ugh, functional programming in C++ sucks.

> size_t
> RuleHash::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> {
>   size_t n = 0;
> 
>   n += PL_DHashTableSizeOfExcludingThis(const_cast<PLDHashTable*>(&mIdTable),
>                                         SizeOfRuleHashTableEntry,
>-                                        aMallocSizeOf);
>+                                        aMallocSizeOf, NULL);
>   n += PL_DHashTableSizeOfExcludingThis(const_cast<PLDHashTable*>(&mClassTable),
>                                         SizeOfRuleHashTableEntry,
>-                                        aMallocSizeOf);
>+                                        aMallocSizeOf, NULL);
>   n += PL_DHashTableSizeOfExcludingThis(const_cast<PLDHashTable*>(&mTagTable),
>                                         SizeOfRuleHashTableEntry,
>-                                        aMallocSizeOf);
>+                                        aMallocSizeOf, NULL);
>   n += PL_DHashTableSizeOfExcludingThis(const_cast<PLDHashTable*>(&mNameSpaceTable),
>                                         SizeOfRuleHashTableEntry,
>-                                        aMallocSizeOf);
>-
>-  n += mUniversalRules.SizeOf();
>+                                        aMallocSizeOf, NULL);

If all these NULL arguments are annoying, I'd be OK with
PL_DHashTableSizeOfExcludingThis setting aArg = NULL by default.

>-PLDHashOperator
>-History::SizeOfEnumerator(KeyClass* aEntry, void* aArg)
>+/* static */ size_t
>+History::SizeOfEntry(KeyClass* aEntry, nsMallocSizeOfFun aMallocSizeOf, void *)
> {
>-  PRInt64 *size = reinterpret_cast<PRInt64*>(aArg);
>-
>-  // Don't add in sizeof(*aEntry); that's already accounted for in
>-  // mObservers.SizeOf().

SizeOfEntryExcludingThis?  :-P

>-  *size += aEntry->array.SizeOf();
>-  return PL_DHASH_NEXT;
>+  return aEntry->array.SizeOfExcludingThis(aMallocSizeOf);
> }

>diff --git a/xpcom/glue/nsTHashtable.h b/xpcom/glue/nsTHashtable.h
>--- a/xpcom/glue/nsTHashtable.h
>+++ b/xpcom/glue/nsTHashtable.h
>@@ -247,23 +247,41 @@ public:
>   void Clear()
>   {
>     NS_ASSERTION(mTable.entrySize, "nsTHashtable was not initialized properly.");
> 
>     PL_DHashTableEnumerate(&mTable, PL_DHashStubEnumRemove, nsnull);
>   }
> 
>   /**
>-   * The "Shallow" means that if the entries contain pointers to other objects,
>-   * their size isn't included in the measuring.
>+   * client must provide a <code>SizeOfEntryFun</code> function for
>+   *   SizeOfExcludingThis.
>+   * @param     aEntry the entry being enumerated
>+   * @param     mallocSizeOf the function used to measure heap-allocated blocks
>+   * @param     arg passed unchanged from <code>SizeOf{In,Ex}cludingThis</code>
>+   * @return    summed size of the things pointed to by the entries
>    */
>-  size_t ShallowSizeOfExcludingThis(nsMallocSizeOfFun mallocSizeOf)
>+  typedef size_t (* SizeOfEntryFun)(EntryType* aEntry, nsMallocSizeOfFun mallocSizeOf, void *arg);
>+
>+  /**
>+   * Measure the size of the table's entry storage, and if |sizeOfEntry| is
>+   * non-NULL, measure the size of things pointed to by entries.
>+   * 
>+   * @param     sizeOfEntry the <code>SizeOfEntryFun</code> function to call
>+   * @param     mallocSizeOf the function used to measure heap-allocated blocks
>+   * @param     userArg a pointer to pass to the
>+   *            <code>SizeOfEntryFun</code> function
>+   * @return    the summed size of all the entries
>+   */
>+  size_t SizeOfExcludingThis(SizeOfEntryFun sizeOfEntry,
>+                             nsMallocSizeOfFun mallocSizeOf, void *userArg)

Similarly, userArg could default to NULL here.  That would match up with the SizeOfExcludingThis API elsewhere.

>+
>+  /**
>+   * passed internally during sizeOf counting.  Allocated on the stack.
>+   *
>+   * @param userFunc the SizeOfEntryFun passed to SizeOf{In,Ex}cludingThis by
>+   *                 the client
>+   * @param userArg the userArg passed unaltered

Nit: There should be some punctuation here, or maybe it could be "userArg the |arg| argument passed to SizeOf{In,Ex}cludingThis.".

Comment 2

[Nicholas Nethercote [inactive]]

> SizeOfEntryExcludingThis?  :-P

Not sure if you're joking, but I agree :)  I'll change it in pldhash/jsdhash and in nsTHashtable.

Comment 3

[Nicholas Nethercote [inactive]]

> >+   * @param userFunc the SizeOfEntryFun passed to SizeOf{In,Ex}cludingThis by
> >+   *                 the client
> >+   * @param userArg the userArg passed unaltered
> 
> Nit: There should be some punctuation here, or maybe it could be "userArg
> the |arg| argument passed to SizeOf{In,Ex}cludingThis.".

AIUI the javadoc form is this:

  @param <name> <desc>

Ideally the <desc> would be a complete sentence, but I'm just following local style for this file :/

Comment 4

[Justin Lebar (not reading bugmail)]

I was thinking "the userArg, passed unaltered," if that's what you mean, anyway.  Sorry to be opaque.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b4007e3cafa

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/7b4007e3cafa