wine/gecko
0
0
Fork 0
mirror of https://gitlab.winehq.org/wine/wine-gecko.git synced 2024-09-13 09:24:08 -07:00
Code Issues Packages Projects Releases Wiki Activity

Bug 1174625 - Overhaul PLDHashTable's iterator. r=froydnj.

Browse Source 
This change splits PLDHashTable::Iterator::NextEntry() into two separate
functions, which allow you to get the current element and advance the iterator
separately, which means you can use a for-loop to iterate instead of a
while-loop.

As part of this change, the internals of PLDHashTable::Iterator were
significantly changed and simplified (and modelled after js::HashTable's
equivalent code). It's no longer duplicating code from PL_DHashTableEnumerator.
The chaos mode code was a casualty of this, but given how unreliable that code
has proven to be (see bug 1173212, bug 1174046) this is for the best. (We can
reimplement chaos mode once PLDHashTable::Iterator is back on more solid
footing again, if we think it's important.)

All these changes will make it much easier to add an alternative Iterator that
removes elements, which was turning out to be difficult with the prior code.

In order to make the for-loop header usually fit on a single line, I
deliberately renamed a bunch of things to have shorter names.

In summary, you used to write this:

  PLDHashTable::Iterator iter(&table);
  while (iter.HasMoreEntries()) {
    auto entry = static_cast<FooEntry*>(iter.NextEntry());
    // ... do stuff with |entry| ...
  }
  // iter's scope extends beyond here

and now you write this:

  for (auto iter = table.Iter(); !iter.Done(); iter.Next()) {
    auto entry = static_cast<FooEntry*>(iter.Get());
    // ... do stuff with |entry| ...
  }
  // iter's scope doesn't reach here
This commit is contained in:
Nicholas Nethercote 2015-06-11 21:19:53 -07:00
parent b55b196337
commit b945b71cfa
11 changed files with 172 additions and 135 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
dom/base/nsContentUtils.cpp
View File

@ -3992,9 +3992,8 @@ nsContentUtils::UnmarkGrayJSListenersInCCGenerationDocuments()
return;
}
PLDHashTable::Iterator iter(sEventListenerManagersHash);
while (iter.HasMoreEntries()) {
auto entry = static_cast<EventListenerManagerMapEntry*>(iter.NextEntry());
for (auto i = sEventListenerManagersHash->Iter(); !i.Done(); i.Next()) {
auto entry = static_cast<EventListenerManagerMapEntry*>(i.Get());
nsINode* n = static_cast<nsINode*>(entry->mListenerManager->GetTarget());
if (n && n->IsInDoc() &&
nsCCUncollectableMarker::InGeneration(n->OwnerDoc()->GetMarkedCCGeneration())) {

20
dom/base/nsDocument.cpp
View File

@ -2006,9 +2006,8 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(nsDocument)
}
if (tmp->mSubDocuments) {
PLDHashTable::Iterator iter(tmp->mSubDocuments);
while (iter.HasMoreEntries()) {
auto entry = static_cast<SubDocMapEntry*>(iter.NextEntry());
for (auto iter = tmp->mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<SubDocMapEntry*>(iter.Get());
NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
"mSubDocuments entry->mKey");
@ -4023,9 +4022,8 @@ nsDocument::FindContentForSubDocument(nsIDocument *aDocument) const
return nullptr;
}
PLDHashTable::Iterator iter(mSubDocuments);
while (iter.HasMoreEntries()) {
auto entry = static_cast<SubDocMapEntry*>(iter.NextEntry());
for (auto iter = mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<SubDocMapEntry*>(iter.Get());
if (entry->mSubDocument == aDocument) {
return entry->mKey;
}
@ -8717,9 +8715,8 @@ nsDocument::EnumerateSubDocuments(nsSubDocEnumFunc aCallback, void *aData)
return;
}
PLDHashTable::Iterator iter(mSubDocuments);
while (iter.HasMoreEntries()) {
auto entry = static_cast<SubDocMapEntry*>(iter.NextEntry());
for (auto iter = mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<SubDocMapEntry*>(iter.Get());
nsIDocument* subdoc = entry->mSubDocument;
bool next = subdoc ? aCallback(subdoc, aData) : true;
if (!next) {
@ -8827,9 +8824,8 @@ nsDocument::CanSavePresentation(nsIRequest *aNewRequest)
}
if (mSubDocuments) {
PLDHashTable::Iterator iter(mSubDocuments);
while (iter.HasMoreEntries()) {
auto entry = static_cast<SubDocMapEntry*>(iter.NextEntry());
for (auto iter = mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<SubDocMapEntry*>(iter.Get());
nsIDocument* subdoc = entry->mSubDocument;
// The aIgnoreRequest we were passed is only for us, so don't pass it on.

10
dom/base/nsPropertyTable.cpp
View File

@ -137,9 +137,8 @@ void
nsPropertyTable::EnumerateAll(NSPropertyFunc aCallBack, void* aData)
{
for (PropertyList* prop = mPropertyList; prop; prop = prop->mNext) {
PLDHashTable::Iterator iter(&prop->mObjectValueMap);
while (iter.HasMoreEntries()) {
auto entry = static_cast<PropertyListMapEntry*>(iter.NextEntry());
for (auto iter = prop->mObjectValueMap.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PropertyListMapEntry*>(iter.Get());
aCallBack(const_cast<void*>(entry->key), prop->mName, entry->value,
aData);
}
@ -285,9 +284,8 @@ nsPropertyTable::PropertyList::Destroy()
{
// Enumerate any remaining object/value pairs and destroy the value object.
if (mDtorFunc) {
PLDHashTable::Iterator iter(&mObjectValueMap);
while (iter.HasMoreEntries()) {
auto entry = static_cast<PropertyListMapEntry*>(iter.NextEntry());
for (auto iter = mObjectValueMap.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PropertyListMapEntry*>(iter.Get());
mDtorFunc(const_cast<void*>(entry->key), mName, entry->value, mDtorData);
}
}

15
netwerk/base/nsLoadGroup.cpp
View File

@ -249,9 +249,8 @@ nsLoadGroup::GetStatus(nsresult *status)
static bool
AppendRequestsToArray(PLDHashTable* aTable, nsTArray<nsIRequest*> *aArray)
{
PLDHashTable::Iterator iter(aTable);
while (iter.HasMoreEntries()) {
auto e = static_cast<RequestMapEntry*>(iter.NextEntry());
for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
auto e = static_cast<RequestMapEntry*>(iter.Get());
nsIRequest *request = e->mKey;
NS_ASSERTION(request, "What? Null key in pldhash entry?");
@ -710,9 +709,8 @@ nsLoadGroup::GetRequests(nsISimpleEnumerator * *aRequests)
nsCOMArray<nsIRequest> requests;
requests.SetCapacity(mRequests.EntryCount());
PLDHashTable::Iterator iter(&mRequests);
while (iter.HasMoreEntries()) {
auto e = static_cast<RequestMapEntry*>(iter.NextEntry());
for (auto iter = mRequests.Iter(); !iter.Done(); iter.Next()) {
auto e = static_cast<RequestMapEntry*>(iter.Get());
requests.AppendObject(e->mKey);
}
@ -848,9 +846,8 @@ nsLoadGroup::AdjustPriority(int32_t aDelta)
// Update the priority for each request that supports nsISupportsPriority
if (aDelta != 0) {
mPriority += aDelta;
PLDHashTable::Iterator iter(&mRequests);
while (iter.HasMoreEntries()) {
auto e = static_cast<RequestMapEntry*>(iter.NextEntry());
for (auto iter = mRequests.Iter(); !iter.Done(); iter.Next()) {
auto e = static_cast<RequestMapEntry*>(iter.Get());
RescheduleRequest(e->mKey, aDelta);
}
}

10
netwerk/cache/nsDiskCacheBinding.cpp vendored
View File

@ -328,9 +328,8 @@ nsDiskCacheBindery::ActiveBindings()
NS_ASSERTION(initialized, "nsDiskCacheBindery not initialized");
if (!initialized) return false;
PLDHashTable::Iterator iter(&table);
while (iter.HasMoreEntries()) {
auto entry = static_cast<HashTableEntry*>(iter.NextEntry());
for (auto iter = table.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<HashTableEntry*>(iter.Get());
nsDiskCacheBinding* binding = entry->mBinding;
nsDiskCacheBinding* head = binding;
do {
@ -356,9 +355,8 @@ nsDiskCacheBindery::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
size_t size = 0;
PLDHashTable::Iterator iter(&table);
while (iter.HasMoreEntries()) {
auto entry = static_cast<HashTableEntry*>(iter.NextEntry());
for (auto iter = table.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<HashTableEntry*>(iter.Get());
nsDiskCacheBinding* binding = entry->mBinding;
nsDiskCacheBinding* head = binding;

5
uriloader/base/nsDocLoader.cpp
View File

@ -1351,9 +1351,8 @@ void nsDocLoader::ClearRequestInfoHash(void)
int64_t nsDocLoader::CalculateMaxProgress()
{
int64_t max = mCompletedTotalProgress;
PLDHashTable::Iterator iter(&mRequestInfoHash);
while (iter.HasMoreEntries()) {
auto info = static_cast<const nsRequestInfo*>(iter.NextEntry());
for (auto iter = mRequestInfoHash.Iter(); !iter.Done(); iter.Next()) {
auto info = static_cast<const nsRequestInfo*>(iter.Get());
if (info->mMaxProgress < info->mCurrentProgress) {
return int64_t(-1);

5
xpcom/ds/nsAtomTable.cpp
View File

@ -327,9 +327,8 @@ NS_PurgeAtomTable()
uint32_t leaked = 0;
printf("*** %d atoms still exist (including permanent):\n",
gAtomTable->EntryCount());
PLDHashTable::Iterator iter(gAtomTable);
while (iter.HasMoreEntries()) {
auto entry = static_cast<AtomTableEntry*>(iter.NextEntry());
for (auto iter = gAtomTable->Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<AtomTableEntry*>(iter.Get());
AtomImpl* atom = entry->mAtom;
if (!atom->IsPermanent()) {
leaked++;

5
xpcom/ds/nsPersistentProperties.cpp
View File

@ -572,9 +572,8 @@ nsPersistentProperties::Enumerate(nsISimpleEnumerator** aResult)
props.SetCapacity(mTable.EntryCount());
// Step through hash entries populating a transient array
PLDHashTable::Iterator iter(&mTable);
while (iter.HasMoreEntries()) {
auto entry = static_cast<PropertyTableEntry*>(iter.NextEntry());
for (auto iter = mTable.Iter(); !iter.Done(); iter.Next()) {
auto entry = static_cast<PropertyTableEntry*>(iter.Get());
nsRefPtr<nsPropertyElement> element =
new nsPropertyElement(nsDependentCString(entry->mKey),

114
xpcom/glue/pldhash.cpp
View File

@ -754,8 +754,6 @@ PLDHashTable::Enumerate(PLDHashEnumerator aEtor, void* aArg)
INCREMENT_RECURSION_LEVEL(this);
// Please keep this method in sync with the PLDHashTable::Iterator constructor
// and ::NextEntry methods below in this file.
char* entryAddr = mEntryStore;
uint32_t capacity = Capacity();
uint32_t tableSize = capacity * mEntrySize;
@ -895,93 +893,69 @@ PL_DHashTableSizeOfIncludingThis(
aMallocSizeOf, aArg);
}
PLDHashTable::Iterator::Iterator(const PLDHashTable* aTable)
: mTable(aTable),
mEntryAddr(mTable->mEntryStore),
mEntryOffset(0)
PLDHashTable::Iterator::Iterator(Iterator&& aOther)
: mTable(aOther.mTable)
, mCurrent(aOther.mCurrent)
, mLimit(aOther.mLimit)
{
// Make sure that modifications can't simultaneously happen while the iterator
// is active.
INCREMENT_RECURSION_LEVEL(mTable);
// The following code is taken from, and should be kept in sync with, the
// PLDHashTable::Enumerate method above. The variables i and entryAddr (which
// vary over the course of the for loop) are converted into mEntryOffset and
// mEntryAddr, respectively.
uint32_t capacity = mTable->Capacity();
if (ChaosMode::isActive(ChaosMode::HashTableIteration) && capacity > 0) {
// Start iterating at a random point in the hashtable. It would be
// even more chaotic to iterate in fully random order, but that's a lot
// more work.
mEntryAddr += ChaosMode::randomUint32LessThan(capacity) * mTable->mEntrySize;
}
// No need to change mRecursionLevel here.
aOther.mTable = nullptr;
aOther.mCurrent = nullptr;
aOther.mLimit = nullptr;
}
PLDHashTable::Iterator::Iterator(const Iterator& aIterator)
: mTable(aIterator.mTable),
mEntryAddr(aIterator.mEntryAddr),
mEntryOffset(aIterator.mEntryOffset)
PLDHashTable::Iterator::Iterator(const PLDHashTable* aTable)
: mTable(aTable)
, mCurrent(mTable->mEntryStore)
, mLimit(mTable->mEntryStore + mTable->Capacity() * mTable->mEntrySize)
{
// We need the copy constructor only so that we can keep the recursion level
// consistent.
// Make sure that modifications can't simultaneously happen while the
// iterator is active.
INCREMENT_RECURSION_LEVEL(mTable);
// Advance to the first live entry, or to the end if there are none.
while (IsOnNonLiveEntry()) {
mCurrent += mTable->mEntrySize;
}
}
PLDHashTable::Iterator::~Iterator()
{
DECREMENT_RECURSION_LEVEL(mTable);
if (mTable) {
DECREMENT_RECURSION_LEVEL(mTable);
}
}
bool
PLDHashTable::Iterator::HasMoreEntries() const
MOZ_ALWAYS_INLINE bool
PLDHashTable::Iterator::Done() const
{
// Check the number of live entries seen, not the total number of entries
// seen. To see why, consider what happens if the last entry is not live: we
// would have to iterate after returning an entry to see if more live entries
// exist.
return mEntryOffset < mTable->EntryCount();
return mCurrent == mLimit;
}
MOZ_ALWAYS_INLINE bool
PLDHashTable::Iterator::IsOnNonLiveEntry() const
{
return !Done() && !ENTRY_IS_LIVE(reinterpret_cast<PLDHashEntryHdr*>(mCurrent));
}
PLDHashEntryHdr*
PLDHashTable::Iterator::NextEntry()
PLDHashTable::Iterator::Get() const
{
MOZ_ASSERT(HasMoreEntries());
MOZ_ASSERT(!Done());
// The following code is taken from, and should be kept in sync with, the
// PLDHashTable::Enumerate method above. The variables i and entryAddr (which
// vary over the course of the for loop) are converted into mEntryOffset and
// mEntryAddr, respectively.
uint32_t capacity = mTable->Capacity();
uint32_t tableSize = capacity * mTable->mEntrySize;
char* entryLimit = mTable->mEntryStore + tableSize;
PLDHashEntryHdr* entry = reinterpret_cast<PLDHashEntryHdr*>(mCurrent);
MOZ_ASSERT(ENTRY_IS_LIVE(entry));
return entry;
}
// Strictly speaking, we don't need to iterate over the full capacity each
// time. However, it is simpler to do so rather than unnecessarily track the
// current number of entries checked as opposed to only live entries. If debug
// 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;
void
PLDHashTable::Iterator::Next()
{
MOZ_ASSERT(!Done());
// Increment the count before returning so we don't keep returning the same
// address. This may wrap around if ChaosMode is enabled.
mEntryAddr += mTable->mEntrySize;
if (mEntryAddr >= entryLimit) {
mEntryAddr -= tableSize;
}
if (ENTRY_IS_LIVE(entry)) {
++mEntryOffset;
return entry;
}
}
// If the debug checks pass, then the above loop should always find a live
// entry. If those checks are disabled, then it may be possible to reach this
// if the table is empty and this method is called.
MOZ_CRASH("Flagrant misuse of hashtable iterators not caught by checks.");
do {
mCurrent += mTable->mEntrySize;
} while (IsOnNonLiveEntry());
}
#ifdef DEBUG

51
xpcom/glue/pldhash.h
View File

@ -292,26 +292,51 @@ public:
void DumpMeter(PLDHashEnumerator aDump, FILE* aFp);
#endif
/**
* This is an iterator that works over the elements of PLDHashtable. It is not
* safe to modify the hashtable while it is being iterated over; on debug
* builds, attempting to do so will result in an assertion failure.
*/
class Iterator {
// 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.
//
// Example usage:
//
// for (auto iter = table.Iter(); !iter.Done(); iter.Next()) {
// auto entry = static_cast<FooEntry*>(iter.Get());
// // ... do stuff with |entry| ...
// }
//
// or:
//
// for (PLDHashTable::Iterator iter(&table); !iter.Done(); iter.Next()) {
// auto entry = static_cast<FooEntry*>(iter.Get());
// // ... do stuff with |entry| ...
// }
//
// The latter form is more verbose but is easier to work with when
// making subclasses of Iterator.
//
class Iterator
{
public:
explicit Iterator(const PLDHashTable* aTable);
Iterator(const Iterator& aIterator);
Iterator(Iterator&& aOther);
~Iterator();
bool HasMoreEntries() const;
PLDHashEntryHdr* NextEntry();
bool Done() const; // Have we finished?
PLDHashEntryHdr* Get() const; // Get the current entry.
void Next(); // Advance to the next entry.
private:
const PLDHashTable* mTable; /* Main table pointer */
char* mEntryAddr; /* Pointer to the next entry to check */
uint32_t mEntryOffset; /* The number of the elements returned */
const PLDHashTable* mTable; // Main table pointer.
char* mCurrent; // Pointer to the current entry.
char* mLimit; // One past the last entry.
bool IsOnNonLiveEntry() const;
Iterator() = delete;
Iterator(const Iterator&) = delete;
Iterator& operator=(const Iterator&) = delete;
Iterator& operator=(const Iterator&&) = delete;
};
Iterator Iterate() const { return Iterator(this); }
Iterator Iter() const { return Iterator(this); }
private:
static bool EntryIsFree(PLDHashEntryHdr* aEntry);

67
xpcom/tests/TestPLDHash.cpp
View File

@ -71,9 +71,7 @@ static bool test_pldhash_lazy_storage()
return false; // enumeration count is non-zero?
}
for (PLDHashTable::Iterator iter = t.Iterate();
iter.HasMoreEntries();
iter.NextEntry()) {
for (auto iter = t.Iter(); !iter.Done(); iter.Get()) {
return false; // shouldn't hit this on an empty table
}
@ -91,17 +89,24 @@ static bool test_pldhash_lazy_storage()
// test_pldhash_grow_to_max_capacity() because we insert the integers 0..,
// which means it's collision-free.
static PLDHashNumber
trivial_hash(PLDHashTable *table, const void *key)
TrivialHash(PLDHashTable *table, const void *key)
{
return (PLDHashNumber)(size_t)key;
}
static void
TrivialInitEntry(PLDHashEntryHdr* aEntry, const void* aKey)
{
auto entry = static_cast<PLDHashEntryStub*>(aEntry);
entry->key = aKey;
}
static const PLDHashTableOps trivialOps = {
trivial_hash,
TrivialHash,
PL_DHashMatchEntryStub,
PL_DHashMoveEntryStub,
PL_DHashClearEntryStub,
nullptr
TrivialInitEntry
};
static bool test_pldhash_move_semantics()
@ -180,6 +185,52 @@ static bool test_pldhash_Clear()
return true;
}
static bool test_pldhash_Iterator()
{
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::Iterator iter1(&t);
PLDHashTable::Iterator iter2(mozilla::Move(iter1));
}
// Iterate through the empty table.
for (PLDHashTable::Iterator iter(&t); !iter.Done(); iter.Next()) {
(void) iter.Get();
return false; // shouldn't hit this
}
// Add three entries.
PL_DHashTableAdd(&t, (const void*)77);
PL_DHashTableAdd(&t, (const void*)88);
PL_DHashTableAdd(&t, (const void*)99);
// Check the iterator goes through each entry once.
bool saw77 = false, saw88 = false, saw99 = false;
int n = 0;
for (auto iter(t.Iter()); !iter.Done(); iter.Next()) {
auto entry = static_cast<PLDHashEntryStub*>(iter.Get());
if (entry->key == (const void*)77) {
saw77 = true;
}
if (entry->key == (const void*)88) {
saw88 = true;
}
if (entry->key == (const void*)99) {
saw99 = true;
}
n++;
}
if (!saw77 || !saw88 || !saw99 || n != 3) {
return false;
}
return true;
}
// See bug 931062, we skip this test on Android due to OOM.
#ifndef MOZ_WIDGET_ANDROID
static bool test_pldhash_grow_to_max_capacity()
@ -222,7 +273,9 @@ static const struct Test {
DECL_TEST(test_pldhash_lazy_storage),
DECL_TEST(test_pldhash_move_semantics),
DECL_TEST(test_pldhash_Clear),
// See bug 931062, we skip this test on Android due to OOM.
DECL_TEST(test_pldhash_Iterator),
// See bug 931062, we skip this test on Android due to OOM. Also, it's slow,
// and so should always be last.
#ifndef MOZ_WIDGET_ANDROID
DECL_TEST(test_pldhash_grow_to_max_capacity),
#endif