Small refactors in component API

Summary:
Add some top level comments about CacheComponent & CacheItem.  Add notes for the following important details:

- Most APIs take string views of keys, the user needs to ensure the string view is constant until the API is finished
- Components must call the item destructor in `release()`
- Children of CacheItem must call the item destructor in `move()` (only needed if the inline buffer is used)

The latter two clarifies responsibility for who calls the cache item destructor.

Also remove setting handle.item_ to nullptr in the destructor, it's unnecessary because it only compensates for buggy implementations that double-free.

Reviewed By: AlnisM

Differential Revision: D102372138

fbshipit-source-id: 342fa1b347c24e2ec9f59b25f7144253c2732f0f

Author: rlyerly

cachelib/interface/CacheComponent.h

@@ -25,6 +25,17 @@
 
 namespace facebook::cachelib::interface {
 
+/**
+ * A cache that manages a section of storage.
+ *
+ * The interface is as generic as possible to give components maximum
+ * implementation flexibility.  APIs are coro-compatible so components can run
+ * work asynchronously (some components may run synchronously under the hood).
+ *
+ * NOTE: most APIs take the key argument as a Key (essentially a string view).
+ * This allows maximum performance, but users *must* ensure the string view is
+ * constant across coroutine suspension points.
+ */
 class CacheComponent {
  public:
   virtual ~CacheComponent() = default;
@@ -177,6 +188,8 @@ class CacheComponent {
    * executes any necessary callbacks. Only meant to be called by the component
    * or cache item handles.
    *
+   * NOTE: the component is responsible for destroying the item!
+   *
    * @param item the item to release
    * @param inserted whether the item was previously inserted into the cache
    * (callbacks are typically only called when it was inserted)

cachelib/interface/CacheItem.h

@@ -106,6 +106,14 @@ class CacheItem {
   virtual uint32_t getTotalSize() const noexcept = 0;
 
  private:
+  /**
+   * Move the cache item to a new location in memory. Only used when the item is
+   * embedded in a Handle that a user is moving.
+   *
+   * NOTE: move is responsible for destroying *this!
+   *
+   * @param dest new location of the cache item
+   */
   virtual void move(void* dest) noexcept = 0;
 
   friend class Handle;

cachelib/interface/Handle.cpp

@@ -37,7 +37,6 @@ Handle::Handle(Handle&& other) noexcept
   XDCHECK_NE(cache_, nullptr) << "Invalid handle";
   if (other.item_ == reinterpret_cast<CacheItem*>(other.buf_)) {
     other.item_->move(buf_);
-    other.item_->~CacheItem();
     item_ = reinterpret_cast<CacheItem*>(buf_);
   } else {
     item_ = other.item_;
@@ -50,7 +49,6 @@ Handle::~Handle() noexcept {
     if (item_->decrementRefCount()) {
       cache_->release(*item_, inserted_);
     }
-    item_ = nullptr;
   }
 }
 

cachelib/interface/components/FlashCacheComponent.cpp

@@ -150,6 +150,7 @@ class FlashCacheItem : public CacheItem {
 
   void move(void* dest) noexcept override {
     new (dest) FlashCacheItem(std::move(*this));
+    this->~FlashCacheItem();
   }
 };
 
@@ -663,6 +664,7 @@ class ConsistentFlashCacheItem : public FlashCacheItem {
 
   void move(void* dest) noexcept override {
     new (dest) ConsistentFlashCacheItem(std::move(*this));
+    this->~ConsistentFlashCacheItem();
   }
 };
 

cachelib/interface/components/RAMCacheComponent.cpp

@@ -399,7 +399,10 @@ UnitResult RAMCacheComponent::writeBack(CacheItem& /* item */) {
 void RAMCacheComponent::release(interface::CacheItem& item, bool inserted) {
   stats_->release_.throughput_.calls_.inc();
   auto latencyGuard = stats_->release_.latency_.start();
-  auto* implItem = reinterpret_cast<RAMCacheItem&>(item).item();
+  auto& genericItem = reinterpret_cast<RAMCacheItem&>(item);
+  auto* implItem = genericItem.item();
+  // call this destructor first since genericItem is embedded in implItem
+  genericItem.~RAMCacheItem();
   cache_->releaseBackToAllocator(*implItem, RemoveContext::kNormal,
                                  /* nascent */ !inserted);
   stats_->release_.throughput_.successes_.inc();