Change memory-reclaim-max-wait-time to max-memory-arbitration-time (#…

…11405) Summary: Pull Request resolved: #11405 Reviewed By: xiaoxmeng Differential Revision: D65320186 Pulled By: tanjialiang fbshipit-source-id: fb1e8d9a52674400e323b008fc64ba934c43dfda
facebookincubator · Nov 14, 2024 · ebd597a · ebd597a
1 parent 88e3e61
commit ebd597a
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 11 deletions.
diff --git 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<std::string, std::string>& configs) {
+  return std::chrono::duration_cast<std::chrono::nanoseconds>(
+             config::toDuration(getConfig<std::string>(
+                 configs,
+                 kMaxMemoryArbitrationTime,
+                 std::string(kDefaultMaxMemoryArbitrationTime))))
+      .count();
+}
+
+// TODO: Remove after name change complete
 uint64_t SharedArbitrator::ExtraConfig::memoryReclaimMaxWaitTimeNs(
     const std::unordered_map<std::string, std::string>& configs) {
   return std::chrono::duration_cast<std::chrono::nanoseconds>(
@@ -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
@@ -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<std::string, std::string>& 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
@@ -479,7 +479,7 @@ class MockSharedArbitrationTest : public testing::Test {
          folly::to<std::string>(globalArbitrationReclaimPct)},
         {std::string(ExtraConfig::kMemoryReclaimThreadsHwMultiplier),
          folly::to<std::string>(memoryReclaimThreadsHwMultiplier)},
-        {std::string(ExtraConfig::kMemoryReclaimMaxWaitTime),
+        {std::string(ExtraConfig::kMaxMemoryArbitrationTime),
          folly::to<std::string>(arbitrationTimeoutNs) + "ns"},
         {std::string(ExtraConfig::kGlobalArbitrationEnabled),
          folly::to<std::string>(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),

diff --git a/velox/exec/tests/utils/ArbitratorTestUtil.cpp b/velox/exec/tests/utils/ArbitratorTestUtil.cpp
@@ -45,7 +45,7 @@ std::shared_ptr<core::QueryCtx> newQueryCtx(
 std::unique_ptr<memory::MemoryManager> createMemoryManager(
     int64_t arbitratorCapacity,
     uint64_t memoryPoolInitCapacity,
-    uint64_t maxReclaimWaitMs,
+    uint64_t maxArbitrationTimeMs,
     uint64_t fastExponentialGrowthCapacityLimit,
     double slowCapacityGrowPct) {
   memory::MemoryManagerOptions options;
@@ -58,8 +58,8 @@ std::unique_ptr<memory::MemoryManager> createMemoryManager(
   options.extraArbitratorConfigs = {
       {std::string(ExtraConfig::kMemoryPoolInitialCapacity),
        folly::to<std::string>(memoryPoolInitCapacity) + "B"},
-      {std::string(ExtraConfig::kMemoryReclaimMaxWaitTime),
-       folly::to<std::string>(maxReclaimWaitMs) + "ms"},
+      {std::string(ExtraConfig::kMaxMemoryArbitrationTime),
+       folly::to<std::string>(maxArbitrationTimeMs) + "ms"},
       {std::string(ExtraConfig::kGlobalArbitrationEnabled), "true"},
       {std::string(ExtraConfig::kFastExponentialGrowthCapacityLimit),
        folly::to<std::string>(fastExponentialGrowthCapacityLimit) + "B"},