From 358fa61943d2ae780b33c9fc5cc0502479639060 Mon Sep 17 00:00:00 2001 From: Artem Selishchev Date: Thu, 16 Oct 2025 10:46:22 -0700 Subject: [PATCH] [presto][native] Init system used memory cache at the beginning of PeriodicMemoryChecker callback (#26152) Summary: in Currently `PeriodicMemoryChecker` and descendants use these 2 methods to get current system used memory: - `systemUsedMemoryBytes`: ``` int64_t systemUsedMemoryBytes() { const auto currentMemBytes = ...; cachedSystemUsedMemoryBytes_ = currentMemBytes; return currentMemBytes; } ``` - `cachedSystemUsedMemoryBytes` ``` int64_t cachedSystemUsedMemoryBytes() const { return cachedSystemUsedMemoryBytes_; } ``` Both methods are `protected` and don't become public in descendants, so their usage is limited by the classes itself. We get value from cache only if there is a prior `systemUsedMemoryBytes` call, before that the cache is empty. All `systemUsedMemoryBytes` and `cachedSystemUsedMemoryBytes` calls only happen inside the callback, this makes it easy to maintain the invariants. ### The problem In one of the upcoming diffs I'll need an access to the value of current system used memory inside `periodicCb()` implementation. The straightforward approach would be to just call `systemUsedMemoryBytes`, but it's a syscall and I noticed, that we can do some optimization in that area. We can do `systemUsedMemoryBytes()` call as the very first line in the callback and then everywhere below use cached value or actual value depends on needs. As example in `PeriodicMemoryChecker::pushbackMemory` we can also get the value from cache, and then update it by calling `systemUsedMemoryBytes()` after the pushback, which we're already doing in the code. Differential Revision: D83218739 --- .../presto_cpp/main/LinuxMemoryChecker.cpp | 8 +------- .../presto_cpp/main/PeriodicMemoryChecker.cpp | 12 +++++++++++- .../presto_cpp/main/PeriodicMemoryChecker.h | 13 ++++++------- .../presto_cpp/main/PrestoServer.cpp | 2 +- .../main/tests/PeriodicMemoryCheckerTest.cpp | 10 ++++------ 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp b/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp index 3d9aa0ac77c71..1863a2b6ff755 100644 --- a/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp +++ b/presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp @@ -62,10 +62,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker { ~LinuxMemoryChecker() override {} - int64_t getUsedMemory() { - return systemUsedMemoryBytes(); - } - void setStatFile(std::string statFile) { memStatFile_ = statFile; LOG(INFO) << fmt::format( @@ -180,7 +176,7 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker { // value. It may be better than what we currently use. For // consistency we will match cgroup V1 and change if // necessary. - int64_t systemUsedMemoryBytes() override { + void loadSystemMemoryUsage() override { size_t memAvailable = 0; size_t memTotal = 0; size_t inactiveAnon = 0; @@ -207,7 +203,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker { // Unit is in bytes. const auto memBytes = inactiveAnon + activeAnon; cachedSystemUsedMemoryBytes_ = memBytes; - return memBytes; } // Last resort use host machine info. @@ -231,7 +226,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker { const auto memBytes = (memAvailable && memTotal) ? memTotal - memAvailable : 0; cachedSystemUsedMemoryBytes_ = memBytes; - return memBytes; } int64_t mallocBytes() const override { diff --git a/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp b/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp index ece456c6c7ebd..a4eadbb4d1d18 100644 --- a/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp +++ b/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp @@ -71,6 +71,7 @@ void PeriodicMemoryChecker::start() { scheduler_->setThreadName("MemoryCheckerThread"); scheduler_->addFunction( [&]() { + loadSystemMemoryUsage(); periodicCb(); if (config_.mallocMemHeapDumpEnabled) { maybeDumpHeap(); @@ -92,6 +93,13 @@ void PeriodicMemoryChecker::stop() { scheduler_.reset(); } +int64_t PeriodicMemoryChecker::systemUsedMemoryBytes(bool fetchFresh) { + if (fetchFresh) { + loadSystemMemoryUsage(); + } + return cachedSystemUsedMemoryBytes_; +} + std::string PeriodicMemoryChecker::createHeapDumpFilePath() const { const size_t now = velox::getCurrentTimeMs() / 1000; // Format as follow: @@ -210,7 +218,9 @@ void PeriodicMemoryChecker::pushbackMemory() { RECORD_HISTOGRAM_METRIC_VALUE( kCounterMemoryPushbackLatencyMs, latencyUs / 1000); const auto actualFreedBytes = std::max( - 0, static_cast(currentMemBytes) - systemUsedMemoryBytes()); + 0, + static_cast(currentMemBytes) - + systemUsedMemoryBytes(/*fetchFresh=*/true)); RECORD_HISTOGRAM_METRIC_VALUE( kCounterMemoryPushbackExpectedReductionBytes, freedBytes); RECORD_HISTOGRAM_METRIC_VALUE( diff --git a/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.h b/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.h index a1b3238458872..48b8ea5ab9236 100644 --- a/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.h +++ b/presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.h @@ -76,15 +76,14 @@ class PeriodicMemoryChecker { /// Stops the 'PeriodicMemoryChecker'. virtual void stop(); - /// Returns the last known cached 'current' system memory usage in bytes. - int64_t cachedSystemUsedMemoryBytes() const { - return cachedSystemUsedMemoryBytes_; - } + /// Returns the last known cached 'current' system memory usage in bytes. If + /// 'fetchFresh' is true, retrieves and returns the current system memory usage. + /// The returned value is used to compare with 'Config::systemMemLimitBytes'. + int64_t systemUsedMemoryBytes(bool fetchFresh = false); protected: - /// Fetches and returns current system memory usage in bytes. - /// The returned value is used to compare with 'Config::systemMemLimitBytes'. - virtual int64_t systemUsedMemoryBytes() = 0; + /// Fetches current system memory usage in bytes and stores it in the cache. + virtual void loadSystemMemoryUsage() = 0; /// Returns current bytes allocated by malloc. The returned value is used to /// compare with 'Config::mallocBytesUsageDumpThreshold' diff --git a/presto-native-execution/presto_cpp/main/PrestoServer.cpp b/presto-native-execution/presto_cpp/main/PrestoServer.cpp index d0d26d7b81b21..f9dc77e218081 100644 --- a/presto-native-execution/presto_cpp/main/PrestoServer.cpp +++ b/presto-native-execution/presto_cpp/main/PrestoServer.cpp @@ -1530,7 +1530,7 @@ void PrestoServer::checkOverload() { systemConfig->workerOverloadedThresholdMemGb() * 1024 * 1024 * 1024; if (overloadedThresholdMemBytes > 0) { const auto currentUsedMemoryBytes = (memoryChecker_ != nullptr) - ? memoryChecker_->cachedSystemUsedMemoryBytes() + ? memoryChecker_->systemUsedMemoryBytes() : 0; const bool memOverloaded = (currentUsedMemoryBytes > overloadedThresholdMemBytes); diff --git a/presto-native-execution/presto_cpp/main/tests/PeriodicMemoryCheckerTest.cpp b/presto-native-execution/presto_cpp/main/tests/PeriodicMemoryCheckerTest.cpp index 445bc82ee9dc4..4c625412cba37 100644 --- a/presto-native-execution/presto_cpp/main/tests/PeriodicMemoryCheckerTest.cpp +++ b/presto-native-execution/presto_cpp/main/tests/PeriodicMemoryCheckerTest.cpp @@ -34,10 +34,11 @@ class PeriodicMemoryCheckerTest : public testing::Test { std::function&& periodicCb = nullptr, std::function&& heapDumpCb = nullptr) : PeriodicMemoryChecker(config), - systemUsedMemoryBytes_(systemUsedMemoryBytes), mallocBytes_(mallocBytes), periodicCb_(std::move(periodicCb)), - heapDumpCb_(std::move(heapDumpCb)) {} + heapDumpCb_(std::move(heapDumpCb)) { + cachedSystemUsedMemoryBytes_ = systemUsedMemoryBytes; + } ~TestPeriodicMemoryChecker() override {} @@ -46,9 +47,7 @@ class PeriodicMemoryCheckerTest : public testing::Test { } protected: - int64_t systemUsedMemoryBytes() override { - return systemUsedMemoryBytes_; - } + void loadSystemMemoryUsage() override {} int64_t mallocBytes() const override { return mallocBytes_; @@ -70,7 +69,6 @@ class PeriodicMemoryCheckerTest : public testing::Test { void removeDumpFile(const std::string& filePath) const override {} private: - int64_t systemUsedMemoryBytes_{0}; int64_t mallocBytes_{0}; std::function periodicCb_; std::function heapDumpCb_;