From ac0221e02bed1ecb9e69c6a2b9040d18260e8074 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 08:58:02 -0400 Subject: [PATCH 1/9] initial --- cachelib/allocator/MM2Q-inl.h | 7 +++--- .../allocator/datastruct/MultiDList-inl.h | 23 +++++++++++++++++++ cachelib/allocator/datastruct/MultiDList.h | 20 ++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/cachelib/allocator/MM2Q-inl.h b/cachelib/allocator/MM2Q-inl.h index e791d6c6c3..4efdcad171 100644 --- a/cachelib/allocator/MM2Q-inl.h +++ b/cachelib/allocator/MM2Q-inl.h @@ -243,16 +243,17 @@ MM2Q::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), lru_.rbegin()}; } + +// returns the head of the hot queue for promotion template T::*HookPtr> template void -MM2Q::Container::withEvictionIterator(F&& fun) { +MM2Q::Container::withPromotionIterator(F&& fun) { lruMutex_->lock_combine([this, &fun]() { - fun(Iterator{LockHolder{}, lru_.rbegin()}); + fun(Iterator{LockHolder{}, lru_.begin(LruType::Hot)}); }); } - template T::*HookPtr> void MM2Q::Container::removeLocked(T& node, bool doRebalance) noexcept { diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index 861eb5e2db..353e838db5 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -71,6 +71,20 @@ void MultiDList::Iterator::initToValidRBeginFrom( : mlist_.lists_[index_]->rbegin(); } +template T::*HookPtr> +void MultiDList::Iterator::initToValidBeginFrom( + size_t listIdx) noexcept { + // Find the first non-empty list. + index_ = listIdx; + while (index_ != std::numeric_limits::max() && + mlist_.lists_[index_]->size() == 0) { + --index_; + } + currIter_ = index_ == std::numeric_limits::max() + ? mlist_.lists_[0]->end() + : mlist_.lists_[index_]->begin(); +} + template T::*HookPtr> typename MultiDList::Iterator& MultiDList::Iterator::operator++() noexcept { @@ -100,6 +114,15 @@ typename MultiDList::Iterator MultiDList::rbegin( return MultiDList::Iterator(*this, listIdx); } +template T::*HookPtr> +typename MultiDList::Iterator MultiDList::begin( + size_t listIdx) const { + if (listIdx >= lists_.size()) { + throw std::invalid_argument("Invalid list index for MultiDList iterator."); + } + return MultiDList::Iterator(*this, listIdx, true); +} + template T::*HookPtr> typename MultiDList::Iterator MultiDList::rend() const noexcept { diff --git a/cachelib/allocator/datastruct/MultiDList.h b/cachelib/allocator/datastruct/MultiDList.h index fd309614ab..b703f5c9e3 100644 --- a/cachelib/allocator/datastruct/MultiDList.h +++ b/cachelib/allocator/datastruct/MultiDList.h @@ -119,6 +119,20 @@ class MultiDList { // which has an invalid index_. XDCHECK(index_ == kInvalidIndex || currIter_.get() != nullptr); } + explicit Iterator(const MultiDList& mlist, + size_t listIdx, bool head) noexcept + : currIter_(mlist.lists_[mlist.lists_.size() - 1]->rbegin()), + mlist_(mlist) { + XDCHECK_LT(listIdx, mlist.lists_.size()); + if (head) { + initToValidBeginFrom(listIdx); + } else { + initToValidRBeginFrom(listIdx); + } + // We should either point to an element or the end() iterator + // which has an invalid index_. + XDCHECK(index_ == kInvalidIndex || currIter_.get() != nullptr); + } virtual ~Iterator() = default; // copyable and movable @@ -169,6 +183,9 @@ class MultiDList { // reset iterator to the beginning of a speicific queue void initToValidRBeginFrom(size_t listIdx) noexcept; + + // reset iterator to the head of a specific queue + void initToValidBeginFrom(size_t listIdx) noexcept; // Index of current list size_t index_{0}; @@ -184,6 +201,9 @@ class MultiDList { // provides an iterator starting from the tail of a specific list. Iterator rbegin(size_t idx) const; + + // provides an iterator starting from the head of a specific list. + Iterator begin(size_t idx) const; // Iterator to compare against for the end. Iterator rend() const noexcept; From 0f2a247e63da3283c3827025d9554ee6c19b0a86 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 09:46:50 -0400 Subject: [PATCH 2/9] fixed invalid iter --- cachelib/allocator/datastruct/MultiDList-inl.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index 353e838db5..ccde42a2b4 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -76,12 +76,12 @@ void MultiDList::Iterator::initToValidBeginFrom( size_t listIdx) noexcept { // Find the first non-empty list. index_ = listIdx; - while (index_ != std::numeric_limits::max() && + while (index_ != mlist_.lists_.size() && mlist_.lists_[index_]->size() == 0) { - --index_; + ++index_; } - currIter_ = index_ == std::numeric_limits::max() - ? mlist_.lists_[0]->end() + currIter_ = index_ == mlist_.lists_.size() + ? mlist_.lists_[mlist_.lists_.size()-1]->begin() : mlist_.lists_[index_]->begin(); } From 70bb6f0d8932c0d3c48281944dd1967fcaa7b15a Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 09:57:02 -0400 Subject: [PATCH 3/9] added check to assert --- cachelib/allocator/datastruct/MultiDList.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cachelib/allocator/datastruct/MultiDList.h b/cachelib/allocator/datastruct/MultiDList.h index b703f5c9e3..68a94f2aa4 100644 --- a/cachelib/allocator/datastruct/MultiDList.h +++ b/cachelib/allocator/datastruct/MultiDList.h @@ -131,7 +131,7 @@ class MultiDList { } // We should either point to an element or the end() iterator // which has an invalid index_. - XDCHECK(index_ == kInvalidIndex || currIter_.get() != nullptr); + XDCHECK(index_ == kInvalidIndex || index_ == mlist.lists_.size() || currIter_.get() != nullptr); } virtual ~Iterator() = default; From dae70c17af69171313b1326429ecee8c98d6892d Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 10:07:22 -0400 Subject: [PATCH 4/9] want a null iterator if empty --- cachelib/allocator/datastruct/MultiDList-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index ccde42a2b4..f27e65661c 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -81,7 +81,7 @@ void MultiDList::Iterator::initToValidBeginFrom( ++index_; } currIter_ = index_ == mlist_.lists_.size() - ? mlist_.lists_[mlist_.lists_.size()-1]->begin() + ? mlist_.lists_[mlist_.lists_.size()-1]->rend() : mlist_.lists_[index_]->begin(); } From 96d69c12c36fa7e6a6546b8eabe9be75d8cc1346 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 10:20:03 -0400 Subject: [PATCH 5/9] use first list if none are avail --- cachelib/allocator/datastruct/MultiDList-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index f27e65661c..c051d74b2e 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -81,7 +81,7 @@ void MultiDList::Iterator::initToValidBeginFrom( ++index_; } currIter_ = index_ == mlist_.lists_.size() - ? mlist_.lists_[mlist_.lists_.size()-1]->rend() + ? mlist_.lists_[0]->rend() : mlist_.lists_[index_]->begin(); } From 9b507586608923b16b01df2b45b2dc162be19d91 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Mon, 11 Jul 2022 11:14:56 -0400 Subject: [PATCH 6/9] no background workers yet --- cachelib/allocator/datastruct/MultiDList-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index c051d74b2e..60ca93ba44 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -81,7 +81,7 @@ void MultiDList::Iterator::initToValidBeginFrom( ++index_; } currIter_ = index_ == mlist_.lists_.size() - ? mlist_.lists_[0]->rend() + ? mlist_.lists_[0]->begin() : mlist_.lists_[index_]->begin(); } From 336221624e47782dc67740fdf94000886b596e23 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Sun, 24 Jul 2022 16:35:14 -0400 Subject: [PATCH 7/9] added tests and made it so we would jump to next list --- cachelib/allocator/datastruct/DList.h | 4 +++ .../allocator/datastruct/MultiDList-inl.h | 33 +++++++++++++++---- cachelib/allocator/tests/MM2QTest.cpp | 33 +++++++++++++++++++ cachelib/allocator/tests/MMTypeTest.h | 2 ++ 4 files changed, 65 insertions(+), 7 deletions(-) diff --git a/cachelib/allocator/datastruct/DList.h b/cachelib/allocator/datastruct/DList.h index d882eb1bca..3a4093447d 100644 --- a/cachelib/allocator/datastruct/DList.h +++ b/cachelib/allocator/datastruct/DList.h @@ -216,6 +216,10 @@ class DList { curr_ = dir_ == Direction::FROM_HEAD ? dlist_->head_ : dlist_->tail_; } + Direction getDirection() noexcept { + return dir_; + } + protected: void goForward() noexcept; void goBackward() noexcept; diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index 60ca93ba44..f06e5df587 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -25,12 +25,26 @@ void MultiDList::Iterator::goForward() noexcept { } // Move iterator forward ++currIter_; - // If we land at the rend of this list, move to the previous list. - while (index_ != kInvalidIndex && - currIter_ == mlist_.lists_[index_]->rend()) { - --index_; - if (index_ != kInvalidIndex) { - currIter_ = mlist_.lists_[index_]->rbegin(); + + if (currIter_.getDirection() == DListIterator::Direction::FROM_HEAD) { + // If we land at the rend of this list, move to the previous list. + while (index_ != kInvalidIndex && index_ != mlist_.lists_.size() && + currIter_ == mlist_.lists_[index_]->end()) { + ++index_; + if (index_ != kInvalidIndex && index_ != mlist_.lists_.size()) { + currIter_ = mlist_.lists_[index_]->begin(); + } else { + return; + } + } + } else { + // If we land at the rend of this list, move to the previous list. + while (index_ != kInvalidIndex && + currIter_ == mlist_.lists_[index_]->rend()) { + --index_; + if (index_ != kInvalidIndex) { + currIter_ = mlist_.lists_[index_]->rbegin(); + } } } } @@ -80,7 +94,12 @@ void MultiDList::Iterator::initToValidBeginFrom( mlist_.lists_[index_]->size() == 0) { ++index_; } - currIter_ = index_ == mlist_.lists_.size() + if (index_ == mlist_.lists_.size()) { + //we reached the end - we should get set to + //invalid index + index_ = std::numeric_limits::max(); + } + currIter_ = index_ == std::numeric_limits::max() ? mlist_.lists_[0]->begin() : mlist_.lists_[index_]->begin(); } diff --git a/cachelib/allocator/tests/MM2QTest.cpp b/cachelib/allocator/tests/MM2QTest.cpp index a4862c2225..17e2eb2646 100644 --- a/cachelib/allocator/tests/MM2QTest.cpp +++ b/cachelib/allocator/tests/MM2QTest.cpp @@ -218,6 +218,19 @@ void MMTypeTest::testIterate(std::vector>& nodes, } } +template +void MMTypeTest::testIterateHot(std::vector>& nodes, + Container& c) { + auto it = nodes.rbegin(); + c.withPromotionIterator([&it,&c](auto &&it2q) { + while (it2q && c.isHot(*it2q)) { + ASSERT_EQ(it2q->getId(), (*it)->getId()); + ++it2q; + ++it; + } + }); +} + template void MMTypeTest::testMatch(std::string expected, MMTypeTest::Container& c) { @@ -234,6 +247,23 @@ void MMTypeTest::testMatch(std::string expected, ASSERT_EQ(expected, actual); } +template +void MMTypeTest::testMatchHot(std::string expected, + MMTypeTest::Container& c) { + int index = -1; + std::string actual; + c.withPromotionIterator([&c,&actual,&index](auto &&it2q) { + while (it2q) { + ++index; + actual += folly::stringPrintf( + "%d:%s, ", it2q->getId(), + (c.isHot(*it2q) ? "H" : (c.isCold(*it2q) ? "C" : "W"))); + ++it2q; + } + }); + ASSERT_EQ(expected, actual); +} + TEST_F(MM2QTest, DetailedTest) { MM2Q::Config config; config.lruRefreshTime = 0; @@ -255,8 +285,11 @@ TEST_F(MM2QTest, DetailedTest) { } testIterate(nodes, c); + testIterateHot(nodes, c); testMatch("0:C, 1:C, 2:C, 3:C, 4:H, 5:H, ", c); + testMatchHot("5:H, 4:H, 3:C, 2:C, 1:C, 0:C, ", c); + // Move 3 to top of the hot cache c.recordAccess(*(nodes[4]), AccessMode::kRead); testMatch("0:C, 1:C, 2:C, 3:C, 5:H, 4:H, ", c); diff --git a/cachelib/allocator/tests/MMTypeTest.h b/cachelib/allocator/tests/MMTypeTest.h index 5c421cf4c1..6376750b35 100644 --- a/cachelib/allocator/tests/MMTypeTest.h +++ b/cachelib/allocator/tests/MMTypeTest.h @@ -147,7 +147,9 @@ class MMTypeTest : public testing::Test { void testRecordAccessBasic(Config c); void testSerializationBasic(Config c); void testIterate(std::vector>& nodes, Container& c); + void testIterateHot(std::vector>& nodes, Container& c); void testMatch(std::string expected, Container& c); + void testMatchHot(std::string expected, Container& c); size_t getListSize(const Container& c, typename MMType::LruType list); }; From cb862f72525c9b6c3ab759f6505a480dcc1bb865 Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Sun, 24 Jul 2022 17:01:05 -0400 Subject: [PATCH 8/9] ready to merge --- cachelib/allocator/MM2Q-inl.h | 8 ++++++++ cachelib/allocator/MM2Q.h | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/cachelib/allocator/MM2Q-inl.h b/cachelib/allocator/MM2Q-inl.h index 4efdcad171..2f1d538612 100644 --- a/cachelib/allocator/MM2Q-inl.h +++ b/cachelib/allocator/MM2Q-inl.h @@ -243,6 +243,14 @@ MM2Q::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), lru_.rbegin()}; } +template T::*HookPtr> +template +void +MM2Q::Container::withEvictionIterator(F&& fun) { + lruMutex_->lock_combine([this, &fun]() { + fun(Iterator{LockHolder{}, lru_.rbegin()}); + }); +} // returns the head of the hot queue for promotion template T::*HookPtr> diff --git a/cachelib/allocator/MM2Q.h b/cachelib/allocator/MM2Q.h index 5138a78421..c7310ee046 100644 --- a/cachelib/allocator/MM2Q.h +++ b/cachelib/allocator/MM2Q.h @@ -442,6 +442,11 @@ class MM2Q { // iterator passed as parameter. template void withEvictionIterator(F&& f); + + // Execute provided function under container lock. Function gets + // iterator passed as parameter. + template + void withPromotionIterator(F&& f); // get the current config as a copy Config getConfig() const; From e34c7c3b2c5c81952156bd9b0bebe5387616a94d Mon Sep 17 00:00:00 2001 From: Daniel Byrne Date: Tue, 9 Aug 2022 08:56:19 -0400 Subject: [PATCH 9/9] updated per review --- cachelib/allocator/datastruct/MultiDList-inl.h | 2 +- cachelib/allocator/datastruct/MultiDList.h | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/cachelib/allocator/datastruct/MultiDList-inl.h b/cachelib/allocator/datastruct/MultiDList-inl.h index f06e5df587..4cbd584815 100644 --- a/cachelib/allocator/datastruct/MultiDList-inl.h +++ b/cachelib/allocator/datastruct/MultiDList-inl.h @@ -130,7 +130,7 @@ typename MultiDList::Iterator MultiDList::rbegin( if (listIdx >= lists_.size()) { throw std::invalid_argument("Invalid list index for MultiDList iterator."); } - return MultiDList::Iterator(*this, listIdx); + return MultiDList::Iterator(*this, listIdx, false); } template T::*HookPtr> diff --git a/cachelib/allocator/datastruct/MultiDList.h b/cachelib/allocator/datastruct/MultiDList.h index 68a94f2aa4..8063cd5471 100644 --- a/cachelib/allocator/datastruct/MultiDList.h +++ b/cachelib/allocator/datastruct/MultiDList.h @@ -109,16 +109,6 @@ class MultiDList { XDCHECK(index_ == kInvalidIndex || currIter_.get() != nullptr); } - explicit Iterator(const MultiDList& mlist, - size_t listIdx) noexcept - : currIter_(mlist.lists_[mlist.lists_.size() - 1]->rbegin()), - mlist_(mlist) { - XDCHECK_LT(listIdx, mlist.lists_.size()); - initToValidRBeginFrom(listIdx); - // We should either point to an element or the end() iterator - // which has an invalid index_. - XDCHECK(index_ == kInvalidIndex || currIter_.get() != nullptr); - } explicit Iterator(const MultiDList& mlist, size_t listIdx, bool head) noexcept : currIter_(mlist.lists_[mlist.lists_.size() - 1]->rbegin()),