Skip to content

Improve build time of setting classes#94676

Merged
Algunenano merged 8 commits intoClickHouse:masterfrom
Algunenano:settings_compilation_time
Jan 22, 2026
Merged

Improve build time of setting classes#94676
Algunenano merged 8 commits intoClickHouse:masterfrom
Algunenano:settings_compilation_time

Conversation

@Algunenano
Copy link
Copy Markdown
Member

@Algunenano Algunenano commented Jan 20, 2026

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):

Improve build time of setting classes

Introduce a base class SettingFieldBase for all SettingField##Type classes in order to remove most of the lambda functions in FieldInfo. This makes the classes larger (because of the virtual tables) but speeds up compilation, which is a pain.

I also tried to move from using one member per setting inside of ::Data to use a std::vector of SettingFieldBase but this made the code way more complex and slower to build (15->25 seconds).

Comparing Settings.cpp (the largest one) in my local dev machine:

  • Before:
real    0m43.484s
user    0m42.696s
sys     0m0.548s
  • After:
real    0m15.649s
user    0m15.095s
sys     0m0.307s

References #58797

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@Algunenano Algunenano requested a review from Copilot January 20, 2026 15:50
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Jan 20, 2026

Workflow [PR], commit [dfc2929]

Summary:

job_name test_name status info comment
AST fuzzer (amd_ubsan) failure
Logical error: Cannot convert nested result of function A with type B to the expected result type C: D (STID: None) FAIL cidb IGNORED
BuzzHouse (amd_debug) failure
Logical error: 'Inconsistent AST formatting: the query: (STID: 1941-1bfa) FAIL cidb, issue ISSUE EXISTS
Stateless tests (arm_asan, targeted) error IGNORED

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jan 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a base class SettingFieldBase for all SettingField* types to improve build time of setting classes by reducing the number of generated lambda functions. The change makes the setting field classes larger (due to virtual tables) but significantly speeds up compilation, reducing build time for Settings.cpp from ~43 seconds to ~15 seconds.

Changes:

  • Introduced SettingFieldBase with virtual methods for common operations
  • Made all SettingField* classes inherit from SettingFieldBase
  • Simplified FieldInfo metadata to only store two function pointers instead of many lambdas
  • Changed getPath() to return const char* and handle empty paths for custom settings

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File Description
src/Core/SettingsFields.h Introduces SettingFieldBase base class and updates all SettingField* classes to inherit from it with virtual method overrides
src/Core/ServerSettings.cpp Updates getPath() usage to handle empty strings for settings without paths
src/Core/BaseSettings.h Refactors FieldInfo to use two function pointers instead of many lambdas, updates getPath() return type, adds extensive documentation
Comments suppressed due to low confidence (1)

src/Core/BaseSettings.h:1

  • Multiple uses of const_cast throughout these methods. Consider adding a const-qualified version of get_data_function or redesigning to avoid casting away constness, which can lead to undefined behavior if the underlying data is truly const.
#pragma once

Comment thread src/Core/SettingsFields.h Outdated

struct SettingFieldBase
{
virtual ~SettingFieldBase() {}
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

Virtual destructor should use = default instead of empty braces {} to clearly indicate default behavior and allow compiler optimizations.

Suggested change
virtual ~SettingFieldBase() {}
virtual ~SettingFieldBase() = default;

Copilot uses AI. Check for mistakes.
Comment thread src/Core/SettingsFields.h
Comment thread src/Core/SettingsFields.h
static ValueType parseValueFromString(const std::string_view str)
{
static const String separators=", ";
constexpr String separators=", ";
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

constexpr String is invalid in C++. Use static const String or static constexpr std::string_view instead.

Copilot uses AI. Check for mistakes.
Comment thread src/Core/SettingsFields.h
Comment thread src/Core/SettingsFields.h
Comment thread src/Core/SettingsFields.h
Comment thread src/Core/BaseSettings.h
Comment thread src/Core/BaseSettings.h
Comment thread src/Core/BaseSettings.h
Comment thread src/Core/BaseSettings.h
@nickitat nickitat self-assigned this Jan 20, 2026
Comment thread src/Core/BaseSettings.h
Comment thread src/Core/BaseSettings.h Outdated
Comment thread src/Core/BaseSettings.h Outdated
Comment thread src/Core/BaseSettings.h Outdated
Comment thread src/Core/SettingsFields.h
Comment thread src/Core/SettingsFields.h
@Algunenano Algunenano added this pull request to the merge queue Jan 22, 2026
Merged via the queue into ClickHouse:master with commit 38ddb47 Jan 22, 2026
253 of 260 checks passed
@Algunenano Algunenano deleted the settings_compilation_time branch January 22, 2026 14:21
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 22, 2026
@clickgapai
Copy link
Copy Markdown
Contributor

Hi @Algunenano @nickitat — while reviewing this PR I found the following:

Happy to discuss — close anything that's wrong or already addressed.

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 pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants