Addressed PR comments

prestodb · Feb 6, 2025 · 98bb354 · 98bb354
1 parent 80ca9b5
commit 98bb354
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 14 deletions.
diff --git a/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp b/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp
@@ -73,13 +73,13 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
     LOG(INFO) << fmt::format("Changed to using memory stat file {}", statFile_);
   }
 
-  void setMemMaxFile(std::string memMaxFile) {
+  void setMemMaxFile(const std::string& memMaxFile) {
     memMaxFile_ = memMaxFile;
     LOG(INFO) << fmt::format(
         "Changed to using memory max file {}", memMaxFile_);
   }
 
-  void setMemInfo(std::string memInfoFile) {
+  void setMemInfoFile(const std::string& memInfoFile) {
     memInfoFile_ = memInfoFile;
     LOG(INFO) << fmt::format("Changed to using meminfo file {}", memInfoFile_);
   }
@@ -94,7 +94,7 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
     PRESTO_STARTUP_LOG(INFO) << fmt::format(
         "System memory limit in bytes: {}", config_.systemMemLimitBytes);
 
-    int64_t memoryLimitForProcess = getMemoryLimitForProcess();
+    auto memoryLimitForProcess = getMemoryLimitForProcess();
     PRESTO_STARTUP_LOG(INFO) << fmt::format(
         "Memory limit for process in bytes: {}", memoryLimitForProcess);
 
@@ -188,10 +188,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
     if (statFile_ != "None") {
       folly::gen::byLine(statFile_.c_str()) |
           [&](const folly::StringPiece& line) -> void {
-        if (activeAnon != 0 && inactiveAnon != 0) {
-          return;
-        }
-
         if (inactiveAnon == 0) {
           inactiveAnon =
               extractNumericConfigValueWithRegex(line, kInactiveAnonRegex);
@@ -201,6 +197,10 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
           activeAnon =
               extractNumericConfigValueWithRegex(line, kActiveAnonRegex);
         }
+
+        if (activeAnon != 0 && inactiveAnon != 0) {
+          return;
+        }
       };
 
       // Unit is in bytes.
@@ -210,10 +210,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
     // Last resort use host machine info.
     folly::gen::byLine(memInfoFile_.c_str()) |
         [&](const folly::StringPiece& line) -> void {
-      if (memAvailable != 0 && memTotal != 0) {
-        return;
-      }
-
       if (memAvailable == 0) {
         memAvailable =
             extractNumericConfigValueWithRegex(line, kMemAvailableRegex) * 1024;
@@ -223,6 +219,10 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
         memTotal =
             extractNumericConfigValueWithRegex(line, kMemTotalRegex) * 1024;
       }
+
+      if (memAvailable != 0 && memTotal != 0) {
+        return;
+      }
     };
     // Unit is in bytes.
     return (memAvailable && memTotal) ? memTotal - memAvailable : 0;

diff --git a/presto-native-execution/presto_cpp/main/tests/LinuxMemoryCheckerTest.cpp b/presto-native-execution/presto_cpp/main/tests/LinuxMemoryCheckerTest.cpp
@@ -39,7 +39,7 @@ class LinuxMemoryCheckerTest : public testing::Test {
     tempTestFile->append(content);
     auto testFilePath = tempTestFile->getPath();
 
-    memChecker_.setMemInfo(memInfoPath);
+    memChecker_.setMemInfoFile(memInfoPath);
     memChecker_.setMemMaxFile(testFilePath);
     ASSERT_EQ(memChecker_.getMemoryLimitForProcess(), expectedMemoryMax);
   }
@@ -165,7 +165,7 @@ TEST_F(LinuxMemoryCheckerTest, sysMemLimitBytesCheck) {
       "prefix",
       5,
       512});
-  memChecker.setMemInfo(memInfoPath);
+  memChecker.setMemInfoFile(memInfoPath);
   memChecker.setMemMaxFile(testFilePath);
   ASSERT_NO_THROW(memChecker.start());
   ASSERT_NO_THROW(memChecker.stop());
@@ -183,7 +183,7 @@ TEST_F(LinuxMemoryCheckerTest, sysMemLimitBytesCheck) {
       "prefix",
       5,
       512});
-  memChecker2.setMemInfo(memInfoPath);
+  memChecker2.setMemInfoFile(memInfoPath);
   memChecker2.setMemMaxFile(testFilePath);
   VELOX_ASSERT_THROW(memChecker2.start(), "(131000000001 vs. 131000000000)");
   VELOX_ASSERT_THROW(memChecker2.stop(), "");