MB-29333: Only decay frequency for items that are eligible for eviction

The original patch did not address the issue correctly.
(see Change-Id: I95d328238176a97d7ac9e80c268c639bbb647953)

It only decrements the frequency count if the item is NOT elgible for
eviction (from the max u8int_t value of 255).  This behaviour is
wrong for two reasons:

1)  We want to decrement the frequency count of items that were eligible
for eviction except that their frequency count is above the threshold.

2)  We don't want to decrement the frequency count that are not eligible
for eviction (regardless of what their frequency count is).

This patch corrects the behaviour to only decrement the fequency count
of items that are eligible for eviction except that their frequency
count is above the threshold.

The original associated test was also found to not exercise the newly
added code.  This has been addressed and an additional test added.

Change-Id: I508964dbf9aefd8055ee7e654caca10d1450aa5b
Reviewed-on: http://review.couchbase.org/93338
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: owend74

engines/ep/src/item_pager.cc

@@ -55,6 +55,7 @@ PagingVisitor::PagingVisitor(KVBucket& s,
                              bool _isEphemeral)
     : VBucketVisitor(vbFilter),
       ejected(0),
+      freqCounterThreshold(0),
       store(s),
       stats(st),
       percent(pcnt),
@@ -67,8 +68,7 @@ PagingVisitor::PagingVisitor(KVBucket& s,
       wasHighMemoryUsage(s.isMemoryUsageTooHigh()),
       taskStart(ProcessClock::now()),
       pager_phase(phase),
-      isEphemeral(_isEphemeral),
-      freqCounterThreshold(0) {
+      isEphemeral(_isEphemeral) {
     }
 
     bool PagingVisitor::visit(const HashTable::HashBucketLock& lh,
@@ -113,7 +113,6 @@ PagingVisitor::PagingVisitor(KVBucket& s,
              * add it to the histogram we want to use the original value.
              */
             auto storedValueFreqCounter = v.getFreqCounterValue();
-            bool evicted = true;
 
             if (storedValueFreqCounter <= freqCounterThreshold) {
                 /*
@@ -129,28 +128,28 @@ PagingVisitor::PagingVisitor(KVBucket& s,
                  */
                 if (!doEviction(lh, &v)) {
                     storedValueFreqCounter = std::numeric_limits<uint8_t>::max();
-                    evicted = false;
                 }
             } else {
                 // If the storedValue is NOT eligible for eviction then
                 // we want to add the maximum value (255).
                 if (!currentBucket->eligibleToPageOut(lh, v)) {
                     storedValueFreqCounter = std::numeric_limits<uint8_t>::max();
-                    evicted = false;
+                } else {
+                    /*
+                     * MB-29333 - For items that we have visited and did not
+                     * evict just because their frequency counter was too high,
+                     * the frequency counter must be decayed by 1 to
+                     * ensure that they will get evicted if repeatedly
+                     * visited (and assuming their frequency counter is not
+                     * incremented in between visits of the item pager).
+                     */
+                    if (storedValueFreqCounter > 0) {
+                        v.setFreqCounterValue(storedValueFreqCounter - 1);
+                    }
                 }
             }
             itemEviction.addValueToFreqHistogram(storedValueFreqCounter);
 
-            /*
-             * MB-29333 - For items that we have visited but not evicted the
-             * storedValue frequency counter must be decayed by 1 to ensure
-             * that an item will get evicted if it is repeatedly visited (and
-             * is eligible to be evicted).
-             */
-            if (!evicted && storedValueFreqCounter > 0) {
-                v.setFreqCounterValue(storedValueFreqCounter - 1);
-            }
-
             // Whilst we are learning it is worth always updating the
             // threshold. We also want to update the threshold at periodic
             // intervals.

engines/ep/src/paging_visitor.h

@@ -93,6 +93,10 @@ class PagingVisitor : public VBucketVisitor, public HashTableVisitor {
     // The current vbucket that the eviction algorithm is operating on.
     VBucketPtr currentBucket;
 
+    // The frequency counter threshold that is used to determine whether we
+    // should evict items from the hash table.
+    uint16_t freqCounterThreshold;
+
 private:
     // Removes checkpoints that are both closed and unreferenced, thereby
     // freeing the associated memory.
@@ -121,8 +125,4 @@ class PagingVisitor : public VBucketVisitor, public HashTableVisitor {
     // Indicates whether the vbucket we are visiting is from an ephemeral
     // bucket.
     bool isEphemeral;
-
-    // The frequency counter threshold that is used to determine whether we
-    // should evict items from the hash table.
-    uint16_t freqCounterThreshold;
 };

engines/ep/tests/mock/mock_paging_visitor.h

@@ -56,6 +56,10 @@ class MockPagingVisitor : public PagingVisitor {
         return ejected;
     }
 
+    void setFreqCounterThreshold(uint16_t threshold) {
+        freqCounterThreshold = threshold;
+    }
+
     void setCurrentBucket(VBucketPtr _currentBucket) {
         currentBucket = _currentBucket;
     }

engines/ep/tests/module_tests/item_pager_test.cc

@@ -598,14 +598,70 @@ TEST_P(STItemPagerTest, decayByOne) {
     }
     int iterationCount = 0;
     while ((pv->getEjected() == 0) &&
-           iterationCount < ItemEviction::initialFreqCount) {
+           iterationCount <= ItemEviction::initialFreqCount) {
+        pv->setFreqCounterThreshold(0);
         VBucketPtr vb = store->getVBucket(vbid);
         vb->ht.visit(*pv);
         iterationCount++;
     }
     EXPECT_EQ(1, pv->getEjected());
 }
 
+/**
+ * MB-29333:  Test that if a vbucket contains a single document with an
+ * execution frequency of ItemEviction::initialFreqCount, but the document
+ * is not eligible for eviction (due to being replica in ephemeral case and
+ * not flushed in the persistent case) check that its frequency count is not
+ * decremented.
+ */
+TEST_P(STItemPagerTest, doNotDecayIfCannotEvict) {
+    const std::string value(512, 'x'); // 512B value to use for documents.
+    auto key = makeStoredDocKey("xxx_0");
+    auto item = make_item(vbid, key, value, time_t(0));
+    storeItem(item);
+
+    std::shared_ptr<std::atomic<bool>> available;
+    std::atomic<item_pager_phase> phase{ACTIVE_AND_PENDING_ONLY};
+    bool isEphemeral = std::get<0>(GetParam()) == "ephemeral";
+    std::unique_ptr<MockPagingVisitor> pv =
+            std::make_unique<MockPagingVisitor>(*engine->getKVBucket(),
+                                                engine->getEpStats(),
+                                                10.0,
+                                                available,
+                                                ITEM_PAGER,
+                                                false,
+                                                0.5,
+                                                VBucketFilter(),
+                                                &phase,
+                                                isEphemeral);
+
+    pv->setCurrentBucket(engine->getKVBucket()->getVBucket(vbid));
+    store->setVBucketState(vbid, vbucket_state_replica, false);
+    for (int ii = 0; ii <= ItemEviction::initialFreqCount; ii++) {
+        pv->setFreqCounterThreshold(0);
+        pv->getItemEviction().reset();
+        VBucketPtr vb = store->getVBucket(vbid);
+        vb->ht.visit(*pv);
+    }
+
+    // Now make the document eligible for eviction.
+    store->setVBucketState(vbid, vbucket_state_active, false);
+    if (std::get<0>(GetParam()) == "persistent") {
+        getEPBucket().flushVBucket(vbid);
+    }
+
+    // Check still not be able to evict, because the frequency count is still
+    // at ItemEviction::initialFreqCount
+    pv->setFreqCounterThreshold(0);
+    pv->getItemEviction().reset();
+    VBucketPtr vb = store->getVBucket(vbid);
+    vb->ht.visit(*pv);
+    auto initialFreqCount = ItemEviction::initialFreqCount;
+    EXPECT_EQ(initialFreqCount, pv->getItemEviction().getFreqThreshold(100.0));
+    EXPECT_EQ(0, pv->getEjected());
+
+}
+
 /**
  * Test fixture for Ephemeral-only item pager tests.
  */