MemoryWorker: use sanitizer info for allocated counter - #2349
Conversation
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
With jemalloc, `MemoryTracker::updateAllocated` gets the resident memory from the current source (cgroup or jemalloc `stats.resident`), not jemalloc `stats.allocated`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUN1x2LvAoA4aBu4cevokZ Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
||
| #include <unistd.h> | ||
|
|
||
| #if defined(ADDRESS_SANITIZER) |
There was a problem hiding this comment.
Is this not required for memory and thread sanitizers?
There was a problem hiding this comment.
We discussed only ASAN and I decided to update logic only for it.
There was a problem hiding this comment.
Do other sanitizers use less memory? @filimonov, we need help!
There was a problem hiding this comment.
Msan was also failing, but since all 3 expose the same interface, probably it would be better to do the same for all
There was a problem hiding this comment.
Do other sanitizers use less memory?
MSAN doesn't use redzones and its overhead (memory) is ussually less, TSAN builds shadow memory and vector clocks for sync primitives (more expensive).
Updated, MSAN and TSAN were added.
|
|
||
| usage.allocated = usage.resident; | ||
| #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || defined(MEMORY_SANITIZER) | ||
| usage.allocated = __sanitizer_get_current_allocated_bytes(); |
There was a problem hiding this comment.
Am I right that
usage.resident - total memory usage from OS point of view (including sanitizer usage)
usage.allocated - "program" memory usage without sanitizer usage
?
There was a problem hiding this comment.
usage.allocated - "program" memory usage without sanitizer usage
Yes, without any additional allocation for sanitizers. For example redzones, shodow memory.
| usage.allocated = usage.resident; | ||
| #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || defined(MEMORY_SANITIZER) | ||
| usage.allocated = __sanitizer_get_current_allocated_bytes(); | ||
| #endif |
There was a problem hiding this comment.
Was warning about double assignment suppressed?
May be change on
#if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || defined(MEMORY_SANITIZER)
usage.allocated = __sanitizer_get_current_allocated_bytes();
#else
usage.allocated = usage.resident;
#endif
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
|
It worth explaining in a comment why do we distinguish 'allocated' and 'resident' for sanitizers and in which cases use one or another. |
|
Agree with @ilejn about comments with description for struct MemoryUsage. |
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
4 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
MemoryWorker now corrects MemoryTracker using allocated memory from ASan/TSan/MSan instead of resident memory in sanitizer builds when cgroups are available, fixing inaccurate memory accounting in such builds.