Bug 1179071 - Merge RemovingIterator into Iterator. r=froydnj.

The original motivation for the Iterator/RemovingIterator split was that
PLDHashTable Checker class would treat them differently. But that didn't end up
happening (see bug 1131308). So this patch merges them. This is a small code
size win now but it will become bigger when I add iterators to nsTHashTable and
nsBaseHashtable.

The only complication is that PLDHashTable::Iter() is now non-const, which is
a problem if you use it in a const method. So I added PLDHashTable::ConstIter()
which is used in just two places. It's a bit of a hack -- effectively a
const_cast -- but I don't think it's too bad.
This commit is contained in:
Nicholas Nethercote 2015-07-06 22:02:26 -07:00
parent a7d456d187
commit fb8b6912c9
17 changed files with 66 additions and 124 deletions

View File

@ -1969,7 +1969,7 @@ nsJSNPRuntime::OnPluginDestroy(NPP npp)
}
if (sNPObjWrappers) {
for (auto i = sNPObjWrappers->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = sNPObjWrappers->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<NPObjWrapperHashEntry*>(i.Get());
if (entry->mNpp == npp) {

View File

@ -808,7 +808,7 @@ XPCJSRuntime::FinalizeCallback(JSFreeOp* fop,
// At this point there may be JSObjects using them that have
// been removed from the other maps.
if (!nsXPConnect::XPConnect()->IsShuttingDown()) {
for (auto i = self->mNativeScriptableSharedMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = self->mNativeScriptableSharedMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<XPCNativeScriptableSharedMap::Entry*>(i.Get());
XPCNativeScriptableShared* shared = entry->key;
if (shared->IsMarked()) {
@ -821,14 +821,14 @@ XPCJSRuntime::FinalizeCallback(JSFreeOp* fop,
}
if (!isCompartmentGC) {
for (auto i = self->mClassInfo2NativeSetMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = self->mClassInfo2NativeSetMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<ClassInfo2NativeSetMap::Entry*>(i.Get());
if (!entry->value->IsMarked())
i.Remove();
}
}
for (auto i = self->mNativeSetMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = self->mNativeSetMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<NativeSetMap::Entry*>(i.Get());
XPCNativeSet* set = entry->key_value;
if (set->IsMarked()) {
@ -839,7 +839,7 @@ XPCJSRuntime::FinalizeCallback(JSFreeOp* fop,
}
}
for (auto i = self->mIID2NativeInterfaceMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = self->mIID2NativeInterfaceMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<IID2NativeInterfaceMap::Entry*>(i.Get());
XPCNativeInterface* iface = entry->value;
if (iface->IsMarked()) {
@ -904,7 +904,7 @@ XPCJSRuntime::FinalizeCallback(JSFreeOp* fop,
// referencing the protos in the dying list are themselves dead.
// So, we can safely delete all the protos in the list.
for (auto i = self->mDyingWrappedNativeProtoMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = self->mDyingWrappedNativeProtoMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<XPCWrappedNativeProtoMap::Entry*>(i.Get());
delete static_cast<const XPCWrappedNativeProto*>(entry->key);
i.Remove();
@ -3658,7 +3658,7 @@ XPCJSRuntime::DebugDump(int16_t depth)
// iterate sets...
if (depth && mNativeSetMap->Count()) {
XPC_LOG_INDENT();
for (auto i = mNativeSetMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = mNativeSetMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<NativeSetMap::Entry*>(i.Get());
entry->key_value->DebugDump(depth);
}

View File

@ -150,8 +150,6 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::Iterator Iter() const { return PLDHashTable::Iterator(mTable); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
~Native2WrappedNativeMap();
@ -263,7 +261,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
@ -319,7 +317,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
// ClassInfo2NativeSetMap holds pointers to *some* XPCNativeSets.
// So we don't want to count those XPCNativeSets, because they are better
@ -377,7 +375,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::Iterator Iter() const { return PLDHashTable::Iterator(mTable); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
@ -448,7 +446,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::Iterator Iter() const { return PLDHashTable::Iterator(mTable); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
@ -545,7 +543,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
~XPCNativeScriptableSharedMap();
private:
@ -586,7 +584,7 @@ public:
inline uint32_t Count() { return mTable->EntryCount(); }
PLDHashTable::Iterator Iter() const { return PLDHashTable::Iterator(mTable); }
PLDHashTable::RemovingIterator RemovingIter() { return PLDHashTable::RemovingIterator(mTable); }
PLDHashTable::Iterator Iter() { return PLDHashTable::Iterator(mTable); }
~XPCWrappedNativeProtoMap();
private:

View File

@ -651,12 +651,12 @@ XPCWrappedNativeScope::SystemIsBeingShutDown()
// Walk the protos first. Wrapper shutdown can leave dangling
// proto pointers in the proto map.
for (auto i = cur->mWrappedNativeProtoMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = cur->mWrappedNativeProtoMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<ClassInfo2WrappedNativeProtoMap::Entry*>(i.Get());
entry->value->SystemIsBeingShutDown();
i.Remove();
}
for (auto i = cur->mWrappedNativeMap->RemovingIter(); !i.Done(); i.Next()) {
for (auto i = cur->mWrappedNativeMap->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<Native2WrappedNativeMap::Entry*>(i.Get());
XPCWrappedNative* wrapper = entry->value;
if (wrapper->IsValid()) {

View File

@ -9375,7 +9375,7 @@ nsRuleNode::SweepChildren(nsTArray<nsRuleNode*>& aSweepQueue)
if (ChildrenAreHashed()) {
PLDHashTable* children = ChildrenHash();
uint32_t oldChildCount = children->EntryCount();
for (auto iter = children->RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = children->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<ChildrenHashEntry*>(iter.Get());
nsRuleNode* node = entry->mRuleNode;
if (node->DestroyIfNotMarked()) {

View File

@ -555,7 +555,7 @@ PREF_DeleteBranch(const char *branch_name)
const char *to_delete = branch_dot.get();
MOZ_ASSERT(to_delete);
len = strlen(to_delete);
for (auto iter = gHashTable->RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PrefHashEntry*>(iter.Get());
/* note if we're deleting "ldap" then we want to delete "ldap.xxx"
@ -604,7 +604,7 @@ PREF_ClearAllUserPrefs()
return NS_ERROR_NOT_INITIALIZED;
std::vector<std::string> prefStrings;
for (auto iter = gHashTable->RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
auto pref = static_cast<PrefHashEntry*>(iter.Get());
if (PREF_HAS_USER_VALUE(pref)) {

View File

@ -463,17 +463,11 @@ nsCacheEntryHashTable::RemoveEntry( nsCacheEntry *cacheEntry)
}
PLDHashTable::Iterator
nsCacheEntryHashTable::Iter() const
nsCacheEntryHashTable::Iter()
{
return PLDHashTable::Iterator(&table);
}
PLDHashTable::RemovingIterator
nsCacheEntryHashTable::RemovingIter()
{
return PLDHashTable::RemovingIterator(&table);
}
/**
* hash table operation callback functions
*/

View File

@ -274,8 +274,7 @@ public:
nsresult AddEntry( nsCacheEntry *entry);
void RemoveEntry( nsCacheEntry *entry);
PLDHashTable::Iterator Iter() const;
PLDHashTable::RemovingIterator RemovingIter();
PLDHashTable::Iterator Iter();
private:
// PLDHashTable operation callbacks

View File

@ -2919,7 +2919,7 @@ nsCacheService::DoomActiveEntries(DoomCheckFn check)
{
nsAutoTArray<nsCacheEntry*, 8> array;
for (auto iter = mActiveEntries.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = mActiveEntries.Iter(); !iter.Done(); iter.Next()) {
nsCacheEntry* entry =
static_cast<nsCacheEntryHashTableEntry*>(iter.Get())->cacheEntry;

View File

@ -625,7 +625,7 @@ nsHostResolver::FlushCache()
}
// Refresh the cache entries that are resolving RIGHT now, remove the rest.
for (auto iter = mDB.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = mDB.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<nsHostDBEnt *>(iter.Get());
// Try to remove the record, or mark it for refresh.
if (entry->rec->RemoveOrRefresh()) {

View File

@ -1808,7 +1808,7 @@ void
InMemoryDataSource::SweepForwardArcsEntries(PLDHashTable* aTable,
SweepInfo* aInfo)
{
for (auto iter = aTable->RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<Entry*>(iter.Get());
Assertion* as = entry->mAssertions;

View File

@ -177,7 +177,7 @@ nsresult nsNSSShutDownList::evaporateAllNSSResources()
// and the behaviour of changing the list while iterating is undefined.
while (true) {
MutexAutoLock lock(mListLock);
auto iter = mObjects.RemovingIter();
auto iter = mObjects.Iter();
if (iter.Done()) {
break;
}

View File

@ -168,7 +168,7 @@ public:
uint32_t EnumerateRead(EnumReadFunction aEnumFunc, void* aUserArg) const
{
uint32_t n = 0;
for (auto iter = this->mTable.Iter(); !iter.Done(); iter.Next()) {
for (auto iter = this->mTable.ConstIter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<EntryType*>(iter.Get());
PLDHashOperator op = aEnumFunc(entry->GetKey(), entry->mData, aUserArg);
n++;
@ -204,7 +204,7 @@ public:
uint32_t Enumerate(EnumFunction aEnumFunc, void* aUserArg)
{
uint32_t n = 0;
for (auto iter = this->mTable.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = this->mTable.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<EntryType*>(iter.Get());
PLDHashOperator op = aEnumFunc(entry->GetKey(), entry->mData, aUserArg);
n++;

View File

@ -213,7 +213,7 @@ public:
uint32_t EnumerateEntries(Enumerator aEnumFunc, void* aUserArg)
{
uint32_t n = 0;
for (auto iter = mTable.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = mTable.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<EntryType*>(iter.Get());
PLDHashOperator op = aEnumFunc(entry, aUserArg);
n++;

View File

@ -753,7 +753,7 @@ PLDHashTable::SizeOfExcludingThis(
size_t n = aMallocSizeOf(mEntryStore);
if (aSizeOfEntryExcludingThis) {
for (auto iter = Iter(); !iter.Done(); iter.Next()) {
for (auto iter = ConstIter(); !iter.Done(); iter.Next()) {
n += aSizeOfEntryExcludingThis(iter.Get(), aMallocSizeOf, aArg);
}
}
@ -794,17 +794,20 @@ PLDHashTable::Iterator::Iterator(Iterator&& aOther)
: mTable(aOther.mTable)
, mCurrent(aOther.mCurrent)
, mLimit(aOther.mLimit)
, mHaveRemoved(aOther.mHaveRemoved)
{
// No need to change |mChecker| here.
aOther.mTable = nullptr;
aOther.mCurrent = nullptr;
aOther.mLimit = nullptr;
aOther.mHaveRemoved = false;
}
PLDHashTable::Iterator::Iterator(const PLDHashTable* aTable)
PLDHashTable::Iterator::Iterator(PLDHashTable* aTable)
: mTable(aTable)
, mCurrent(mTable->mEntryStore)
, mLimit(mTable->mEntryStore + mTable->Capacity() * mTable->mEntrySize)
, mHaveRemoved(false)
{
#ifdef DEBUG
mTable->mChecker.StartReadOp();
@ -818,11 +821,14 @@ PLDHashTable::Iterator::Iterator(const PLDHashTable* aTable)
PLDHashTable::Iterator::~Iterator()
{
#ifdef DEBUG
if (mTable) {
if (mHaveRemoved) {
mTable->ShrinkIfAppropriate();
}
#ifdef DEBUG
mTable->mChecker.EndReadOp();
}
#endif
}
}
bool
@ -857,41 +863,11 @@ PLDHashTable::Iterator::Next()
} while (IsOnNonLiveEntry());
}
PLDHashTable::RemovingIterator::RemovingIterator(RemovingIterator&& aOther)
: Iterator(mozilla::Move(aOther))
, mHaveRemoved(aOther.mHaveRemoved)
{
// Do nothing with mChecker here. We treat RemovingIterator like Iterator --
// i.e. as a read operation -- until the very end. Then, if any elements have
// been removed, we temporarily treat it as a write operation.
}
PLDHashTable::RemovingIterator::RemovingIterator(PLDHashTable* aTable)
: Iterator(aTable)
, mHaveRemoved(false)
{
}
PLDHashTable::RemovingIterator::~RemovingIterator()
{
if (mHaveRemoved) {
#ifdef DEBUG
AutoIteratorRemovalOp op(mTable->mChecker);
#endif
// Why is this cast needed? In Iterator, |mTable| is const. In
// RemovingIterator it should be non-const, but it inherits from Iterator
// so that's not possible. But it's ok because RemovingIterator's
// constructor takes a pointer to a non-const table in the first place.
const_cast<PLDHashTable*>(mTable)->ShrinkIfAppropriate();
}
}
void
PLDHashTable::RemovingIterator::Remove()
PLDHashTable::Iterator::Remove()
{
// This cast is needed for the same reason as the one in the destructor.
const_cast<PLDHashTable*>(mTable)->RawRemove(Get());
mTable->RawRemove(Get());
mHaveRemoved = true;
}

View File

@ -348,15 +348,19 @@ public:
void ClearEntryStub(PLDHashEntryHdr* aEntry);
// This is an iterator for PLDHashtable. It is not safe to modify the
// table while it is being iterated over; on debug builds, attempting to do
// so will result in an assertion failure.
// This is an iterator for PLDHashtable. Assertions will detect some, but not
// all, mid-iteration table modifications that might invalidate (e.g.
// reallocate) the entry storage.
//
// Any element can be removed during iteration using Remove(). If any
// elements are removed, the table may be resized once iteration ends.
//
// Example usage:
//
// for (auto iter = table.Iter(); !iter.Done(); iter.Next()) {
// auto entry = static_cast<FooEntry*>(iter.Get());
// // ... do stuff with |entry| ...
// // ... possibly call iter.Remove() once ...
// }
//
// or:
@ -364,6 +368,7 @@ public:
// for (PLDHashTable::Iterator iter(&table); !iter.Done(); iter.Next()) {
// auto entry = static_cast<FooEntry*>(iter.Get());
// // ... do stuff with |entry| ...
// // ... possibly call iter.Remove() once ...
// }
//
// The latter form is more verbose but is easier to work with when
@ -372,20 +377,27 @@ public:
class Iterator
{
public:
explicit Iterator(const PLDHashTable* aTable);
explicit Iterator(PLDHashTable* aTable);
Iterator(Iterator&& aOther);
~Iterator();
bool Done() const; // Have we finished?
PLDHashEntryHdr* Get() const; // Get the current entry.
void Next(); // Advance to the next entry.
// Remove the current entry. Must only be called once per entry, and Get()
// must not be called on that entry afterwards.
void Remove();
protected:
const PLDHashTable* mTable; // Main table pointer.
PLDHashTable* mTable; // Main table pointer.
private:
char* mCurrent; // Pointer to the current entry.
char* mLimit; // One past the last entry.
bool mHaveRemoved; // Have any elements been removed?
bool IsOnNonLiveEntry() const;
Iterator() = delete;
@ -394,35 +406,13 @@ public:
Iterator& operator=(const Iterator&&) = delete;
};
Iterator Iter() const { return Iterator(this); }
Iterator Iter() { return Iterator(this); }
// This is an iterator that allows elements to be removed during iteration.
// If any elements are removed, the table may be resized once iteration ends.
// Its usage is similar to that of Iterator, with the addition that Remove()
// can be called once per element.
class RemovingIterator : public Iterator
// Use this if you need to initialize an Iterator in a const method. If you
// use this case, you should not call Remove() on the iterator.
Iterator ConstIter() const
{
public:
explicit RemovingIterator(PLDHashTable* aTable);
RemovingIterator(RemovingIterator&& aOther);
~RemovingIterator();
// Remove the current entry. Must only be called once per entry, and Get()
// must not be called on that entry afterwards.
void Remove();
private:
bool mHaveRemoved; // Have any elements been removed?
RemovingIterator() = delete;
RemovingIterator(const RemovingIterator&) = delete;
RemovingIterator& operator=(const RemovingIterator&) = delete;
RemovingIterator& operator=(const RemovingIterator&&) = delete;
};
RemovingIterator RemovingIter() const
{
return RemovingIterator(const_cast<PLDHashTable*>(this));
return Iterator(const_cast<PLDHashTable*>(this));
}
private:

View File

@ -51,10 +51,6 @@ TEST(PLDHashTableTest, LazyStorage)
ASSERT_TRUE(false); // shouldn't hit this on an empty table
}
for (auto iter = t.RemovingIter(); !iter.Done(); iter.Next()) {
ASSERT_TRUE(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;
@ -185,19 +181,8 @@ TEST(PLDHashTableIterator, Iterator)
n++;
}
ASSERT_TRUE(saw77 && saw88 && saw99 && n == 3);
}
TEST(PLDHashTableTest, RemovingIterator)
{
PLDHashTable t(&trivialOps, sizeof(PLDHashEntryStub));
// Explicitly test the move constructor. We do this because, due to copy
// elision, compilers might optimize away move constructor calls for normal
// iterator use.
{
PLDHashTable::RemovingIterator iter1(&t);
PLDHashTable::RemovingIterator iter2(mozilla::Move(iter1));
}
t.Clear();
// First, we insert 64 items, which results in a capacity of 128, and a load
// factor of 50%.
@ -209,7 +194,7 @@ TEST(PLDHashTableTest, RemovingIterator)
// The first removing iterator does no removing; capacity and entry count are
// unchanged.
for (PLDHashTable::RemovingIterator iter(&t); !iter.Done(); iter.Next()) {
for (PLDHashTable::Iterator iter(&t); !iter.Done(); iter.Next()) {
(void) iter.Get();
}
ASSERT_EQ(t.EntryCount(), 64u);
@ -217,7 +202,7 @@ TEST(PLDHashTableTest, RemovingIterator)
// The second removing iterator removes 16 items. This reduces the load
// factor to 37.5% (48 / 128), which isn't low enough to shrink the table.
for (auto iter = t.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = t.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PLDHashEntryStub*>(iter.Get());
if ((intptr_t)(entry->key) % 4 == 0) {
iter.Remove();
@ -228,7 +213,7 @@ TEST(PLDHashTableTest, RemovingIterator)
// The third removing iterator removes another 16 items. This reduces
// the load factor to 25% (32 / 128), so the table is shrunk.
for (auto iter = t.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = t.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PLDHashEntryStub*>(iter.Get());
if ((intptr_t)(entry->key) % 2 == 0) {
iter.Remove();
@ -239,7 +224,7 @@ TEST(PLDHashTableTest, RemovingIterator)
// The fourth removing iterator removes all remaining items. This reduces
// the capacity to the minimum.
for (auto iter = t.RemovingIter(); !iter.Done(); iter.Next()) {
for (auto iter = t.Iter(); !iter.Done(); iter.Next()) {
iter.Remove();
}
ASSERT_EQ(t.EntryCount(), 0u);