Skip to content

Ignore only well known and minor jemalloc errors and add entry about disabled per-cpu arena into system.warnings - #116899

Open
azat wants to merge 2 commits into
ClickHouse:masterfrom
azat:jemalloc-warnings
Open

Ignore only well known and minor jemalloc errors and add entry about disabled per-cpu arena into system.warnings#116899
azat wants to merge 2 commits into
ClickHouse:masterfrom
azat:jemalloc-warnings

Conversation

@azat

@azat azat commented Aug 28, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Ignore only well known and minor jemalloc errors and add entry about disabled per-cpu arena into system.warnings


Workflow [PR]
Sync PR [sync-upstream/pr/116899]

Previusly any message will be ignored, i.e. "Unsupported system page
size", and all you see is crash.
@clickhouse-gh

clickhouse-gh Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [1900639]


AI Review

Summary

This PR stops swallowing unexpected jemalloc startup diagnostics in the main binaries and adds a system.warnings entry when opt.percpu_arena ends up disabled at server startup. The current implementation now keys that warning off the effective runtime jemalloc mode rather than the build-time default, and I did not find an unresolved code-level correctness issue in the updated diff.

PR Metadata
  • 💡 Changelog category looks mismatched. The PR changes user-visible behavior (stderr diagnostics and system.warnings), so Improvement fits better than Not for changelog.
  • 💡 With Improvement, the current changelog entry should also be rewritten as a user-facing sentence. Suggested replacement: ClickHouse now surfaces disabled jemalloc per-CPU arenas in system.warnings and no longer hides unexpected jemalloc startup diagnostics, making allocator configuration problems easier to detect.
Final Verdict
  • Status: ✅ Approve
  • Minimum required actions: none for code correctness; consider fixing the changelog metadata before merge.

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 28, 2026
@antonio2368 antonio2368 self-assigned this Aug 28, 2026
@azat
azat force-pushed the jemalloc-warnings branch from e1f7527 to 72dcbfc Compare August 28, 2026 14:44
Comment thread programs/server/Server.cpp Outdated
Comment on lines +1032 to +1046
#if USE_JEMALLOC
{
/// "disabled" is also the jemalloc default, so only warn when the build-time configuration
/// requested per-CPU arenas (jemalloc overwrites the option value when it disables them at runtime).
const char * effective_mode = nullptr;
const char * compiled_conf = nullptr;
if (Jemalloc::tryGetValue("opt.percpu_arena", effective_mode) && effective_mode == std::string_view("disabled")
&& Jemalloc::tryGetValue("config.malloc_conf", compiled_conf) && std::string_view(compiled_conf).contains("percpu_arena:percpu"))
{
server.context()->addOrUpdateWarningMessage(
Context::WarningType::JEMALLOC_PERCPU_ARENA_DISABLED,
PreformattedMessage::create("Per-CPU arenas were requested (`percpu_arena:percpu`), but jemalloc disabled them at startup"));
}
}
#endif

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Don't want to add integration test for this, but I've checked manually with taskset -c0, and the warning works

Comment thread programs/server/Server.cpp Outdated
@azat
azat force-pushed the jemalloc-warnings branch from 72dcbfc to 6606782 Compare August 28, 2026 15:52
Comment thread programs/server/Server.cpp Outdated
@clickhouse-gh

clickhouse-gh Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing 19006390d with master 9adeb11c7 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes
Binary Master PR Δ
programs/clickhouse-stripped 710.14 MiB 707.09 MiB -3.05 MiB (-0.43%)

Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction.

Compile time of recompiled translation units

1265 translation units recompiled, 7587 s compile time in total, 1264 of them have a recent master baseline.

Job report

@azat
azat force-pushed the jemalloc-warnings branch from 6606782 to 5dfaffa Compare August 29, 2026 08:21
@azat
azat force-pushed the jemalloc-warnings branch from 5dfaffa to 1900639 Compare August 29, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog v26.8-must-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants