From e700b410791ecdb83bfb4ea2100cf377e6d8ad41 Mon Sep 17 00:00:00 2001 From: Michal Novotny Date: Thu, 6 Oct 2016 11:23:52 +0200 Subject: [PATCH] Bug 1249304 - Optimize sorting of CacheIndex::mFrecencyArray, r=honzab --- netwerk/cache2/CacheIndex.cpp | 182 +++++++++++++++++++------- netwerk/cache2/CacheIndex.h | 83 ++++++++++-- netwerk/cache2/CacheIndexIterator.cpp | 8 -- netwerk/cache2/CacheIndexIterator.h | 1 - 4 files changed, 209 insertions(+), 65 deletions(-) diff --git a/netwerk/cache2/CacheIndex.cpp b/netwerk/cache2/CacheIndex.cpp index 0b0c5890e284..5c521cdf27cd 100644 --- a/netwerk/cache2/CacheIndex.cpp +++ b/netwerk/cache2/CacheIndex.cpp @@ -43,16 +43,29 @@ class FrecencyComparator { public: bool Equals(CacheIndexRecord* a, CacheIndexRecord* b) const { + if (!a || !b) { + return false; + } + return a->mFrecency == b->mFrecency; } bool LessThan(CacheIndexRecord* a, CacheIndexRecord* b) const { - // Place entries with frecency 0 at the end of the array. + // Removed (=null) entries must be at the end of the array. + if (!a) { + return false; + } + if (!b) { + return true; + } + + // Place entries with frecency 0 at the end of the non-removed entries. if (a->mFrecency == 0) { return false; } if (b->mFrecency == 0) { return true; } + return a->mFrecency < b->mFrecency; } }; @@ -95,19 +108,39 @@ public: } if (entry && !mOldRecord) { - mIndex->InsertRecordToFrecencyArray(entry->mRec); + mIndex->mFrecencyArray.AppendRecord(entry->mRec); mIndex->AddRecordToIterators(entry->mRec); } else if (!entry && mOldRecord) { - mIndex->RemoveRecordFromFrecencyArray(mOldRecord); + mIndex->mFrecencyArray.RemoveRecord(mOldRecord); mIndex->RemoveRecordFromIterators(mOldRecord); } else if (entry && mOldRecord) { if (entry->mRec != mOldRecord) { // record has a different address, we have to replace it mIndex->ReplaceRecordInIterators(mOldRecord, entry->mRec); - mIndex->RemoveRecordFromFrecencyArray(mOldRecord); - mIndex->InsertRecordToFrecencyArray(entry->mRec); + + if (entry->mRec->mFrecency == mOldFrecency) { + // If frecency hasn't changed simply replace the pointer + mIndex->mFrecencyArray.ReplaceRecord(mOldRecord, entry->mRec); + } else { + // It is expected that when the frecency is changed the new value is + // always bigger than the old one. There is also a special case when + // we zero the frecency if eviction of the entry fails. If the above + // isn't true, this algorithm might not work properly and needs to be + // changed. + MOZ_ASSERT(entry->mRec->mFrecency == 0 || + entry->mRec->mFrecency > mOldFrecency); + + // Remove old pointer and insert the new one at the end of the array + mIndex->mFrecencyArray.RemoveRecord(mOldRecord); + mIndex->mFrecencyArray.AppendRecord(entry->mRec); + } } else if (entry->mRec->mFrecency != mOldFrecency) { - mIndex->mFrecencyArraySorted = false; + MOZ_ASSERT(entry->mRec->mFrecency == 0 || + entry->mRec->mFrecency > mOldFrecency); + + // Move the element at the end of the array + mIndex->mFrecencyArray.RemoveRecord(entry->mRec); + mIndex->mFrecencyArray.AppendRecord(entry->mRec); } } else { // both entries were removed or not initialized, do nothing @@ -252,7 +285,6 @@ CacheIndex::CacheIndex() , mRWBufPos(0) , mRWPending(false) , mJournalReadSuccessfully(false) - , mFrecencyArraySorted(false) , mAsyncGetDiskConsumptionBlocked(false) { sLock.AssertCurrentThreadOwns(); @@ -1190,43 +1222,44 @@ CacheIndex::GetEntryForEviction(bool aIgnoreEmptyEntries, SHA1Sum::Hash *aHash, } SHA1Sum::Hash hash; - bool foundEntry = false; - uint32_t i; + CacheIndexRecord *foundRecord = nullptr; + uint32_t skipped = 0; // find first non-forced valid and unpinned entry with the lowest frecency - if (!index->mFrecencyArraySorted) { - index->mFrecencyArray.Sort(FrecencyComparator()); - index->mFrecencyArraySorted = true; - } + index->mFrecencyArray.SortIfNeeded(); - for (i = 0; i < index->mFrecencyArray.Length(); ++i) { - memcpy(&hash, &index->mFrecencyArray[i]->mHash, sizeof(SHA1Sum::Hash)); + for (auto iter = index->mFrecencyArray.Iter(); !iter.Done(); iter.Next()) { + CacheIndexRecord *rec = iter.Get(); + + memcpy(&hash, rec->mHash, sizeof(SHA1Sum::Hash)); + + ++skipped; if (IsForcedValidEntry(&hash)) { continue; } - if (CacheIndexEntry::IsPinned(index->mFrecencyArray[i])) { + if (CacheIndexEntry::IsPinned(rec)) { continue; } - if (aIgnoreEmptyEntries && - !CacheIndexEntry::GetFileSize(index->mFrecencyArray[i])) { + if (aIgnoreEmptyEntries && !CacheIndexEntry::GetFileSize(rec)) { continue; } - foundEntry = true; + --skipped; + foundRecord = rec; break; } - if (!foundEntry) + if (!foundRecord) return NS_ERROR_NOT_AVAILABLE; - *aCnt = index->mFrecencyArray.Length() - i; + *aCnt = skipped; LOG(("CacheIndex::GetEntryForEviction() - returning entry from frecency " "array [hash=%08x%08x%08x%08x%08x, cnt=%u, frecency=%u]", - LOGSHA1(&hash), *aCnt, index->mFrecencyArray[i]->mFrecency)); + LOGSHA1(&hash), *aCnt, foundRecord->mFrecency)); memcpy(aHash, &hash, sizeof(SHA1Sum::Hash)); @@ -1320,8 +1353,8 @@ CacheIndex::GetCacheStats(nsILoadContextInfo *aInfo, uint32_t *aSize, uint32_t * *aSize = 0; *aCount = 0; - for (uint32_t i = 0; i < index->mFrecencyArray.Length(); ++i) { - CacheIndexRecord* record = index->mFrecencyArray[i]; + for (auto iter = index->mFrecencyArray.Iter(); !iter.Done(); iter.Next()) { + CacheIndexRecord *record = iter.Get(); if (!CacheIndexEntry::RecordMatchesLoadContextInfo(record, aInfo)) continue; @@ -1404,22 +1437,21 @@ CacheIndex::GetIterator(nsILoadContextInfo *aInfo, bool aAddNew, return NS_ERROR_NOT_AVAILABLE; } - RefPtr iter; + RefPtr idxIter; if (aInfo) { - iter = new CacheIndexContextIterator(index, aAddNew, aInfo); + idxIter = new CacheIndexContextIterator(index, aAddNew, aInfo); } else { - iter = new CacheIndexIterator(index, aAddNew); + idxIter = new CacheIndexIterator(index, aAddNew); } - if (!index->mFrecencyArraySorted) { - index->mFrecencyArray.Sort(FrecencyComparator()); - index->mFrecencyArraySorted = true; + index->mFrecencyArray.SortIfNeeded(); + + for (auto iter = index->mFrecencyArray.Iter(); !iter.Done(); iter.Next()) { + idxIter->AddRecord(iter.Get()); } - iter->AddRecords(index->mFrecencyArray); - - index->mIterators.AppendElement(iter); - iter.swap(*_retval); + index->mIterators.AppendElement(idxIter); + idxIter.swap(*_retval); return NS_OK; } @@ -3231,24 +3263,80 @@ CacheIndex::ReleaseBuffer() } void -CacheIndex::InsertRecordToFrecencyArray(CacheIndexRecord *aRecord) +CacheIndex::FrecencyArray::AppendRecord(CacheIndexRecord *aRecord) { - LOG(("CacheIndex::InsertRecordToFrecencyArray() [record=%p, hash=%08x%08x%08x" + LOG(("CacheIndex::FrecencyArray::AppendRecord() [record=%p, hash=%08x%08x%08x" "%08x%08x]", aRecord, LOGSHA1(aRecord->mHash))); - MOZ_ASSERT(!mFrecencyArray.Contains(aRecord)); - mFrecencyArray.AppendElement(aRecord); - mFrecencyArraySorted = false; + MOZ_ASSERT(!mRecs.Contains(aRecord)); + mRecs.AppendElement(aRecord); + + // If the new frecency is 0, the element should be at the end of the array, + // i.e. this change doesn't affect order of the array + if (aRecord->mFrecency != 0) { + ++mUnsortedElements; + } } void -CacheIndex::RemoveRecordFromFrecencyArray(CacheIndexRecord *aRecord) +CacheIndex::FrecencyArray::RemoveRecord(CacheIndexRecord *aRecord) { - LOG(("CacheIndex::RemoveRecordFromFrecencyArray() [record=%p]", aRecord)); + LOG(("CacheIndex::FrecencyArray::RemoveRecord() [record=%p]", aRecord)); - DebugOnly removed; - removed = mFrecencyArray.RemoveElement(aRecord); - MOZ_ASSERT(removed); + decltype(mRecs)::index_type idx; + idx = mRecs.IndexOf(aRecord); + MOZ_RELEASE_ASSERT(idx != mRecs.NoIndex); + mRecs[idx] = nullptr; + ++mRemovedElements; + + // Calling SortIfNeeded ensures that we get rid of removed elements in the + // array once we hit the limit. + SortIfNeeded(); +} + +void +CacheIndex::FrecencyArray::ReplaceRecord(CacheIndexRecord *aOldRecord, + CacheIndexRecord *aNewRecord) +{ + LOG(("CacheIndex::FrecencyArray::ReplaceRecord() [oldRecord=%p, " + "newRecord=%p]", aOldRecord, aNewRecord)); + + decltype(mRecs)::index_type idx; + idx = mRecs.IndexOf(aOldRecord); + MOZ_RELEASE_ASSERT(idx != mRecs.NoIndex); + mRecs[idx] = aNewRecord; +} + +void +CacheIndex::FrecencyArray::SortIfNeeded() +{ + const uint32_t kMaxUnsortedCount = 512; + const uint32_t kMaxUnsortedPercent = 10; + const uint32_t kMaxRemovedCount = 512; + + uint32_t unsortedLimit = + std::min(kMaxUnsortedCount, Length() * kMaxUnsortedPercent / 100); + + if (mUnsortedElements > unsortedLimit || + mRemovedElements > kMaxRemovedCount) { + LOG(("CacheIndex::FrecencyArray::SortIfNeeded() - Sorting array " + "[unsortedElements=%u, unsortedLimit=%u, removedElements=%u, " + "maxRemovedCount=%u]", mUnsortedElements, unsortedLimit, + mRemovedElements, kMaxRemovedCount)); + + mRecs.Sort(FrecencyComparator()); + mUnsortedElements = 0; + if (mRemovedElements) { +#ifdef DEBUG + for (uint32_t i = Length(); i < mRecs.Length(); ++i) { + MOZ_ASSERT(!mRecs[i]); + } +#endif + // Removed elements are at the end after sorting. + mRecs.RemoveElementsAt(Length(), mRemovedElements); + mRemovedElements = 0; + } + } } void @@ -3626,7 +3714,7 @@ CacheIndex::SizeOfExcludingThisInternal(mozilla::MallocSizeOf mallocSizeOf) cons n += mTmpJournal.SizeOfExcludingThis(mallocSizeOf); // mFrecencyArray items are reported by mIndex/mPendingUpdates - n += mFrecencyArray.ShallowSizeOfExcludingThis(mallocSizeOf); + n += mFrecencyArray.mRecs.ShallowSizeOfExcludingThis(mallocSizeOf); n += mDiskConsumptionObservers.ShallowSizeOfExcludingThis(mallocSizeOf); return n; @@ -3710,7 +3798,9 @@ CacheIndex::ReportHashStats() } nsTArray records; - records.AppendElements(mFrecencyArray); + for (auto iter = mFrecencyArray.Iter(); !iter.Done(); iter.Next()) { + records.AppendElement(iter.Get()); + } records.Sort(HashComparator()); diff --git a/netwerk/cache2/CacheIndex.h b/netwerk/cache2/CacheIndex.h index 69111c05d47c..88d45091314d 100644 --- a/netwerk/cache2/CacheIndex.h +++ b/netwerk/cache2/CacheIndex.h @@ -921,10 +921,6 @@ private: void AllocBuffer(); void ReleaseBuffer(); - // Methods used by CacheIndexEntryAutoManage to keep the arrays up to date. - void InsertRecordToFrecencyArray(CacheIndexRecord *aRecord); - void RemoveRecordFromFrecencyArray(CacheIndexRecord *aRecord); - // Methods used by CacheIndexEntryAutoManage to keep the iterators up to date. void AddRecordToIterators(CacheIndexRecord *aRecord); void RemoveRecordFromIterators(CacheIndexRecord *aRecord); @@ -1031,12 +1027,79 @@ private: // of the journal fails or the hash does not match. nsTHashtable mTmpJournal; - // An array that keeps entry records ordered by eviction preference; we take - // the entry with lowest valid frecency. Zero frecency is an initial value - // and such entries are stored at the end of the array. Uninitialized entries - // and entries marked as deleted are not present in this array. - nsTArray mFrecencyArray; - bool mFrecencyArraySorted; + // FrecencyArray maintains order of entry records for eviction. Ideally, the + // records would be ordered by frecency all the time, but since this would be + // quite expensive, we allow certain amount of entries to be out of order. + // When the frecency is updated the new value is always bigger than the old + // one. Instead of keeping updated entries at the same position, we move them + // at the end of the array. This protects recently updated entries from + // eviction. The array is sorted once we hit the limit of maximum unsorted + // entries. + class FrecencyArray + { + class Iterator + { + public: + explicit Iterator(nsTArray *aRecs) + : mRecs(aRecs) + , mIdx(0) + { + while (!Done() && !(*mRecs)[mIdx]) { + mIdx++; + } + } + + bool Done() const { return mIdx == mRecs->Length(); } + + CacheIndexRecord* Get() const + { + MOZ_ASSERT(!Done()); + return (*mRecs)[mIdx]; + } + + void Next() + { + MOZ_ASSERT(!Done()); + ++mIdx; + while (!Done() && !(*mRecs)[mIdx]) { + mIdx++; + } + } + + private: + nsTArray *mRecs; + uint32_t mIdx; + }; + + public: + Iterator Iter() { return Iterator(&mRecs); } + + FrecencyArray() : mUnsortedElements(0) + , mRemovedElements(0) {} + + // Methods used by CacheIndexEntryAutoManage to keep the array up to date. + void AppendRecord(CacheIndexRecord *aRecord); + void RemoveRecord(CacheIndexRecord *aRecord); + void ReplaceRecord(CacheIndexRecord *aOldRecord, + CacheIndexRecord *aNewRecord); + void SortIfNeeded(); + + size_t Length() const { return mRecs.Length() - mRemovedElements; } + void Clear() { mRecs.Clear(); } + + private: + friend class CacheIndex; + + nsTArray mRecs; + uint32_t mUnsortedElements; + // Instead of removing elements from the array immediately, we null them out + // and the iterator skips them when accessing the array. The null pointers + // are placed at the end during sorting and we strip them out all at once. + // This saves moving a lot of memory in nsTArray::RemoveElementsAt. + uint32_t mRemovedElements; + }; + + FrecencyArray mFrecencyArray; nsTArray mIterators; diff --git a/netwerk/cache2/CacheIndexIterator.cpp b/netwerk/cache2/CacheIndexIterator.cpp index e21ae60e9045..0d56ec81f531 100644 --- a/netwerk/cache2/CacheIndexIterator.cpp +++ b/netwerk/cache2/CacheIndexIterator.cpp @@ -90,14 +90,6 @@ CacheIndexIterator::AddRecord(CacheIndexRecord *aRecord) mRecords.AppendElement(aRecord); } -void -CacheIndexIterator::AddRecords(const nsTArray &aRecords) -{ - LOG(("CacheIndexIterator::AddRecords() [this=%p]", this)); - - mRecords.AppendElements(aRecords); -} - bool CacheIndexIterator::RemoveRecord(CacheIndexRecord *aRecord) { diff --git a/netwerk/cache2/CacheIndexIterator.h b/netwerk/cache2/CacheIndexIterator.h index 9ec06a676f9c..9fe96989ecb1 100644 --- a/netwerk/cache2/CacheIndexIterator.h +++ b/netwerk/cache2/CacheIndexIterator.h @@ -43,7 +43,6 @@ protected: bool ShouldBeNewAdded() { return mAddNew; } virtual void AddRecord(CacheIndexRecord *aRecord); - virtual void AddRecords(const nsTArray &aRecords); bool RemoveRecord(CacheIndexRecord *aRecord); bool ReplaceRecord(CacheIndexRecord *aOldRecord, CacheIndexRecord *aNewRecord);