The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

38.76 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
Attachment #579194 - Flags: review?(justin.lebar+bug)
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.".
Attachment #579194 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 2

5 years ago
> SizeOfEntryExcludingThis?  :-P

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

Comment 3

5 years ago
> >+   * @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 :/
I was thinking "the userArg, passed unaltered," if that's what you mean, anyway.  Sorry to be opaque.
(Assignee)

Updated

5 years ago
Blocks: 697335
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b4007e3cafa
https://hg.mozilla.org/mozilla-central/rev/7b4007e3cafa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.