Adds comptime option to link jemalloc instead of tcmalloc#44113
Conversation
Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
| build:compile-time-options --define=log_debug_assert_in_release=enabled | ||
| build:compile-time-options --define=path_normalization_by_default=true | ||
| build:compile-time-options --define=deprecated_features=disabled | ||
| build:compile-time-options --define=tcmalloc=gperftools |
There was a problem hiding this comment.
this is to be able to run jemalloc enabled tests since gperftools will conflict with jemalloc. Instead, added gperftools option explicitly in the CI script
| // Purge all arenas to release dirty pages back to the OS. | ||
| // MALLCTL_ARENAS_ALL is jemalloc's pseudo-index for addressing all arenas at once. | ||
| char purge_cmd[32]; | ||
| snprintf(purge_cmd, sizeof(purge_cmd), "arena.%u.purge", MALLCTL_ARENAS_ALL); | ||
| mallctl(purge_cmd, nullptr, nullptr, nullptr, 0); |
There was a problem hiding this comment.
this is the only place I need to learn things more about jemalloc, but it looks correct too me
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
| lyft | ||
| maglev | ||
| malloc | ||
| mallctl |
There was a problem hiding this comment.
These don't need to be in the dictionary. But backticks around them in comments. Same for jemalloc above
There was a problem hiding this comment.
I would keep jemalloc as tcmalloc exists here - removing others
| ) | ||
|
|
||
| bool_flag( | ||
| name = "jemalloc", |
There was a problem hiding this comment.
Should it be tcmalloc=jemalloc instead for consistency with gperftools?
There was a problem hiding this comment.
yeah i did that initially, and thought it's just weird. Either works for me but will leave it to @phlax for the final call
There was a problem hiding this comment.
you can use a string_flag similar to how the define works - ie something like --//bazel:alloc=jemalloc/tcmalloc
There was a problem hiding this comment.
i would rather keep the bool flag as is for this PR, then refactor the flags in a follow. wdyt?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the jemalloc memory allocator as an alternative to tcmalloc. It includes the necessary Bazel build configurations, dependency management, and CI updates to enable jemalloc. Additionally, it implements memory statistics and heap management utilities for jemalloc within the Envoy codebase. The review feedback highlights several instances where a call to refreshJemallocEpoch() is required before querying jemalloc statistics to ensure the returned values are up-to-date, and suggests using stats.mapped instead of stats.resident for consistency with existing memory reservation metrics.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
ok i think ready |
|
/retest possible flakes |
|
Good to go ? |
phlax
left a comment
There was a problem hiding this comment.
lgtm, thanks @mathetake - sorry - i thought i approved
…#44113) Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>
…#44113) Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Jonathan Wu <jtwu@google.com>
…#44113) Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>
…#44113) Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com> Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Commit Message: Adds comptime option to link jemalloc instead of tcmalloc
Additional Description:
This adds a new comptime option to use jemalloc instead of tcmalloc.
Risk Level: none (not enabled by default)
Testing: existing tests are run on comptime option CI
Docs Changes: follow up once it becomes stable
Release Notes: n/a
Platform Specific Features: n/a
I declare that this is mostly written by Claude with some guidance from me on how to test on CI.