Micro-optimizations for the new analyzer by Algunenano · Pull Request #63429 · ClickHouse/ClickHouse

Algunenano · 2024-05-06T19:59:18Z

Changelog category (leave one):

Performance Improvement

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

Micro-optimizations for the new analyzer

Documentation entry for user-facing changes

2 changes:

Rework updateHeader. It doesn't seem that we needed a list or copy the input (we can pass a const reference and deal with the copies manually). Let's see if the CI agrees.
Remove expensive logging of to stage {}. Keep the stage names, but don't print the query tree since that might be too expensive and doesn't give useful information.

#39996 goes from ~52 QPS to ~59 QPS
Closes #39996

robot-ch-test-poll · 2024-05-06T20:04:05Z

This is an automated comment for commit 2165cc3 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	❌ failure
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	❌ failure

Successful checks

Check name	Description	Status
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
PR Check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	✅ success

novikd · 2024-05-07T09:16:32Z

What do you think about adding a performance test with query from #39996?

Algunenano · 2024-05-07T10:47:43Z

What do you think about adding a performance test with query from #39996?

I expect query_interpretation_join.xml to reflect some of this improvements since it's basically testing query interpretation. If not I'll add another benchmark

Algunenano · 2024-05-07T20:18:45Z

More or less the expected:

Although I have no idea why set_index would be improved. It's true that it's a long query (in terms of characters) but I didn't expect the query interpretation to be the bottleneck there

novikd · 2024-05-08T10:47:02Z

Although I have no idea why set_index would be improved. It's true that it's a long query (in terms of characters) but I didn't expect the query interpretation to be the bottleneck there

Some of performance tests are really impacted by the interpretation speed. Mostly it's because of #62245.

novikd

LGTM

novikd · 2024-05-08T10:49:36Z

src/Processors/Transforms/JoiningTransform.cpp

 Block JoiningTransform::transformHeader(Block header, const JoinPtr & join)
 {
-    LOG_DEBUG(getLogger("JoiningTransform"), "Before join block: '{}'", header.dumpStructure());
+    LOG_TEST(getLogger("JoiningTransform"), "Before join block: '{}'", header.dumpStructure());


Maybe better to use TRACE level?

I don't mind either way. I'll set it to trace

Let's see if there is any change to one_thousand_joins with TRACE vs TEST just in case

Well, TEST apparently means 10% faster in one_thousand_joins. WDYT?

IMHO 10% worth to get rid of these logs, but I'm not sure how useful is it for debugging. @vdimir WHYT?

LOG_TEST would be fine, in case of a bug investigation, more debug needs to be added anyway.

novikd · 2024-05-08T10:50:20Z

.clang-format

 PenaltyExcessCharacter: 1000000
 PenaltyReturnTypeOnItsOwnLine: 60
-RemoveBracesLLVM: true
+RemoveBracesLLVM: false


Is it intentional?

Yes. clang-format keeps trying to transform:

for (...) { if (...) whatever }

And remove the parenthesis around the for, as it considers the if as a single line. I don't like it and it's a PITA.

Algunenano added 3 commits May 6, 2024 20:58

Optimize ActionsDAG::updateHeader

ef81f33

Stop clang-format trying to remove branches in for loops

f34a47d

Remove expensive logging in query stages

36e23d6

robot-ch-test-poll added the pr-performance Pull request with some performance improvements label May 6, 2024

Algunenano marked this pull request as draft May 7, 2024 09:00

novikd self-assigned this May 7, 2024

Algunenano added 2 commits May 7, 2024 12:42

updateHeader: Needs to keep track of all the matches

85096f0

Reduce excessive logging from joins

124b696

novikd added the analyzer Issues and pull-requests related to new analyzer label May 7, 2024

Fix useful warning by clang tidy

c652d2e

Algunenano marked this pull request as ready for review May 7, 2024 20:22

novikd reviewed May 8, 2024

View reviewed changes

Algunenano added 2 commits May 8, 2024 14:39

Raise log to trace

c178539

Reduce join logging again

0e2b2ab

Algunenano enabled auto-merge May 9, 2024 17:28

Algunenano disabled auto-merge May 13, 2024 17:51

Merge remote-tracking branch 'blessed/master' into micro_analyzer

61fb090

Algunenano mentioned this pull request May 14, 2024

[Bug]: clang-analyzer-optin.core.EnumCastOutOfRange for static_cast<ctrl_t>(0) abseil/abseil-cpp#1667

Closed

Algunenano force-pushed the micro_analyzer branch from 4d0042e to 2dd8b22 Compare May 14, 2024 10:25

More helpful optimizations found by clang-tidy

2165cc3

Algunenano force-pushed the micro_analyzer branch from 734a422 to 2165cc3 Compare May 15, 2024 18:26

Algunenano added this pull request to the merge queue May 16, 2024

Merged via the queue into ClickHouse:master with commit ad5403a May 16, 2024

Algunenano deleted the micro_analyzer branch May 16, 2024 09:17

robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label May 16, 2024

Conversation

Algunenano commented May 6, 2024

Changelog category (leave one):

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

Documentation entry for user-facing changes

Uh oh!

robot-ch-test-poll commented May 6, 2024 • edited by robot-ch-test-poll4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

novikd commented May 7, 2024

Uh oh!

Algunenano commented May 7, 2024

Uh oh!

Algunenano commented May 7, 2024

Uh oh!

novikd commented May 8, 2024

Uh oh!

novikd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robot-ch-test-poll commented May 6, 2024 •

edited by robot-ch-test-poll4

Loading