Skip to content

Commit a3e53c4

Browse files
fix(native): Init system used memory cache at the beginning of PeriodicMemoryChecker callback (#26347)
## Description The initial idea behind this change was to preload SystemMemoryUsage at the beginning of the `PeriodicMemoryChecker` callback. If the memory is preloaded, in the subsequent `periodicCb()` and `pushbackMemory()` implementation calls, we can decide whether to use the preloaded value or refetch it from the system. For our use case, we want to report the value obtained to the monitoring system. This will help in understanding the exact memory value used in the pushback mechanism. This PR was imported from Meta Phabricator: D83218739 The discussion there led to a desire to change the API to make it clearer for users. So in this diff I'm changing the API from: ```c++ int64_t cachedSystemUsedMemoryBytes() const { return cachedSystemUsedMemoryBytes_; } virtual int64_t systemUsedMemoryBytes() = 0; ``` to: ```c++ int64_t systemUsedMemoryBytes(bool fetchFresh = false); virtual void loadSystemMemoryUsage() = 0; ``` With the new API, we can clearly specify whether we want to preload the value, retrieve it from the cache, or fetch the current one. The API change only affects `PeriodicMemoryChecker` descendants. If this PR broke your build, follow this guide to fix the code: 1) Replace `systemUsedMemoryBytes()` calls with `systemUsedMemoryBytes(/*fetchFresh=*/true)` 2) Replace `cachedSystemUsedMemoryBytes()` calls with `systemUsedMemoryBytes()` 3) Put `systemUsedMemoryBytes()` impl to `loadSystemMemoryUsage()` impl ## Test Plan This PR is more like a refactoring, but I tested it in prod-like environment in Meta. ``` == NO RELEASE NOTE == ```
1 parent 69c4ba8 commit a3e53c4

File tree

5 files changed

+25
-19
lines changed

5 files changed

+25
-19
lines changed

presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
6363
~LinuxMemoryChecker() override {}
6464

6565
int64_t getUsedMemory() {
66-
return systemUsedMemoryBytes();
66+
return systemUsedMemoryBytes(/*fetchFresh=*/true);
6767
}
6868

6969
void setStatFile(std::string statFile) {
@@ -180,7 +180,7 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
180180
// value. It may be better than what we currently use. For
181181
// consistency we will match cgroup V1 and change if
182182
// necessary.
183-
int64_t systemUsedMemoryBytes() override {
183+
void loadSystemMemoryUsage() override {
184184
size_t memAvailable = 0;
185185
size_t memTotal = 0;
186186
size_t inactiveAnon = 0;
@@ -207,7 +207,7 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
207207
// Unit is in bytes.
208208
const auto memBytes = inactiveAnon + activeAnon;
209209
cachedSystemUsedMemoryBytes_ = memBytes;
210-
return memBytes;
210+
return;
211211
}
212212

213213
// Last resort use host machine info.
@@ -231,7 +231,6 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
231231
const auto memBytes =
232232
(memAvailable && memTotal) ? memTotal - memAvailable : 0;
233233
cachedSystemUsedMemoryBytes_ = memBytes;
234-
return memBytes;
235234
}
236235

237236
int64_t mallocBytes() const override {

presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.cpp

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ void PeriodicMemoryChecker::start() {
7171
scheduler_->setThreadName("MemoryCheckerThread");
7272
scheduler_->addFunction(
7373
[&]() {
74+
loadSystemMemoryUsage();
7475
periodicCb();
7576
if (config_.mallocMemHeapDumpEnabled) {
7677
maybeDumpHeap();
@@ -92,6 +93,13 @@ void PeriodicMemoryChecker::stop() {
9293
scheduler_.reset();
9394
}
9495

96+
int64_t PeriodicMemoryChecker::systemUsedMemoryBytes(bool fetchFresh) {
97+
if (fetchFresh) {
98+
loadSystemMemoryUsage();
99+
}
100+
return cachedSystemUsedMemoryBytes_;
101+
}
102+
95103
std::string PeriodicMemoryChecker::createHeapDumpFilePath() const {
96104
const size_t now = velox::getCurrentTimeMs() / 1000;
97105
// Format as follow:
@@ -210,7 +218,9 @@ void PeriodicMemoryChecker::pushbackMemory() {
210218
RECORD_HISTOGRAM_METRIC_VALUE(
211219
kCounterMemoryPushbackLatencyMs, latencyUs / 1000);
212220
const auto actualFreedBytes = std::max<int64_t>(
213-
0, static_cast<int64_t>(currentMemBytes) - systemUsedMemoryBytes());
221+
0,
222+
static_cast<int64_t>(currentMemBytes) -
223+
systemUsedMemoryBytes(/*fetchFresh=*/true));
214224
RECORD_HISTOGRAM_METRIC_VALUE(
215225
kCounterMemoryPushbackExpectedReductionBytes, freedBytes);
216226
RECORD_HISTOGRAM_METRIC_VALUE(

presto-native-execution/presto_cpp/main/PeriodicMemoryChecker.h

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,14 @@ class PeriodicMemoryChecker {
7676
/// Stops the 'PeriodicMemoryChecker'.
7777
virtual void stop();
7878

79-
/// Returns the last known cached 'current' system memory usage in bytes.
80-
int64_t cachedSystemUsedMemoryBytes() const {
81-
return cachedSystemUsedMemoryBytes_;
82-
}
79+
/// Returns the last known cached 'current' system memory usage in bytes. If
80+
/// 'fetchFresh' is true, retrieves and returns the current system memory usage.
81+
/// The returned value is used to compare with 'Config::systemMemLimitBytes'.
82+
int64_t systemUsedMemoryBytes(bool fetchFresh = false);
8383

8484
protected:
85-
/// Fetches and returns current system memory usage in bytes.
86-
/// The returned value is used to compare with 'Config::systemMemLimitBytes'.
87-
virtual int64_t systemUsedMemoryBytes() = 0;
85+
/// Fetches current system memory usage in bytes and stores it in the cache.
86+
virtual void loadSystemMemoryUsage() = 0;
8887

8988
/// Returns current bytes allocated by malloc. The returned value is used to
9089
/// compare with 'Config::mallocBytesUsageDumpThreshold'

presto-native-execution/presto_cpp/main/PrestoServer.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1532,7 +1532,7 @@ void PrestoServer::checkOverload() {
15321532
systemConfig->workerOverloadedThresholdMemGb() * 1024 * 1024 * 1024;
15331533
if (overloadedThresholdMemBytes > 0) {
15341534
const auto currentUsedMemoryBytes = (memoryChecker_ != nullptr)
1535-
? memoryChecker_->cachedSystemUsedMemoryBytes()
1535+
? memoryChecker_->systemUsedMemoryBytes()
15361536
: 0;
15371537
const bool memOverloaded =
15381538
(currentUsedMemoryBytes > overloadedThresholdMemBytes);

presto-native-execution/presto_cpp/main/tests/PeriodicMemoryCheckerTest.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,11 @@ class PeriodicMemoryCheckerTest : public testing::Test {
3434
std::function<void()>&& periodicCb = nullptr,
3535
std::function<bool(const std::string&)>&& heapDumpCb = nullptr)
3636
: PeriodicMemoryChecker(config),
37-
systemUsedMemoryBytes_(systemUsedMemoryBytes),
3837
mallocBytes_(mallocBytes),
3938
periodicCb_(std::move(periodicCb)),
40-
heapDumpCb_(std::move(heapDumpCb)) {}
39+
heapDumpCb_(std::move(heapDumpCb)) {
40+
cachedSystemUsedMemoryBytes_ = systemUsedMemoryBytes;
41+
}
4142

4243
~TestPeriodicMemoryChecker() override {}
4344

@@ -46,9 +47,7 @@ class PeriodicMemoryCheckerTest : public testing::Test {
4647
}
4748

4849
protected:
49-
int64_t systemUsedMemoryBytes() override {
50-
return systemUsedMemoryBytes_;
51-
}
50+
void loadSystemMemoryUsage() override {}
5251

5352
int64_t mallocBytes() const override {
5453
return mallocBytes_;
@@ -70,7 +69,6 @@ class PeriodicMemoryCheckerTest : public testing::Test {
7069
void removeDumpFile(const std::string& filePath) const override {}
7170

7271
private:
73-
int64_t systemUsedMemoryBytes_{0};
7472
int64_t mallocBytes_{0};
7573
std::function<void()> periodicCb_;
7674
std::function<bool(const std::string&)> heapDumpCb_;

0 commit comments

Comments
 (0)