Add expired check for random item sampling

Summary:
Previously, we were not checking the expiration for the sampled item, which could give an incorrect
view of the item compositions. This could be more of a problem for items sampled from NVM cache
because NVM cache is deferring the evictions for expired items until the reclaims. This fixes the
issue by adding a check for expiration for item sampling for both DRAM and NVM items.

Reviewed By: udippant

Differential Revision: D52179872

fbshipit-source-id: b721f2b55fefd694ce84e99f6270f1de462a672e

cachelib/allocator/CacheAllocator-inl.h

@@ -1897,7 +1897,7 @@ CacheAllocator<CacheTrait>::getSampleItem() {
 
   // Sampling from DRAM cache
   auto item = reinterpret_cast<const Item*>(allocator_->getRandomAlloc());
-  if (!item) {
+  if (!item || UNLIKELY(item->isExpired())) {
     return SampleItem{false /* fromNvm */};
   }
 

cachelib/allocator/nvmcache/NvmCache-inl.h

@@ -430,13 +430,14 @@ NvmCache<C>::NvmCache(C& c,
                       const ItemDestructor& itemDestructor)
     : config_(config.validateAndSetDefaults()),
       cache_(c),
+      checkExpired_([](navy::BufferView v) -> bool {
+        const auto& nvmItem = *reinterpret_cast<const NvmItem*>(v.data());
+        return nvmItem.isExpired();
+      }),
       itemDestructor_(itemDestructor) {
   navyCache_ = createNavyCache(
       config_.navyConfig,
-      [](navy::BufferView v) -> bool {
-        const auto& nvmItem = *reinterpret_cast<const NvmItem*>(v.data());
-        return nvmItem.isExpired();
-      },
+      checkExpired_,
       [this](HashedKey hk, navy::BufferView v, navy::DestructorEvent e) {
         this->evictCB(hk, v, e);
       },
@@ -849,7 +850,7 @@ template <typename C>
 typename NvmCache<C>::SampleItem NvmCache<C>::getSampleItem() {
   navy::Buffer value;
   auto [status, keyStr] = navyCache_->getRandomAlloc(value);
-  if (status != navy::Status::Ok) {
+  if (status != navy::Status::Ok || checkExpired_(value.view())) {
     return SampleItem{true /* fromNvm */};
   }
 

cachelib/allocator/nvmcache/NvmCache.h

@@ -437,6 +437,9 @@ class NvmCache {
 
   static constexpr size_t kShards = 8192;
 
+  // a function to check if an item is expired
+  const navy::ExpiredCheck checkExpired_;
+
   // a map of all pending fills to prevent thundering herds
   struct {
     alignas(folly::hardware_destructive_interference_size) FillMap fills_;

cachelib/allocator/nvmcache/tests/NvmCacheTests.cpp

@@ -2316,11 +2316,13 @@ TEST_F(NvmCacheTest, testSampleItem) {
   auto pid = this->poolId();
 
   size_t nKeys = 0;
+  static constexpr unsigned kEvenKeyTTL = 5;
   // Insert items until either RAM or NVM cache is full
   for (; numEvicted == 0 && nKeys < numMax; nKeys++) {
+    unsigned ttl = nKeys % 2 == 0 ? kEvenKeyTTL : 0;
     auto key = folly::sformat("key{}", nKeys);
     // the pool's allocsize is
-    auto it = cache.allocate(pid, key, 16 * 1024);
+    auto it = cache.allocate(pid, key, 16 * 1024, ttl);
     ASSERT_NE(nullptr, it);
     cache.insertOrReplace(it);
 
@@ -2331,14 +2333,15 @@ TEST_F(NvmCacheTest, testSampleItem) {
     ASSERT_TRUE(this->pushToNvmCacheFromRamForTesting(key));
   }
 
-  // remove even numbered keys to make holes
-  for (size_t i = 0; i < nKeys; i += 2) {
-    auto key = folly::sformat("key{}", i);
-    cache.remove(key);
-  }
   // wait for async remove finish
   cache.flushNvmCache();
 
+  XLOGF(INFO,
+        "Wait {}s until all items with even numbered keys are expired by TTL",
+        kEvenKeyTTL + 1);
+  /* sleep override */ std::this_thread::sleep_for(
+      std::chrono::seconds(kEvenKeyTTL + 1));
+
   {
     std::unique_lock<std::mutex> l(mtx);
     nKeys = cachedKeys.size();
@@ -2349,10 +2352,19 @@ TEST_F(NvmCacheTest, testSampleItem) {
   for (size_t i = 0; i < nKeys * 10; i++) {
     auto sample = cache.getSampleItem();
     if (sample.isValid()) {
+      auto keyStr = sample->getKey().toString();
       {
         std::unique_lock<std::mutex> l(mtx);
-        ASSERT_EQ(1, cachedKeys.count(sample->getKey().toString()));
+        ASSERT_EQ(1, cachedKeys.count(keyStr));
       }
+
+      unsigned idx;
+      ASSERT_GT(std::sscanf(keyStr.c_str(), "key%u", &idx), 0);
+      ASSERT_TRUE(idx % 2 != 0) << fmt::format(
+          "Error: expired item with key ({}) and nvm ({}) returned",
+          keyStr,
+          sample.isNvmItem());
+
       if (sample.isNvmItem()) {
         numNvm++;
       } else {