From 27902c8e43bfc2874ee2eebc1fa1e0ec483da9f3 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Wed, 12 Dec 2018 14:57:21 -0500 Subject: [PATCH] Bug 1508873 - part 2 - convert HashTable to work primarily in terms of slots; r=luke HashTableEntry's data layout currently wastes a fair amount of space due to ABI-mandated padding. For instance, HashTableEntry on a 64-bit platform looks like: class HashTableEntry { HashNumber mKeyHash; // Four bytes of wasted space here to pad mValueData to the correct place. unsigned char mValueData[sizeof(T*)]; }; This wasted space means that sets of pointers backed by mozilla::HashTable waste a quarter of their entry storage space. Maps of pointers to pointers waste a sixth of their entry storage space. We'd like to fix this by packing all the cached hashes together, followed by all the hash table entries. As a first step to laying out the hash table storage differently, we have to make HashTable not access entries directly, but go through an abstraction that represents the key and the entry. We call this abstraction "slots". This commit is similar to the change done for PLDHashTable previously. Parts of HashTable still work in terms of Entry; the creation and destruction of tables was not worth changing here. We'll address that in the next commit. --- mfbt/HashTable.h | 266 ++++++++++++++++++++++++++++++----------------- 1 file changed, 170 insertions(+), 96 deletions(-) diff --git a/mfbt/HashTable.h b/mfbt/HashTable.h index 921cc47d7c68..13ed74c0a04e 100644 --- a/mfbt/HashTable.h +++ b/mfbt/HashTable.h @@ -80,6 +80,7 @@ #include "mozilla/Casting.h" #include "mozilla/HashFunctions.h" #include "mozilla/MathAlgorithms.h" +#include "mozilla/Maybe.h" #include "mozilla/MemoryChecking.h" #include "mozilla/MemoryReporting.h" #include "mozilla/Move.h" @@ -1078,6 +1079,71 @@ class HashTableEntry { } }; +// A slot represents a cached hash value and its associated entry stored +// in the hash table. While these two things currently belong to the same +// object, HashTableEntry, above, they do not necessarily need to be +// contiguous in memory and this abstraction helps enforce the separation +// between the two. +template +class EntrySlot { + using NonConstT = typename RemoveConst::Type; + + using Entry = HashTableEntry; + + Entry* mEntry; + + public: + EntrySlot(Entry* aEntry) : mEntry(aEntry) {} + + EntrySlot(const EntrySlot&) = default; + EntrySlot(EntrySlot&& aOther) = default; + + EntrySlot& operator=(const EntrySlot&) = default; + EntrySlot& operator=(EntrySlot&&) = default; + + bool operator==(const EntrySlot& aRhs) const { return mEntry == aRhs.mEntry; } + + bool operator<(const EntrySlot& aRhs) const { return mEntry < aRhs.mEntry; } + + EntrySlot& operator++() { + ++mEntry; + return *this; + } + + void destroy() { mEntry->destroy(); } + + void swap(EntrySlot& aOther) { mEntry->swap(aOther.mEntry); } + + T& get() const { return mEntry->get(); } + + NonConstT& getMutable() { return mEntry->getMutable(); } + + bool isFree() const { return mEntry->isFree(); } + + void clearLive() { mEntry->clearLive(); } + + void clear() { mEntry->clear(); } + + bool isRemoved() const { return mEntry->isRemoved(); } + + void removeLive() { mEntry->removeLive(); } + + bool isLive() const { return mEntry->isLive(); } + + void setCollision() { mEntry->setCollision(); } + void unsetCollision() { mEntry->unsetCollision(); } + bool hasCollision() const { return mEntry->hasCollision(); } + bool matchHash(HashNumber hn) { return mEntry->matchHash(hn); } + HashNumber getKeyHash() const { return mEntry->getKeyHash(); } + + template + void setLive(HashNumber aHashNumber, Args&&... aArgs) { + mEntry->setLive(aHashNumber, std::forward(aArgs)...); + } + + Entry* toEntry() const { return mEntry; } +}; + template class HashTable : private AllocPolicy { friend class mozilla::ReentrancyGuard; @@ -1088,6 +1154,16 @@ class HashTable : private AllocPolicy { public: using Entry = HashTableEntry; + using Slot = EntrySlot; + + template + static void forEachSlot(Entry* aEntries, uint32_t aCapacity, F&& f) { + Entry* end = aEntries + aCapacity; + for (Entry* start = aEntries; start < end; ++start) { + Slot slot(start); + f(slot); + } + } // A nullable pointer to a hash table element. A Ptr |p| can be tested // either explicitly |if (p.found()) p->...| or using boolean conversion @@ -1096,15 +1172,15 @@ class HashTable : private AllocPolicy { class Ptr { friend class HashTable; - Entry* mEntry; + Slot mSlot; #ifdef DEBUG const HashTable* mTable; Generation mGeneration; #endif protected: - Ptr(Entry& aEntry, const HashTable& aTable) - : mEntry(&aEntry) + Ptr(Slot aSlot, const HashTable& aTable) + : mSlot(aSlot) #ifdef DEBUG , mTable(&aTable), @@ -1115,7 +1191,7 @@ class HashTable : private AllocPolicy { // This constructor is used only by AddPtr() within lookupForAdd(). explicit Ptr(const HashTable& aTable) - : mEntry(nullptr) + : mSlot(nullptr) #ifdef DEBUG , mTable(&aTable), @@ -1124,11 +1200,11 @@ class HashTable : private AllocPolicy { { } - bool isValid() const { return !!mEntry; } + bool isValid() const { return !!mSlot.toEntry(); } public: Ptr() - : mEntry(nullptr) + : mSlot(nullptr) #ifdef DEBUG , mTable(nullptr), @@ -1144,14 +1220,14 @@ class HashTable : private AllocPolicy { #ifdef DEBUG MOZ_ASSERT(mGeneration == mTable->generation()); #endif - return mEntry->isLive(); + return mSlot.isLive(); } explicit operator bool() const { return found(); } bool operator==(const Ptr& aRhs) const { MOZ_ASSERT(found() && aRhs.found()); - return mEntry == aRhs.mEntry; + return mSlot == aRhs.mSlot; } bool operator!=(const Ptr& aRhs) const { @@ -1166,7 +1242,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(found()); MOZ_ASSERT(mGeneration == mTable->generation()); #endif - return mEntry->get(); + return mSlot.get(); } T* operator->() const { @@ -1174,7 +1250,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(found()); MOZ_ASSERT(mGeneration == mTable->generation()); #endif - return &mEntry->get(); + return &mSlot.get(); } }; @@ -1187,8 +1263,8 @@ class HashTable : private AllocPolicy { uint64_t mMutationCount; #endif - AddPtr(Entry& aEntry, const HashTable& aTable, HashNumber aHashNumber) - : Ptr(aEntry, aTable), + AddPtr(Slot aSlot, const HashTable& aTable, HashNumber aHashNumber) + : Ptr(aSlot, aTable), mKeyHash(aHashNumber) #ifdef DEBUG , @@ -1198,7 +1274,7 @@ class HashTable : private AllocPolicy { } // This constructor is used when lookupForAdd() is performed on a table - // lacking entry storage; it leaves mEntry null but initializes everything + // lacking entry storage; it leaves mSlot null but initializes everything // else. AddPtr(const HashTable& aTable, HashNumber aHashNumber) : Ptr(aTable), @@ -1225,8 +1301,8 @@ class HashTable : private AllocPolicy { friend class HashTable; explicit Iterator(const HashTable& aTable) - : mCur(aTable.mTable), - mEnd(aTable.mTable + aTable.capacity()) + : mCur(aTable.slotForIndex(0)), + mEnd(aTable.slotForIndex(aTable.capacity())) #ifdef DEBUG , mTable(aTable), @@ -1235,13 +1311,13 @@ class HashTable : private AllocPolicy { mValidEntry(true) #endif { - while (mCur < mEnd && !mCur->isLive()) { + while (mCur < mEnd && !mCur.isLive()) { ++mCur; } } - Entry* mCur; - Entry* mEnd; + Slot mCur; + Slot mEnd; #ifdef DEBUG const HashTable& mTable; uint64_t mMutationCount; @@ -1265,7 +1341,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(mGeneration == mTable.generation()); MOZ_ASSERT(mMutationCount == mTable.mMutationCount); #endif - return mCur->get(); + return mCur.get(); } void next() { @@ -1274,7 +1350,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(mGeneration == mTable.generation()); MOZ_ASSERT(mMutationCount == mTable.mMutationCount); #endif - while (++mCur < mEnd && !mCur->isLive()) { + while (++mCur < mEnd && !mCur.isLive()) { continue; } #ifdef DEBUG @@ -1316,7 +1392,7 @@ class HashTable : private AllocPolicy { // Removes the current element from the table, leaving |get()| // invalid until the next call to |next()|. void remove() { - mTable.remove(*this->mCur); + mTable.remove(this->mCur); mRemoved = true; #ifdef DEBUG this->mValidEntry = false; @@ -1331,7 +1407,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(this->mGeneration == this->Iterator::mTable.generation()); MOZ_ASSERT(this->mMutationCount == this->Iterator::mTable.mMutationCount); #endif - return this->mCur->getMutable(); + return this->mCur.getMutable(); } // Removes the current element and re-inserts it into the table with @@ -1339,7 +1415,7 @@ class HashTable : private AllocPolicy { // this operation until the next call to |next()|. void rekey(const Lookup& l, const Key& k) { MOZ_ASSERT(&k != &HashPolicy::getKey(this->mCur->get())); - Ptr p(*this->mCur, mTable); + Ptr p(this->mCur, mTable); mTable.rekeyWithoutRehash(p, l, k); mRekeyed = true; #ifdef DEBUG @@ -1514,19 +1590,17 @@ class HashTable : private AllocPolicy { ? aAllocPolicy.template pod_malloc(aCapacity) : aAllocPolicy.template maybe_pod_malloc(aCapacity); if (table) { - for (uint32_t i = 0; i < aCapacity; i++) { - new (KnownNotNull, &table[i]) Entry(); - } + forEachSlot(table, aCapacity, [&](const Slot& slot) { + new (KnownNotNull, slot.toEntry()) Entry(); + }); } return table; } static void destroyTable(AllocPolicy& aAllocPolicy, Entry* aOldTable, uint32_t aCapacity) { - Entry* end = aOldTable + aCapacity; - for (Entry* e = aOldTable; e < end; ++e) { - e->~Entry(); - } + forEachSlot(aOldTable, aCapacity, + [&](const Slot& slot) { slot.toEntry()->~Entry(); }); aAllocPolicy.free_(aOldTable, aCapacity); } @@ -1575,59 +1649,61 @@ class HashTable : private AllocPolicy { return (aHash1 - aDoubleHash.mHash2) & aDoubleHash.mSizeMask; } - static MOZ_ALWAYS_INLINE bool match(Entry& aEntry, const Lookup& aLookup) { - return HashPolicy::match(HashPolicy::getKey(aEntry.get()), aLookup); + static MOZ_ALWAYS_INLINE bool match(T& aEntry, const Lookup& aLookup) { + return HashPolicy::match(HashPolicy::getKey(aEntry), aLookup); } enum LookupReason { ForNonAdd, ForAdd }; + Slot slotForIndex(HashNumber aIndex) const { return Slot(&mTable[aIndex]); } + // Warning: in order for readonlyThreadsafeLookup() to be safe this // function must not modify the table in any way when Reason==ForNonAdd. template - MOZ_ALWAYS_INLINE Entry& lookup(const Lookup& aLookup, - HashNumber aKeyHash) const { + MOZ_ALWAYS_INLINE Slot lookup(const Lookup& aLookup, + HashNumber aKeyHash) const { MOZ_ASSERT(isLiveHash(aKeyHash)); MOZ_ASSERT(!(aKeyHash & sCollisionBit)); MOZ_ASSERT(mTable); // Compute the primary hash address. HashNumber h1 = hash1(aKeyHash); - Entry* entry = &mTable[h1]; + Slot slot = slotForIndex(h1); // Miss: return space for a new entry. - if (entry->isFree()) { - return *entry; + if (slot.isFree()) { + return slot; } // Hit: return entry. - if (entry->matchHash(aKeyHash) && match(*entry, aLookup)) { - return *entry; + if (slot.matchHash(aKeyHash) && match(slot.get(), aLookup)) { + return slot; } // Collision: double hash. DoubleHash dh = hash2(aKeyHash); // Save the first removed entry pointer so we can recycle later. - Entry* firstRemoved = nullptr; + Maybe firstRemoved; while (true) { if (Reason == ForAdd && !firstRemoved) { - if (MOZ_UNLIKELY(entry->isRemoved())) { - firstRemoved = entry; + if (MOZ_UNLIKELY(slot.isRemoved())) { + firstRemoved.emplace(slot); } else { - entry->setCollision(); + slot.setCollision(); } } h1 = applyDoubleHash(h1, dh); - entry = &mTable[h1]; - if (entry->isFree()) { - return firstRemoved ? *firstRemoved : *entry; + slot = slotForIndex(h1); + if (slot.isFree()) { + return firstRemoved.refOr(slot); } - if (entry->matchHash(aKeyHash) && match(*entry, aLookup)) { - return *entry; + if (slot.matchHash(aKeyHash) && match(slot.get(), aLookup)) { + return slot; } } } @@ -1635,7 +1711,7 @@ class HashTable : private AllocPolicy { // This is a copy of lookup() hardcoded to the assumptions: // 1. the lookup is for an add; // 2. the key, whose |keyHash| has been passed, is not in the table. - Entry& findNonLiveEntry(HashNumber aKeyHash) { + Slot findNonLiveSlot(HashNumber aKeyHash) { MOZ_ASSERT(!(aKeyHash & sCollisionBit)); MOZ_ASSERT(mTable); @@ -1643,24 +1719,24 @@ class HashTable : private AllocPolicy { // Compute the primary hash address. HashNumber h1 = hash1(aKeyHash); - Entry* entry = &mTable[h1]; + Slot slot = slotForIndex(h1); // Miss: return space for a new entry. - if (!entry->isLive()) { - return *entry; + if (!slot.isLive()) { + return slot; } // Collision: double hash. DoubleHash dh = hash2(aKeyHash); while (true) { - entry->setCollision(); + slot.setCollision(); h1 = applyDoubleHash(h1, dh); - entry = &mTable[h1]; - if (!entry->isLive()) { - return *entry; + slot = slotForIndex(h1); + if (!slot.isLive()) { + return slot; } } } @@ -1696,16 +1772,15 @@ class HashTable : private AllocPolicy { mTable = newTable; // Copy only live entries, leaving removed ones behind. - Entry* end = oldTable + oldCapacity; - for (Entry* src = oldTable; src < end; ++src) { - if (src->isLive()) { - HashNumber hn = src->getKeyHash(); - findNonLiveEntry(hn).setLive( - hn, std::move(const_cast(src->get()))); + forEachSlot(oldTable, oldCapacity, [&](Slot& slot) { + if (slot.isLive()) { + HashNumber hn = slot.getKeyHash(); + findNonLiveSlot(hn).setLive( + hn, std::move(const_cast(slot.get()))); } - src->~Entry(); - } + slot.clear(); + }); // All entries have been destroyed, no need to destroyTable. this->free_(oldTable, oldCapacity); @@ -1741,14 +1816,14 @@ class HashTable : private AllocPolicy { } } - void remove(Entry& aEntry) { + void remove(Slot& aSlot) { MOZ_ASSERT(mTable); - if (aEntry.hasCollision()) { - aEntry.removeLive(); + if (aSlot.hasCollision()) { + aSlot.removeLive(); mRemovedCount++; } else { - aEntry.clearLive(); + aSlot.clearLive(); } mEntryCount--; #ifdef DEBUG @@ -1756,6 +1831,11 @@ class HashTable : private AllocPolicy { #endif } + void remove(Entry& aEntry) { + Slot slot(&aEntry); + remove(slot); + } + void shrinkIfUnderloaded() { static_assert(sMaxCapacity <= UINT32_MAX / sMinAlphaNumerator, "multiplication below could overflow"); @@ -1776,30 +1856,28 @@ class HashTable : private AllocPolicy { void rehashTableInPlace() { mRemovedCount = 0; mGen++; - for (uint32_t i = 0; i < capacity(); ++i) { - mTable[i].unsetCollision(); - } + forEachSlot(mTable, capacity(), [&](Slot& slot) { slot.unsetCollision(); }); for (uint32_t i = 0; i < capacity();) { - Entry* src = &mTable[i]; + Slot src = slotForIndex(i); - if (!src->isLive() || src->hasCollision()) { + if (!src.isLive() || src.hasCollision()) { ++i; continue; } - HashNumber keyHash = src->getKeyHash(); + HashNumber keyHash = src.getKeyHash(); HashNumber h1 = hash1(keyHash); DoubleHash dh = hash2(keyHash); - Entry* tgt = &mTable[h1]; + Slot tgt = slotForIndex(h1); while (true) { - if (!tgt->hasCollision()) { - src->swap(tgt); - tgt->setCollision(); + if (!tgt.hasCollision()) { + src.swap(tgt); + tgt.setCollision(); break; } h1 = applyDoubleHash(h1, dh); - tgt = &mTable[h1]; + tgt = slotForIndex(h1); } } @@ -1820,15 +1898,14 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(mTable); HashNumber keyHash = prepareHash(aLookup); - Entry* entry = &findNonLiveEntry(keyHash); - MOZ_ASSERT(entry); + Slot slot = findNonLiveSlot(keyHash); - if (entry->isRemoved()) { + if (slot.isRemoved()) { mRemovedCount--; keyHash |= sCollisionBit; } - entry->setLive(keyHash, std::forward(aArgs)...); + slot.setLive(keyHash, std::forward(aArgs)...); mEntryCount++; #ifdef DEBUG mMutationCount++; @@ -1837,10 +1914,7 @@ class HashTable : private AllocPolicy { public: void clear() { - Entry* end = mTable + capacity(); - for (Entry* e = mTable; e < end; ++e) { - e->clear(); - } + forEachSlot(mTable, capacity(), [&](Slot& slot) { slot.clear(); }); mRemovedCount = 0; mEntryCount = 0; #ifdef DEBUG @@ -1971,9 +2045,9 @@ class HashTable : private AllocPolicy { if (status == RehashFailed) { return false; } - aPtr.mEntry = &findNonLiveEntry(aPtr.mKeyHash); + aPtr.mSlot = findNonLiveSlot(aPtr.mKeyHash); - } else if (aPtr.mEntry->isRemoved()) { + } else if (aPtr.mSlot.isRemoved()) { // Changing an entry from removed to live does not affect whether we are // overloaded and can be handled separately. if (!this->checkSimulatedOOM()) { @@ -1983,7 +2057,7 @@ class HashTable : private AllocPolicy { aPtr.mKeyHash |= sCollisionBit; } else { - // Preserve the validity of |aPtr.mEntry|. + // Preserve the validity of |aPtr.mSlot|. RebuildStatus status = rehashIfOverloaded(); if (status == RehashFailed) { return false; @@ -1992,11 +2066,11 @@ class HashTable : private AllocPolicy { return false; } if (status == Rehashed) { - aPtr.mEntry = &findNonLiveEntry(aPtr.mKeyHash); + aPtr.mSlot = findNonLiveSlot(aPtr.mKeyHash); } } - aPtr.mEntry->setLive(aPtr.mKeyHash, std::forward(aArgs)...); + aPtr.mSlot.setLive(aPtr.mKeyHash, std::forward(aArgs)...); mEntryCount++; #ifdef DEBUG mMutationCount++; @@ -2049,14 +2123,14 @@ class HashTable : private AllocPolicy { ReentrancyGuard g(*this); // Check that aLookup has not been destroyed. MOZ_ASSERT(prepareHash(aLookup) == aPtr.mKeyHash); - aPtr.mEntry = &lookup(aLookup, aPtr.mKeyHash); + aPtr.mSlot = lookup(aLookup, aPtr.mKeyHash); if (aPtr.found()) { return true; } } else { // Clear aPtr so it's invalid; add() will allocate storage and redo the // lookup. - aPtr.mEntry = nullptr; + aPtr.mSlot = Slot(nullptr); } return add(aPtr, std::forward(aArgs)...); } @@ -2066,7 +2140,7 @@ class HashTable : private AllocPolicy { ReentrancyGuard g(*this); MOZ_ASSERT(aPtr.found()); MOZ_ASSERT(aPtr.mGeneration == generation()); - remove(*aPtr.mEntry); + remove(aPtr.mSlot); shrinkIfUnderloaded(); } @@ -2077,7 +2151,7 @@ class HashTable : private AllocPolicy { MOZ_ASSERT(aPtr.mGeneration == generation()); typename HashTableEntry::NonConstT t(std::move(*aPtr)); HashPolicy::setKey(t, const_cast(aKey)); - remove(*aPtr.mEntry); + remove(aPtr.mSlot); putNewInfallibleInternal(aLookup, std::move(t)); }