Add error messages when malloc allocator hits capacity limit #8075

bikramSingh91 · 2023-12-15T19:12:54Z

Adds useful error messages when capacity limit is hit while
using MallocAllocator
Added a log line in the memory pool leak checking codepath
that would help attribute leaks to its respective query in
non-debug builds.

Test Plan:
Added unit tests

netlify · 2023-12-15T19:13:00Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`35a7736`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65833ae173dc4900084b3465

xiaoxmeng

@bikramSingh91 thanks for the change!

velox/common/memory/MallocAllocator.cpp

xiaoxmeng · 2023-12-16T20:51:53Z

velox/common/memory/MallocAllocator.cpp

+        "total allocation of {} pages, the memory allocator capacity is {}",
+        mix.totalPages,
+        numPages,
+        succinctBytes(capacity_));


Do we want add a log with VELOX_MEM_LOG_EVERY_MS?
Also we could consider to add a static utility in MemoryAllocator::logAllocationError(const std::string& errMsg); It seems that have bee used in more than one places? Thanks!

A few thoughts come to mind:

It's possible to incorporate VELOX_MEM_LOG_EVERY_MS into setAllocatorFailureMessage, which could potentially eliminate the need for an additional function call.

However, on second thought there are situations where logging every time is necessary, so this approach may not result in significant cleanup.
(example

velox/velox/common/memory/MmapAllocator.cpp

Line 139 in 050c16c

VELOX_MEM_LOG(WARNING) << errorMsg;

)

Finally, adding VELOX_MEM_LOG_EVERY_MS inside a function would mean that it gets invoked everytime we log an error, so the 1000th iteration can be for any of those error messages and we might miss some (not sure if forcing an inline function can offset this).

what do you think?

Yeah, it might be better not to log inside setAllocatorFailureMessage

facebook-github-bot · 2023-12-19T17:56:39Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@bikramSingh91 thanks for the update!

xiaoxmeng · 2023-12-19T18:06:20Z

velox/common/memory/MallocAllocator.cpp

+        "total allocation of {} pages, the memory allocator capacity is {}",
+        mix.totalPages,
+        numPages,
+        succinctBytes(capacity_));


Yeah, it might be better not to log inside setAllocatorFailureMessage

velox/common/memory/MallocAllocator.cpp

xiaoxmeng · 2023-12-19T18:11:41Z

velox/common/memory/MallocAllocator.cpp

+    VELOX_MEM_LOG_EVERY_MS(WARNING, 1000)
+        << "Failed to allocateBytes " << succinctBytes(bytes)
+        << ": Exceeded memory allocator capacity limit of "
+        << succinctBytes(capacity_);


Do we want to set allocation error message here?

added and also added corresponding unit tests

velox/common/memory/MallocAllocator.cpp

facebook-github-bot · 2023-12-19T22:47:14Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@bikramSingh91 LGTM. Thanks!

velox/common/memory/MallocAllocator.cpp

xiaoxmeng · 2023-12-19T23:14:03Z

velox/common/memory/MemoryPool.cpp

@@ -408,11 +408,13 @@ MemoryPoolImpl::~MemoryPoolImpl() {

  if (isLeaf()) {
    if (usedReservationBytes_ > 0) {
+      LOG(ERROR) << "Bad memory usage track state : " << toString();


How about?

Used memory leak:

Used this instead to make it consistent with the reserved memory leak message:

Memory leak (Used memory):

xiaoxmeng · 2023-12-19T23:14:15Z

velox/common/memory/MemoryPool.cpp

      RECORD_METRIC_VALUE(
          kMetricMemoryPoolUsageLeakBytes, usedReservationBytes_);
    }

    if (minReservationBytes_ > 0) {
+      LOG(ERROR) << "Bad memory usage track state : " << toString();


Memory reservation leak:

Used this instead to make it consistent with the used memory leak message:

Memory leak (Reserved memory):

facebook-github-bot · 2023-12-20T18:27:40Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Adds useful error messages when capacity limit is hit while using MallocAllocator Added a log line in the memory pool leak checking codepath that would help attribute leaks to its respective query in non-debug builds. Test Plan: Added unit tests

facebook-github-bot · 2023-12-20T19:05:26Z

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-12-20T23:03:50Z

@bikramSingh91 merged this pull request in 4ab8c0d.

conbench-facebook · 2023-12-20T23:24:55Z

Conbench analyzed the 1 benchmark run on commit 4ab8c0d7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

bikramSingh91 requested a review from xiaoxmeng December 15, 2023 19:12

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 15, 2023

xiaoxmeng approved these changes Dec 16, 2023

View reviewed changes

bikramSingh91 force-pushed the mallocAllocatorErrorMsgs branch from 808e9ef to c5fa308 Compare December 19, 2023 17:49

xiaoxmeng reviewed Dec 19, 2023

View reviewed changes

bikramSingh91 force-pushed the mallocAllocatorErrorMsgs branch from c5fa308 to 6bfc987 Compare December 19, 2023 22:45

xiaoxmeng approved these changes Dec 19, 2023

View reviewed changes

bikramSingh91 force-pushed the mallocAllocatorErrorMsgs branch from 6bfc987 to ecf95ca Compare December 20, 2023 18:26

bikramSingh91 force-pushed the mallocAllocatorErrorMsgs branch from ecf95ca to 35a7736 Compare December 20, 2023 19:05

facebook-github-bot closed this in 4ab8c0d Dec 20, 2023

facebook-github-bot added the Merged label Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error messages when malloc allocator hits capacity limit #8075

Add error messages when malloc allocator hits capacity limit #8075

bikramSingh91 commented Dec 15, 2023 •

edited

Loading

netlify bot commented Dec 15, 2023 •

edited

Loading

xiaoxmeng left a comment

xiaoxmeng Dec 16, 2023

bikramSingh91 Dec 19, 2023

xiaoxmeng Dec 19, 2023

facebook-github-bot commented Dec 19, 2023

xiaoxmeng left a comment

xiaoxmeng Dec 19, 2023

xiaoxmeng Dec 19, 2023

bikramSingh91 Dec 19, 2023

facebook-github-bot commented Dec 19, 2023

xiaoxmeng left a comment

xiaoxmeng Dec 19, 2023

bikramSingh91 Dec 20, 2023

xiaoxmeng Dec 19, 2023

bikramSingh91 Dec 20, 2023

facebook-github-bot commented Dec 20, 2023

facebook-github-bot commented Dec 20, 2023

facebook-github-bot commented Dec 20, 2023

conbench-facebook bot commented Dec 20, 2023

Add error messages when malloc allocator hits capacity limit #8075

Add error messages when malloc allocator hits capacity limit #8075

Conversation

bikramSingh91 commented Dec 15, 2023 • edited Loading

netlify bot commented Dec 15, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 19, 2023

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 19, 2023

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 20, 2023

facebook-github-bot commented Dec 20, 2023

facebook-github-bot commented Dec 20, 2023

conbench-facebook bot commented Dec 20, 2023

bikramSingh91 commented Dec 15, 2023 •

edited

Loading

netlify bot commented Dec 15, 2023 •

edited

Loading