Skip to content

Make jemalloc profiling cover all threads when enabled#98232

Merged
alexey-milovidov merged 5 commits intomasterfrom
jemalloc-setup-once
Mar 2, 2026
Merged

Make jemalloc profiling cover all threads when enabled#98232
alexey-milovidov merged 5 commits intomasterfrom
jemalloc-setup-once

Conversation

@antonio2368
Copy link
Copy Markdown
Member

When jemalloc_enable_global_profiler is set, the profiler must be activated before any threads are created — otherwise those threads won't be sampled. Move the Jemalloc::setup call to early in BaseDaemon::initialize (right after loadConfiguration) so that profiling is enabled before thread pools and background threads start.

Guard setup with std::call_once so the later call from server settings is a safe no-op (with warnings if settings diverge).

Extract default constants to Jemalloc.h and reuse them in ServerSettings.cpp and BaseDaemon.cpp to keep them in sync.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

It seems we do miss some of the threads, most problematic the async logger thread.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Feb 27, 2026

Workflow [PR], commit [c527e0b]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 27, 2026
@azat azat self-assigned this Feb 27, 2026
When `jemalloc_enable_global_profiler` is set, the profiler must be
activated before any threads are created — otherwise those threads won't
be sampled. Move the `Jemalloc::setup` call to early in
`BaseDaemon::initialize` (right after `loadConfiguration`) so that
profiling is enabled before thread pools and background threads start.

Guard `setup` with `std::call_once` so the later call from server
settings is a safe no-op (with warnings if settings diverge).

Extract default constants to `Jemalloc.h` and reuse them in
`ServerSettings.cpp` and `BaseDaemon.cpp` to keep them in sync.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…USE_JEMALLOC guard

The default constants (`default_enable_global_profiler`, etc.) are
referenced by `ServerSettings.cpp` unconditionally, but were defined
inside `#if USE_JEMALLOC`. Sanitizer builds don't use jemalloc, so the
`Jemalloc` namespace was undefined, causing compilation failures.

Move the constants before the guard so they are always available.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
antonio2368 and others added 3 commits February 28, 2026 23:17
Add assertion before the warning in `Jemalloc::setup` to catch
mismatches between server settings defaults and the manually defined
config names in `BaseDaemon` early in debug builds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous implementation used `std::call_once` which fired in the
parent process during `BaseDaemon::initialize`. After the watchdog
`fork`, jemalloc state (e.g. `background_thread`) resets in the child
but the `once_flag` remains set, so the verification check saw a
mismatch and fired `chassert`.

Split `setup` and `verifySetup` into separate functions:
- `setup` always applies settings (idempotent), called in
  `BaseDaemon::initialize` and re-applied after watchdog fork
- `verifySetup` checks jemalloc state matches server settings,
  called from `Server::main` and `Keeper::main`

Also extract config key names into constants in `Jemalloc.h` to keep
them in sync between `BaseDaemon` and `ServerSettings`.

https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=98232&sha=b69293471789706abde22ec86f33104dbfaf0382&name_0=PR&name_1=Stateless%20tests%20%28amd_debug%2C%20parallel%29

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@alexey-milovidov alexey-milovidov added this pull request to the merge queue Mar 2, 2026
Merged via the queue into master with commit 1c7c44d Mar 2, 2026
148 checks passed
@alexey-milovidov alexey-milovidov deleted the jemalloc-setup-once branch March 2, 2026 04:05
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 2, 2026
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Mar 2, 2026
robot-clickhouse added a commit that referenced this pull request Mar 2, 2026
Cherry pick #98232 to 26.2: Make jemalloc profiling cover all threads when enabled
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Mar 2, 2026
clickhouse-gh bot added a commit that referenced this pull request Mar 2, 2026
Backport #98232 to 26.2: Make jemalloc profiling cover all threads when enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR 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 v26.2-must-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants