Skip to content
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

Less clock_gettime calls #27325

Merged
merged 1 commit into from
Aug 7, 2021

Conversation

filimonov
Copy link
Contributor

@filimonov filimonov commented Aug 6, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Performance Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Improve the performance of fast queries when max_execution_time=0 by reducing the number of clock_gettime system calls.

Detailed description / Documentation draft:

-- before 
SELECT count() FROM zeros(100000000000)

┌──────count()─┐
│ 100000000000 │
└──────────────┘

1 rows in set. Elapsed: 4.933 sec. Processed 100.00 billion rows, 100.00 GB (20.27 billion rows/s., 20.27 GB/s.)

-- after 
select count() from zeros(100000000000);

┌──────count()─┐
│ 100000000000 │
└──────────────┘

1 rows in set. Elapsed: 3.964 sec. Processed 100.00 billion rows, 100.00 GB (25.23 billion rows/s., 25.23 GB/s.)

@robot-clickhouse robot-clickhouse added the pr-performance Pull request with some performance improvements label Aug 6, 2021
@nikitamikhaylov nikitamikhaylov self-assigned this Aug 6, 2021
@@ -3,6 +3,7 @@
#include <Poco/Timespan.h>
#include <common/types.h>
#include <DataStreams/SizeLimits.h>
#include <Common/Stopwatch.h>
Copy link
Member

Choose a reason for hiding this comment

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

BTW if only reference/pointer is used, forward declaration is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite a light header, but sure: #27569

@filimonov
Copy link
Contributor Author

Performance tests don't show any difference because of

@filimonov
Copy link
Contributor Author

Yandex synchronization - maybe i should overload the function instead of changing its interface?

@alexey-milovidov alexey-milovidov merged commit 0ee7b11 into ClickHouse:master Aug 7, 2021
@alexey-milovidov
Copy link
Member

Yandex synchronization - maybe i should overload the function instead of changing its interface?

Don't worry about "Yandex synchronization" at all.

@filimonov
Copy link
Contributor Author

filimonov commented Aug 9, 2021

BTW: it looks like part of the original issue is build-related, on official builds difference is smaller, while still clearly visible:

SET max_execution_time = 0; SELECT count() FROM zeros_mt(1000000000000);

1 rows in set. Elapsed: 10.922 sec. Processed 1.00 trillion rows, 1.00 TB (91.56 billion rows/s., 91.56 GB/s.)

SET max_execution_time = 300; SELECT count() FROM zeros_mt(1000000000000);

1 rows in set. Elapsed: 13.124 sec. Processed 1.00 trillion rows, 1.00 TB (76.20 billion rows/s., 76.20 GB/s.)

I've updated also the numbers in the original message.

@alexey-milovidov
Copy link
Member

Why do you even try to use unofficial builds?
Only official builds are supported. And we give no guarantee about any third-party builds.
Moreover we are confident that third-party builds will work terribly in multiple ways, because they are not covered by our continuous integration, including performance and fuzz testing.

@filimonov
Copy link
Contributor Author

filimonov commented Aug 9, 2021

Why do you even try to use unofficial builds?

It was my local dev build of master using clang-11. I was debugging a different thing.

But the issue exists, just locally i had it more obvious.

@filimonov filimonov mentioned this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-performance Pull request with some performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants