The core loop of Iterator::Next() requires multiple branches, one to
check for entry liveness and one to check for wraparound. We can
rewrite this to use masking instead, as well as iterating only over the
hashes, and reconstructing the entry pointer when we know we've reached
a live entry. This change cuts the time taken on the collections
benchmark by the iteration portion in half.
As discussed in the previous commit message, PLDHashTable's storage
wastes space for common entry types. This commit reorganizes the
storage to store all the hashes packed together, followed by all the
entries, which eliminates said waste.
PLDHashTable requires that all items stored therein inherit from
PLDHashEntryHdr:
struct PLDHashEntryHdr {
// PLDHashNumber is a uint32_t.
PLDHashNumber mKeyHash; // Cached hash key for object.
};
class MyType : public PLDHashEntryHdr {
// Data members, etc.
};
PLDHashEntryHdr::mKeyHash is used to cache the computed hash value of
the entry, so we aren't rehashing entries on every lookup/add/etc.
Because of structure layout requirements on 64-bit platforms, the data
members of MyType will typically start at offset 8:
MyType, offset 0: mKeyHash
MyType, offset 4: padding required by alignment
MyType, offset 8: first data member of MyType
MyType, offset N: ...
The padding at offset 4 is dead, unused space.
We'd like to change this state of affairs by having PLDHashTable manage
the cached hash key itself, which would eliminate the dead space in the
object and would enable packing the table storage more tightly. But
PLDHashTable pervasively assumes that its internal storage is an array
of PLDHashEntryHdrs (with an associated object size to account for
subclassing).
As a first step to laying out the hash table storage differently, we
have to make the internals of PLDHashTable not access PLDHashEntryHdr
items directly, but layer an abstraction on top. We call this
abstraction "slots": a slot represents storage for the cached hash key
and the associated entry, and can produce a PLDHashEntryHdr* on demand.
The only place where this is used where the const-ness matters is in
AddressEntry, and that use const_cast's away the const-ness. So let's
just ditch the method that attempts to return const pointers.
This change is satisfying insofar as it removes a load from every
iteration step over the hashtable, but it doesn't seem to affect
our collection benchmarks in any way. The change does not make
iterators any larger, as there is padding at the end of the iterator
structure on both 32- and 64-bit platforms.
We only use its value in one place, and said value is easily computable
from readily available information. This change makes iterators
slightly smaller.
Everything that goes in a PLDHashtable (and its derivatives, like
nsTHashtable) needs to inherit from PLDHashEntryHdr. But through a lack
of enforcement, copy constructors for these derived classes didn't
explicitly invoke the copy constructor for PLDHashEntryHdr (and the
compiler didn't invoke the copy constructor for us). Instead,
PLDHashTable explicitly copied around the bits that the copy constructor
would have.
The current setup has two problems:
1) Derived classes should be using move construction, not copy
construction, since anything that's shuffling hash table keys/entries
around will be using move construction.
2) Derived classes should take responsibility for transferring bits of
superclass state around, and not rely on something else to handle that.
The second point is not a huge problem for PLDHashTable (PLDHashTable
only has to copy PLDHashEntryHdr's bits in a single place), but future
hash table implementations that might move entries around more
aggressively would have to insert compensation code all over the
place. Additionally, if moving entries is implemented via memcpy (which
is quite common), PLDHashTable copying around bits *again* is
inefficient.
Let's fix all these problems in one go, by:
1) Explicitly declaring the set of constructors that PLDHashEntryHdr
implements (and does not implement). In particular, the copy
constructor is deleted, so any derived classes that attempt to make
themselves copyable will be detected at compile time: the compiler
will complain that the superclass type is not copyable.
This change on its own will result in many compiler errors, so...
2) Change any derived classes to implement move constructors instead of
copy constructors. Note that some of these move constructors are,
strictly speaking, unnecessary, since the relevant classes are moved
via memcpy in nsTHashtable and its derivatives.
Currently we have three ways of representing hash values.
- uint32_t: used in HashFunctions.h.
- PLDHashNumber: defined in PLDHashTable.{h,cpp}.
- js::HashNumber: defined in js/public/Utility.h.
Functions that create hash values with functions from HashFunctions.h use a mix
of these three types. It's a bit of a mess.
This patch introduces mozilla::HashNumber, and redefines PLDHashNumber and
js::HashNumber as synonyms. It also changes HashFunctions.h to use
mozilla::HashNumber throughout instead of uint32_t.
This leaves plenty of places that still use uint32_t that should use
mozilla::HashNumber or one of its synonyms, but I didn't want to tackle that
now.
The patch also:
- Does similar things for the constants defining the number of bits in each
hash number type.
- Moves js::HashNumber from Utility.h to HashTable.h, which is a better spot
for it. (This required changing the signature of ScrambleHashCode(); that's
ok, it'll get moved by the next patch anyway.)
MozReview-Commit-ID: EdoWlCm7OUC
--HG--
extra : rebase_source : 5b92c0c3560eb56850cd8832f8ee514d25e3c16f
Everything that goes in a PLDHashtable (and its derivatives, like
nsTHashtable) needs to inherit from PLDHashEntryHdr. But through a lack
of enforcement, copy constructors for these derived classes didn't
explicitly invoke the copy constructor for PLDHashEntryHdr (and the
compiler didn't invoke the copy constructor for us). Instead,
PLDHashTable explicitly copied around the bits that the copy constructor
would have.
The current setup has two problems:
1) Derived classes should be using move construction, not copy
construction, since anything that's shuffling hash table keys/entries
around will be using move construction.
2) Derived classes should take responsibility for transferring bits of
superclass state around, and not rely on something else to handle
that.
The second point is not a huge problem for PLDHashTable (PLDHashTable
only has to copy PLDHashEntryHdr's bits in a single place), but future
hash table implementations that might move entries around more
aggressively would have to insert compensation code all over the place.
Additionally, if moving entries is implemented via memcpy (which is
quite common), PLDHashTable copying around bits *again* is inefficient.
Let's fix all these problems in one go, by:
1) Explicitly declaring the set of constructors that PLDHashEntryHdr
implements (and does not implement). In particular, the copy
constructor is deleted, so any derived classes that attempt to make
themselves copyable will be detected at compile time: the compiler
will complain that the superclass type is not copyable.
This change on its own will result in many compiler errors, so...
2) Change any derived classes to implement move constructors instead
of copy constructors. Note that some of these move constructors are,
strictly speaking, unnecessary, since the relevant classes are moved
via memcpy in nsTHashtable and its derivatives.
PLDHashTable::Search() does not modify any members. So, this method and
methods called by it should be marked as const.
MozReview-Commit-ID: 6g4jrYK1j9E
--HG--
extra : rebase_source : eda6c50c538fec0e8c09cb2ba629735eea6ec711
This was done automatically replacing:
s/mozilla::Move/std::move/
s/ Move(/ std::move(/
s/(Move(/(std::move(/
Removing the 'using mozilla::Move;' lines.
And then with a few manual fixups, see the bug for the split series..
MozReview-Commit-ID: Jxze3adipUh
This patch reduces sizeof(PLDHashTable) as follows.
- 64-bit: from 40 bytes to 32
- 32-bit: from 28 bytes to 20
It does this by doing the following.
- It moves mGeneration from EntryStore to PLDHashTable, to avoid unnecessary
padding on 64-bit. This requires tweaking EntryStore::Set() as explained in a
comment.
- It also shrinks mGeneration from uint32_t to uint16_t, saving 2 bytes of
data.
- It shrinks mEntrySize from uint32_t to uint8_t, to cut 3 bytes of data.
- It shrinks mHashShift from int16_t to uint8_t, trimming another byte of data,
and moves it, saving another 2 bytes of padding.
And it reorders the fields so the word-sized ones are at the start, which makes
it easier to imagine the memory layout.
The patch also adds a test, and fixes some misordered function arguments in
existing tests.
--HG--
extra : rebase_source : 6ed6f7be68477fd4a82f07dd2f51c1f1d9b92dcc
Sylvestre Ledru
Brian Hackett
Nathan Froyd
Nicholas Nethercote
Cosmin Sabou
Masayuki Nakano
Emilio Cobos Álvarez
Ehsan Akhgari
Sebastian Hengst
Benjamin Smedberg