Fix HashStringAllocator::clear() and cumulativeBytes_ #10260

wypb · 2024-06-19T08:06:45Z

Summary: This PR solves the following issues:

cumulativeBytes_ is the counter of allocated bytes, if we allocate n blocks and then execute free() on these blocks, the final value of cumulativeBytes_ should be equal to 0. But without this PR, the value of cumulativeBytes_ is not equal to 0. see HashStringAllocatorTest#multipleFreeAncCheckCumulativeBytes test case in this PR. This is because we don't include the size of the Header when calculating the allocated bytes.
When executing HashStringAllocator::clear(), we need to decrement memory counters. PR Fix HashStringAllocator::clear #10053 only decrement memory counters when releasing allocationsFromPool_. We also need to decrement the memory counters when releasing memory requested by memory::AllocationPool.
After executing HashStringAllocator::clear(), the values of cumulativeBytes_ and sizeFromPool_ should be equal to 0.

netlify · 2024-06-19T08:07:01Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`6d2b30e`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/667d0bd96b59ee00083891b0

wypb · 2024-06-24T08:06:30Z

Hi @xiaoxmeng can you please help review this? Thanks!

xiaoxmeng

@wypb thanks!

xiaoxmeng · 2024-06-26T04:56:53Z

velox/common/memory/HashStringAllocator.h

@@ -305,7 +305,8 @@ class HashStringAllocator : public StreamArena {
  /// the pointer because in the worst case we would have one allocation that
  /// chains many small free blocks together via kContinued.
  uint64_t freeSpace() const {
-    int64_t minFree = freeBytes_ - numFree_ * (sizeof(Header) + sizeof(void*));
+    int64_t minFree =


const int64_t minFree

xiaoxmeng · 2024-06-26T05:00:59Z

velox/common/memory/HashStringAllocator.cpp

@@ -433,8 +470,8 @@ void HashStringAllocator::free(Header* header) {
      freeToPool(headerToFree, headerToFree->size() + sizeof(Header));
    } else {
      VELOX_CHECK(!headerToFree->isFree());
-      freeBytes_ += headerToFree->size() + sizeof(Header);
-      cumulativeBytes_ -= headerToFree->size();
+      freeBytes_ += headerToFree->size() + kHeaderSize;


Can we add a anonymous function say

inline size_t blockBytes(blockHeader) { return blockHeader->size() + kHeaderSize; }

xiaoxmeng · 2024-06-26T05:01:27Z

velox/common/memory/HashStringAllocator.cpp

@@ -697,15 +734,15 @@ int64_t HashStringAllocator::checkConsistency() const {
                *(reinterpret_cast<int32_t*>(header->end()) - 1));
          }
          ++numFree;
-          freeBytes += sizeof(Header) + header->size();
+          freeBytes += kHeaderSize + header->size();


xiaoxmeng · 2024-06-26T05:01:32Z

velox/common/memory/HashStringAllocator.cpp

        } else if (header->isContinued()) {
          // If the content of the header is continued, check the continued
          // header is readable and not free.
          auto* continued = header->nextContinued();
          VELOX_CHECK(!continued->isFree());
-          allocatedBytes += header->size() - sizeof(void*);
+          allocatedBytes += header->size() + kHeaderSize;


velox/common/memory/HashStringAllocator.cpp

xiaoxmeng · 2024-06-26T05:19:18Z

velox/common/memory/HashStringAllocator.cpp

+
+  static const auto kHugePageSize = memory::AllocationTraits::kHugePageSize;
+  for (auto i = 0; i < pool_.numRanges(); ++i) {
+    auto topRange = pool_.rangeAt(i);


const auto topRange = pool_.rangeAt(i); const auto topRangeSize = topRange.size();

xiaoxmeng · 2024-06-26T05:28:08Z

velox/common/memory/HashStringAllocator.cpp

+          std::min<int64_t>(topRangeSize, kHugePageSize));
+      const auto size = range.size() - simd::kPadding;
+      auto* end = reinterpret_cast<Header*>(range.data() + size);
+      auto* header = reinterpret_cast<Header*>(range.data());


Can we have define macro for this?

#define HEADER(ptr) reinterpret_cast<Header*>(ptr)

Because Velox does not recommend using macros, so I created a new static Header* castToHeader(const void* data) method.

velox/common/memory/HashStringAllocator.cpp

wypb · 2024-06-26T11:25:36Z

Hi @xiaoxmeng I have refactored the code based on the review comments. please help me review it again when you have a chance. Thanks!

xiaoxmeng

@wypb LGTM. Thanks for helping on this!

velox/common/memory/HashStringAllocator.h

xiaoxmeng · 2024-06-26T16:49:52Z

velox/common/memory/HashStringAllocator.cpp

@@ -83,6 +83,10 @@ std::string HashStringAllocator::Header::toString() {

 HashStringAllocator::~HashStringAllocator() {
  clear();
+#ifdef NDEBUG
+  VELOX_CHECK_EQ(state_.cumulativeBytes(), 0);


You can use VELOX_DCHECK_EQ here?

Can you move these two checks in clear()?

I've moved it in clear() as suggested. Thanks.

xiaoxmeng · 2024-06-26T16:51:51Z

velox/common/memory/HashStringAllocator.cpp

+#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 range = state_.pool().rangeAt(i); const auto rangeSize = topRange.size();

xiaoxmeng · 2024-06-26T16:52:17Z

velox/common/memory/HashStringAllocator.cpp

+      VELOX_CHECK_EQ(0, topRangeSize % kHugePageSize);
+    }
+
+    for (int64_t subRangeStart = 0; subRangeStart < topRangeSize;


s/subRangeStart/blockOffset/

xiaoxmeng · 2024-06-26T16:53:26Z

velox/common/memory/HashStringAllocator.cpp

+            reinterpret_cast<char*>(header->end()),
+            reinterpret_cast<char*>(end));
+
+        if (header->isFree()) {


if (!header->isFree()) { ... }

velox/common/memory/HashStringAllocator.cpp

xiaoxmeng · 2024-06-26T16:55:04Z

velox/common/memory/HashStringAllocator.cpp

+
+    for (int64_t subRangeStart = 0; subRangeStart < topRangeSize;
+         subRangeStart += kHugePageSize) {
+      auto range = folly::Range<char*>(


s/range/blockRange/

velox/common/memory/HashStringAllocator.cpp

facebook-github-bot · 2024-06-26T16:59:48Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2024-06-27T05:50:04Z

Hi @xiaoxmeng I have refactored the code based on the review comments. please help me review it again when you have a chance. Thanks!

@wypb thanks and left few more nits. Thanks!

facebook-github-bot · 2024-06-27T06:51:12Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-06-29T01:05:50Z

@xiaoxmeng merged this pull request in c54e59d.

conbench-facebook · 2024-06-29T01:38:17Z

Conbench analyzed the 1 benchmark run on commit c54e59db.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 19, 2024

wypb force-pushed the fix_HashStringAllocator branch from 16f6ff9 to 3eaa100 Compare June 19, 2024 08:12

wypb changed the title ~~Fix HashStringAllocator~~ Fix HashStringAllocator::clear() and cumulativeBytes_ Jun 19, 2024

wypb force-pushed the fix_HashStringAllocator branch 3 times, most recently from 0e426e0 to 34b7f9e Compare June 20, 2024 04:03

mbasmanova requested a review from xiaoxmeng June 25, 2024 14:56

xiaoxmeng reviewed Jun 26, 2024

View reviewed changes

wypb force-pushed the fix_HashStringAllocator branch 5 times, most recently from 587fd5d to 15ed556 Compare June 26, 2024 10:00

xiaoxmeng approved these changes Jun 26, 2024

View reviewed changes

wypb force-pushed the fix_HashStringAllocator branch 2 times, most recently from 55ee1c2 to 13106b4 Compare June 27, 2024 06:47

Fix HashStringAllocator::clear() and cumulativeBytes_

6d2b30e

wypb force-pushed the fix_HashStringAllocator branch from ab38741 to 6d2b30e Compare June 27, 2024 06:51

Yuhta approved these changes Jun 27, 2024

View reviewed changes

facebook-github-bot closed this in c54e59d Jun 29, 2024

facebook-github-bot added the Merged label Jun 29, 2024

wypb deleted the fix_HashStringAllocator branch June 29, 2024 05:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HashStringAllocator::clear() and cumulativeBytes_ #10260

Fix HashStringAllocator::clear() and cumulativeBytes_ #10260

wypb commented Jun 19, 2024 •

edited

Loading

netlify bot commented Jun 19, 2024 •

edited

Loading

wypb commented Jun 24, 2024

xiaoxmeng left a comment

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

wypb Jun 26, 2024

wypb commented Jun 26, 2024

xiaoxmeng left a comment

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

wypb Jun 27, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

xiaoxmeng Jun 26, 2024

facebook-github-bot commented Jun 26, 2024

xiaoxmeng commented Jun 27, 2024

facebook-github-bot commented Jun 27, 2024

facebook-github-bot commented Jun 29, 2024

conbench-facebook bot commented Jun 29, 2024

Fix HashStringAllocator::clear() and cumulativeBytes_ #10260

Fix HashStringAllocator::clear() and cumulativeBytes_ #10260

Conversation

wypb commented Jun 19, 2024 • edited Loading

netlify bot commented Jun 19, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

wypb commented Jun 24, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wypb commented Jun 26, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 26, 2024

xiaoxmeng commented Jun 27, 2024

facebook-github-bot commented Jun 27, 2024

facebook-github-bot commented Jun 29, 2024

conbench-facebook bot commented Jun 29, 2024

wypb commented Jun 19, 2024 •

edited

Loading

netlify bot commented Jun 19, 2024 •

edited

Loading