Add stats collection to memory reclaimer #6895

tanjialiang · 2023-10-04T20:37:24Z

During reclaiming there are information that also needs to be collected. Adding the stats collection framework to facilitate reclaiming stats collection.

netlify · 2023-10-04T20:37:29Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`88a7f41`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6520fa38acb813000886e187

xiaoxmeng

@tanjialiang LGTM. Thanks!

xiaoxmeng · 2023-10-04T20:44:40Z

velox/common/memory/MemoryArbitrator.h

+  struct Stats {
+    /// The total number of times of the reclaim attempts that end up failing
+    /// due to reclaiming at non-reclaimable stage.
+    std::atomic<uint64_t> numNonReclaimableAttempts{0};


The reclaim is single thread executed so we can just use uint64_t.

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

velox/common/memory/SharedArbitrator.h

@@ -200,5 +200,6 @@ class SharedArbitrator : public MemoryArbitrator {
  tsan_atomic<uint64_t> numShrunkBytes_{0};
  tsan_atomic<uint64_t> numReclaimedBytes_{0};
  tsan_atomic<uint64_t> reclaimTimeUs_{0};
+  MemoryReclaimer::Stats reclaimerStats_;


tsan_atomic<uint64_t> nonReclaimableAttempts_{0};

xiaoxmeng · 2023-10-04T20:47:48Z

velox/common/memory/SharedArbitrator.cpp

@@ -404,7 +404,7 @@ uint64_t SharedArbitrator::reclaim(
    try {
      freedBytes = pool->shrink(targetBytes);
      if (freedBytes < targetBytes) {
-        pool->reclaim(targetBytes - freedBytes);
+        pool->reclaim(targetBytes - freedBytes, reclaimerStats_);


We can pass the stats as a local variable here?

xiaoxmeng

@tanjialiang thanks for the update!

xiaoxmeng · 2023-10-05T05:59:40Z

velox/common/memory/MemoryArbitrator.h

@@ -252,6 +256,12 @@ FOLLY_ALWAYS_INLINE std::ostream& operator<<(
 /// through techniques such as disks spilling.
 class MemoryReclaimer {
 public:
+  struct Stats {


/// Used to collect memory reclaim execution stats.

xiaoxmeng · 2023-10-05T06:00:44Z

velox/common/memory/tests/MemoryArbitratorTest.cpp

-      ASSERT_EQ(pool->reclaim(kMaxMemory), 0);
+      ASSERT_EQ(pool->reclaim(0, stats_), 0);
+      ASSERT_EQ(pool->reclaim(100, stats_), 0);
+      ASSERT_EQ(pool->reclaim(kMaxMemory, stats_), 0);


Can you check the stats_ are all zero?

xiaoxmeng · 2023-10-05T06:01:22Z

velox/common/memory/tests/MemoryArbitratorTest.cpp

@@ -591,15 +595,16 @@ TEST_F(MemoryReclaimerTest, orderedReclaim) {
  // child and 4 from 4th child and 1 from 2nd.
  // So expected reclaimable allocation units are {0, 0, 2, *0*, *0*}
  ASSERT_EQ(
-      root->reclaimer()->reclaim(root.get(), 10 * allocUnitBytes),
+      root->reclaimer()->reclaim(root.get(), 10 * allocUnitBytes, stats_),


Can we have a fake test here to verify the stats has been bumped and aggregated? Thanks!

velox/exec/OrderBy.h

xiaoxmeng · 2023-10-05T06:05:07Z

velox/common/memory/tests/SharedArbitratorTest.cpp

+    allocation1.pool = op->pool();
+    allocation1.size = 16 * MB;
+
+    TestAllocation ret;


return TestAllocation{}

You didn't change?

velox/common/memory/tests/SharedArbitratorTest.cpp

xiaoxmeng

@tanjialiang thanks for the update!

xiaoxmeng · 2023-10-06T02:07:49Z

velox/common/memory/MemoryArbitrator.cpp

@@ -243,6 +244,17 @@ void MemoryReclaimer::abort(MemoryPool* pool, const std::exception_ptr& error) {
  });
 }

+bool MemoryReclaimer::Stats::operator==(
+    const MemoryReclaimer::Stats& other) const {
+  return std::tie(numNonReclaimableAttempts) ==


Don't need for std::tie as there is only one element

For later extension. But yeah we can just compare the single variable for now.

xiaoxmeng · 2023-10-06T02:11:20Z

velox/exec/tests/AggregationTest.cpp

      // The hash table itself in the grouping set is not cleared so it still
      // uses some memory.
      ASSERT_LT(op->pool()->currentBytes(), usedMemory);
    } else {
      VELOX_ASSERT_THROW(
          op->reclaim(
-              folly::Random::oneIn(2) ? 0 : folly::Random::rand32(rng_)),
+              folly::Random::oneIn(2) ? 0 : folly::Random::rand32(rng_),
+              reclaimerStats_),


Can you add check stats for these tests to improve coverage? thanks!

xiaoxmeng · 2023-10-06T02:11:36Z

velox/exec/tests/OrderByTest.cpp

@@ -216,6 +216,7 @@ class OrderByTest : public OperatorTestBase {
  }

  folly::Random::DefaultGenerator rng_;
+  memory::MemoryReclaimer::Stats reclaimerStats_;


xiaoxmeng · 2023-10-06T02:11:48Z

velox/exec/tests/HashJoinTest.cpp

@@ -845,6 +845,7 @@ class HashJoinTest : public HiveConnectorTestBase {
  RowTypePtr probeType_;
  RowTypePtr buildType_;

+  memory::MemoryReclaimer::Stats reclaimerStats_;


xiaoxmeng · 2023-10-06T02:12:32Z

velox/common/memory/tests/SharedArbitratorTest.cpp

+    allocation1.pool = op->pool();
+    allocation1.size = 16 * MB;
+
+    TestAllocation ret;


You didn't change?

xiaoxmeng

@tanjialiang LGTM. We could consider to add actual reclaim time and reclaim wait time in this stats to tell how much time spent on spilling and how much time spent on waiting for task to pause.

facebook-github-bot · 2023-10-06T18:10:20Z

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

xiaoxmeng

@tanjialiang thanks for the update!

xiaoxmeng · 2023-10-06T18:34:56Z

velox/exec/tests/HashJoinTest.cpp

@@ -5538,6 +5557,7 @@ DEBUG_ONLY_TEST_F(HashJoinTest, reclaimDuringWaitForProbe) {
  task.reset();

  taskThread.join();
+  ASSERT_EQ(reclaimerStats_, memory::MemoryReclaimer::Stats{});


Consider to provide empty() in followup. Thanks!

facebook-github-bot · 2023-10-07T04:38:43Z

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

facebook-github-bot · 2023-10-07T06:27:33Z

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

facebook-github-bot · 2023-10-07T21:23:24Z

@tanjialiang merged this pull request in 89a9eb1.

conbench-facebook · 2023-10-07T22:00:51Z

Conbench analyzed the 1 benchmark run on commit 89a9eb1f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: During reclaiming there are information that also needs to be collected. Adding the stats collection framework to facilitate reclaiming stats collection. Pull Request resolved: facebookincubator#6895 Reviewed By: xiaoxmeng Differential Revision: D50020233 Pulled By: tanjialiang fbshipit-source-id: 9ebd78e9b0bfdf26652fd7fa9a452a2ffaf93869

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

xiaoxmeng reviewed Oct 4, 2023

View reviewed changes

tanjialiang force-pushed the reclaim_stats branch from 52ca239 to 6a581b3 Compare October 5, 2023 00:26

xiaoxmeng reviewed Oct 5, 2023

View reviewed changes

tanjialiang force-pushed the reclaim_stats branch 5 times, most recently from 9f328d6 to bdb72fe Compare October 6, 2023 01:51

xiaoxmeng reviewed Oct 6, 2023

View reviewed changes

tanjialiang force-pushed the reclaim_stats branch 2 times, most recently from cfcb787 to 99e204a Compare October 6, 2023 07:29

xiaoxmeng approved these changes Oct 6, 2023

View reviewed changes

tanjialiang force-pushed the reclaim_stats branch from 99e204a to 56a5ecd Compare October 7, 2023 04:38

Add stats collection to memory reclaimer

88a7f41

tanjialiang force-pushed the reclaim_stats branch from 56a5ecd to 88a7f41 Compare October 7, 2023 06:27

facebook-github-bot closed this in 89a9eb1 Oct 7, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stats collection to memory reclaimer #6895

Add stats collection to memory reclaimer #6895

tanjialiang commented Oct 4, 2023

netlify bot commented Oct 4, 2023 •

edited

Loading

xiaoxmeng left a comment

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng Oct 4, 2023

xiaoxmeng left a comment

xiaoxmeng Oct 5, 2023

xiaoxmeng Oct 5, 2023

xiaoxmeng Oct 5, 2023

xiaoxmeng Oct 5, 2023

xiaoxmeng Oct 6, 2023

xiaoxmeng left a comment

xiaoxmeng Oct 6, 2023

tanjialiang Oct 6, 2023

xiaoxmeng Oct 6, 2023

xiaoxmeng Oct 6, 2023

xiaoxmeng Oct 6, 2023

xiaoxmeng Oct 6, 2023

xiaoxmeng left a comment

facebook-github-bot commented Oct 6, 2023

xiaoxmeng left a comment

xiaoxmeng Oct 6, 2023

facebook-github-bot commented Oct 7, 2023

facebook-github-bot commented Oct 7, 2023

facebook-github-bot commented Oct 7, 2023

conbench-facebook bot commented Oct 7, 2023

Add stats collection to memory reclaimer #6895

Add stats collection to memory reclaimer #6895

Conversation

tanjialiang commented Oct 4, 2023

netlify bot commented Oct 4, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

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

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

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

xiaoxmeng left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 6, 2023

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 7, 2023

facebook-github-bot commented Oct 7, 2023

facebook-github-bot commented Oct 7, 2023

conbench-facebook bot commented Oct 7, 2023

netlify bot commented Oct 4, 2023 •

edited

Loading