Last Comment Bug 707865 - Convert nsTArray::SizeOf() to nsTArray::SizeOfExcludingThis()
: Convert nsTArray::SizeOf() to nsTArray::SizeOfExcludingThis()
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 705602 705987
Blocks: DarkMatter 697335
  Show dependency treegraph
 
Reported: 2011-12-05 19:03 PST by Nicholas Nethercote [:njn]
Modified: 2011-12-19 05:40 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (38.76 KB, patch)
2011-12-05 19:03 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-12-05 19:03:34 PST
Created attachment 579194 [details] [diff] [review]
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) 2011-12-05 23:48:22 PST
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 [:njn] 2011-12-07 15:13:12 PST
> 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 [:njn] 2011-12-07 19:32:01 PST
> >+   * @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) 2011-12-07 19:37:43 PST
I was thinking "the userArg, passed unaltered," if that's what you mean, anyway.  Sorry to be opaque.
Comment 5 Nicholas Nethercote [:njn] 2011-12-18 14:34:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b4007e3cafa
Comment 6 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-19 05:40:48 PST
https://hg.mozilla.org/mozilla-central/rev/7b4007e3cafa

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