Report successful ObjectCache replacements in destructor data

Summary: ObjectCache reports explicit removal, TTL reaping, and insertOrReplace through the same kRemoved destructor context. Add removedBySuccessfulReplacement so consumers can suppress work only when insertOrReplace actually installed a newer object. The old cache item is marked after insertOrReplace succeeds while its returned handle prevents the destructor from running. The marker uses an available bit in the existing atomic item flags, so the persisted item layout does not change across warm rolls.

Reviewed By: rlyerly

Differential Revision: D113092104

fbshipit-source-id: 87b4df52fd39e80e59bc5f8b54475ad332380f2e

Author: Prateek Goyal

cachelib/allocator/CacheAllocator.h

@@ -3856,6 +3856,7 @@ CacheAllocator<CacheTrait>::insertOrReplace(const WriteHandle& handle) {
   // Remove from LRU as well if we do have a handle of old item
   if (replaced) {
     stats_.numInsertOrReplaceReplaced.inc();
+    replaced->markRemovedByReplacement();
     removeFromMMContainer(*replaced);
   } else {
     stats_.numInsertOrReplaceInserted.inc();

cachelib/allocator/CacheItem.h

@@ -264,6 +264,9 @@ class CACHELIB_PACKED_ATTR CacheItem {
   void markNvmLargeItem() noexcept;
   bool isNvmLargeItem() const noexcept;
 
+  void markRemovedByReplacement() noexcept;
+  bool isRemovedByReplacement() const noexcept;
+
   /**
    * Function to set the timestamp for when to expire an item
    *
@@ -909,6 +912,16 @@ bool CacheItem<CacheTrait>::isNvmLargeItem() const noexcept {
   return ref_.isNvmLargeItem();
 }
 
+template <typename CacheTrait>
+void CacheItem<CacheTrait>::markRemovedByReplacement() noexcept {
+  ref_.markRemovedByReplacement();
+}
+
+template <typename CacheTrait>
+bool CacheItem<CacheTrait>::isRemovedByReplacement() const noexcept {
+  return ref_.isRemovedByReplacement();
+}
+
 template <typename CacheTrait>
 void CacheItem<CacheTrait>::markIsChainedItem() noexcept {
   XDCHECK(!hasChainedItem());

cachelib/allocator/Refcount.h

@@ -122,6 +122,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
     // Set during NVM promotion when the exact NVM buffer size is known.
     kNvmLargeItem,
 
+    // Item was removed by a successful replacement.
+    kRemovedByReplacement,
+
     // Unused. This is just to indciate the maximum number of flags
     kFlagMax,
   };
@@ -455,6 +458,13 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
   void markNvmLargeItem() noexcept { return setFlag<kNvmLargeItem>(); }
   bool isNvmLargeItem() const noexcept { return isFlagSet<kNvmLargeItem>(); }
 
+  void markRemovedByReplacement() noexcept {
+    return setFlag<kRemovedByReplacement>();
+  }
+  bool isRemovedByReplacement() const noexcept {
+    return isFlagSet<kRemovedByReplacement>();
+  }
+
   // Whether or not an item is completely drained of access
   // Refcount is 0 and the item is not linked, accessible, nor exclusive
   bool isDrained() const noexcept { return getRefWithAccessAndAdmin() == 0; }

cachelib/object_cache/ObjectCache.h

@@ -51,7 +51,9 @@ struct ObjectCacheItem {
 enum class ObjectCacheDestructorContext {
   // evicted from cache
   kEvicted,
-  // removed by user calling remove()/insertOrReplace() or due to expired
+  // removed by user calling remove()/insertOrReplace() or due to expired.
+  // Use ObjectCacheDestructorData::removedBySuccessfulReplacement to
+  // distinguish a successful insertOrReplace() from other removals.
   kRemoved,
   // unknown cases
   kUnknown,
@@ -63,13 +65,15 @@ struct ObjectCacheDestructorData {
                             const KAllocation::Key& k,
                             uint32_t expiryTime,
                             uint32_t creationTime,
-                            uint32_t lastAccessTime)
+                            uint32_t lastAccessTime,
+                            bool removedBySuccessfulReplacement = false)
       : context(ctx),
         objectPtr(ptr),
         key(k),
         expiryTime(expiryTime),
         creationTime(creationTime),
-        lastAccessTime(lastAccessTime) {}
+        lastAccessTime(lastAccessTime),
+        removedBySuccessfulReplacement(removedBySuccessfulReplacement) {}
 
   // release the evicted/removed/expired object memory
   template <typename T>
@@ -94,6 +98,10 @@ struct ObjectCacheDestructorData {
 
   // the last time this object was accessed
   uint32_t lastAccessTime;
+
+  // Whether the object was removed by an insertOrReplace() that successfully
+  // installed its replacement. Always false for kEvicted/kUnknown.
+  bool removedBySuccessfulReplacement;
 };
 
 // Information about cache memory capacity calculated from configuration
@@ -792,6 +800,9 @@ void ObjectCache<AllocatorT>::init() {
           auto& item = data.item;
 
           auto itemPtr = getAlignedItemPtr(item.getMemory());
+          const bool removedBySuccessfulReplacement =
+              ctx == ObjectCacheDestructorContext::kRemoved &&
+              item.isRemovedByReplacement();
 
           SCOPE_EXIT {
             if (config_.objectSizeTrackingEnabled) {
@@ -804,7 +815,8 @@ void ObjectCache<AllocatorT>::init() {
             // execute user defined item destructor
             config_.itemDestructor(ObjectCacheDestructorData(
                 ctx, itemPtr->objectPtr, item.getKey(), item.getExpiryTime(),
-                item.getCreationTime(), item.getLastAccessTime()));
+                item.getCreationTime(), item.getLastAccessTime(),
+                removedBySuccessfulReplacement));
           };
         });
   } else {
@@ -822,6 +834,9 @@ void ObjectCache<AllocatorT>::init() {
       auto& item = data.item;
 
       auto itemPtr = getAlignedItemPtr(item.getMemory());
+      const bool removedBySuccessfulReplacement =
+          ctx == ObjectCacheDestructorContext::kRemoved &&
+          item.isRemovedByReplacement();
 
       SCOPE_EXIT {
         if (config_.objectSizeTrackingEnabled) {
@@ -834,7 +849,8 @@ void ObjectCache<AllocatorT>::init() {
         // execute user defined item destructor
         config_.removeCb(ObjectCacheDestructorData(
             ctx, itemPtr->objectPtr, item.getKey(), item.getExpiryTime(),
-            item.getCreationTime(), item.getLastAccessTime()));
+            item.getCreationTime(), item.getLastAccessTime(),
+            removedBySuccessfulReplacement));
       };
     });
   }

cachelib/object_cache/tests/ObjectCacheTest.cpp

@@ -19,6 +19,7 @@
 #include <folly/Random.h>
 #include <gtest/gtest.h>
 
+#include <atomic>
 #include <cstddef>
 #include <memory>
 
@@ -407,6 +408,48 @@ class ObjectCacheTest : public ::testing::Test {
     ASSERT_EQ(nullptr, found2);
   }
 
+  void testSuccessfulReplacementFlag() {
+    std::atomic<int> numReplaced{0};
+    std::atomic<int> numNotReplaced{0};
+    ObjectCacheConfig config;
+    config.setCacheName("test")
+        .setCacheCapacity(10'000)
+        .setItemReaperInterval(std::chrono::seconds{1})
+        .setItemDestructor([&](ObjectCacheDestructorData data) {
+          EXPECT_EQ(data.context, ObjectCacheDestructorContext::kRemoved);
+          if (data.removedBySuccessfulReplacement) {
+            ++numReplaced;
+          } else {
+            ++numNotReplaced;
+          }
+          data.deleteObject<Foo>();
+        });
+    auto objcache = ObjectCache::create(config);
+
+    objcache->insertOrReplace("k", std::make_unique<Foo>(), 0 /*object size*/,
+                              1000 /*ttlSecs*/);
+    auto heldOldObject = objcache->template find<Foo>("k");
+    auto [status, newObject, replacedObject] = objcache->insertOrReplace(
+        "k", std::make_unique<Foo>(), 0 /*object size*/, 1000 /*ttlSecs*/);
+    ASSERT_EQ(ObjectCache::AllocStatus::kSuccess, status);
+
+    heldOldObject.reset();
+    EXPECT_EQ(0, numReplaced.load());
+    replacedObject.reset();
+    EXPECT_EQ(1, numReplaced.load());
+    EXPECT_EQ(0, numNotReplaced.load());
+
+    ASSERT_TRUE(objcache->remove("k"));
+    newObject.reset();
+    EXPECT_EQ(1, numReplaced.load());
+    EXPECT_EQ(1, numNotReplaced.load());
+
+    objcache->insertOrReplace("e", std::make_unique<Foo>(), 0 /*object size*/,
+                              1 /*ttlSecs*/);
+    ASSERT_EVENTUALLY_TRUE([&] { return numNotReplaced.load() == 2; });
+    EXPECT_EQ(1, numReplaced.load());
+  }
+
   void testExpirationWithCustomizedReaper() {
     ObjectCacheConfig config;
     config.setCacheName("test")
@@ -2025,6 +2068,9 @@ TYPED_TEST(ObjectCacheTest, UserItemDestructor) {
   this->testUserItemDestructor();
 }
 TYPED_TEST(ObjectCacheTest, Expiration) { this->testExpiration(); }
+TYPED_TEST(ObjectCacheTest, SuccessfulReplacementFlag) {
+  this->testSuccessfulReplacementFlag();
+}
 TYPED_TEST(ObjectCacheTest, ExpirationWithCustomizedReaper) {
   this->testExpirationWithCustomizedReaper();
 }