Remove redundant copy on the flush path

Summary:
On the flush path we allocate a new IO-aligned buffer, copy the region buffer into it and then write the copy to the device.  I'm not sure why this extra copy exists, especially since our Region buffers are already [IO-aligned](https://www.internalfb.com/code/fbsource/[d0fda3b969458b99e1492aafece94ea93ca7c4a4]/fbcode/cachelib/navy/block_cache/RegionManager.cpp?lines=65-66).  It also doesn't increase concurrency by, for example, allowing us to return the Region buffer to the clean pool earlier because we're still blocking on the device write.

Remove this redundant copy and just directly write the region buffer to the device.

Reviewed By: AlnisM, byahn0996

Differential Revision: D94241607

fbshipit-source-id: a5c94f75ade247df658e981803461a2ee9d2af71

Author: rlyerly

cachelib/allocator/nvmcache/NavyConfig.h

@@ -555,6 +555,11 @@ class BlockCacheConfig {
     return *this;
   }
 
+  BlockCacheConfig& setDirectFlush(bool enable) noexcept {
+    directFlush_ = enable;
+    return *this;
+  }
+
   BlockCacheConfig& setAllocatorCount(uint32_t numAllocators) noexcept {
     allocatorsPerPriority_ = {numAllocators};
     return *this;
@@ -617,6 +622,8 @@ class BlockCacheConfig {
 
   bool isRecoverEvictionPolicy() const { return recoverEvictionPolicy_; }
 
+  bool isDirectFlush() const { return directFlush_; }
+
   bool isCombinedEntryBlockEnabled() const { return useCombinedEntryBlock_; }
 
   const BlockCacheReinsertionConfig& getReinsertionConfig() const {
@@ -666,6 +673,9 @@ class BlockCacheConfig {
   // Whether to persist and recover eviction policy ordering across restarts.
   bool recoverEvictionPolicy_{false};
 
+  // Whether to write region buffer directly without intermediate copy.
+  bool directFlush_{false};
+
   // Whether to use Combined entry block (For index entries and small sized
   // items).
   // Only FixedSizeIndex will support this and it doesn't work with

cachelib/allocator/nvmcache/NavySetup.cpp

@@ -181,6 +181,7 @@ uint64_t setupBlockCache(const navy::BlockCacheConfig& blockCacheConfig,
 
   blockCache->setRecoverEvictionPolicy(
       blockCacheConfig.isRecoverEvictionPolicy());
+  blockCache->setDirectFlush(blockCacheConfig.isDirectFlush());
   blockCache->setUseCombinedEntryBlock(
       blockCacheConfig.isCombinedEntryBlockEnabled());
   blockCache->setNumAllocatorsPerPriority(

cachelib/navy/Factory.cpp

@@ -135,6 +135,8 @@ class BlockCacheProtoImpl final : public BlockCacheProto {
     config_.recoverEvictionPolicy = enable;
   }
 
+  void setDirectFlush(bool enable) override { config_.directFlush = enable; }
+
   void setUseCombinedEntryBlock(bool useCombinedEntryBlock) override {
     config_.useCombinedEntryBlock = useCombinedEntryBlock;
   }

cachelib/navy/Factory.h

@@ -95,6 +95,9 @@ class BlockCacheProto {
   // (Optional) Persist and recover eviction policy ordering across restarts.
   virtual void setRecoverEvictionPolicy(bool enable) = 0;
 
+  // (Optional) Set if direct flush without intermediate copy is enabled.
+  virtual void setDirectFlush(bool enable) = 0;
+
   // (Optional) Set if the combined entry block is enabled.
   virtual void setUseCombinedEntryBlock(bool useCombinedEntryBlock) = 0;
 

cachelib/navy/block_cache/BlockCache.cpp

@@ -248,7 +248,8 @@ BlockCache::BlockCache(Config&& config, ValidConfigTag)
                      config.inMemBufFlushRetryLimit,
                      config.regionManagerFlushAsync,
                      true /* allowReadDuringReclaim */,
-                     config.recoverEvictionPolicy},
+                     config.recoverEvictionPolicy,
+                     config.directFlush},
       allocator_{regionManager_, config.allocatorsPerPriority},
       reinsertionPolicy_{makeReinsertionPolicy(config.reinsertionConfig)} {
   validate(config);

cachelib/navy/block_cache/BlockCache.h

@@ -111,6 +111,11 @@ class BlockCache final : public Engine {
     // no effect on eviction ordering.
     bool recoverEvictionPolicy{false};
 
+    // Whether to write region buffer directly to device on flush path without
+    // allocating an intermediate IO buffer and copying. Default false preserves
+    // old behavior for safe rollout. When true, avoids redundant copy.
+    bool directFlush{false};
+
     // name of this BC instance
     std::string name{};
 

cachelib/navy/block_cache/RegionManager.cpp

@@ -36,7 +36,8 @@ RegionManager::RegionManager(uint32_t numRegions,
                              uint16_t inMemBufFlushRetryLimit,
                              bool workerFlushAsync,
                              bool allowReadDuringReclaim,
-                             bool recoverEvictionPolicy)
+                             bool recoverEvictionPolicy,
+                             bool directFlush)
     : numPriorities_{numPriorities},
       inMemBufFlushRetryLimit_{inMemBufFlushRetryLimit},
       numRegions_{numRegions},
@@ -49,12 +50,14 @@ RegionManager::RegionManager(uint32_t numRegions,
       workerFlushAsync_{workerFlushAsync},
       allowReadDuringReclaim_(allowReadDuringReclaim),
       recoverEvictionPolicy_{recoverEvictionPolicy},
+      directFlush_{directFlush},
       evictCb_{evictCb},
       cleanupCb_{cleanupCb},
       numInMemBuffers_{numInMemBuffers},
       placementHandle_{device_.allocatePlacementHandle()} {
-  XLOGF(INFO, "{} regions, {} bytes each, allowReadDuringReclaim {}",
-        numRegions_, regionSize_, allowReadDuringReclaim);
+  XLOGF(INFO,
+        "{} regions, {} bytes each, allowReadDuringReclaim {}, directFlush {}",
+        numRegions_, regionSize_, allowReadDuringReclaim, directFlush);
   for (uint32_t i = 0; i < numRegions; i++) {
     regions_[i] = std::make_unique<Region>(RegionId{i}, regionSize_);
   }
@@ -125,10 +128,19 @@ void RegionManager::reset() {
 Region::FlushRes RegionManager::flushBuffer(const RegionId& rid) {
   auto& region = getRegion(rid);
   auto callBack = [this](RelAddress addr, BufferView view) {
-    auto writeBuffer = device_.makeIOBuffer(view.size());
-    writeBuffer.copyFrom(0, view);
-    if (!deviceWrite(addr, std::move(writeBuffer))) {
-      return false;
+    if (directFlush_) {
+      XDCHECK_EQ(0u, reinterpret_cast<uintptr_t>(view.data()) %
+                         device_.getIOAlignmentSize());
+      XDCHECK_EQ(0u, view.size() % device_.getIOAlignmentSize());
+      if (!deviceWrite(addr, view)) {
+        return false;
+      }
+    } else {
+      auto writeBuffer = device_.makeIOBuffer(view.size());
+      writeBuffer.copyFrom(0, view);
+      if (!deviceWrite(addr, std::move(writeBuffer))) {
+        return false;
+      }
     }
     numInMemBufWaitingFlush_.dec();
     return true;

cachelib/navy/block_cache/RegionManager.h

@@ -90,6 +90,8 @@ class RegionManager {
   //                                  convinced there's no missing corner cases.
   // @param recoverEvictionPolicy     whether to persist and recover eviction
   //                                  policy ordering across restarts
+  // @param directFlush               whether to write region buffer directly
+  //                                  to device without intermediate copy
   RegionManager(uint32_t numRegions,
                 uint64_t regionSize,
                 uint64_t baseOffset,
@@ -105,7 +107,8 @@ class RegionManager {
                 uint16_t inMemBufFlushRetryLimit,
                 bool workerFlushAsync,
                 bool allowReadDuringReclaim = false,
-                bool recoverEvictionPolicy = false);
+                bool recoverEvictionPolicy = false,
+                bool directFlush = false);
   RegionManager(const RegionManager&) = delete;
   RegionManager& operator=(const RegionManager&) = delete;
 
@@ -373,6 +376,9 @@ class RegionManager {
   // Whether to persist and recover eviction policy ordering across restarts
   const bool recoverEvictionPolicy_{false};
 
+  // Whether to write region buffer directly without intermediate copy
+  const bool directFlush_{false};
+
   const RegionEvictCallback evictCb_;
   const RegionCleanupCallback cleanupCb_;