MultiSizeBufferPool: remove invalid thrown exception, avoid overflow (#2960)

Due to the thread model of the pool, which avoid as much as possible
from locking, I have to remove one of the exception thrown. The
exception logic is invalid in some rare cases.
Furthermore, due to the chance for overflow, we convert some of the
statistics class members into signed integers - overflow on unsigned
integers is undefined according to the C++ standard.

Author: glevkovich

libs/util/MultiSizeBufferPool.hpp

@@ -253,39 +253,39 @@ class MultiSizeBufferPool {
       // Subpool level:
       // numAllocatedBytes_ + numNonAllocatedBytes_ = SubpoolConfig::numMaxBuffers * (SubpoolConfig::bufferSize +
       // chunkMetadataSize())
-      std::atomic_uint64_t numAllocatedBytes_{};
-      std::atomic_uint64_t numNonAllocatedBytes_{};
+      std::atomic_int64_t numAllocatedBytes_{};
+      std::atomic_int64_t numNonAllocatedBytes_{};
 
       // numAllocatedBytes_ = numUnusedBytes_ + numUsedBytes_
-      std::atomic_uint64_t numUsedBytes_{};
-      std::atomic_uint64_t numUnusedBytes_{};
+      std::atomic_int64_t numUsedBytes_{};
+      std::atomic_int64_t numUnusedBytes_{};
 
       // numAllocatedChunks_ + numNonAllocatedChunks_ = SubpoolConfig::numMaxBuffers
-      std::atomic_uint64_t numAllocatedChunks_{};
-      std::atomic_uint64_t numNonAllocatedChunks_{};
+      std::atomic_int64_t numAllocatedChunks_{};
+      std::atomic_int64_t numNonAllocatedChunks_{};
 
       // numAllocatedChunks_ = numUsedChunks_ + numUnusedChunks_
-      std::atomic_uint64_t numUsedChunks_{};
-      std::atomic_uint64_t numUnusedChunks_{};
+      std::atomic_int64_t numUsedChunks_{};
+      std::atomic_int64_t numUnusedChunks_{};
     } current_;
 
     // Overall statistics are accumulating
     struct OverallStatistics {
-      std::atomic_uint64_t numAllocatedBytes_{};
-      std::atomic_uint64_t numDeletedBytes_{};
-      std::atomic_uint64_t numUsedBytes_{};
+      std::atomic_int64_t numAllocatedBytes_{};
+      std::atomic_int64_t numDeletedBytes_{};
+      std::atomic_int64_t numUsedBytes_{};
 
-      std::atomic_uint64_t numAllocatedChunks_{};
-      std::atomic_uint64_t numDeletedChunks_{};
-      std::atomic_uint64_t numUsedChunks_{};
+      std::atomic_int64_t numAllocatedChunks_{};
+      std::atomic_int64_t numDeletedChunks_{};
+      std::atomic_int64_t numUsedChunks_{};
 
       // Amount of times where a chunk did not fit into the needed buffer size and returned to pool without use
-      std::atomic_uint64_t numBufferUsageFailed_{};
+      std::atomic_int64_t numBufferUsageFailed_{};
       // Amount of times where a chunk fit into the needed buffer size and returned to pool without use
-      std::atomic_uint64_t numBufferUsageSuccess_{};
+      std::atomic_int64_t numBufferUsageSuccess_{};
 
-      std::atomic_uint64_t maxNumAllocatedBytes_{};
-      std::atomic_uint64_t maxNumAllocatedChunks_{};
+      std::atomic_int64_t maxNumAllocatedBytes_{};
+      std::atomic_int64_t maxNumAllocatedChunks_{};
     } overall_;
 
     // relevant only if reportOnChangesOnly_ is true. Used to mark if the content need to be reported, only if there was
@@ -337,7 +337,7 @@ class MultiSizeBufferPool {
 
     std::atomic_bool stopFlag_;
     std::atomic_bool purgeFlag_;
-    std::atomic_uint64_t purgeLimit_;
+    std::atomic_int64_t purgeLimit_;
     const logging::Logger& logger_;
     std::string name_;
     const uint32_t chunkSize_;

libs/util/src/MultiSizeBufferPool.cpp

@@ -545,12 +545,6 @@ MultiSizeBufferPool::SubPool::AllocationResult MultiSizeBufferPool::SubPool::get
 
 void MultiSizeBufferPool::SubPool::returnChunk(char* chunk) {
   DEBUG_PRINT(logger_, "returnChunk enter" << KVLOG(KVLOG_PREFIX));
-  if ((stats_.current_.numUsedChunks_ == 0) || (stats_.current_.numUnusedChunks_ == config_.numMaxBuffers)) {
-    std::stringstream ss;
-    ss << KVLOG(KVLOG_PREFIX, config_.numMaxBuffers, (void*)chunk) << stats_;
-    throwException<std::runtime_error>(logger_, "No more chunks are marked as unused:", ss.str());
-  }
-
   bool shouldPurge = (purgeFlag_ && (stats_.current_.numAllocatedChunks_ > purgeLimit_));
   auto opType = shouldPurge ? Statistics::operationType::DELETE : Statistics::operationType::PUSH;
   stats_.onChunkBecomeUnused(chunkSize_, opType);
@@ -587,7 +581,7 @@ std::pair<char*, MultiSizeBufferPool::SubPool::AllocationStatus> MultiSizeBuffer
     return std::make_pair(nullptr, AllocationStatus::EXCEED_MAX_BUFFERS);
   }
   if ((poolConfig_.maxAllocatedBytes != 0) &&
-      ((poolStats_.current_.numAllocatedBytes_ + chunkSize_) > poolConfig_.maxAllocatedBytes)) {
+      ((poolStats_.current_.numAllocatedBytes_ + chunkSize_) > static_cast<int64_t>(poolConfig_.maxAllocatedBytes))) {
     LOG_WARN(logger_, "allocateChunk exit" << KVLOG(KVLOG_PREFIX));
     return std::make_pair(nullptr, AllocationStatus::EXCEED_POOL_MAX_BYTES);
   }
@@ -688,7 +682,7 @@ void MultiSizeBufferPool::Statistics::onInit(uint32_t numMaxBuffers, uint32_t ch
 
 void MultiSizeBufferPool::Statistics::setLimits(uint64_t maxAllocatedBytes) {
   if (maxAllocatedBytes == 0) return;
-  if (maxAllocatedBytes < current_.numNonAllocatedBytes_) {
+  if (static_cast<int64_t>(maxAllocatedBytes) < current_.numNonAllocatedBytes_) {
     current_.numNonAllocatedBytes_ = maxAllocatedBytes;
   }
 }