Removing meaningless 'slotSize' return value from Allocator::allocate()

Summary:
Currently Allocator::allocate() is returning allocated size as one of the return values, but there's no logic to allocate different size from the given 'size' parameter and it will always be the same value with the given size. (Unless it failed and returns error)
Removing this from the return values since it's confusing and whenever this part of codes is looked up, it needs another look to confirm it should be the same 'size'

Reviewed By: AlnisM

Differential Revision: D88243894

fbshipit-source-id: 3e82430c683c77f89786e016201d915f2126e3d8

Author: byahn0996

cachelib/navy/block_cache/Allocator.cpp

@@ -48,12 +48,13 @@ Allocator::Allocator(RegionManager& regionManager,
   }
 }
 
-std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocate(
-    uint32_t size, uint16_t priority, bool canWait, uint64_t keyHash) {
+std::pair<RegionDescriptor, RelAddress> Allocator::allocate(uint32_t size,
+                                                            uint16_t priority,
+                                                            bool canWait,
+                                                            uint64_t keyHash) {
   XDCHECK_LT(priority, allocators_.size());
   if (size == 0 || size > regionManager_.regionSize()) {
-    return std::make_tuple(RegionDescriptor{OpenStatus::Error}, size,
-                           RelAddress());
+    return std::make_pair(RegionDescriptor{OpenStatus::Error}, RelAddress());
   }
   RegionAllocator* ra =
       &allocators_[priority][keyHash % allocators_[priority].size()];
@@ -66,7 +67,7 @@ std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocate(
 // have to wait. Every time we take a region from the clean list, we schedule
 // new reclaim job to refill it. Caller must close the region after data
 // written to the slot.
-std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocateWith(
+std::pair<RegionDescriptor, RelAddress> Allocator::allocateWith(
     RegionAllocator& ra, uint32_t size, bool canWait) {
   std::unique_lock lock{ra.getLock()};
   RegionId rid = ra.getAllocationRegion();
@@ -75,7 +76,7 @@ std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocateWith(
     auto [desc, addr] = region.openAndAllocate(size);
     XDCHECK_NE(OpenStatus::Retry, desc.status());
     if (desc.isReady()) {
-      return std::make_tuple(std::move(desc), size, addr);
+      return std::make_pair(std::move(desc), addr);
     }
     XDCHECK_EQ(OpenStatus::Error, desc.status());
     // Buffer has been fully allocated. Release the region by scheduling an
@@ -101,7 +102,7 @@ std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocateWith(
       allocRetryWaits_.inc();
       waiter->baton_.wait();
     }
-    return std::make_tuple(RegionDescriptor{status}, size, RelAddress{});
+    return std::make_pair(RegionDescriptor{status}, RelAddress{});
   }
   XDCHECK(status == OpenStatus::Ready);
   XDCHECK(!waiter);
@@ -114,7 +115,7 @@ std::tuple<RegionDescriptor, uint32_t, RelAddress> Allocator::allocateWith(
   ra.setAllocationRegion(rid);
   auto [desc, addr] = region.openAndAllocate(size);
   XDCHECK_EQ(OpenStatus::Ready, desc.status());
-  return std::make_tuple(std::move(desc), size, addr);
+  return std::make_pair(std::move(desc), addr);
 }
 
 void Allocator::close(RegionDescriptor&& desc) {

cachelib/navy/block_cache/Allocator.h

@@ -95,20 +95,19 @@ class Allocator {
   // @param keyHash       Hash of the key to allocate for. Used for deciding the
   // allocator if there's more than one for the priority.
   //
-  // Returns a tuple containing region descriptor, allocated slotSize and
-  // allocated address
+  // Returns a pair containing region descriptor and allocated address
   // The region descriptor contains region id, open mode and status,
   // which is one of the following
-  //  - Ready   Fills @addr and @slotSize
+  //  - Ready   Fills @addr
   //  - Retry   Retry later, reclaim is running
   //  - Error   Can't allocate this size even later (hard failure)
   // When allocating with a priority, the priority must NOT exceed the
   // max priority which is (@numPriorities - 1) specified when constructing
   // this allocator.
-  std::tuple<RegionDescriptor, uint32_t, RelAddress> allocate(uint32_t size,
-                                                              uint16_t priority,
-                                                              bool canWait,
-                                                              uint64_t keyHash);
+  std::pair<RegionDescriptor, RelAddress> allocate(uint32_t size,
+                                                   uint16_t priority,
+                                                   bool canWait,
+                                                   uint64_t keyHash);
 
   // Closes the region.
   void close(RegionDescriptor&& rid);
@@ -134,9 +133,10 @@ class Allocator {
   void flushAndReleaseRegionFromRALocked(RegionAllocator& ra, bool flushAsync);
 
   // Allocates @size bytes in region allocator @ra. If succeed (enough space),
-  // returns region descriptor, size and address.
-  std::tuple<RegionDescriptor, uint32_t, RelAddress> allocateWith(
-      RegionAllocator& ra, uint32_t size, bool wait);
+  // returns region descriptor and address.
+  std::pair<RegionDescriptor, RelAddress> allocateWith(RegionAllocator& ra,
+                                                       uint32_t size,
+                                                       bool wait);
 
   RegionManager& regionManager_;
   // Multiple allocators when we use priority-based allocation

cachelib/navy/block_cache/BlockCache.cpp

@@ -213,8 +213,8 @@ Status BlockCache::insert(HashedKey hk, BufferView value) {
   }
 
   // All newly inserted items are assigned with the lowest priority
-  auto [desc, slotSize, addr] = allocator_.allocate(
-      size, kDefaultItemPriority, true /* canWait */, hk.keyHash());
+  auto [desc, addr] = allocator_.allocate(size, kDefaultItemPriority,
+                                          true /* canWait */, hk.keyHash());
 
   switch (desc.status()) {
   case OpenStatus::Error:
@@ -231,11 +231,11 @@ Status BlockCache::insert(HashedKey hk, BufferView value) {
 
   // After allocation a region is opened for writing. Until we close it, the
   // region would not be reclaimed and index never gets an invalid entry.
-  const auto status = writeEntry(addr, slotSize, hk, value);
-  auto newObjSizeHint = encodeSizeHint(slotSize);
+  const auto status = writeEntry(addr, size, hk, value);
+  auto newObjSizeHint = encodeSizeHint(size);
   if (status == Status::Ok) {
     const auto lr = index_->insert(
-        hk.keyHash(), encodeRelAddress(addr.add(slotSize)), newObjSizeHint);
+        hk.keyHash(), encodeRelAddress(addr.add(size)), newObjSizeHint);
     // We replaced an existing key in the index
     uint64_t newObjSize = decodeSizeHint(newObjSizeHint);
     uint64_t oldObjSize = 0;
@@ -811,7 +811,7 @@ AllocatorApiResult BlockCache::reinsertOrRemoveItem(
           : std::min<uint16_t>(lr.currentHits(), numPriorities_ - 1);
 
   uint32_t size = serializedSize(hk.key().size(), value.size());
-  auto [desc, slotSize, addr] =
+  auto [desc, addr] =
       allocator_.allocate(size, priority, false /* canWait */, hk.keyHash());
 
   switch (desc.status()) {
@@ -838,18 +838,17 @@ AllocatorApiResult BlockCache::reinsertOrRemoveItem(
 
   // After allocation a region is opened for writing. Until we close it, the
   // region would not be reclaimed and index never gets an invalid entry.
-  const auto status = writeEntry(addr, slotSize, hk, value);
+  const auto status = writeEntry(addr, size, hk, value);
   if (status != Status::Ok) {
     recordEvent(hk.key(), AllocatorApiEvent::NVM_REINSERT,
                 AllocatorApiResult::FAILED, entrySize);
     reinsertionErrorCount_.inc();
     return removeItem(false);
   }
 
-  const auto replaced =
-      index_->replaceIfMatch(hk.keyHash(),
-                             encodeRelAddress(addr.add(slotSize)),
-                             encodeRelAddress(currAddr));
+  const auto replaced = index_->replaceIfMatch(hk.keyHash(),
+                                               encodeRelAddress(addr.add(size)),
+                                               encodeRelAddress(currAddr));
   if (!replaced) {
     recordEvent(hk.key(), AllocatorApiEvent::NVM_REINSERT,
                 AllocatorApiResult::INVALIDATED, entrySize);
@@ -866,13 +865,13 @@ AllocatorApiResult BlockCache::reinsertOrRemoveItem(
 }
 
 Status BlockCache::writeEntry(RelAddress addr,
-                              uint32_t slotSize,
+                              uint32_t size,
                               HashedKey hk,
                               BufferView value) {
-  XDCHECK_LE(addr.offset() + slotSize, regionManager_.regionSize());
-  XDCHECK_EQ(slotSize % allocAlignSize_, 0ULL)
-      << folly::sformat(" alignSize={}, size={}", allocAlignSize_, slotSize);
-  auto buffer = Buffer(slotSize);
+  XDCHECK_LE(addr.offset() + size, regionManager_.regionSize());
+  XDCHECK_EQ(size % allocAlignSize_, 0ULL)
+      << folly::sformat(" alignSize={}, size={}", allocAlignSize_, size);
+  auto buffer = Buffer(size);
 
   // Copy descriptor and the key to the end
   size_t descOffset = buffer.size() - sizeof(EntryDesc);
@@ -945,7 +944,7 @@ Status BlockCache::readEntry(const RegionDescriptor& readDesc,
     return Status::NotFound;
   }
 
-  // Update slot size to actual, defined by key and value size
+  // Update the size to actual, defined by key and value size
   uint32_t size = serializedSize(desc.keySize, desc.valueSize);
   if (buffer.size() > size) {
     // Read more than actual size. Trim the invalid data in the beginning

cachelib/navy/block_cache/BlockCache.h

@@ -267,11 +267,11 @@ class BlockCache final : public Engine {
   // the performance point of view, but makes sense to track them for perf:
   // especially portion on CPU time spent in std::memcpy.
   // @param addr        Address to write this entry into
-  // @param slotSize    Number of bytes this entry will take up on the device
+  // @param size        Number of bytes this entry will take up on the device
   // @param hk          Key of the entry
   // @param value       Payload of the entry
   Status writeEntry(RelAddress addr,
-                    uint32_t slotSize,
+                    uint32_t size,
                     HashedKey hk,
                     BufferView value);
   // @param readDesc      Descriptor for reading. This must be valid

cachelib/navy/block_cache/tests/AllocatorTest.cpp

@@ -62,12 +62,11 @@ TEST(Allocator, RegionSyncInMemBuffers) {
 
   // Write to 3 regions
   RelAddress addr;
-  uint32_t slotSize = 0;
 
   // First allocation will fail with kicking a reclaim
   {
     RegionDescriptor desc{OpenStatus::Retry};
-    std::tie(desc, slotSize, addr) =
+    std::tie(desc, addr) =
         allocator.allocate(1024, kNoPriority, false, kKeyHash);
     EXPECT_EQ(OpenStatus::Retry, desc.status());
     // Reclaim should have been started; complete the reclaim
@@ -79,7 +78,7 @@ TEST(Allocator, RegionSyncInMemBuffers) {
     // tracks the current region that is full region if present.
     {
       RegionDescriptor desc{OpenStatus::Retry};
-      std::tie(desc, slotSize, addr) =
+      std::tie(desc, addr) =
           allocator.allocate(1024, kNoPriority, false, kKeyHash);
       EXPECT_TRUE(desc.isReady());
 
@@ -99,7 +98,7 @@ TEST(Allocator, RegionSyncInMemBuffers) {
     // 15 allocs exhaust region's space,but no reclaims scheduled.
     for (uint32_t j = 0; j < 15; j++) {
       RegionDescriptor desc{OpenStatus::Retry};
-      std::tie(desc, slotSize, addr) =
+      std::tie(desc, addr) =
           allocator.allocate(1024, kNoPriority, false, kKeyHash);
       EXPECT_TRUE(desc.isReady());
       EXPECT_EQ(RegionId{i}, addr.rid());
@@ -120,7 +119,7 @@ TEST(Allocator, RegionSyncInMemBuffers) {
   // of region 0 again
   {
     RegionDescriptor desc{OpenStatus::Retry};
-    std::tie(desc, slotSize, addr) =
+    std::tie(desc, addr) =
         allocator.allocate(1024, kNoPriority, false, kKeyHash);
     EXPECT_TRUE(desc.isReady());
     EXPECT_EQ(RegionId{3}, addr.rid());
@@ -161,10 +160,9 @@ TEST(Allocator, TestInMemBufferStates) {
   injectPauseSet("pause_flush_done");
 
   RelAddress addr;
-  uint32_t slotSize = 0;
   {
     RegionDescriptor desc{OpenStatus::Retry};
-    std::tie(desc, slotSize, addr) =
+    std::tie(desc, addr) =
         allocator.allocate(1024, kNoPriority, false, kKeyHash);
     EXPECT_EQ(OpenStatus::Retry, desc.status());
   }
@@ -177,7 +175,7 @@ TEST(Allocator, TestInMemBufferStates) {
     {
       RegionDescriptor wdesc{OpenStatus::Retry};
       // There should be clean region available
-      std::tie(wdesc, slotSize, addr) =
+      std::tie(wdesc, addr) =
           allocator.allocate(1024, kNoPriority, false, kKeyHash);
       EXPECT_TRUE(wdesc.isReady());
       EXPECT_EQ(0, wdesc.id().index());
@@ -190,7 +188,7 @@ TEST(Allocator, TestInMemBufferStates) {
       // Fill the remaining space in the first region
       for (uint32_t j = 0; j < 15; j++) {
         RegionDescriptor desc{OpenStatus::Retry};
-        std::tie(desc, slotSize, addr) =
+        std::tie(desc, addr) =
             allocator.allocate(1024, kNoPriority, false, kKeyHash);
         EXPECT_TRUE(desc.isReady());
         EXPECT_EQ(0, desc.id().index());
@@ -202,7 +200,7 @@ TEST(Allocator, TestInMemBufferStates) {
         // Do another allocation to flush/close the first region.
         // A reclaim will also be triggered
         RegionDescriptor desc{OpenStatus::Retry};
-        std::tie(desc, slotSize, addr) =
+        std::tie(desc, addr) =
             allocator.allocate(1024, kNoPriority, false, kKeyHash);
         EXPECT_EQ(OpenStatus::Ready, desc.status());
         EXPECT_EQ(1, desc.id().index());
@@ -252,15 +250,14 @@ TEST(Allocator, UsePriorities) {
   injectPauseSet("pause_reclaim_done");
 
   // Allocate to make sure a reclaim is triggered
-  auto [desc, slotSize, addr] = allocator.allocate(1024, 0, false, kKeyHash);
+  auto [desc, addr] = allocator.allocate(1024, 0, false, kKeyHash);
   EXPECT_EQ(OpenStatus::Retry, desc.status());
   EXPECT_TRUE(injectPauseWait("pause_reclaim_done"));
 
   // Allocate one item from each priortiy, we should see each allocation
   // results in a new region being allocated for its priority
   for (uint16_t pri = 0; pri < 3; pri++) {
-    std::tie(desc, slotSize, addr) =
-        allocator.allocate(1024, pri, false, kKeyHash);
+    std::tie(desc, addr) = allocator.allocate(1024, pri, false, kKeyHash);
     EXPECT_TRUE(desc.isReady());
     EXPECT_EQ(RegionId{pri}, addr.rid());
     EXPECT_EQ(pri, rm->getRegion(addr.rid()).getPriority());
@@ -299,7 +296,7 @@ TEST(Allocator, UseDifferentAllocatorsForPriorities) {
   injectPauseSet("pause_reclaim_done");
 
   // Allocate to make sure a reclaim is triggered
-  auto [desc, slotSize, addr] = allocator.allocate(1024, 0, false, kKeyHash);
+  auto [desc, addr] = allocator.allocate(1024, 0, false, kKeyHash);
   EXPECT_EQ(OpenStatus::Retry, desc.status());
   EXPECT_TRUE(injectPauseWait("pause_reclaim_done"));
 
@@ -310,8 +307,7 @@ TEST(Allocator, UseDifferentAllocatorsForPriorities) {
     // For each priority, allocate one item per allocator
     for (uint32_t index = 0; index < allocatorsPerPriority[pri]; index++) {
       // Use index as keyhash to allocate via a specific allocator.
-      std::tie(desc, slotSize, addr) =
-          allocator.allocate(1024, pri, false, index);
+      std::tie(desc, addr) = allocator.allocate(1024, pri, false, index);
       EXPECT_TRUE(desc.isReady());
       EXPECT_EQ(RegionId{allocatedRegion++}, addr.rid());
       EXPECT_EQ(pri, rm->getRegion(addr.rid()).getPriority());
@@ -325,8 +321,7 @@ TEST(Allocator, UseDifferentAllocatorsForPriorities) {
     // reclaim.
     for (uint32_t index = 0; index < allocatorsPerPriority[pri]; index++) {
       // Use index as keyhash to allocate via a specific allocator.
-      std::tie(desc, slotSize, addr) =
-          allocator.allocate(1024, pri, false, index);
+      std::tie(desc, addr) = allocator.allocate(1024, pri, false, index);
       EXPECT_TRUE(desc.isReady());
       EXPECT_EQ(pri, rm->getRegion(addr.rid()).getPriority());
       // The allocation should be from an existing region as the second item