MB-40531: Change order of vbuckets paging visitor visits

Testing after http://review.couchbase.org/c/kv_engine/+/133197
uncovered unexpected increases in cache miss ratio for a working set
which fits within the quota comfortably. This occurred after some time,
despite the workload reading the same keys in a loop.

Investigation discovered a large disparity in per-vbucket residency
(see MB for info).

Vbuckets with a lower vbid could be <20% resident while other vbuckets
remained fully resident. The fully resident vbs logically contain
resident cold data which has never been evicted despite never being
accessed.

This patch changes the order the paging visitor visits vbuckets. They
are now visited:

 * replicas before other vbucket states
 * from highest pageable memory to lowest (within a given vb state)

Testing demonstrates a lower standard deviation of the distribution of
per-vbucket residency ratios.

Change-Id: Ia8a0fa368821518a25659da03852076b70ac478a
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/136628
Tested-by: James Harrison <[email protected]>
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Paolo Cocchi <[email protected]>

Author: jameseh96

engines/ep/src/paging_visitor.cc

@@ -38,6 +38,7 @@
 
 #include <phosphor/phosphor.h>
 #include <memory>
+#include <utility>
 
 static const size_t MAX_PERSISTENCE_QUEUE_SIZE = 1000000;
 
@@ -331,6 +332,32 @@ void PagingVisitor::complete() {
     }
 }
 
+std::function<bool(const Vbid&, const Vbid&)>
+PagingVisitor::getVBucketComparator() const {
+    // Get the pageable mem used of each vb _once_ and cache it.
+    // Fetching these values repeatedly in the comparator could cause issues as
+    // the values can change _during_ a given sort call.
+
+    std::map<Vbid, size_t> pageableMemUsed;
+
+    for (const auto& vbid : store.getVBuckets().getBuckets()) {
+        auto vb = store.getVBucket(vbid);
+        pageableMemUsed[vbid] = vb ? vb->getPageableMemUsage() : 0;
+    }
+
+    return [pageableMemUsed = std::move(pageableMemUsed), &store = store](
+                   const Vbid& a, const Vbid& b) mutable {
+        auto vbA = store.getVBucket(a);
+        auto vbB = store.getVBucket(b);
+        bool aReplica = vbA && vbA->getState() == vbucket_state_replica;
+        bool bReplica = vbB && vbB->getState() == vbucket_state_replica;
+        // sort replicas before all other vbucket states, then sort by
+        // pageableMemUsed
+        return std::make_pair(aReplica, pageableMemUsed[a]) >
+               std::make_pair(bReplica, pageableMemUsed[b]);
+    };
+}
+
 // Removes checkpoints that are both closed and unreferenced, thereby
 // freeing the associated memory.
 // @param vb  The vbucket whose eligible checkpoints are removed from.

engines/ep/src/paging_visitor.h

