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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → nfroyd
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...
Attachment #8766485 - Flags: review?(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.
Attachment #8766485 - Flags: review?(erahm) → feedback+
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.
Attachment #8766548 - Flags: review?(erahm)
Attachment #8766485 - Attachment is obsolete: true
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
Attachment #8766548 - Flags: review?(erahm) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a080c24f5f
specialize nsTHashtable<nsPtrHashKey<T>> to share a common implementation; r=erahm
https://hg.mozilla.org/mozilla-central/rev/58a080c24f5f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.