1189156 - Don't use enumeration style in hash table SizeOf*() functions

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

PLDHashTable, nsTHashtable and nsBaseHashtable all have enumeration-style SizeOf*() functions, i.e. they use a function pointer and a void* environment to measure anything hanging off live entries.

Now that we have iterators for all these hash table it's easier to just use the iterators directly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't use enumeration style for PLDHashTable::SizeOf{In,Ex}cludingThis()

After this change, we have PLDHashTable::ShallowSizeOf{In,Ex}cludingThis(),
which don't do anything to measure children. (They can be combined with
iteration to measure children.)

This patch also removes the PL_DHashTableSizeOf{In,Ex}cludingThis() functions.
They're not necessary because the methods can be used instead.

Finally, the patch deliberately converts some SizeOfExcludingThis() calls to
SizeOfIncludingThis(). These are all done on heap pointers so this change is
valid.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't use enumeration style for nsTHashtable::SizeOf{In,Ex}cludingThis()

After this change, we have ShallowSizeOf{In,Ex}cludingThis(), which don't do
anything to measure children. (They can be combined with iteration to measure
children.)

And we still have the existing single-arg SizeOf{In,Ex}cluding() functions,
which work if the entry type itself defines SizeOfExcludingThis().

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Factor out FontTable better

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Don't use enumeration style for

jkew, can you please review the gfx/ changes.

froydnj, can you please review everything else.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Don't use enumeration style for nsBaseHashtable::SizeOf{In,Ex}cludingThis()

(fixed log message)

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Don't use enumeration style for nsBaseHashtable::SizeOf{In,Ex}cludingThis()

(fix log message *again*; sorry)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/525bb2034be2

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8641478 [details] [diff] [review]
(part 3) - Factor out FontTable better

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

Nice cleanup (thanks!), but while we're here, how about renaming it to FontFamilyTable, to more accurately reflect what it is, and to distinguish it from the similar tables that map names to individual font entries? I'm about to suggest adding a typedef (FontEntryTable) for those, too.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8641486 [details] [diff] [review]
(part 4) - Don't use enumeration style for nsBaseHashtable::SizeOf{In,Ex}cludingThis()

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

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +1079,5 @@
> +             !iter.Done();
> +             iter.Next()) {
> +            aSizes->mFontListSize +=
> +                iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +        }

My first reaction was to wonder why we aren't using SizeOfFontTableExcludingThis() here?

@@ +1087,5 @@
> +             !iter.Done();
> +             iter.Next()) {
> +            aSizes->mFontListSize +=
> +                iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +        }

And here?

And then it all became clear... these are tables of |gfxFontEntry| rather than |gfxFontFamily|.

Maybe worth typedef'ing that as FontEntryTable (similar to FontTable for families, which might be better renamed FontFamilyTable for increased clarity), and providing a similar utility.

Or maybe we can even write SizeOfFontTableExcludingThis as a template that handles either kind of font table?

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/525bb2034be2

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8641477 [details] [diff] [review]
(part 2) - Don't use enumeration style for nsTHashtable::SizeOf{In,Ex}cludingThis()

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

Eric said he wanted to review code.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8641477 [details] [diff] [review]
(part 2) - Don't use enumeration style for nsTHashtable::SizeOf{In,Ex}cludingThis()

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

Nice cleanup! r=me, one small question, one small suggestion.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +399,5 @@
>    }
>  
>    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +    size_t size = 0;
> +    size += mFileStats.ShallowSizeOfExcludingThis(aMallocSizeOf);

If I'm understanding things correctly, I think we can just use the default |mFileStats.SizeOfExcludingThis| here and get rid of the loop below.

There's a |nsStringHashKey::SizeOfExcludingThis| function that does the right thing (calling IfUnshared on key), so using the default implementation which calls |(*Get()).SizeOfExcludingThis| should work.

@@ +403,5 @@
> +    size += mFileStats.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +    for (auto iter = mFileStats.ConstIter(); !iter.Done(); iter.Next()) {
> +      size += iter.Get()->GetKey().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    }
> +    size += mSafeDirs.ShallowSizeOfExcludingThis(aMallocSizeOf);

|mSafeDirs| is an |nsTArray|, should that be |SizeOfExcludingThis|? Or maybe this bled over from a another patch that hasn't landed yet?

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8641486 [details] [diff] [review]
(part 4) - Don't use enumeration style for nsBaseHashtable::SizeOf{In,Ex}cludingThis()

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

Looks good to me, a few minor comments and a possible bug (not introduced by you, we could just file a follow up).

In general it feels like we're duplicating a lot of boilerplate, most noticeable:
> n += mTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
> for (auto iter = mTable.ConstIter(); !iter.Done(); iter.Next()) {
>   // NB: We don't own the key
>   n += iter.Data()->SizeOfIncludingThis(aMallocSizeOf);
> }

