1046281 - Use the simpler version of nsTHashtable::SizeOfExcludingThis and SizeOfIncludingThis in more places

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

My plan here was to rename these functions, then see what breaks, then audit each place to see if it can be easily simplified.  I can probably get to this eventually, but if somebody else wants to look at this, feel free.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Use the simpler version of nsTHashtable::SizeOfExcludingThis and SizeOfIncludingThis in more places. WIP

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Remaining places that call the old nsTHashtable size function. DO NOT LAND.

These are mostly places that pass in nullptr. I should take a look at them and see if any should actually use the new one. For instance, maybe somebody didn't bother to go through the hassle with a string key. Also, ideally with some kind of SFINAE magic we could avoid the nullptr argument, where you fall back on that behavior when there's no sizeof method on the entry type, but I don't think I am motivated to do that myself. Maybe I'll file a bug.

Aside from that, there are two places in the font code that actually do something interesting. Their entries report sizeofexcluding this as 0, but then add the size of stuff hanging off the entry into some other counter, which seems weird to me, but I wasn't motivated enough to figure it out. It smells a bit off to me, so maybe somebody should dig into that in a followup bug.

Comment 3

[Andrew McCreight [:mccr8]]

I looked at all of the hash tables in the second patch that don't report memory owned by their entries, and it looks like only a bunch of the Telemetry hash tables own memory. I filed bug 1047523 for that.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Use the simpler version of nsTHashtable memory reporters.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8466453 [details] [diff] [review]
Use the simpler version of nsTHashtable memory reporters.

try run: https://tbpl.mozilla.org/?tree=Try&rev=2c1fd7b1cd8c

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8466453 [details] [diff] [review]
Use the simpler version of nsTHashtable memory reporters.

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

r=me with the reversions below.

::: content/base/src/nsDocument.cpp
@@ -670,5 @@
> -nsIdentifierMapEntry::SizeOfExcludingThis(nsIdentifierMapEntry* aEntry,
> -                                          MallocSizeOf aMallocSizeOf,
> -                                          void*)
> -{
> -  return aEntry->GetKey().SizeOfExcludingThisIfUnshared(aMallocSizeOf);

This one is similar to the telemetry one (see "This one is weird" below) and so I'd like it to be reverted.

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

This one is weird. |mFileStats| is a |nsBaseHashtableET<nsStringHashKey, FileStatsByStage>|, and the new code just measures the keys. Turns out this is ok, measurement-wise, because the value doesn't have any heap blocks hanging off it. And it compiles because |nsBaseHashtableET| inherits from |KeyClass|, and |KeyClass| has a |SizeOfExcludingThis| method.

But I feel like using the simpler form here is glossing over things too much. If |FileStatsByStage| later had something a heap pointer added to it, it would be non-trivial to see how to rewrite things. Similar, if |nsBaseHashtableET| use composition instead of inheritance it wouldn't work. So I'd prefer if this change was reverted.

::: xpcom/glue/nsHashKeys.h
@@ +596,5 @@
>    }
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +  {
> +    return aMallocSizeOf(mKey);

This assumes that |mKey| is heap-allocated, which isn't guaranteed. I don't see a good way to protect against this... but do you actually need this for any of the other changes in this patch? If not, just revert it.

(You might be wondering if the same comment applies to nsStringHashKey et al. They're a bit different... for bug 1046431 I've found that sometimes nsStrings can have static buffers. What will probably be the best way to proceed is to add a flag to nsStringBuffer so it knows if its own buffer is statically allocated, and thus whether |aMallocSizeOf| should be called on it. But that's not an option for a raw char16_t pointer.)

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This one is similar to the telemetry one (see "This one is weird" below) and
> so I'd like it to be reverted.

If there are fields added to nsIdentifierMapEntry, can't somebody just override the SizeOf method on nsIdentifierMapEntry?  That seems easy enough to me to deal with.  If that's still not okay with you, I will revert.

> ::: toolkit/components/telemetry/Telemetry.cpp
> This one is weird. |mFileStats| is a |nsBaseHashtableET<nsStringHashKey,
> FileStatsByStage>|, and the new code just measures the keys. Turns out this
> is ok, measurement-wise, because the value doesn't have any heap blocks
> hanging off it. And it compiles because |nsBaseHashtableET| inherits from
> |KeyClass|, and |KeyClass| has a |SizeOfExcludingThis| method.
> 
> But I feel like using the simpler form here is glossing over things too
> much. If |FileStatsByStage| later had something a heap pointer added to it,
> it would be non-trivial to see how to rewrite things. Similar, if
> |nsBaseHashtableET| use composition instead of inheritance it wouldn't work.
> So I'd prefer if this change was reverted.

Yeah, that's true it is pretty magical.  I'll file a new bug for adding a SizeOf method to nsBaseHashtableET and revert these changes.  Then using some kind of template magic we could define a SizeOf method that calls SizeOf methods on the key and value, if they exist.

> ::: xpcom/glue/nsHashKeys.h
> This assumes that |mKey| is heap-allocated, which isn't guaranteed.

The mKey field of nsUnicharPtrHashKey isn't an arbitrary char16_t pointer, it is the result of a call to NS_strdup (which ultimately calls malloc), so I think it is okay to assume it is heap allocated.  This is the same thing as nsCharPtrHashKey.

Comment 8

[Nicholas Nethercote [inactive]]

> If there are fields added to nsIdentifierMapEntry, can't somebody just
> override the SizeOf method on nsIdentifierMapEntry?  That seems easy enough
> to me to deal with.  If that's still not okay with you, I will revert.

Hmm, true... this one is less magical than the telemetry one. Would a non-static SizeOfExcludingThis() that just calls nsStringHashKey::SizeOfExcludingThis() be a reasonable compromise? I'm not too fussed by this one, so I'll let you decide.

> Yeah, that's true it is pretty magical.  I'll file a new bug for adding a
> SizeOf method to nsBaseHashtableET and revert these changes.  Then using
> some kind of template magic we could define a SizeOf method that calls
> SizeOf methods on the key and value, if they exist.

Sounds good.

> The mKey field of nsUnicharPtrHashKey isn't an arbitrary char16_t pointer,
> it is the result of a call to NS_strdup (which ultimately calls malloc), so
> I think it is okay to assume it is heap allocated.  This is the same thing
> as nsCharPtrHashKey.

Ah, ok. That's fine, then.

Comment 9

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50b153848427

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Would a non-static SizeOfExcludingThis() that just calls
> nsStringHashKey::SizeOfExcludingThis() be a reasonable compromise?

Good suggestion.  Done.

I reverted the Telemetry.cpp changes.

I also added const to some of the new SizeOf methods that I'd forgotten to do before.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50b153848427