Fix HashStringAllocator::clear() and cumulativeBytes_

facebookincubator · Jun 27, 2024 · 55ee1c2 · 55ee1c2
1 parent dcaa676
commit 55ee1c2
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 21 deletions.
diff --git a/velox/common/memory/HashStringAllocator.cpp b/velox/common/memory/HashStringAllocator.cpp
@@ -83,10 +83,6 @@ std::string HashStringAllocator::Header::toString() {
 
 HashStringAllocator::~HashStringAllocator() {
   clear();
-#ifdef NDEBUG
-  VELOX_CHECK_EQ(state_.cumulativeBytes(), 0);
-  VELOX_CHECK_EQ(state_.sizeFromPool(), 0);
-#endif
 }
 
 void HashStringAllocator::clear() {
@@ -108,39 +104,41 @@ void HashStringAllocator::clear() {
 #ifdef NDEBUG
   static const auto kHugePageSize = memory::AllocationTraits::kHugePageSize;
   for (auto i = 0; i < state_.pool().numRanges(); ++i) {
-    const auto topRange = state_.pool().rangeAt(i);
-    const auto topRangeSize = topRange.size();
-    if (topRangeSize >= kHugePageSize) {
-      VELOX_CHECK_EQ(0, topRangeSize % kHugePageSize);
+    const auto range = state_.pool().rangeAt(i);
+    const auto rangeSize = range.size();
+    if (rangeSize >= kHugePageSize) {
+      VELOX_CHECK_EQ(0, rangeSize % kHugePageSize);
     }
 
-    for (int64_t subRangeStart = 0; subRangeStart < topRangeSize;
-         subRangeStart += kHugePageSize) {
-      auto range = folly::Range<char*>(
-          topRange.data() + subRangeStart,
-          std::min<int64_t>(topRangeSize, kHugePageSize));
-      const auto size = range.size() - simd::kPadding;
-      auto* end = castToHeader(range.data() + size);
-      auto* header = castToHeader(range.data());
+    for (int64_t blockOffset = 0; blockOffset < rangeSize;
+         blockOffset += kHugePageSize) {
+      auto blockRange = folly::Range<char*>(
+          range.data() + blockOffset,
+          std::min<int64_t>(rangeSize, kHugePageSize));
+      const auto size = blockRange.size() - simd::kPadding;
+      auto* end = castToHeader(blockRange.data() + size);
+      auto* header = castToHeader(blockRange.data());
       while (header != end) {
-        VELOX_CHECK_GE(reinterpret_cast<char*>(header), range.data());
+        VELOX_CHECK_GE(reinterpret_cast<char*>(header), blockRange.data());
         VELOX_CHECK_LT(
             reinterpret_cast<char*>(header), reinterpret_cast<char*>(end));
         VELOX_CHECK_LE(
             reinterpret_cast<char*>(header->end()),
             reinterpret_cast<char*>(end));
 
-        if (header->isFree()) {
-          // No-op
-        } else {
-          // Continued block & Non-free block.
+        // Continued block & Non-free block.
+        if (!header->isFree()) {
           state_.cumulativeBytes() -= blockBytes(header);
         }
         header = castToHeader(header->end());
       }
     }
   }
+
+  VELOX_DCHECK_EQ(state_.cumulativeBytes(), 0);
+  VELOX_DCHECK_EQ(state_.sizeFromPool(), 0);
 #endif
+
   state_.pool().clear();
 }
 

diff --git a/velox/common/memory/HashStringAllocator.h b/velox/common/memory/HashStringAllocator.h
@@ -222,11 +222,13 @@ class HashStringAllocator : public StreamArena {
     return castToHeader(data) - 1;
   }
 
+  /// Returns the header below 'data'.
   static Header* castToHeader(const void* data) {
     return reinterpret_cast<Header*>(
         const_cast<char*>(reinterpret_cast<const char*>(data)));
   }
 
+  /// Returns the byte size of block pointed by 'header'.
   inline size_t blockBytes(const Header* header) const {
     return header->size() + kHeaderSize;
   }