I wonder if it would make sense to add a helper to accomplish this, roughly replacing that block w/ a one liner:
n += mTable.SizeOfDataEntriesIncludingThis(aMallocSizeOf);

So we'd get rid of the need for a helper function (which I think is the point of all this), but we'd also get rid of the for loop boilerplate. Either way I don't think that needs to block these patches, but might be a nice improvement further along.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +1040,5 @@
>  
> +// this is also used by subclasses that hold additional font tables
> +/*static*/ size_t
> +gfxPlatformFontList::SizeOfFontTableExcludingThis
> +    (const FontTable& aTable,

style nit: breaking on the '(' seems a little odd

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +461,5 @@
> +    n += mInProgressImports.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +    for (auto iter = mInProgressImports.Iter(); !iter.Done(); iter.Next()) {
> +        n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +        n += iter.Data()->SizeOfIncludingThis(aMallocSizeOf);
> +    }

nit: It would be really nice if we could remove the code duplication (X3) here, but I understand if that's more effort than it's worth.

::: layout/style/Loader.cpp
@@ +2550,4 @@
>  size_t
>  Loader::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
>  {
> +  size_t n = aMallocSizeOf(this);

Why |s| -> |n|? It might not be worth the blame, but I guess consistency with other SizeOf functions is a reasonable argument.

@@ +2562,5 @@
> +      // then the document that node is in will report it.
> +      const nsRefPtr<CSSStyleSheet>& aSheet = iter.Data();
> +      n += (aSheet->GetOwnerNode() || aSheet->GetParentSheet())
> +         ? 0
> +         : aSheet->SizeOfIncludingThis(aMallocSizeOf);

style nit: Just using an if here might be cleaner than += ? 0, ie:
if (!aSheet->GetOwnerNode() && !aSheet->GetParentSheet())
  n += ...

::: netwerk/cache2/CacheStorageService.cpp
@@ +2007,5 @@
> +  mozilla::MallocSizeOf mallocSizeOf = CacheStorageService::MallocSizeOf;
> +
> +  size += aTable->ShallowSizeOfIncludingThis(mallocSizeOf);
> +  for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
> +    CacheStorageService::Self()->Lock().AssertCurrentThreadOwns();

I don't think we need this assertion at this point (it's asserted above).

@@ +2016,5 @@
> +    // the memory only table. Memory-only entries are stored in both ALL_ENTRIES
> +    // and MEMORY_ONLY hashtables.
> +    nsRefPtr<mozilla::net::CacheEntry> const& entry = iter.Data();
> +    if (aTable->Type() == CacheEntryTable::MEMORY_ONLY ||
> +        entry->IsUsingDisk()) {

Not your bug, but this seems wrong: The |if| implies what we're including memory only entries, but the comment above states the intention to bypass memory only entries.

::: startupcache/StartupCache.h
@@ +84,5 @@
>    ~CacheEntry()
>    {
>    }
>  
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) {

Presumably this is fixing an error where we should have been including this?

Comment 14

[Nicholas Nethercote [inactive]]

> If I'm understanding things correctly, I think we can just use the default
> |mFileStats.SizeOfExcludingThis| here and get rid of the loop below.
> 
> There's a |nsStringHashKey::SizeOfExcludingThis| function that does the
> right thing (calling IfUnshared on key), so using the default implementation
> which calls |(*Get()).SizeOfExcludingThis| should work.

I think it would work and give the same behaviour. But I think it's slightly misleading and I prefer to keep the current form.

mFileStats has an entry type of FileIOEntryType, which is nsBaseHashtableET<nsStringHashKey, FileStatsByStage>. nsBaseHashtableET is a subclass of its first type parameter, and FileStatsByStage does not include any pointers to other heap blocks. But if FileStatsByStage *did* have pointers to other heap blocks, doing it the way you suggest would miss measuring those heap blocks, and it would be easy to miss.

In contrast, with the existing code it's a little more obvious that we'd need to modify the loop like this:

> for (auto iter = mFileStats.ConstIter(); !iter.Done(); iter.Next()) {
>   size += iter.Get()->GetKey().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
>   size += iter.Get()->mData.SizeOfExcludingThis(aMallocSizeOf);   // added
> }

I hope that makes sense. It's all a bit gross that nsBaseHastableET is used here. It's because AutoHashTable is made to work only with nsTHashtable, not nsBaseHashtable.


> |mSafeDirs| is an |nsTArray|, should that be |SizeOfExcludingThis|? Or maybe
> this bled over from a another patch that hasn't landed yet?

It's from bug 1188745 which landed a couple of days ago.

Comment 15

[Nicholas Nethercote [inactive]]

> In general it feels like we're duplicating a lot of boilerplate, most
> noticeable:
> > n += mTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
> > for (auto iter = mTable.ConstIter(); !iter.Done(); iter.Next()) {
> >   // NB: We don't own the key
> >   n += iter.Data()->SizeOfIncludingThis(aMallocSizeOf);
> > }
> 
> I wonder if it would make sense to add a helper to accomplish this, roughly
> replacing that block w/ a one liner:
> n += mTable.SizeOfDataEntriesIncludingThis(aMallocSizeOf);

Yeah, but sometimes you measure just the keys, and sometimes just the values, and sometimes both. And sometimes the values are pointers so you want to call SizeOfIncludingThis() and sometimes they're not so you want to call SizeOfExcludingThis(). And sometimes the keys are strings so you want to call SizeOfIncludingThisIfUnshared()... etc.

Maybe there's a nice way to handle all those case (an extra argument?) but I'll save it for another day. The patch reduces the code size by 157 lines.


> style nit: breaking on the '(' seems a little odd

It's used elsewhere in this code, but it's weird enough that not following local style seems reasonable. I'll change it.


> nit: It would be really nice if we could remove the code duplication (X3)
> here, but I understand if that's more effort than it's worth.

Ok. I factored it out into a templated function.


> Why |s| -> |n|? It might not be worth the blame, but I guess consistency
> with other SizeOf functions is a reasonable argument.

Yeah. |size| sometimes gets used, but |s| was strange enough that I changed it while in here.


> style nit: Just using an if here might be cleaner than += ? 0, ie:
> if (!aSheet->GetOwnerNode() && !aSheet->GetParentSheet())
>   n += ...

Done.


> I don't think we need this assertion at this point (it's asserted above).

Good catch. Removed.


> Not your bug, but this seems wrong: The |if| implies what we're including
> memory only entries, but the comment above states the intention to bypass
> memory only entries.

True. I'll file a follow-up.


> Presumably this is fixing an error where we should have been including this?

Yes. StartupCache::mTable is a nsClassHashtable<nsStringHashKey, CacheEntry>, which has entries that are AutoPtr<CacheEntry>, so we want to include this for the values.

Comment 16

[Nicholas Nethercote [inactive]]

> > style nit: Just using an if here might be cleaner than += ? 0, ie:
> > if (!aSheet->GetOwnerNode() && !aSheet->GetParentSheet())
> >   n += ...
> 
> Done.

Actually, I'll keep it the way it was, because then the sense of the condition matches the wording in the comment.

Comment 17

[Nicholas Nethercote [inactive]]

Thank you for the thorough reviews.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8296e64c47e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a998270ed671
https://hg.mozilla.org/integration/mozilla-inbound/rev/909e4b1913a9

Comment 19

[Nicholas Nethercote [inactive]]

jfkthame: I just mistakenly overlooked the fact that part 4 was awaiting your re-review and landed it. I renamed FontTable as FontFamilyTable (and likewise for the related functions). Are you happy with the patch as it landed? I can do a follow-up if there is anything left you'd like to change. (Maybe adding a FontEntryTable typedef?) Sorry!

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> jfkthame: I just mistakenly overlooked the fact that part 4 was awaiting
> your re-review and landed it. I renamed FontTable as FontFamilyTable (and
> likewise for the related functions). Are you happy with the patch as it
> landed? I can do a follow-up if there is anything left you'd like to change.
> (Maybe adding a FontEntryTable typedef?) Sorry!

No worries - thanks for making all this so much tidier!

If it works to do the suggestion from comment 9, adding the FontEntryTable typedef and using a template function SizeOfFontTableExcludingThis<T> to handle both FontFamilyTable and FontEntryTable, I'd take that as a bonus followup. But I think there are only two relevant tables, so it's not a big deal; I won't fuss if for some reason it doesn't happen for now.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8296e64c47e8
https://hg.mozilla.org/mozilla-central/rev/a998270ed671
https://hg.mozilla.org/mozilla-central/rev/909e4b1913a9

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Add FontEntryTable typedef and factor out some related code

I ended up choosing to make SizeOfFontEntryExcludingThis() separate from
SizeOfFontFamilyExcludingThis(). Although the code for the two functions is the
same, the comments within the loop are different, and so merging them seemed
like a bad idea.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8643408 [details] [diff] [review]
(part 5) - Add FontEntryTable typedef and factor out some related code

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

LGTM, thanks!

Comment 24

[Nicholas Nethercote [inactive]]

> > Not your bug, but this seems wrong: The |if| implies what we're including
> > memory only entries, but the comment above states the intention to bypass
> > memory only entries.
> 
> True. I'll file a follow-up.

I filed bug 1191650.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a62230eaed4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a62230eaed4

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8641486 [details] [diff] [review]
(part 4) - Don't use enumeration style for nsBaseHashtable::SizeOf{In,Ex}cludingThis()

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

Just setting r+ here (after the fact) to get it off the dashboard. :)