@@ -84,6 +84,9 @@ class PagingVisitor : public PausableVBucketVisitor, public HashTableVisitor {
 
     void complete() override;
 
+    std::function<bool(const Vbid&, const Vbid&)> getVBucketComparator()
+            const override;
+
     /**
      * Override the setUpHashBucketVisit method so that we can acquire a
      * Collections::VB::Manifest::ReadHandle. Required if we evict any items in

engines/ep/tests/mock/mock_paging_visitor.h

@@ -21,6 +21,8 @@
 #include "paging_visitor.h"
 #include <string>
 
+#include <folly/portability/GMock.h>
+
 /**
  * Mock PagingVisitor class.  Provide access to ItemEviction data structure.
  */
@@ -44,6 +46,11 @@ class MockPagingVisitor : public PagingVisitor {
                         vbFilter,
                         agePercentage,
                         freqCounterAgeThreshold) {
+        using namespace testing;
+        ON_CALL(*this, visitBucket(_))
+                .WillByDefault(Invoke([this](const VBucketPtr& vb) {
+                    PagingVisitor::visitBucket(vb);
+                }));
     }
 
     ItemEviction& getItemEviction() {
@@ -61,4 +68,6 @@ class MockPagingVisitor : public PagingVisitor {
     void setCurrentBucket(VBucketPtr _currentBucket) {
         currentBucket = _currentBucket;
     }
+
+    MOCK_METHOD1(visitBucket, void(const VBucketPtr&));
 };

engines/ep/tests/module_tests/item_pager_test.cc

@@ -1000,6 +1000,98 @@ TEST_P(STItemPagerTest, ActiveEvictedIfReplicaEvictionInsufficient) {
     EXPECT_LT(replicaRR, 5);
 }
 
+MATCHER_P(VBPtrVbidMatcher,
+          vbid,
+          "Check the provided VBucket pointer points to a vbucket with the "
+          "given vbid") {
+    return arg && arg->getId() == vbid;
+}
+
+TEST_P(STItemPagerTest, ItemPagerEvictionOrder) {
+    // MB-40531: Test that the paging visitor visits vbuckets ordered by:
+    // * replica vbuckets before active/pending
+    // * highest pageableMemUsage first
+
+    std::vector<Vbid> activeVBs = {Vbid(0), Vbid(1)};
+    std::vector<Vbid> replicaVBs = {Vbid(2), Vbid(3)};
+    std::vector<Vbid> allVBs = {Vbid(0), Vbid(1), Vbid(2), Vbid(3)};
+
+    // Set all vbs, including replicaVBs, as active initially (so we can
+    // populate them easily).
+    for (const auto& vbid : allVBs) {
+        setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
+    }
+
+    // populate the vbuckets with different numbers of equal sized items.
+    // Pageable mem usage should be proportional to the number of items.
+    // The visitor should therefore visit them in the commented order
+    for (const auto& setup : std::vector<std::pair<Vbid, int>>{
+                 {activeVBs[0], 30}, // 4th
+                 {activeVBs[1], 40}, // 3rd
+                 {replicaVBs[0], 20}, // 1st
+                 {replicaVBs[1], 10}, // 2nd
+         }) {
+        Vbid vbid;
+        int itemCount;
+        std::tie(vbid, itemCount) = setup;
+        for (int i = 0; i < itemCount; ++i) {
+            auto key = makeStoredDocKey("key-" + std::to_string(i));
+            auto item = make_item(vbid, key, std::string(100, 'x'));
+            ASSERT_EQ(ENGINE_SUCCESS, storeItem(item));
+        }
+    }
+
+    // flush all vbs
+    for (const auto& vbid : allVBs) {
+        flushDirectlyIfPersistent(Vbid(vbid));
+    }
+
+    // flip the replica vbs to the correct state
+    setVBucketStateAndRunPersistTask(replicaVBs[0], vbucket_state_replica);
+    setVBucketStateAndRunPersistTask(replicaVBs[1], vbucket_state_replica);
+
+    auto& stats = engine->getEpStats();
+    auto available = std::make_shared<std::atomic<bool>>();
+    auto& config = engine->getConfiguration();
+
+    auto pv = std::make_unique<MockPagingVisitor>(
+            *store,
+            stats,
+            EvictionRatios{1.0 /*active*/, 1.0 /*replica*/},
+            available,
+            ITEM_PAGER,
+            false,
+            VBucketFilter(ephemeral() ? activeVBs : allVBs),
+            config.getItemEvictionAgePercentage(),
+            config.getItemEvictionFreqCounterAgeThreshold());
+
+    using namespace testing;
+    InSequence s;
+
+    // set up expectations _before_ moving the pv into the VBCBAdaptor
+    if (!ephemeral()) {
+        // ephemeral cannot evict from replicas, so the pager visitor won't
+        // visit them at all.
+        EXPECT_CALL(*pv, visitBucket(VBPtrVbidMatcher(replicaVBs[0])));
+        EXPECT_CALL(*pv, visitBucket(VBPtrVbidMatcher(replicaVBs[1])));
+    }
+    EXPECT_CALL(*pv, visitBucket(VBPtrVbidMatcher(activeVBs[1])));
+    EXPECT_CALL(*pv, visitBucket(VBPtrVbidMatcher(activeVBs[0])));
+
+    auto label = "Item pager";
+    auto taskid = TaskId::ItemPagerVisitor;
+    VBCBAdaptor task(store,
+                     taskid,
+                     std::move(pv),
+                     label,
+                     /*shutdown*/ false);
+
+    // call the run method repeatedly until it returns false, indicating the
+    // visitor is definitely done, rather than paused
+    while (task.run())
+        ;
+}
+
 /**
  * Test fixture for Ephemeral-only item pager tests.
  */