Skip to content

Store jemalloc allocation samples in trace_log#85438

Merged
antonio2368 merged 20 commits intomasterfrom
store-jemalloc-samples-in-trace-log
Sep 4, 2025
Merged

Store jemalloc allocation samples in trace_log#85438
antonio2368 merged 20 commits intomasterfrom
store-jemalloc-samples-in-trace-log

Conversation

@antonio2368
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 commented Aug 12, 2025 •
edited
Loading

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Update jemalloc to newer version.
Improve allocation profiling based on jemalloc's internal tooling.
Global jemalloc profiler can now be enabled with config jemalloc_enable_global_profiler. Sampled global allocations and deallocations can be stored in system.trace_log under JemallocSample type by enabling config jemalloc_collect_global_profile_samples_in_trace_log.
Jemalloc profiling can now be enabled for each query independently using setting jemalloc_enable_profiler. Storing samples in system.trace_log can be controlled per query using setting jemalloc_collect_profile_samples_in_trace_log.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Aug 12, 2025 •
edited
Loading

Workflow [PR], commit [6f85bad]

Summary: ✅

@antonio2368 antonio2368 force-pushed the store-jemalloc-samples-in-trace-log branch from 844f903 to 2e0f7a3 Compare August 12, 2025 14:29
@clickhouse-gh clickhouse-gh bot added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Aug 12, 2025
@azat azat self-assigned this Aug 12, 2025
azat
azat requested changes Aug 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

I hope that with jemalloc upgrade there will be no new bugs :)

Do you have any thoughts on how this can be used? I mean after we started tracing malloc's, what is the different between this and MemorySample with max_untracked_memory=0?

@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Aug 13, 2025

Do you have any thoughts on how this can be used? I mean after we started tracing malloc's, what is the different between this and MemorySample with max_untracked_memory=0

The main idea is to avoid dependency on jeprof. Also it will be persistent and we don't have to time flushes.

what is the different between this and MemorySample with max_untracked_memory=0

Deciding when to sample allocation is a really tricky problem and jemalloc has a really good implementation.
https://github.com/jemalloc/jemalloc/blob/dev/doc_internal/PROFILING_INTERNALS.md
We have a really simply approach where we sample with some step but this often gives wrong picture (i.e. it misses many smaller allocations that can add up to a lot of memory usage).

@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Aug 13, 2025

So a single run pushed 14mil rows to system.trace_log and it can't keep up 😞

@azat
Copy link
Copy Markdown
Member

azat commented Aug 13, 2025

The main idea is to avoid dependency on jeprof. Also it will be persistent and we don't have to time flushes.

I wish, but, the main benefit of jemalloc profiler (with jeprof), that it shows current state, while it is unlikely that we will be able to insert all this data into trace_log

Deciding when to sample allocation is a really tricky problem and jemalloc has a really good implementation

That is a good point

@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Aug 13, 2025

I wish, but, the main benefit of jemalloc profiler (with jeprof), that it shows current state, while it is unlikely that we will be able to insert all this data into trace_log

You can recreate it with a query so this would be similar to jemalloc profiler with accumulation.
Global seems too much so I added way to enable jemalloc profiler for a single query and optionally store it in trace_log.
Global profiler can now be enabled with a server setting, same for storing global samples in trace_log.

azat
azat reviewed Aug 18, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

New life for jemalloc profiler 👍

src/Interpreters/ThreadStatusExt.cpp
{
Jemalloc::getThreadProfileActiveMib().setValue(true);
Jemalloc::setCollectLocalProfileSamplesInTraceLog(settings[Setting::jemalloc_collect_profile_samples_in_trace_log]);
}
Copy link
Copy Markdown
Member

@azat azat Aug 18, 2025

Choose a reason for hiding this comment

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

What do you think about adding a setting to flush profile automatically on query end?

Copy link
Copy Markdown
Member Author

@antonio2368 antonio2368 Sep 1, 2025

Choose a reason for hiding this comment

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

I would prefer to do it in a different PR as it should probably go in QueryScope and I would like to not complicate this PR even more.
But a good idea.

@antonio2368 antonio2368 force-pushed the store-jemalloc-samples-in-trace-log branch from 14a6b8a to 60d2652 Compare September 1, 2025 12:17
@antonio2368 antonio2368 requested a review from azat September 1, 2025 12:18
azat
azat requested changes Sep 3, 2025
View reviewed changes
azat
azat reviewed Sep 3, 2025
View reviewed changes
azat
azat reviewed Sep 3, 2025
View reviewed changes
azat
azat reviewed Sep 3, 2025
View reviewed changes
azat
azat reviewed Sep 3, 2025
View reviewed changes
@azat
Copy link
Copy Markdown
Member

azat commented Sep 3, 2025

Also

Not for changelog (changelog entry is not required)

This is definitely for changelog I would say, it adds new profiling and it updates jemalloc

azat
azat approved these changes Sep 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@azat azat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks ok 🤞, let's merge it early in the release cycle to have more time for testing until stable release

@antonio2368 antonio2368 mentioned this pull request Sep 4, 2025
Merged
1 task
@antonio2368 antonio2368 added this pull request to the merge queue Sep 4, 2025
Merged via the queue into master with commit 9d1330b Sep 4, 2025
237 of 240 checks passed
@antonio2368 antonio2368 deleted the store-jemalloc-samples-in-trace-log branch September 4, 2025 12:00
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 4, 2025
@murphy-4o murphy-4o mentioned this pull request Mar 24, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat approved these changes

Assignees

@azat azat

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@antonio2368 @azat @robot-clickhouse-ci-2