-
Notifications
You must be signed in to change notification settings - Fork 876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server): Memory tracker #2501
Conversation
@@ -270,6 +249,38 @@ void MemoryCmd::Stats() { | |||
} | |||
} | |||
|
|||
void MemoryCmd::MallocStats(CmdArgList args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is a literal cut-paste, no other changes
Please add a commit description. |
src/server/allocation_tracker.cc
Outdated
return; | ||
} | ||
|
||
inside_process_new = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it to prevent a recursive endless loop? care to add a comment?
src/server/allocation_tracker.cc
Outdated
// See LICENSE for licensing terms. | ||
// | ||
|
||
#include "allocation_tracker.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put it under core/
#include "absl/container/inlined_vector.h" | ||
#include "absl/strings/numbers.h" | ||
|
||
#define INJECT_ALLOCATION_TRACKER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please introduce a CMakeLists option DFLY_MEMORY_TRACKING
in src/CMakelists.txt ?
Let's set it to ON by default and it will add the conditional define to dfly_main.cc.
In case it's disabled we fallback to #include <mimalloc-new-delete.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see you add this define to every file but use it only here. ok
#include "absl/cleanup/cleanup.h" | ||
#include "absl/container/inlined_vector.h" | ||
#include "absl/strings/numbers.h" | ||
#ifdef NDEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we now always allocate via mimalloc. worth adding this to commit description as well.
@@ -289,4 +300,73 @@ void MemoryCmd::Usage(std::string_view key) { | |||
rb->SendLong(memory_usage); | |||
} | |||
|
|||
void MemoryCmd::Track(CmdArgList args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add comment above how the command looks like:
// MEMORY TRACK ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// | ||
// This command is not documented in `MEMORY HELP` because it's meant to be used internally. | ||
void MemoryCmd::Track(CmdArgList args) { | ||
#ifndef DFLY_ENABLE_MEMORY_TRACKING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
🤓 |
This PR adds
AllocationTracker
class, which can track (currently print) memory allocations vianew
anddelete
based on a given criteria (size & sampling fraction).As a side effect, now we use mimalloc for
new
anddelete
also in debug builds (which wasn't the case before).Here's the comment above
Track()
, provided here for completeness: