-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize arrayReduce, -Array and -State combinators #7608
Conversation
4ec156b
to
5c160d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but please also add the performance test.
Results of performance test look strange: https://clickhouse-test-reports.s3.yandex.net/7608/5c160d5efb0ca08b6b515d66d2abb0bb847713b4/performance_test/comparison_min_time_gcc_9.html Could you please compare the most different result manually with |
0a8fa71
to
206d959
Compare
SummaryThis pr optimizes three use cases.
The actual improvements depend on aggregate functions, group by methods, group by size, etc. clickhoues-benchmark results
|
The perf results almost don't make any sense to me. The slow down queries aren't related to any of this pr's changes |
btw, some perf tests use |
I have checked manually the query that has the most slowdown on performance test:
I have downloaded two binaries, one from master and one from this PR:
Unpacked them to
And run clickhouse-benchmark:
And have found that this performance regression is real. Although the code looks reasonable and performance improvement that you've demonstrated is massive. Let's look at perf top. |
Then I run two servers simultaneously in comparison mode (this is the new feature of clickhouse-benchmark that should show statistical significant results if there any):
|
Ok, it seems some compiler tweaks are required. I'll investigate...
That's awesome! |
dc9b1d9
to
6eb1bfc
Compare
Also devirtualize -State combinator.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
For changelog. Remove if this is non-significant change.
Category (leave one):
Short description (up to few sentences):
vectorize processing arrayReduce similar to Aggregator addBatch. Might also serve as an infrastructure for #7550