Properly move items stored in inline Handle buffers

Summary: Don't just memcpy, actually move the items stored in the inline Handle buffer.  This allows properly moving items, including those with self-references (the CacheItem sub-class needs to know how to move).

Reviewed By: pbhandar2

Differential Revision: D100844276

fbshipit-source-id: 3bbb9b80d5e85d7b55418bdda9fc732cfa9e0fd0

Author: rlyerly

cachelib/interface/CacheItem.h

@@ -30,11 +30,13 @@ class CacheItem {
   CacheItem() = default;
   virtual ~CacheItem() = default;
 
-  // CacheItem is not copyable or movable
+  // CacheItem is not copyable
   CacheItem(const CacheItem&) = delete;
   CacheItem& operator=(const CacheItem&) = delete;
-  CacheItem(CacheItem&&) = delete;
-  CacheItem& operator=(CacheItem&&) = delete;
+
+  // CacheItem is movable
+  CacheItem(CacheItem&&) noexcept = default;
+  CacheItem& operator=(CacheItem&&) noexcept = default;
 
   // ------------------------------ Interface ------------------------------ //
 
@@ -102,6 +104,11 @@ class CacheItem {
    * @return total size of cache item
    */
   virtual uint32_t getTotalSize() const noexcept = 0;
+
+ private:
+  virtual void move(void* dest) noexcept = 0;
+
+  friend class Handle;
 };
 
 } // namespace facebook::cachelib::interface

cachelib/interface/Handle.cpp

@@ -18,8 +18,6 @@
 
 #include <folly/logging/xlog.h>
 
-#include <cstring>
-
 #include "cachelib/interface/CacheComponent.h"
 
 namespace facebook::cachelib::interface {
@@ -38,7 +36,8 @@ Handle::Handle(Handle&& other) noexcept
     : cache_(other.cache_), inserted_(other.inserted_) {
   XDCHECK_NE(cache_, nullptr) << "Invalid handle";
   if (other.item_ == reinterpret_cast<CacheItem*>(other.buf_)) {
-    std::memcpy(buf_, other.buf_, kInlineBufSize);
+    other.item_->move(buf_);
+    other.item_->~CacheItem();
     item_ = reinterpret_cast<CacheItem*>(buf_);
   } else {
     item_ = other.item_;

cachelib/interface/Handle.h

@@ -72,10 +72,6 @@ class Handle {
    * Note: the caller is responsible for incrementing the item's refcount after
    * allocating it in the inline buffer
    *
-   * Note: the Handle move constructor relocates inline items via memcpy. This
-   * is safe only if the item type has no self-referential pointers (i.e.,
-   * members that point to other members or to `this`).
-   *
    * @param cache the cache component that owns the cache item
    * @param inserted whether the cache item has been inserted into cache
    */

cachelib/interface/components/FlashCacheComponent.cpp

@@ -147,6 +147,10 @@ class FlashCacheItem : public CacheItem {
   // Allocation and findToWrite paths use MutableBufferView (points into region
   // buffer). Find path uses Buffer (owned copy from device read).
   std::variant<Buffer, MutableBufferView> data_;
+
+  void move(void* dest) noexcept override {
+    new (dest) FlashCacheItem(std::move(*this));
+  }
 };
 
 static_assert(sizeof(FlashCacheItem) <= Handle::kInlineBufSize,
@@ -656,6 +660,10 @@ class ConsistentFlashCacheItem : public FlashCacheItem {
 
  private:
   utils::ShardedSerializer::WriteLock lock_;
+
+  void move(void* dest) noexcept override {
+    new (dest) ConsistentFlashCacheItem(std::move(*this));
+  }
 };
 
 static_assert(sizeof(ConsistentFlashCacheItem) <= Handle::kInlineBufSize,

cachelib/interface/components/RAMCacheComponent.cpp

@@ -91,6 +91,11 @@ class RAMCacheItem : public interface::CacheItem {
   LruCacheItem* item_;
 
   explicit RAMCacheItem(LruCacheItem* item) : item_(item) {}
+
+  void move(void* /* dest */) noexcept override {
+    // We don't use the inline Handle buffer, move() should never be called
+    XLOG(FATAL) << "Should not use RAMCacheItem::move()";
+  }
 };
 // RAMCacheItem should only contain vtable and LruCacheItem pointers
 static_assert(sizeof(RAMCacheItem) == (2 * sizeof(void*)));

cachelib/interface/tests/InterfaceTest.cpp

@@ -55,6 +55,10 @@ class TestCacheItem : public CacheItem {
   std::string key_;
   std::vector<uint8_t> memory_;
   std::atomic_size_t refCount_;
+
+  void move(void* /* dest */) noexcept override {
+    XLOG(FATAL) << "Should not use TestCacheItem::move()";
+  }
 };
 
 class TestCacheComponent : public CacheComponent {