Add shrinkPool API to MemoryManager/MemoryArbitrator #6102

tanjialiang · 2023-08-14T17:01:57Z

Adds shrinkPool API to MemoryManager and MemoryArbitrator to allow manual memory give back to MemoryAribitrator outside of arbitration process. This allows more flexible integration with external arbitration system for customized purposes.

netlify · 2023-08-14T17:02:02Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`262f97a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64dd0d65f50ec5000831760e

xiaoxmeng

@tanjialiang thanks for the change. LGTM % minors.

xiaoxmeng · 2023-08-15T04:33:38Z

velox/common/memory/Memory.h

@@ -157,6 +157,11 @@ class MemoryManager {
  /// 'incrementBytes'. The function returns true on success, otherwise false.
  bool growPool(MemoryPool* pool, uint64_t incrementBytes);

+  /// Invoked to shrink currently alive pools by 'targetBytes', by reducing this


Remove "reducing this"? we can trigger memory reclaiming?

Consider

/// Invoked to shrink alive pools to free at least 'targetBytes' capacity. The function returns the actual freed memory capacity in bytes.

I removed 'at least' because there might not be enough available memory to free to 'targetBytes' so the actual bytes freed might be smaller than 'targetBytes'. Also I think this is arbitrator implementation dependent so leaving more flexibility in words.

xiaoxmeng · 2023-08-15T04:36:24Z

velox/common/memory/Memory.cpp

@@ -181,6 +181,11 @@ bool MemoryManager::growPool(MemoryPool* pool, uint64_t incrementBytes) {
  return arbitrator_->growMemory(pool, candidates, incrementBytes);
 }

+uint64_t MemoryManager::shrinkPools(uint64_t targetBytes) {
+  VELOX_CHECK_NOT_NULL(arbitrator_);
+  return arbitrator_->shrinkMemory(getAlivePools(), targetBytes);


getAlivePools() holds the memory pool while shrinking memory. So let's change to the following (as what growPool does)

// Holds a shared reference to each alive memory pool in 'pools_' to keep // their aliveness during the memory arbitration process. std::vector<std::shared_ptr<MemoryPool>> candidates = getAlivePools(); return arbitrator_->shrinkMemory(candidates, targetBytes);

What's the difference? Calling getAlivePools() will pass a copy of every shared_ptr to shrinkMemory() and hence keeping them alive. growPools() should make them a single line for simplicity.

You are right. Could you make a change to growMemory? Thanks!

xiaoxmeng · 2023-08-15T04:38:04Z

velox/common/memory/MemoryArbitrator.h

@@ -140,6 +140,13 @@ class MemoryArbitrator {
      const std::vector<std::shared_ptr<MemoryPool>>& candidatePools,
      uint64_t targetBytes) = 0;

+  /// Invoked by the memory manager to shrink memory for a given vector of


/// Invoked by the memory manager to shrink memory from a given set/list? of memory pools. The freed memory capacity is given back to the arbitrator. The function returns the actual freed memory capacity in bytes.

xiaoxmeng · 2023-08-15T04:38:57Z

velox/common/memory/tests/MemoryArbitratorTest.cpp

@@ -95,6 +95,10 @@ class FakeTestArbitrator : public MemoryArbitrator {
      const std::vector<std::shared_ptr<MemoryPool>>& candidatePools,
      uint64_t targetBytes) override{VELOX_NYI()}

+  uint64_t shrinkMemory(
+      const std::vector<std::shared_ptr<MemoryPool>>& pools,
+      uint64_t targetBytes) override{VELOX_NYI()}


VELOX_NYI();

The same to the other places in this file. Thanks!

xiaoxmeng · 2023-08-15T04:39:05Z

velox/common/memory/tests/MemoryManagerTest.cpp

@@ -113,6 +113,10 @@ class FakeTestArbitrator : public MemoryArbitrator {
      const std::vector<std::shared_ptr<MemoryPool>>& candidatePools,
      uint64_t targetBytes) override{VELOX_NYI()}

+  uint64_t shrinkMemory(
+      const std::vector<std::shared_ptr<MemoryPool>>& pools,
+      uint64_t targetBytes) override{VELOX_NYI()}


facebook-github-bot · 2023-08-15T18:29:49Z

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

kgpai · 2023-08-16T17:24:34Z

@tanjialiang can you investigate why we see an inf loop during the presto fuzzer runs ?

kgpai · 2023-08-16T17:40:12Z

@tanjialiang Command to use :

./_build/debug/velox/expression/tests/velox_expression_fuzzer_test --seed 604401866 --lazy_vector_generation_ratio 0.2 \
--duration_sec 600 \
--enable_variadic_signatures \
--velox_fuzzer_enable_complex_types \
--velox_fuzzer_enable_column_reuse \
--velox_fuzzer_enable_expression_reuse \
--max_expression_trees_per_step 2 \
--retry_with_try \
--enable_dereference \
--logtostderr=1 --minloglevel=0 \
--repro_persist_path=/tmp/fuzzer_repro

facebook-github-bot · 2023-08-16T18:01:08Z

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

facebook-github-bot · 2023-08-17T17:25:34Z

@tanjialiang merged this pull request in d2ebb4e.

conbench-facebook · 2023-08-17T18:02:51Z

Conbench analyzed the 1 benchmark run on commit d2ebb4e3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…or#6102) Summary: Pull Request resolved: facebookincubator#6102 Reviewed By: xiaoxmeng Differential Revision: D48362262 Pulled By: tanjialiang fbshipit-source-id: 5ac3d1deef75cb6a745a4d8644d2f7fc398d5469

Summary: Add NoopArbitrator and move the default arbitration behaviors defined in MemoryManager into it. This would make it clear to user to understand what will happen around memory pool's capacity when the arbitrator is not set explicitly. This also reduces code complexity in memory manager by removing "if (arbitrator_ == nullptr)" if-else blocks. Also, fixed some codes which may cause static initialization order issue. Based on #6102 's apporved code. Pull Request resolved: #6127 Reviewed By: tanjialiang Differential Revision: D48593508 Pulled By: xiaoxmeng fbshipit-source-id: f74cac2e096a03a523eb55afb65ca308c3d6c2ca

…or#6102) Summary: Pull Request resolved: facebookincubator#6102 Reviewed By: xiaoxmeng Differential Revision: D48362262 Pulled By: tanjialiang fbshipit-source-id: 5ac3d1deef75cb6a745a4d8644d2f7fc398d5469

…or#6127) Summary: Add NoopArbitrator and move the default arbitration behaviors defined in MemoryManager into it. This would make it clear to user to understand what will happen around memory pool's capacity when the arbitrator is not set explicitly. This also reduces code complexity in memory manager by removing "if (arbitrator_ == nullptr)" if-else blocks. Also, fixed some codes which may cause static initialization order issue. Based on facebookincubator#6102 's apporved code. Pull Request resolved: facebookincubator#6127 Reviewed By: tanjialiang Differential Revision: D48593508 Pulled By: xiaoxmeng fbshipit-source-id: f74cac2e096a03a523eb55afb65ca308c3d6c2ca

tanjialiang requested a review from xiaoxmeng August 14, 2023 17:02

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 Aug 14, 2023

tanjialiang force-pushed the shrink_pool branch from c3ccfaf to f192b70 Compare August 14, 2023 17:10

xiaoxmeng approved these changes Aug 15, 2023

View reviewed changes

tanjialiang force-pushed the shrink_pool branch from f192b70 to 3ef57e9 Compare August 16, 2023 05:34

xiaoxmeng approved these changes Aug 16, 2023

View reviewed changes

zhztheplayer mentioned this pull request Aug 16, 2023

Add NoopArbitrator for default arbitration behaviors #6127

Closed

Add shrinkPool API to MemoryManager/MemoryArbitrator

262f97a

tanjialiang force-pushed the shrink_pool branch from 3ef57e9 to 262f97a Compare August 16, 2023 17:54

facebook-github-bot closed this in d2ebb4e Aug 17, 2023

facebook-github-bot added the Merged label Aug 17, 2023

zhztheplayer mentioned this pull request Aug 22, 2023

Let MemoryPool give back memory to arbitrator upon reclaim #6086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shrinkPool API to MemoryManager/MemoryArbitrator #6102

Add shrinkPool API to MemoryManager/MemoryArbitrator #6102

tanjialiang commented Aug 14, 2023 •

edited

Loading

netlify bot commented Aug 14, 2023 •

edited

Loading

xiaoxmeng left a comment

xiaoxmeng Aug 15, 2023

tanjialiang Aug 16, 2023

xiaoxmeng Aug 15, 2023

tanjialiang Aug 16, 2023 •

edited

Loading

xiaoxmeng Aug 16, 2023

xiaoxmeng Aug 15, 2023

xiaoxmeng Aug 15, 2023

xiaoxmeng Aug 15, 2023

facebook-github-bot commented Aug 15, 2023

kgpai commented Aug 16, 2023

kgpai commented Aug 16, 2023

facebook-github-bot commented Aug 16, 2023

facebook-github-bot commented Aug 17, 2023

conbench-facebook bot commented Aug 17, 2023

Add shrinkPool API to MemoryManager/MemoryArbitrator #6102

Add shrinkPool API to MemoryManager/MemoryArbitrator #6102

Conversation

tanjialiang commented Aug 14, 2023 • edited Loading

netlify bot commented Aug 14, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

xiaoxmeng left a comment

Choose a reason for hiding this comment

xiaoxmeng Aug 15, 2023

Choose a reason for hiding this comment

tanjialiang Aug 16, 2023

Choose a reason for hiding this comment

xiaoxmeng Aug 15, 2023

Choose a reason for hiding this comment

tanjialiang Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

xiaoxmeng Aug 16, 2023

Choose a reason for hiding this comment

xiaoxmeng Aug 15, 2023

Choose a reason for hiding this comment

xiaoxmeng Aug 15, 2023

Choose a reason for hiding this comment

xiaoxmeng Aug 15, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Aug 15, 2023

kgpai commented Aug 16, 2023

kgpai commented Aug 16, 2023

facebook-github-bot commented Aug 16, 2023

facebook-github-bot commented Aug 17, 2023

conbench-facebook bot commented Aug 17, 2023

tanjialiang commented Aug 14, 2023 •

edited

Loading

netlify bot commented Aug 14, 2023 •

edited

Loading

tanjialiang Aug 16, 2023 •

edited

Loading