-
Notifications
You must be signed in to change notification settings - Fork 909
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
new(cmake,ci): added support for using jemalloc allocator instead of glibc one #3406
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/milestone 0.40.0 |
@@ -37,6 +37,9 @@ limitations under the License. | |||
#include "event_drops.h" | |||
#include "falco_outputs.h" | |||
|
|||
// Falco only metric | |||
#define METRICS_V2_JEMALLOC_STATS 1 << 31 |
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.
All other METRICS_V2_...
defines live in libs; give this a big number to avoid conflicts.
…glibc one. The jemalloc allocator is enabled by default for published packages. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…theus metircs. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
bf62f0a
to
f93e696
Compare
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
There must be some issues with sanitizer + jemalloc i think :/ |
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
/cc @sgaist |
@@ -141,6 +142,10 @@ set(CMD_MAKE make) | |||
|
|||
include(ExternalProject) | |||
|
|||
if(USE_JEMALLOC) |
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.
Seeing the comment about jemalloc and ASAN above, shouldn't there be a check done here that fails the configuration if both are enabled ? Otherwise we will likely have users that will get problematic builds.
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 agree, i'll just add a warning!
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.
Pushed!
Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Samuel Gaist <samuel.gaist@idiap.ch>
Co-authored-by: Samuel Gaist <samuel.gaist@idiap.ch> Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area build
/area CI
What this PR does / why we need it:
We never really tried different allocators, but #2495 (comment) seems to suggest that jemalloc can help us prevent memory starving the system.
Also, jemalloc allows us to customize its behavior and profile the application via
malloc.conf
(https://github.com/jemalloc/jemalloc/wiki/Getting-Started).For example:
Also, the PR adds support for jemalloc stats to stats_writer and prometheus metrics, by enabling the new config key:
metrics.jemalloc_stats_enabled: true
. Disabled by default.Example output:
Or, for prometheus:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The jemalloc allocator is enabled by default for published packages.
Does this PR introduce a user-facing change?: