Improve allocation success when evicting from cache #6844

oerling · 2023-10-01T23:06:02Z

An allocation will evict cache to make space for data. If the the evicted data is freed and then an allocation is attempted, it can be that another thread has snatched the freed data. With high transient memory allocation rate and many threads, it can happen that a large allocation runs out of retries even if it could be satisfied.

Therefore, when evicting in order to make space for an allocation, we keep hold of the non-contiguous Allocations we remove from cache. Many allocations can in this way be concatenated into an allocation that covers the needed number of pages. This can then atomically be converted into a new non-contiguous or contiguous allocation. The allocate*WithoutRetry methods accept a non-contiguous allocation to be freed to provide pages for the new allocation. Memory mapping magic can convert non-contiguous freed pages to contiguous ones.

Note that the allocate*WithoutRetry methods mishandle the atomicity of the exchange: The allocated count is decremented by the size of the freed collateral and then incremented by the size of the new allocation. This should be a single atomic increment of allocatedSize

collateralSize. This always succeeds if the collateral is >= the allocated amount. Even so, the window of vulnerability to another thread snatching the freed capacity before it is reacquired is probably negligible, only a few instructions.

The AsyncDataCache::makeSpace loop first tries to allocate. If it fails, it evicts and keeps up to the required amount in a a grab bag allocation. It loops until this allocation is large enough to convert into the desired allocation. If nothing is evicted and the allocation repeatedly fails, it gives up.

netlify · 2023-10-01T23:06:07Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`3cb81e1`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65272ea607884d000899e45a

facebook-github-bot · 2023-10-02T15:43:26Z

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

xiaoxmeng

@oerling overall looks good to me. Thanks!

xiaoxmeng · 2023-10-04T20:59:25Z

velox/common/caching/AsyncDataCache.cpp

@@ -380,7 +384,14 @@ void CacheShard::evict(uint64_t bytesToFree, bool evictAllUnpinned) {
          continue;
        }
        largeFreed += candidate->data_.byteSize();
