From ce2b90796a82dd92e82ec5943012ac2dad4d49a4 Mon Sep 17 00:00:00 2001 From: Jialiang Tan Date: Thu, 26 Sep 2024 16:56:46 -0700 Subject: [PATCH] Deprecate transfer capacity for shared arbitrator (#11084) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/11084 Reviewed By: xiaoxmeng Differential Revision: D63472474 Pulled By: tanjialiang fbshipit-source-id: 47041957eb41069f1435fa035a4f63bd1134c62c --- velox/common/memory/SharedArbitrator.cpp | 22 ++--------- velox/common/memory/SharedArbitrator.h | 16 +------- .../memory/tests/MemoryArbitratorTest.cpp | 11 +++--- .../memory/tests/MockSharedArbitratorTest.cpp | 38 +++---------------- velox/exec/tests/HashJoinTest.cpp | 2 +- velox/exec/tests/utils/ArbitratorTestUtil.cpp | 3 -- velox/exec/tests/utils/ArbitratorTestUtil.h | 2 - 7 files changed, 17 insertions(+), 77 deletions(-) diff --git a/velox/common/memory/SharedArbitrator.cpp b/velox/common/memory/SharedArbitrator.cpp index beaa9b8cfe43..fd45ec232344 100644 --- a/velox/common/memory/SharedArbitrator.cpp +++ b/velox/common/memory/SharedArbitrator.cpp @@ -109,16 +109,6 @@ uint64_t SharedArbitrator::ExtraConfig::getMemoryPoolReservedCapacity( config::CapacityUnit::BYTE); } -uint64_t SharedArbitrator::ExtraConfig::getMemoryPoolTransferCapacity( - const std::unordered_map& configs) { - return config::toCapacity( - getConfig( - configs, - kMemoryPoolTransferCapacity, - std::string(kDefaultMemoryPoolTransferCapacity)), - config::CapacityUnit::BYTE); -} - uint64_t SharedArbitrator::ExtraConfig::getMemoryReclaimMaxWaitTimeMs( const std::unordered_map& configs) { return std::chrono::duration_cast( @@ -182,8 +172,6 @@ SharedArbitrator::SharedArbitrator(const Config& config) ExtraConfig::getMemoryPoolInitialCapacity(config.extraConfigs)), memoryPoolReservedCapacity_( ExtraConfig::getMemoryPoolReservedCapacity(config.extraConfigs)), - memoryPoolTransferCapacity_( - ExtraConfig::getMemoryPoolTransferCapacity(config.extraConfigs)), memoryReclaimWaitMs_( ExtraConfig::getMemoryReclaimMaxWaitTimeMs(config.extraConfigs)), globalArbitrationEnabled_( @@ -542,7 +530,7 @@ uint64_t SharedArbitrator::getCapacityGrowthTarget( const MemoryPool& pool, uint64_t requestBytes) const { if (fastExponentialGrowthCapacityLimit_ == 0 && slowCapacityGrowPct_ == 0) { - return std::max(requestBytes, memoryPoolTransferCapacity_); + return requestBytes; } uint64_t targetBytes{0}; const auto capacity = pool.capacity(); @@ -551,8 +539,7 @@ uint64_t SharedArbitrator::getCapacityGrowthTarget( } else { targetBytes = capacity * slowCapacityGrowPct_; } - return std::max( - std::max(requestBytes, targetBytes), memoryPoolTransferCapacity_); + return std::max(requestBytes, targetBytes); } bool SharedArbitrator::growCapacity(MemoryPool* pool, uint64_t requestBytes) { @@ -938,9 +925,8 @@ uint64_t SharedArbitrator::reclaim( MemoryPool* pool, uint64_t targetBytes, bool isLocalArbitration) noexcept { - int64_t bytesToReclaim = std::min( - std::max(targetBytes, memoryPoolTransferCapacity_), - maxReclaimableCapacity(*pool, true)); + int64_t bytesToReclaim = + std::min(targetBytes, maxReclaimableCapacity(*pool, true)); if (bytesToReclaim == 0) { return 0; } diff --git a/velox/common/memory/SharedArbitrator.h b/velox/common/memory/SharedArbitrator.h index 6c784f5373f2..a8af9ca7b92b 100644 --- a/velox/common/memory/SharedArbitrator.h +++ b/velox/common/memory/SharedArbitrator.h @@ -61,15 +61,6 @@ class SharedArbitrator : public memory::MemoryArbitrator { static uint64_t getMemoryPoolReservedCapacity( const std::unordered_map& configs); - /// The minimal memory capacity to transfer out of or into a memory pool - /// during the memory arbitration. - static constexpr std::string_view kMemoryPoolTransferCapacity{ - "memory-pool-transfer-capacity"}; - static constexpr std::string_view kDefaultMemoryPoolTransferCapacity{ - "128MB"}; - static uint64_t getMemoryPoolTransferCapacity( - const std::unordered_map& configs); - /// Specifies the max time to wait for memory reclaim by arbitration. The /// memory reclaim might fail if the max time has exceeded. This prevents /// the memory arbitration from getting stuck when the memory reclaim waits @@ -231,11 +222,7 @@ class SharedArbitrator : public memory::MemoryArbitrator { // The adjusted grow bytes based on 'requestBytes'. This 'targetBytes' is a // best effort target, and hence will not be guaranteed. The adjustment is // based on 'SharedArbitrator::fastExponentialGrowthCapacityLimit_' - // 'SharedArbitrator::slowCapacityGrowPct_' and - // 'MemoryArbitrator::memoryPoolTransferCapacity_'. - // - // TODO: deprecate 'MemoryArbitrator::memoryPoolTransferCapacity_' once - // exponential growth works well in production. + // 'SharedArbitrator::slowCapacityGrowPct_' const std::optional targetBytes; // The start time of this arbitration operation. @@ -516,7 +503,6 @@ class SharedArbitrator : public memory::MemoryArbitrator { const uint64_t reservedCapacity_; const uint64_t memoryPoolInitialCapacity_; const uint64_t memoryPoolReservedCapacity_; - const uint64_t memoryPoolTransferCapacity_; const uint64_t memoryReclaimWaitMs_; const bool globalArbitrationEnabled_; const bool checkUsageLeak_; diff --git a/velox/common/memory/tests/MemoryArbitratorTest.cpp b/velox/common/memory/tests/MemoryArbitratorTest.cpp index 61a956d7dde6..2e71aa4390a0 100644 --- a/velox/common/memory/tests/MemoryArbitratorTest.cpp +++ b/velox/common/memory/tests/MemoryArbitratorTest.cpp @@ -144,6 +144,7 @@ TEST_F(MemoryArbitrationTest, queryMemoryCapacity) { ASSERT_FALSE(manager.arbitrator()->growCapacity(rootPool.get(), 6 << 20)); ASSERT_EQ(rootPool->capacity(), 1 << 20); ASSERT_TRUE(manager.arbitrator()->growCapacity(rootPool.get(), 2 << 20)); + ASSERT_TRUE(manager.arbitrator()->growCapacity(rootPool.get(), 1 << 20)); ASSERT_EQ(rootPool->capacity(), 4 << 20); ASSERT_EQ(manager.arbitrator()->stats().freeCapacityBytes, 2 << 20); ASSERT_EQ(manager.arbitrator()->stats().freeReservedCapacityBytes, 2 << 20); @@ -154,19 +155,19 @@ TEST_F(MemoryArbitrationTest, queryMemoryCapacity) { "Exceeded memory pool capacity after attempt to grow capacity through " "arbitration. Requestor pool name 'leaf-1.0', request size 7.00MB, " "memory pool capacity 4.00MB, memory pool max capacity 8.00MB"); - ASSERT_EQ(manager.arbitrator()->shrinkCapacity(rootPool.get(), 0), 1 << 20); + ASSERT_EQ(manager.arbitrator()->shrinkCapacity(rootPool.get(), 0), 0); ASSERT_EQ(manager.arbitrator()->shrinkCapacity(leafPool.get(), 0), 0); ASSERT_EQ(manager.arbitrator()->shrinkCapacity(leafPool.get(), 1), 0); ASSERT_EQ(manager.arbitrator()->shrinkCapacity(rootPool.get(), 1), 0); - ASSERT_EQ(rootPool->capacity(), 3 << 20); + ASSERT_EQ(rootPool->capacity(), 4 << 20); static_cast(rootPool.get())->testingSetReservation(0); ASSERT_EQ( manager.arbitrator()->shrinkCapacity(leafPool.get(), 1 << 20), 1 << 20); ASSERT_EQ( manager.arbitrator()->shrinkCapacity(rootPool.get(), 1 << 20), 1 << 20); - ASSERT_EQ(rootPool->capacity(), 1 << 20); - ASSERT_EQ(leafPool->capacity(), 1 << 20); - ASSERT_EQ(manager.arbitrator()->shrinkCapacity(leafPool.get(), 0), 1 << 20); + ASSERT_EQ(rootPool->capacity(), 2 << 20); + ASSERT_EQ(leafPool->capacity(), 2 << 20); + ASSERT_EQ(manager.arbitrator()->shrinkCapacity(leafPool.get(), 0), 2 << 20); ASSERT_EQ(rootPool->capacity(), 0); ASSERT_EQ(leafPool->capacity(), 0); } diff --git a/velox/common/memory/tests/MockSharedArbitratorTest.cpp b/velox/common/memory/tests/MockSharedArbitratorTest.cpp index cc32ef111791..320aa892fc53 100644 --- a/velox/common/memory/tests/MockSharedArbitratorTest.cpp +++ b/velox/common/memory/tests/MockSharedArbitratorTest.cpp @@ -65,8 +65,6 @@ constexpr int64_t MB = 1024L * KB; constexpr uint64_t kMemoryCapacity = 512 * MB; constexpr uint64_t kReservedMemoryCapacity = 128 * MB; constexpr uint64_t kMemoryPoolInitCapacity = 16 * MB; -// TODO(jtan6): Remove after complete transfer capacity deprecation -constexpr uint64_t kMemoryPoolTransferCapacity = 0; constexpr uint64_t kMemoryPoolReservedCapacity = 8 * MB; constexpr uint64_t kFastExponentialGrowthCapacityLimit = 32 * MB; constexpr double kSlowCapacityGrowPct = 0.25; @@ -429,7 +427,6 @@ class MockSharedArbitrationTest : public testing::Test { int64_t reservedMemoryCapacity = kReservedMemoryCapacity, uint64_t memoryPoolInitCapacity = kMemoryPoolInitCapacity, uint64_t memoryPoolReserveCapacity = kMemoryPoolReservedCapacity, - uint64_t memoryPoolTransferCapacity = kMemoryPoolTransferCapacity, uint64_t fastExponentialGrowthCapacityLimit = kFastExponentialGrowthCapacityLimit, double slowCapacityGrowPct = kSlowCapacityGrowPct, @@ -450,8 +447,6 @@ class MockSharedArbitrationTest : public testing::Test { folly::to(memoryPoolInitCapacity) + "B"}, {std::string(ExtraConfig::kMemoryPoolReservedCapacity), folly::to(memoryPoolReserveCapacity) + "B"}, - {std::string(ExtraConfig::kMemoryPoolTransferCapacity), - folly::to(memoryPoolTransferCapacity) + "B"}, {std::string(ExtraConfig::kFastExponentialGrowthCapacityLimit), folly::to(fastExponentialGrowthCapacityLimit) + "B"}, {std::string(ExtraConfig::kSlowCapacityGrowPct), @@ -555,10 +550,6 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { ASSERT_EQ( SharedArbitrator::ExtraConfig::getMemoryPoolInitialCapacity(emptyConfigs), 256 << 20); - ASSERT_EQ( - SharedArbitrator::ExtraConfig::getMemoryPoolTransferCapacity( - emptyConfigs), - 128 << 20); ASSERT_EQ( SharedArbitrator::ExtraConfig::getMemoryReclaimMaxWaitTimeMs( emptyConfigs), @@ -578,8 +569,6 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { SharedArbitrator::ExtraConfig::kMemoryPoolInitialCapacity)] = "512MB"; configs[std::string( SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "200B"; - configs[std::string( - SharedArbitrator::ExtraConfig::kMemoryPoolTransferCapacity)] = "256MB"; configs[std::string( SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "5000ms"; configs[std::string( @@ -593,9 +582,6 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { ASSERT_EQ( SharedArbitrator::ExtraConfig::getMemoryPoolReservedCapacity(configs), 200); - ASSERT_EQ( - SharedArbitrator::ExtraConfig::getMemoryPoolTransferCapacity(configs), - 256 << 20); ASSERT_EQ( SharedArbitrator::ExtraConfig::getMemoryReclaimMaxWaitTimeMs(configs), 5000); @@ -610,8 +596,6 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { SharedArbitrator::ExtraConfig::kMemoryPoolInitialCapacity)] = "invalid"; configs[std::string( SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "invalid"; - configs[std::string( - SharedArbitrator::ExtraConfig::kMemoryPoolTransferCapacity)] = "invalid"; configs[std::string( SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "invalid"; configs[std::string( @@ -627,9 +611,6 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { VELOX_ASSERT_THROW( SharedArbitrator::ExtraConfig::getMemoryPoolReservedCapacity(configs), "Invalid capacity string 'invalid'"); - VELOX_ASSERT_THROW( - SharedArbitrator::ExtraConfig::getMemoryPoolTransferCapacity(configs), - "Invalid capacity string 'invalid'"); VELOX_ASSERT_THROW( SharedArbitrator::ExtraConfig::getMemoryReclaimMaxWaitTimeMs(configs), "Invalid duration 'invalid'"); @@ -678,7 +659,7 @@ TEST_F(MockSharedArbitrationTest, arbitrationStateCheck) { ASSERT_TRUE(RE2::FullMatch(pool.name(), re)) << pool.name(); ++checkCount; }; - setupMemory(memCapacity, 0, 0, 0, 0, 0, 0, 0, 0, checkCountCb); + setupMemory(memCapacity, 0, 0, 0, 0, 0, 0, 0, checkCountCb); const int numTasks{5}; std::vector> tasks; @@ -703,7 +684,7 @@ TEST_F(MockSharedArbitrationTest, arbitrationStateCheck) { MemoryArbitrationStateCheckCB badCheckCb = [&](MemoryPool& /*unused*/) { VELOX_FAIL("bad check"); }; - setupMemory(memCapacity, 0, 0, 0, 0, 0, 0, 0, 0, badCheckCb); + setupMemory(memCapacity, 0, 0, 0, 0, 0, 0, 0, badCheckCb); std::shared_ptr task = addTask(kMemoryCapacity); ASSERT_EQ(task->capacity(), 0); MockMemoryOperator* memOp = task->addMemoryOp(); @@ -1238,7 +1219,6 @@ DEBUG_ONLY_TEST_F( 0, memoryPoolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, 0, @@ -1331,7 +1311,6 @@ DEBUG_ONLY_TEST_F( 0, memoryPoolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, 0, @@ -1615,7 +1594,6 @@ DEBUG_ONLY_TEST_F(MockSharedArbitrationTest, globalArbitrationEnableCheck) { 0, memoryPoolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, kMemoryPoolMinFreeCapacity, @@ -1652,7 +1630,6 @@ DEBUG_ONLY_TEST_F( 0, memoryPoolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, 0, @@ -1871,7 +1848,6 @@ TEST_F(MockSharedArbitrationTest, singlePoolShrinkWithoutArbitration) { 0, 0, 0, - 0, testParam.memoryPoolMinFreeCapacity, testParam.memoryPoolMinFreeCapacityPct), "both need to be set (non-zero) at the same time to enable shrink " @@ -1885,7 +1861,6 @@ TEST_F(MockSharedArbitrationTest, singlePoolShrinkWithoutArbitration) { 0, 0, 0, - 0, testParam.memoryPoolMinFreeCapacity, testParam.memoryPoolMinFreeCapacityPct); } @@ -1931,7 +1906,6 @@ TEST_F(MockSharedArbitrationTest, singlePoolGrowWithoutArbitration) { 0, memoryPoolInitCapacity, 0, - 0, testParam.fastExponentialGrowthCapacityLimit, testParam.slowCapacityGrowPct); @@ -2146,7 +2120,6 @@ TEST_F(MockSharedArbitrationTest, ensureMemoryPoolMaxCapacity) { 0, poolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, 0, @@ -2681,7 +2654,7 @@ DEBUG_ONLY_TEST_F(MockSharedArbitrationTest, failedToReclaimFromRequestor) { 0}}; for (const auto& testData : testSettings) { SCOPED_TRACE(testData.debugString()); - setupMemory(kMemoryCapacity, 0, kMemoryPoolInitCapacity, 0, 0, 0, 0, 0, 0); + setupMemory(kMemoryCapacity, 0, kMemoryPoolInitCapacity, 0, 0, 0, 0, 0); std::vector> otherTasks; std::vector otherTaskOps; @@ -2865,7 +2838,7 @@ DEBUG_ONLY_TEST_F(MockSharedArbitrationTest, failedToReclaimFromOtherTask) { nonFailTaskMemoryCapacity}}; for (const auto& testData : testSettings) { SCOPED_TRACE(testData.debugString()); - setupMemory(kMemoryCapacity, 0, kMemoryPoolInitCapacity, 0, 0, 0, 0, 0, 0); + setupMemory(kMemoryCapacity, 0, kMemoryPoolInitCapacity, 0, 0, 0, 0, 0); std::vector> nonFailedTasks; std::vector nonFailedTaskOps; @@ -3015,7 +2988,6 @@ TEST_F(MockSharedArbitrationTest, memoryPoolAbortThrow) { 0, kMemoryPoolInitCapacity, 0, - 0, kFastExponentialGrowthCapacityLimit, kSlowCapacityGrowPct, 0, @@ -3065,7 +3037,7 @@ TEST_F(MockSharedArbitrationTest, memoryPoolAbortThrow) { // This test makes sure the memory capacity grows as expected. DEBUG_ONLY_TEST_F(MockSharedArbitrationTest, concurrentArbitrationRequests) { - setupMemory(kMemoryCapacity, 0, 0, 0, 128 << 20); + setupMemory(kMemoryCapacity, 0, 0, 0); std::shared_ptr task = addTask(); MockMemoryOperator* op1 = addMemoryOp(task); MockMemoryOperator* op2 = addMemoryOp(task); diff --git a/velox/exec/tests/HashJoinTest.cpp b/velox/exec/tests/HashJoinTest.cpp index 7c497c55cb50..9ce08a5e5e5e 100644 --- a/velox/exec/tests/HashJoinTest.cpp +++ b/velox/exec/tests/HashJoinTest.cpp @@ -7506,7 +7506,7 @@ DEBUG_ONLY_TEST_F(HashJoinTest, taskWaitTimeout) { for (uint64_t timeoutMs : {0, 1'000, 30'000}) { SCOPED_TRACE(fmt::format("timeout {}", succinctMillis(timeoutMs))); - auto memoryManager = createMemoryManager(512 << 20, 0, 0, timeoutMs); + auto memoryManager = createMemoryManager(512 << 20, 0, timeoutMs); auto queryCtx = newQueryCtx(memoryManager.get(), executor_.get(), queryMemoryCapacity); diff --git a/velox/exec/tests/utils/ArbitratorTestUtil.cpp b/velox/exec/tests/utils/ArbitratorTestUtil.cpp index da19a289eb2c..160d4e5d228a 100644 --- a/velox/exec/tests/utils/ArbitratorTestUtil.cpp +++ b/velox/exec/tests/utils/ArbitratorTestUtil.cpp @@ -45,7 +45,6 @@ std::shared_ptr newQueryCtx( std::unique_ptr createMemoryManager( int64_t arbitratorCapacity, uint64_t memoryPoolInitCapacity, - uint64_t memoryPoolTransferCapacity, uint64_t maxReclaimWaitMs, uint64_t fastExponentialGrowthCapacityLimit, double slowCapacityGrowPct) { @@ -59,8 +58,6 @@ std::unique_ptr createMemoryManager( options.extraArbitratorConfigs = { {std::string(ExtraConfig::kMemoryPoolInitialCapacity), folly::to(memoryPoolInitCapacity) + "B"}, - {std::string(ExtraConfig::kMemoryPoolTransferCapacity), - folly::to(memoryPoolTransferCapacity) + "B"}, {std::string(ExtraConfig::kMemoryReclaimMaxWaitTime), folly::to(maxReclaimWaitMs) + "ms"}, {std::string(ExtraConfig::kGlobalArbitrationEnabled), "true"}, diff --git a/velox/exec/tests/utils/ArbitratorTestUtil.h b/velox/exec/tests/utils/ArbitratorTestUtil.h index 9aa50153b17d..3c1cb6191b98 100644 --- a/velox/exec/tests/utils/ArbitratorTestUtil.h +++ b/velox/exec/tests/utils/ArbitratorTestUtil.h @@ -33,7 +33,6 @@ constexpr int64_t MB = 1024L * KB; constexpr uint64_t kMemoryCapacity = 512 * MB; constexpr uint64_t kMemoryPoolInitCapacity = 16 * MB; -constexpr uint64_t kMemoryPoolTransferCapacity = 8 * MB; class FakeMemoryReclaimer : public exec::MemoryReclaimer { public: @@ -95,7 +94,6 @@ std::shared_ptr newQueryCtx( std::unique_ptr createMemoryManager( int64_t arbitratorCapacity = kMemoryCapacity, uint64_t memoryPoolInitCapacity = kMemoryPoolInitCapacity, - uint64_t memoryPoolTransferCapacity = kMemoryPoolTransferCapacity, uint64_t maxReclaimWaitMs = 0, uint64_t fastExponentialGrowthCapacityLimit = 0, double slowCapacityGrowPct = 0);