1262826 - reduce code required by nsTHashtable<nsPtrHashKey<T>>::Ops()::sOps

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

We have ~300 instantiations for nsTHashtable<nsPtrHashKey<T>>, mostly due to code generation in IPDL.  Every one of these has a separate sOps structure for function pointers used to implement the hash table, which all contain essentially the same functions.  Identical code folding in the linker doesn't seem to be able to fold the functions together (at least on Linux) (?), but even if it could, it can't fold the sOps structures themselves together.

We ought to be able to do better by having nsTHashtable<nsPtrHashKey<void>>, and then having nsTHashtable<nsPtrHashKey<T>> inherit from that and perform appropriate casts for its methods, without instantiating a separate sOps.

This change would save ~12K/6K on 64-bit/32-bit just from sOps elimination, plus whatever relocations we need for the function pointers, plus eliminating the duplicate functions if identical code folding doesn't kick in.  Not a huge win (20-30K total), but something to think about.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation

We have a number of nsTHashtable<nsPtrHashKey<T>> instantiations, mostly
from IPDL-generated code.  There's no reason in principle that all of
these instantiations couldn't share code, since they're all storing POD
entries of the same size.  Let's specialize nsTHashtable for such types,
providing a thin layer over a hashtable that stores void pointers.  This
change saves about 100K of space (!) on x86-64 Linux.

I'm really surprised that codesize went down so much; the data size (i.e. the
nsTHashtable::sOps structures) were about what I expected, but apparently we
had a lot of duplicated, bloaty functions lying about too...

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8766485 [details] [diff] [review]
specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation

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

Looks good, just need to see the new header. Also standard question: do we have tests for this?

::: xpcom/glue/moz.build
@@ +43,5 @@
>      'nsISupportsUtils.h',
>      'nsIWeakReferenceUtils.h',
>      'nsJSThingHashtable.h',
>      'nsMemory.h',
> +    'nsPointerHashKeys.h',

Did you forget to |hg add| this?

::: xpcom/glue/nsTHashtable.h
@@ +468,5 @@
> + * See the main nsTHashtable documentation for descriptions of this class's
> + * methods.
> + */
> +template<typename T>
> +class nsTHashtable<nsPtrHashKey<T>> : protected nsTHashtable<::detail::VoidPtrHashKey>

This seems fine, I just wish there was a less tedious way to forward all the functions.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation

Now with the missing file.

We don't have tests for this; I suppose we could add test-only interfaces for
getting at the hashtable ops and ensuring that:

  nsTHashtable<nsPtrHashKey<A>>::Ops() == nsTHashtable<nsPtrHashKey<B>>::Ops()

but beyond that, I'm not quite sure what to test.

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8766548 [details] [diff] [review]
specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation

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

r=me

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a080c24f5f
specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation; r=erahm

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58a080c24f5f