From 4d1ef09788d6eb13f742331b80396d7c365fb67c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Sun, 1 Feb 2015 14:56:33 -0800 Subject: [PATCH] Bug 1050035 (part 1, attempt 2) - Lazily allocate PLDHashTable::mEntryStore. r=froydnj. This makes zero-element hash tables, which are common, smaller, and also avoids unnecessary malloc/free pairs. I did some measurements during some basic browsing of a few sites. I found that 35% of all live tables were empty with a few tabs open. And cumulatively, for the whole session, 45% of tables never had an element added to them. --- xpcom/glue/pldhash.cpp | 80 +++++++++++++++++++++++++++++-------- xpcom/glue/pldhash.h | 32 ++++++++++----- xpcom/tests/TestPLDHash.cpp | 68 +++++++++++++++++++++++++++++-- 3 files changed, 150 insertions(+), 30 deletions(-) diff --git a/xpcom/glue/pldhash.cpp b/xpcom/glue/pldhash.cpp index 9d353241ee2..90b525f9c4d 100644 --- a/xpcom/glue/pldhash.cpp +++ b/xpcom/glue/pldhash.cpp @@ -250,11 +250,8 @@ PLDHashTable::Init(const PLDHashTableOps* aOps, return false; // overflowed } - mEntryStore = (char*)malloc(nbytes); - if (!mEntryStore) { - return false; - } - memset(mEntryStore, 0, nbytes); + mEntryStore = nullptr; + METER(memset(&mStats, 0, sizeof(mStats))); // Set this only once we reach a point where we know we can't fail. @@ -371,6 +368,7 @@ template PLDHashEntryHdr* PL_DHASH_FASTCALL PLDHashTable::SearchTable(const void* aKey, PLDHashNumber aKeyHash) { + MOZ_ASSERT(mEntryStore); METER(mStats.mSearches++); NS_ASSERTION(!(aKeyHash & COLLISION_FLAG), "!(aKeyHash & COLLISION_FLAG)"); @@ -451,6 +449,7 @@ PLDHashEntryHdr* PL_DHASH_FASTCALL PLDHashTable::FindFreeEntry(PLDHashNumber aKeyHash) { METER(mStats.mSearches++); + MOZ_ASSERT(mEntryStore); NS_ASSERTION(!(aKeyHash & COLLISION_FLAG), "!(aKeyHash & COLLISION_FLAG)"); @@ -492,6 +491,8 @@ PLDHashTable::FindFreeEntry(PLDHashNumber aKeyHash) bool PLDHashTable::ChangeTable(int aDeltaLog2) { + MOZ_ASSERT(mEntryStore); + /* Look, but don't touch, until we succeed in getting new entry store. */ int oldLog2 = PL_DHASH_BITS - mHashShift; int newLog2 = oldLog2 + aDeltaLog2; @@ -550,6 +551,8 @@ PLDHashTable::ChangeTable(int aDeltaLog2) MOZ_ALWAYS_INLINE PLDHashNumber PLDHashTable::ComputeKeyHash(const void* aKey) { + MOZ_ASSERT(mEntryStore); + PLDHashNumber keyHash = mOps->hashKey(this, aKey); keyHash *= PL_DHASH_GOLDEN_RATIO; @@ -569,8 +572,9 @@ PLDHashTable::Search(const void* aKey) METER(mStats.mSearches++); - PLDHashNumber keyHash = ComputeKeyHash(aKey); - PLDHashEntryHdr* entry = SearchTable(aKey, keyHash); + PLDHashEntryHdr* entry = + mEntryStore ? SearchTable(aKey, ComputeKeyHash(aKey)) + : nullptr; DECREMENT_RECURSION_LEVEL(this); @@ -584,16 +588,32 @@ PLDHashTable::Add(const void* aKey, const mozilla::fallible_t&) PLDHashNumber keyHash; PLDHashEntryHdr* entry; + uint32_t capacity; MOZ_ASSERT(mRecursionLevel == 0); INCREMENT_RECURSION_LEVEL(this); + // Allocate the entry storage if it hasn't already been allocated. + if (!mEntryStore) { + uint32_t nbytes; + // We already checked this in Init(), so it must still be true. + MOZ_RELEASE_ASSERT(SizeOfEntryStore(CapacityFromHashShift(), mEntrySize, + &nbytes)); + mEntryStore = (char*)malloc(nbytes); + if (!mEntryStore) { + METER(mStats.mAddFailures++); + entry = nullptr; + goto exit; + } + memset(mEntryStore, 0, nbytes); + } + /* * If alpha is >= .75, grow or compress the table. If aKey is already * in the table, we may grow once more than necessary, but only if we * are on the edge of being overloaded. */ - uint32_t capacity = Capacity(); + capacity = Capacity(); if (mEntryCount + mRemovedCount >= MaxLoad(capacity)) { /* Compress if a quarter or more of all entries are removed. */ int deltaLog2; @@ -647,6 +667,27 @@ exit: return entry; } +MOZ_ALWAYS_INLINE PLDHashEntryHdr* +PLDHashTable::Add(const void* aKey) +{ + PLDHashEntryHdr* entry = Add(aKey, fallible); + if (!entry) { + if (!mEntryStore) { + // We OOM'd while allocating the initial entry storage. + uint32_t nbytes; + (void) SizeOfEntryStore(CapacityFromHashShift(), mEntrySize, &nbytes); + NS_ABORT_OOM(nbytes); + } else { + // We failed to resize the existing entry storage, either due to OOM or + // because we exceeded the maximum table capacity or size; report it as + // an OOM. The multiplication by 2 gets us the size we tried to allocate, + // which is double the current size. + NS_ABORT_OOM(2 * EntrySize() * EntryCount()); + } + } + return entry; +} + MOZ_ALWAYS_INLINE void PLDHashTable::Remove(const void* aKey) { @@ -655,8 +696,9 @@ PLDHashTable::Remove(const void* aKey) MOZ_ASSERT(mRecursionLevel == 0); INCREMENT_RECURSION_LEVEL(this); - PLDHashNumber keyHash = ComputeKeyHash(aKey); - PLDHashEntryHdr* entry = SearchTable(aKey, keyHash); + PLDHashEntryHdr* entry = + mEntryStore ? SearchTable(aKey, ComputeKeyHash(aKey)) + : nullptr; if (entry) { /* Clear this entry and mark it as "removed". */ METER(mStats.mRemoveHits++); @@ -693,12 +735,7 @@ PL_DHashTableAdd(PLDHashTable* aTable, const void* aKey, PLDHashEntryHdr* PL_DHASH_FASTCALL PL_DHashTableAdd(PLDHashTable* aTable, const void* aKey) { - PLDHashEntryHdr* entry = PL_DHashTableAdd(aTable, aKey, fallible); - if (!entry) { - // Entry storage reallocation failed. - NS_ABORT_OOM(aTable->EntrySize() * aTable->EntryCount()); - } - return entry; + return aTable->Add(aKey); } void PL_DHASH_FASTCALL @@ -711,6 +748,7 @@ MOZ_ALWAYS_INLINE void PLDHashTable::RawRemove(PLDHashEntryHdr* aEntry) { MOZ_ASSERT(IsInitialized()); + MOZ_ASSERT(mEntryStore); MOZ_ASSERT(mRecursionLevel != IMMUTABLE_RECURSION_LEVEL); @@ -740,6 +778,10 @@ PLDHashTable::Enumerate(PLDHashEnumerator aEtor, void* aArg) { MOZ_ASSERT(IsInitialized()); + if (!mEntryStore) { + return 0; + } + INCREMENT_RECURSION_LEVEL(this); // Please keep this method in sync with the PLDHashTable::Iterator constructor @@ -842,6 +884,10 @@ PLDHashTable::SizeOfExcludingThis( { MOZ_ASSERT(IsInitialized()); + if (!mEntryStore) { + return 0; + } + size_t n = 0; n += aMallocSizeOf(mEntryStore); if (aSizeOfEntryExcludingThis) { @@ -964,6 +1010,7 @@ PLDHashEntryHdr* PLDHashTable::Iterator::NextEntry() // checks pass, then this method will only iterate through the full capacity // once. If they fail, then this loop may end up returning the early entries // more than once. + MOZ_ASSERT_IF(capacity > 0, mTable->mEntryStore); for (uint32_t e = 0; e < capacity; ++e) { PLDHashEntryHdr* entry = (PLDHashEntryHdr*)mEntryAddr; @@ -1021,6 +1068,7 @@ PLDHashTable::DumpMeter(PLDHashEnumerator aDump, FILE* aFp) hash2 = 0; sqsum = 0; + MOZ_ASSERT_IF(capacity > 0, mEntryStore); for (uint32_t i = 0; i < capacity; i++) { entry = (PLDHashEntryHdr*)entryAddr; entryAddr += mEntrySize; diff --git a/xpcom/glue/pldhash.h b/xpcom/glue/pldhash.h index 2bc443eec27..150506652a2 100644 --- a/xpcom/glue/pldhash.h +++ b/xpcom/glue/pldhash.h @@ -147,6 +147,9 @@ typedef size_t (*PLDHashSizeOfEntryExcludingThisFun)( * on most architectures, and may be allocated on the stack or within another * structure or class (see below for the Init and Finish functions to use). * + * No entry storage is allocated until the first element is added. This means + * that empty hash tables are cheap, which is good because they are common. + * * There used to be a long, math-heavy comment here about the merits of * double hashing vs. chaining; it was removed in bug 1058335. In short, double * hashing is more space-efficient unless the element size gets large (in which @@ -175,8 +178,7 @@ private: uint32_t mEntryCount; /* number of entries in table */ uint32_t mRemovedCount; /* removed entry sentinels in table */ uint32_t mGeneration; /* entry storage generation number */ - char* mEntryStore; /* entry storage */ - + char* mEntryStore; /* entry storage; allocated lazily */ #ifdef PL_DHASHMETER struct PLDHashStats { @@ -226,12 +228,12 @@ public: /* * Size in entries (gross, not net of free and removed sentinels) for table. - * We store mHashShift rather than sizeLog2 to optimize the collision-free - * case in SearchTable. + * This can be zero if no elements have been added yet, in which case the + * entry storage will not have yet been allocated. */ uint32_t Capacity() const { - return ((uint32_t)1 << (PL_DHASH_BITS - mHashShift)); + return mEntryStore ? CapacityFromHashShift() : 0; } uint32_t EntrySize() const { return mEntrySize; } @@ -245,6 +247,7 @@ public: PLDHashEntryHdr* Search(const void* aKey); PLDHashEntryHdr* Add(const void* aKey, const mozilla::fallible_t&); + PLDHashEntryHdr* Add(const void* aKey); void Remove(const void* aKey); void RawRemove(PLDHashEntryHdr* aEntry); @@ -297,6 +300,13 @@ public: private: static bool EntryIsFree(PLDHashEntryHdr* aEntry); + // We store mHashShift rather than sizeLog2 to optimize the collision-free + // case in SearchTable. + uint32_t CapacityFromHashShift() const + { + return ((uint32_t)1 << (PL_DHASH_BITS - mHashShift)); + } + PLDHashNumber ComputeKeyHash(const void* aKey); enum SearchReason { ForSearchOrRemove, ForAdd }; @@ -439,12 +449,14 @@ PLDHashTable* PL_NewDHashTable( void PL_DHashTableDestroy(PLDHashTable* aTable); /* - * Initialize aTable with aOps, aEntrySize, and aCapacity. The table's initial - * capacity will be chosen such that |aLength| elements can be inserted without - * rehashing. If |aLength| is a power-of-two, this capacity will be |2*length|. + * Initialize aTable with aOps and aEntrySize. The table's initial capacity + * will be chosen such that |aLength| elements can be inserted without + * rehashing; if |aLength| is a power-of-two, this capacity will be |2*length|. + * However, because entry storage is allocated lazily, this initial capacity + * won't be relevant until the first element is added; prior to that the + * capacity will be zero. * - * This function will crash if it can't allocate enough memory, or if - * |aEntrySize| and/or |aLength| are too large. + * This function will crash if |aEntrySize| and/or |aLength| are too large. */ void PL_DHashTableInit( PLDHashTable* aTable, const PLDHashTableOps* aOps, diff --git a/xpcom/tests/TestPLDHash.cpp b/xpcom/tests/TestPLDHash.cpp index cac6ace5555..ff2a7f48bac 100644 --- a/xpcom/tests/TestPLDHash.cpp +++ b/xpcom/tests/TestPLDHash.cpp @@ -7,10 +7,8 @@ #include #include "pldhash.h" -// pldhash is very widely used and so any basic bugs in it are likely to be -// exposed through normal usage. Therefore, this test currently focusses on -// extreme cases relating to maximum table capacity and potential overflows, -// which are unlikely to be hit during normal execution. +// This test mostly focuses on edge cases. But more coverage of normal +// operations wouldn't be a bad thing. namespace TestPLDHash { @@ -106,6 +104,67 @@ static bool test_pldhash_Init_overflow() return true; } +static bool test_pldhash_lazy_storage() +{ + PLDHashTable t; + PL_DHashTableInit(&t, PL_DHashGetStubOps(), sizeof(PLDHashEntryStub)); + + // PLDHashTable allocates entry storage lazily. Check that all the non-add + // operations work appropriately when the table is empty and the storage + // hasn't yet been allocated. + + if (!t.IsInitialized()) { + return false; + } + + if (t.Capacity() != 0) { + return false; + } + + if (t.EntrySize() != sizeof(PLDHashEntryStub)) { + return false; + } + + if (t.EntryCount() != 0) { + return false; + } + + if (t.Generation() != 0) { + return false; + } + + if (PL_DHashTableSearch(&t, (const void*)1)) { + return false; // search succeeded? + } + + // No result to check here, but call it to make sure it doesn't crash. + PL_DHashTableRemove(&t, (const void*)2); + + // Using a null |enumerator| should be fine because it shouldn't be called + // for an empty table. + PLDHashEnumerator enumerator = nullptr; + if (PL_DHashTableEnumerate(&t, enumerator, nullptr) != 0) { + return false; // enumeration count is non-zero? + } + + for (PLDHashTable::Iterator iter = t.Iterate(); + iter.HasMoreEntries(); + iter.NextEntry()) { + return false; // shouldn't hit this on an empty table + } + + // Using a null |mallocSizeOf| should be fine because it shouldn't be called + // for an empty table. + mozilla::MallocSizeOf mallocSizeOf = nullptr; + if (PL_DHashTableSizeOfExcludingThis(&t, nullptr, mallocSizeOf) != 0) { + return false; // size is non-zero? + } + + PL_DHashTableFinish(&t); + + return true; +} + // See bug 931062, we skip this test on Android due to OOM. #ifndef MOZ_WIDGET_ANDROID // We insert the integers 0.., so this is has function is (a) as simple as @@ -168,6 +227,7 @@ static const struct Test { DECL_TEST(test_pldhash_Init_capacity_ok), DECL_TEST(test_pldhash_Init_capacity_too_large), DECL_TEST(test_pldhash_Init_overflow), + DECL_TEST(test_pldhash_lazy_storage), // See bug 931062, we skip this test on Android due to OOM. #ifndef MOZ_WIDGET_ANDROID DECL_TEST(test_pldhash_grow_to_max_capacity),