commit ac7345a87f96a7d94d98e4db4db35029f204ecd5
parent 4861f0df57a2936cfd3e43f1c842b4328e000569
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date: Tue, 4 Nov 2025 10:31:01 +0000
Bug 1996591 - Don't free hash table allocation after ModIterator::remove() removed all entries r=glandium,mccr8
As described in the bug, it's less surprising for this to resize the table to
its best capacity and let clients call compact() if they really want the table to be freed.
Differential Revision: https://phabricator.services.mozilla.com/D270192
Diffstat:
6 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/gfx/thebes/gfxFont.cpp b/gfx/thebes/gfxFont.cpp
@@ -3234,6 +3234,7 @@ bool gfxFont::AgeCachedWords() {
it.remove();
}
}
+ mWordCache->compact();
return mWordCache->empty();
}
return true;
diff --git a/js/public/GCHashTable.h b/js/public/GCHashTable.h
@@ -85,6 +85,7 @@ class GCHashMap : public js::HashMap<Key, Value, HashPolicy, AllocPolicy> {
bool traceWeak(JSTracer* trc) {
typename Base::Enum e(*this);
traceWeakEntries(trc, e);
+ Base::compact();
return !this->empty();
}
@@ -274,6 +275,7 @@ class GCHashSet : public js::HashSet<T, HashPolicy, AllocPolicy> {
bool traceWeak(JSTracer* trc) {
typename Base::Enum e(*this);
traceWeakEntries(trc, e);
+ Base::compact();
return !this->empty();
}
diff --git a/js/src/builtin/FinalizationRegistryObject.cpp b/js/src/builtin/FinalizationRegistryObject.cpp
@@ -272,6 +272,7 @@ void FinalizationRegistryObject::traceWeak(JSTracer* trc) {
// Trace and update the contents of the registrations map's keys, which
// are weakly held.
MOZ_ASSERT(registrations());
+
for (auto iter = registrations()->modIter(); !iter.done(); iter.next()) {
auto result = TraceWeakEdge(trc, &iter.getMutable().mutableKey(),
"FinalizationRegistry unregister token");
@@ -285,6 +286,8 @@ void FinalizationRegistryObject::traceWeak(JSTracer* trc) {
iter.remove();
}
}
+
+ registrations()->compact();
}
/* static */
diff --git a/js/src/vm/Compartment.h b/js/src/vm/Compartment.h
@@ -236,6 +236,7 @@ class ObjectWrapperMap {
e.removeFront();
}
}
+ map.compact();
}
};
diff --git a/mfbt/HashTable.h b/mfbt/HashTable.h
@@ -1456,8 +1456,11 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
aOther.mRemoved = false;
}
- // Removes the current element from the table, leaving |get()|
- // invalid until the next call to |next()|.
+ // Removes the current element from the table, leaving |get()| invalid until
+ // the next call to |next()|.
+ //
+ // See the comments on ~ModIterator about table resizing after removing
+ // entries.
void remove() {
mTable.remove(this->mCur);
mRemoved = true;
@@ -1491,7 +1494,12 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
void rekey(const Key& k) { rekey(k, k); }
- // Potentially rehashes the table.
+ // This can rehash the table or resize it if entries were removed.
+ //
+ // This does not go as far as freeing the table if it is now empty, as that
+ // can lead to repeatedly allocating and freeing the table when a small
+ // number of entries are repeatedly added and removed. If callers require
+ // memory to be minimised after removing entries they should call compact().
~ModIterator() {
if (mRekeyed) {
mTable.mGen++;
@@ -1499,7 +1507,7 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
}
if (mRemoved) {
- mTable.compact();
+ mTable.shrinkToBestCapacity();
}
}
};
@@ -1541,6 +1549,8 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
void popFront() { return mIter.next(); }
+ // See the comments on ~ModIterator about table resizing after removing
+ // entries.
void removeFront() { mIter.remove(); }
NonConstT& mutableFront() { return mIter.getMutable(); }
@@ -2043,9 +2053,12 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
#endif
}
- // Resize the table down to the smallest capacity that doesn't overload the
- // table. Since we call shrinkIfUnderloaded() on every remove, you only need
- // to call this after a bulk removal of items done without calling remove().
+ // Minimise the memory used. If there are no entries the table is freed,
+ // otherwise the table is resized to the smallest capacity that doesn't
+ // overload the table and that is at least sMinCapacity entries.
+ //
+ // Since we shrink the table after every remove, you only need to call this if
+ // you want to free the table when it's empty.
void compact() {
if (empty()) {
// Free the entry storage.
@@ -2057,9 +2070,11 @@ class MOZ_STANDALONE_DEBUG HashTable : private AllocPolicy {
return;
}
- uint32_t bestCapacity = this->bestCapacity(mEntryCount);
- MOZ_ASSERT(bestCapacity <= capacity());
+ shrinkToBestCapacity();
+ }
+ void shrinkToBestCapacity() {
+ uint32_t bestCapacity = this->bestCapacity(mEntryCount);
if (bestCapacity < capacity()) {
(void)changeTableSize(bestCapacity, DontReportFailure);
}
diff --git a/mfbt/tests/TestHashTable.cpp b/mfbt/tests/TestHashTable.cpp
@@ -132,9 +132,44 @@ void TestHashPair() {
}
}
+void TestCapacityAfterRemove() {
+ mozilla::HashMap<int, int> map;
+ MOZ_RELEASE_ASSERT(map.count() == 0);
+ MOZ_RELEASE_ASSERT(map.capacity() == 0);
+
+ MOZ_RELEASE_ASSERT(map.putNew(1, 1));
+ MOZ_RELEASE_ASSERT(map.count() == 1);
+ MOZ_RELEASE_ASSERT(map.capacity() != 0);
+
+ map.remove(1);
+ MOZ_RELEASE_ASSERT(map.count() == 0);
+ MOZ_RELEASE_ASSERT(map.capacity() != 0);
+
+ map.compact();
+ MOZ_RELEASE_ASSERT(map.count() == 0);
+ MOZ_RELEASE_ASSERT(map.capacity() == 0);
+
+ MOZ_RELEASE_ASSERT(map.putNew(1, 1));
+ MOZ_RELEASE_ASSERT(map.count() == 1);
+ MOZ_RELEASE_ASSERT(map.capacity() != 0);
+
+ {
+ auto iter = map.modIter();
+ MOZ_RELEASE_ASSERT(!iter.done());
+ iter.remove();
+ }
+ MOZ_RELEASE_ASSERT(map.count() == 0);
+ MOZ_RELEASE_ASSERT(map.capacity() != 0);
+
+ map.compact();
+ MOZ_RELEASE_ASSERT(map.count() == 0);
+ MOZ_RELEASE_ASSERT(map.capacity() == 0);
+}
+
int main() {
TestMoveConstructor();
TestEnumHash();
TestHashPair();
+ TestCapacityAfterRemove();
return 0;
}