-        toFree.push_back(std::move(candidate->data()));
+        if (acquirePages) {


if (acquiredPages > 0) {

xiaoxmeng · 2023-10-04T21:22:47Z

velox/common/caching/AsyncDataCache.cpp

+    if (canTryAllocate(numPages - collateral.numPages(), evicted)) {
+      try {
+        if (allocate(evicted)) {
+          if (isCounted) {


Shall we just put isCounted is safe guard processing? Thanks!

xiaoxmeng · 2023-10-04T21:24:17Z

velox/common/caching/AsyncDataCache.cpp

        if (isCounted) {
          --numThreadsInAllocate_;
        }
-        return true;
+        throw;


Shall we use rethrow here?

xiaoxmeng · 2023-10-04T21:26:32Z

velox/common/caching/AsyncDataCache.cpp

    }
    ++shardCounter_;
+    int32_t toAcquire =


const int32_t numPagesToAcquire =

xiaoxmeng · 2023-10-04T21:27:47Z

velox/common/caching/AsyncDataCache.cpp

-        numPages * sizeMultiplier * memory::AllocationTraits::kPageSize,
-        nthAttempt >= kNumShards);
+        std::max<int32_t>(kMinEvictPages, numPages) * sizeMultiplier *
+            memory::AllocationTraits::kPageSize,


Use memory::AllocationTraits::pageBytes()

xiaoxmeng · 2023-10-04T21:29:53Z

velox/common/caching/AsyncDataCache.cpp

+          auto candidatePages = candidate->data().numPages();
+          acquirePages =
+              candidatePages > acquirePages ? 0 : acquirePages - candidatePages;
+          allocation->appendMove(candidate->data());


Add a check candidate is empty?

xiaoxmeng · 2023-10-04T21:30:19Z

velox/common/memory/Allocation.cpp

@@ -37,6 +37,14 @@ void Allocation::append(uint8_t* address, int32_t numPages) {
      "Appending a duplicate address into a PageRun");
  runs_.emplace_back(address, numPages);
 }
+void Allocation::appendMove(Allocation& other) {


Can you add a unit test for this? Thanks!

xiaoxmeng · 2023-10-04T21:35:35Z

velox/common/memory/MemoryAllocator.cpp

@@ -185,9 +186,14 @@ bool MemoryAllocator::allocateContiguous(
    return allocateContiguousWithoutRetry(
        numPages, collateral, allocation, reservationCB, maxPages);
  }
-  return cache()->makeSpace(numPages, [&]() {
+  Allocation toFree;
+  if (collateral) {


if (collateral != nullptr) {

xiaoxmeng · 2023-10-04T21:36:38Z

velox/common/memory/MemoryAllocator.cpp

@@ -198,7 +204,9 @@ bool MemoryAllocator::growContiguous(
  if (cache() == nullptr) {
    return growContiguousWithoutRetry(increment, allocation, reservationCB);
  }
-  return cache()->makeSpace(increment, [&]() {
+  Allocation empty;
+  return cache()->makeSpace(increment, empty, [&](Allocation& evicted) {


Can we change collateral passed to makeSpace to a pointer? Thanks!

xiaoxmeng · 2023-10-04T21:37:36Z

velox/common/caching/AsyncDataCache.h

+  void evict(
+      uint64_t bytesToFree,
+      bool evictAllUnpinned,
+      int32_t acquirePages = 0,


s/acquirePages/pagesToAcquire/

facebook-github-bot · 2023-10-04T22:45:11Z

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

xiaoxmeng · 2023-10-04T23:20:29Z

velox/common/caching/AsyncDataCache.cpp

@@ -706,7 +744,8 @@ CacheStats AsyncDataCache::refreshStats() const {

 void AsyncDataCache::clear() {
  for (auto& shard : shards_) {
-    shard->evict(std::numeric_limits<int32_t>::max(), true);
+    memory::Allocation acquired;
+    shard->evict(std::numeric_limits<int32_t>::max(), true, 0, acquired);


Put an assert acquired is empty?

xiaoxmeng · 2023-10-04T23:20:48Z

velox/common/caching/AsyncDataCache.h

@@ -548,8 +548,14 @@ class CacheShard {
  // not pinned. This favors first removing older and less frequently
  // used entries. If 'evictAllUnpinned' is true, anything that is
  // not pinned is evicted at first sight. This is for out of memory
-  // emergencies.
-  void evict(uint64_t bytesToFree, bool evictAllUnpinned);
+  // emergencies. If 'acquireBytes' is set, up to this amount is added to


s/acquireBytes/pagesToAcquire/

xiaoxmeng · 2023-10-04T23:21:36Z

velox/common/memory/MemoryAllocator.h

  virtual bool makeSpace(
      memory::MachinePageCount numPages,
-      std::function<bool()> allocate) = 0;
+      std::function<bool(Allocation& evicted)> allocate) = 0;


std::function<bool(Allocation&)>

facebook-github-bot · 2023-10-05T14:36:23Z

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

…r#6844) Summary: An allocation will evict cache to make space for data. If the the evicted data is freed and then an allocation is attempted, it can be that another thread has snatched the freed data. With high transient memory allocation rate and many threads, it can happen that a large allocation runs out of retries even if it could be satisfied. Therefore, when evicting in order to make space for an allocation, we keep hold of the non-contiguous Allocations we remove from cache. Many allocations can in this way be concatenated into an allocation that covers the needed number of pages. This can then atomically be converted into a new non-contiguous or contiguous allocation. The allocate*WithoutRetry methods accept a non-contiguous allocation to be freed to provide pages for the new allocation. Memory mapping magic can convert non-contiguous freed pages to contiguous ones. Note that the allocate*WithoutRetry methods mishandle the atomicity of the exchange: The allocated count is decremented by the size of the freed collateral and then incremented by the size of the new allocation. This should be a single atomic increment of allocatedSize - collateralSize. This always succeeds if the collateral is >= the allocated amount. Even so, the window of vulnerability to another thread snatching the freed capacity before it is reacquired is probably negligible, only a few instructions. The AsyncDataCache::makeSpace loop first tries to allocate. If it fails, it evicts and keeps up to the required amount in a a grab bag allocation. It loops until this allocation is large enough to convert into the desired allocation. If nothing is evicted and the allocation repeatedly fails, it gives up. Reviewed By: xiaoxmeng Differential Revision: D49829731 Pulled By: oerling

facebook-github-bot · 2023-10-05T15:05:51Z

This pull request was exported from Phabricator. Differential Revision: D49829731

facebook-github-bot · 2023-10-09T14:47:45Z

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

facebook-github-bot · 2023-10-09T21:33:48Z

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

facebook-github-bot · 2023-10-11T21:58:29Z

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

…r#6844) Summary: An allocation will evict cache to make space for data. If the the evicted data is freed and then an allocation is attempted, it can be that another thread has snatched the freed data. With high transient memory allocation rate and many threads, it can happen that a large allocation runs out of retries even if it could be satisfied. Therefore, when evicting in order to make space for an allocation, we keep hold of the non-contiguous Allocations we remove from cache. Many allocations can in this way be concatenated into an allocation that covers the needed number of pages. This can then atomically be converted into a new non-contiguous or contiguous allocation. The allocate*WithoutRetry methods accept a non-contiguous allocation to be freed to provide pages for the new allocation. Memory mapping magic can convert non-contiguous freed pages to contiguous ones. Note that the allocate*WithoutRetry methods mishandle the atomicity of the exchange: The allocated count is decremented by the size of the freed collateral and then incremented by the size of the new allocation. This should be a single atomic increment of allocatedSize - collateralSize. This always succeeds if the collateral is >= the allocated amount. Even so, the window of vulnerability to another thread snatching the freed capacity before it is reacquired is probably negligible, only a few instructions. The AsyncDataCache::makeSpace loop first tries to allocate. If it fails, it evicts and keeps up to the required amount in a a grab bag allocation. It loops until this allocation is large enough to convert into the desired allocation. If nothing is evicted and the allocation repeatedly fails, it gives up. Reviewed By: xiaoxmeng Differential Revision: D49829731 Pulled By: oerling

facebook-github-bot · 2023-10-11T23:24:30Z

This pull request was exported from Phabricator. Differential Revision: D49829731

facebook-github-bot · 2023-10-12T05:54:09Z

@oerling merged this pull request in 68e7698.

conbench-facebook · 2023-10-12T06:33:53Z

Conbench analyzed the 1 benchmark run on commit 68e76988.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…r#6844) Summary: An allocation will evict cache to make space for data. If the the evicted data is freed and then an allocation is attempted, it can be that another thread has snatched the freed data. With high transient memory allocation rate and many threads, it can happen that a large allocation runs out of retries even if it could be satisfied. Therefore, when evicting in order to make space for an allocation, we keep hold of the non-contiguous Allocations we remove from cache. Many allocations can in this way be concatenated into an allocation that covers the needed number of pages. This can then atomically be converted into a new non-contiguous or contiguous allocation. The allocate*WithoutRetry methods accept a non-contiguous allocation to be freed to provide pages for the new allocation. Memory mapping magic can convert non-contiguous freed pages to contiguous ones. Note that the allocate*WithoutRetry methods mishandle the atomicity of the exchange: The allocated count is decremented by the size of the freed collateral and then incremented by the size of the new allocation. This should be a single atomic increment of allocatedSize - collateralSize. This always succeeds if the collateral is >= the allocated amount. Even so, the window of vulnerability to another thread snatching the freed capacity before it is reacquired is probably negligible, only a few instructions. The AsyncDataCache::makeSpace loop first tries to allocate. If it fails, it evicts and keeps up to the required amount in a a grab bag allocation. It loops until this allocation is large enough to convert into the desired allocation. If nothing is evicted and the allocation repeatedly fails, it gives up. Pull Request resolved: facebookincubator#6844 Reviewed By: xiaoxmeng Differential Revision: D49829731 Pulled By: oerling fbshipit-source-id: ef5dae3fd6a35ea5eb2c81f41f41f2c41fc1fea2

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 Oct 1, 2023

oerling requested a review from xiaoxmeng October 1, 2023 23:07

oerling force-pushed the evict-pr branch from 4a0174d to 3ffd470 Compare October 3, 2023 15:46

xiaoxmeng reviewed Oct 4, 2023

View reviewed changes

oerling force-pushed the evict-pr branch from 3ffd470 to 1f22d7b Compare October 4, 2023 22:43

xiaoxmeng reviewed Oct 4, 2023

View reviewed changes

xiaoxmeng approved these changes Oct 4, 2023

View reviewed changes

oerling force-pushed the evict-pr branch from 1f22d7b to c289532 Compare October 5, 2023 14:36

facebook-github-bot added the fb-exported label Oct 5, 2023

oerling force-pushed the evict-pr branch 4 times, most recently from c828bc8 to 9e437f0 Compare October 9, 2023 14:35

oerling force-pushed the evict-pr branch from 9e437f0 to 169b298 Compare October 9, 2023 20:07

oerling force-pushed the evict-pr branch 2 times, most recently from 6ae77bb to f49b59d Compare October 11, 2023 21:57

oerling force-pushed the evict-pr branch from f49b59d to 3cb81e1 Compare October 11, 2023 23:24

facebook-github-bot closed this in 68e7698 Oct 12, 2023

facebook-github-bot added the Merged label Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve allocation success when evicting from cache #6844

Improve allocation success when evicting from cache #6844

oerling commented Oct 1, 2023

netlify bot commented Oct 1, 2023 •

edited

Loading

facebook-github-bot commented Oct 2, 2023

xiaoxmeng left a comment

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

facebook-github-bot commented Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

facebook-github-bot commented Oct 5, 2023

facebook-github-bot commented Oct 5, 2023

facebook-github-bot commented Oct 9, 2023

facebook-github-bot commented Oct 9, 2023

facebook-github-bot commented Oct 11, 2023

facebook-github-bot commented Oct 11, 2023

facebook-github-bot commented Oct 12, 2023

conbench-facebook bot commented Oct 12, 2023

Improve allocation success when evicting from cache #6844

Improve allocation success when evicting from cache #6844

Conversation

oerling commented Oct 1, 2023

netlify bot commented Oct 1, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Oct 2, 2023

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

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 Oct 4, 2023

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 Oct 5, 2023

facebook-github-bot commented Oct 5, 2023

facebook-github-bot commented Oct 9, 2023

facebook-github-bot commented Oct 9, 2023

facebook-github-bot commented Oct 11, 2023

facebook-github-bot commented Oct 11, 2023

facebook-github-bot commented Oct 12, 2023

conbench-facebook bot commented Oct 12, 2023

netlify bot commented Oct 1, 2023 •

edited

Loading