From c44d5f6c18baa311300bc4260a721f21d590b363 Mon Sep 17 00:00:00 2001 From: Jialiang Tan Date: Thu, 31 Oct 2024 21:57:54 -0700 Subject: [PATCH] Change memory-reclaim-max-wait-time to max-memory-arbitration-time --- velox/common/memory/SharedArbitrator.cpp | 13 ++++++++++++- velox/common/memory/SharedArbitrator.h | 7 +++++++ .../memory/tests/MockSharedArbitratorTest.cpp | 14 +++++++------- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/velox/common/memory/SharedArbitrator.cpp b/velox/common/memory/SharedArbitrator.cpp index 4c8f1426e9849..b4ebda15eb347 100644 --- a/velox/common/memory/SharedArbitrator.cpp +++ b/velox/common/memory/SharedArbitrator.cpp @@ -94,6 +94,17 @@ uint64_t SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity( config::CapacityUnit::BYTE); } +uint64_t SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs( + const std::unordered_map& configs) { + return std::chrono::duration_cast( + config::toDuration(getConfig( + configs, + kMaxMemoryArbitrationTime, + std::string(kDefaultMaxMemoryArbitrationTime)))) + .count(); +} + +// TODO: Remove after name change complete uint64_t SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs( const std::unordered_map& configs) { return std::chrono::duration_cast( @@ -206,7 +217,7 @@ SharedArbitrator::SharedArbitrator(const Config& config) reservedCapacity_(ExtraConfig::reservedCapacity(config.extraConfigs)), checkUsageLeak_(ExtraConfig::checkUsageLeak(config.extraConfigs)), maxArbitrationTimeNs_( - ExtraConfig::memoryReclaimMaxWaitTimeNs(config.extraConfigs)), + ExtraConfig::maxMemoryArbitrationTimeNs(config.extraConfigs)), participantConfig_( ExtraConfig::memoryPoolInitialCapacity(config.extraConfigs), ExtraConfig::memoryPoolReservedCapacity(config.extraConfigs), diff --git a/velox/common/memory/SharedArbitrator.h b/velox/common/memory/SharedArbitrator.h index 1f39ddb3858db..702c3f3402c38 100644 --- a/velox/common/memory/SharedArbitrator.h +++ b/velox/common/memory/SharedArbitrator.h @@ -81,6 +81,13 @@ class SharedArbitrator : public memory::MemoryArbitrator { /// the memory arbitration from getting stuck when the memory reclaim waits /// for a hanging query task to pause. If it is zero, then there is no /// timeout. + static constexpr std::string_view kMaxMemoryArbitrationTime{ + "max-memory-arbitration-time"}; + static constexpr std::string_view kDefaultMaxMemoryArbitrationTime{"5m"}; + static uint64_t maxMemoryArbitrationTimeNs( + const std::unordered_map& configs); + + // TODO: Remove after name change complete static constexpr std::string_view kMemoryReclaimMaxWaitTime{ "memory-reclaim-max-wait-time"}; static constexpr std::string_view kDefaultMemoryReclaimMaxWaitTime{"5m"}; diff --git a/velox/common/memory/tests/MockSharedArbitratorTest.cpp b/velox/common/memory/tests/MockSharedArbitratorTest.cpp index 2acdb3f6cac2d..61efea38717b1 100644 --- a/velox/common/memory/tests/MockSharedArbitratorTest.cpp +++ b/velox/common/memory/tests/MockSharedArbitratorTest.cpp @@ -479,7 +479,7 @@ class MockSharedArbitrationTest : public testing::Test { folly::to(globalArbitrationReclaimPct)}, {std::string(ExtraConfig::kMemoryReclaimThreadsHwMultiplier), folly::to(memoryReclaimThreadsHwMultiplier)}, - {std::string(ExtraConfig::kMemoryReclaimMaxWaitTime), + {std::string(ExtraConfig::kMaxMemoryArbitrationTime), folly::to(arbitrationTimeoutNs) + "ns"}, {std::string(ExtraConfig::kGlobalArbitrationEnabled), folly::to(globalArtbitrationEnabled)}, @@ -576,7 +576,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { SharedArbitrator::ExtraConfig::memoryPoolInitialCapacity(emptyConfigs), 256 << 20); ASSERT_EQ( - SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(emptyConfigs), + SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(emptyConfigs), 300'000'000'000UL); ASSERT_EQ( SharedArbitrator::ExtraConfig::globalArbitrationEnabled(emptyConfigs), @@ -616,7 +616,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { configs[std::string( SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "200B"; configs[std::string( - SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "5000ms"; + SharedArbitrator::ExtraConfig::kMaxMemoryArbitrationTime)] = "5000ms"; configs[std::string( SharedArbitrator::ExtraConfig::kGlobalArbitrationEnabled)] = "true"; configs[std::string(SharedArbitrator::ExtraConfig::kCheckUsageLeak)] = @@ -643,8 +643,8 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { ASSERT_EQ( SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity(configs), 200); ASSERT_EQ( - SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(configs), - 5'000'000'000); + SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(configs), + 5'000'000'000UL); ASSERT_TRUE(SharedArbitrator::ExtraConfig::globalArbitrationEnabled(configs)); ASSERT_FALSE(SharedArbitrator::ExtraConfig::checkUsageLeak(configs)); ASSERT_EQ( @@ -674,7 +674,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { configs[std::string( SharedArbitrator::ExtraConfig::kMemoryPoolReservedCapacity)] = "invalid"; configs[std::string( - SharedArbitrator::ExtraConfig::kMemoryReclaimMaxWaitTime)] = "invalid"; + SharedArbitrator::ExtraConfig::kMaxMemoryArbitrationTime)] = "invalid"; configs[std::string( SharedArbitrator::ExtraConfig::kGlobalArbitrationEnabled)] = "invalid"; configs[std::string(SharedArbitrator::ExtraConfig::kCheckUsageLeak)] = @@ -707,7 +707,7 @@ TEST_F(MockSharedArbitrationTest, extraConfigs) { SharedArbitrator::ExtraConfig::memoryPoolReservedCapacity(configs), "Invalid capacity string 'invalid'"); VELOX_ASSERT_THROW( - SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(configs), + SharedArbitrator::ExtraConfig::maxMemoryArbitrationTimeNs(configs), "Invalid duration 'invalid'"); VELOX_ASSERT_THROW( SharedArbitrator::ExtraConfig::globalArbitrationEnabled(configs),