Skip to content

Add minimal support for reliable memory tracking and limiting#3

Closed
al13n321 wants to merge 1 commit intotempfrom
chmem
Closed

Add minimal support for reliable memory tracking and limiting#3
al13n321 wants to merge 1 commit intotempfrom
chmem

Conversation

@al13n321
Copy link
Copy Markdown
Member

@al13n321 al13n321 commented Jan 15, 2025

Here's a way to do (hopefully) solid memory limiting using the smallest amount of jemalloc changes.

(I spent a while following the code paths and am pretty sure these are the only code sites that need to be updated, as long as hugepage allocator ("hpa" option in jemalloc config) is disabled.)
(Tested it a little, and the je_clickhouse_resident_bytes and je_clickhouse_active_bytes seem accurate, ~2-15% above RssAnon.)

@antonio2368 antonio2368 self-assigned this Jan 15, 2025
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'm interested in integration with CH

/// change state between active/dirty/neither.
/// Indended use is to subtract the values before and after an alloc/free call.
/// Absolute values are not meaningful.
int64_t active_bytes_delta; // total size of active pages (i.e. containing any live allocations)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int64_t active_bytes_delta; // total size of active pages (i.e. containing any live allocations)
int64_t active_bytes_delta; // total size of active pages (i.e. containing any live allocations)

@al13n321
Copy link
Copy Markdown
Member Author

After looking more at the MemoryTracker-adjacent code, I see what you mean, this wouldn't be easy to fit in there...

Also noticed that kernel memory usage is not negligible (e.g. in a small local test I saw 4.7 GB out of 8.3 GB being used by sockets, "sock" in /sys/fs/cgroup/<name>/memory.stat), so we have to look at some OS memory stats in background thread even if we do something like this with jemalloc. (And instrument mmap calls (outside jemalloc) to add anon mappings to the stats, or periodically look at /proc/self/smaps to find non-jemalloc anon mappings (and add up their e.g. rss). Or take total memory usage from cgroup and subtract jemalloc mmaps from /proc/self/smaps.)

@al13n321 al13n321 marked this pull request as draft January 17, 2025 03:16
@al13n321
Copy link
Copy Markdown
Member Author

(Abandoning for now, can revive from the list of PRs later if needed. This PR seems good, but the integration with MemoryTracker et al would be a little complicated.)

@al13n321 al13n321 closed this Jan 28, 2025
antonio2368 pushed a commit that referenced this pull request Apr 1, 2026
We tried to load `g` from `bitmap[i]` before checking it is actually a
valid load. Tweaked a loop a bit to `break` early, when we are done
scanning for bits.

Before this commit undefined behaviour sanitizer from GCC 14+ was
unhappy at `test/unit/bitmap` test with following error.

```
../include/jemalloc/internal/bitmap.h:293:5: runtime error: load of
address 0x7bb1c2e08008 with insufficient space for an object of type
'const bitmap_t'
<...>
    #0 0x62671a149954 in bitmap_ffu ../include/jemalloc/internal/bitmap.h:293
    #1 0x62671a149954 in test_bitmap_xfu_body ../test/unit/bitmap.c:275
    #2 0x62671a14b767 in test_bitmap_xfu ../test/unit/bitmap.c:323
    #3 0x62671a376ad1 in p_test_impl ../test/src/test.c:149
    #4 0x62671a377135 in p_test ../test/src/test.c:200
    jemalloc#5 0x62671a13da06 in main ../test/unit/bitmap.c:336
<...>
```
antonio2368 pushed a commit that referenced this pull request Apr 1, 2026
This commit allows to enable sanitizers with autoconf options, instead
of modifying `CFLAGS`, `CXXFLAGS` and `LDFLAGS` directly.

* `--enable-tsan` option to enable Thread Sanitizer.
* `--enable-ubsan` option to enable Undefined Behaviour Sanitizer.

End goal is to speedup development by finding problems quickly, early
and easier. Eventually, when all current issues will be fixed, we can
enable sanitizers in CI. Fortunately, there are not a lot of problems we
need to fix.

Address Sanitizer is a bit controversial, because it replaces memory
allocator, so we decided to left it out for a while.

Below are couple of examples of how tests look like under different
sanitizers at the moment.

```
$  ../configure --enable-tsan --enable-debug
<...>
asan               : 0
tsan               : 1
ubsan              : 0
$ make -j`nproc` check
<...>
  Thread T13 (tid=332043, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x61748)
    #1 thd_create ../test/src/thd.c:25 (bin_batching+0x5631ca)
    #2 stress_run ../test/unit/bin_batching.c:148
(bin_batching+0x40364c)
    #3 test_races ../test/unit/bin_batching.c:249
(bin_batching+0x403d79)
    #4 p_test_impl ../test/src/test.c:149 (bin_batching+0x562811)
    jemalloc#5 p_test_no_reentrancy ../test/src/test.c:213
(bin_batching+0x562d35)
    jemalloc#6 main ../test/unit/bin_batching.c:268 (bin_batching+0x40417e)

SUMMARY: ThreadSanitizer: data race
../include/jemalloc/internal/edata.h:498 in edata_nfree_inc
```

```
$ ../configure --enable-ubsan --enable-debug
<...>
asan               : 0
tsan               : 0
ubsan              : 1
$ make -j`nproc` check
<...>
=== test/unit/hash ===
../test/unit/hash.c:119:16: runtime error: left shift of 176 by 24
places cannot be represented in type 'int'
<...>
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants