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

Add function alignment for better performance #21431

Merged
merged 3 commits into from Apr 14, 2021
Merged

Add function alignment for better performance #21431

merged 3 commits into from Apr 14, 2021

Conversation

danlark1
Copy link
Contributor

@danlark1 danlark1 commented Mar 3, 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):
Add function alignment for possibly better performance.

Detailed description / Documentation draft:
Let's try to add compression of debug symbols and reduce the binary size by 1.2GB. Also, adding the function alignment for more reproducible results in the performance tests. It is a known technique to reduce such randomness and should potentially help. Either way, let's try and see the build and performance metrics. falign-functions add 0.1% to the binary size which is nothing compared to other clickhouse

@robot-clickhouse robot-clickhouse added the pr-performance Pull request with some performance improvements label Mar 3, 2021
@azat
Copy link
Collaborator

azat commented Mar 4, 2021

Note that this will break path/line numbers in stacktrace (as pointed by @alexey-milovidov in #18952 (comment))

@alexey-milovidov
Copy link
Member

@azat Need to check it - maybe it started to work.

@alexey-milovidov
Copy link
Member

For function alignment - the changes in performance are unclear, maybe it is faster for 0.3% in average. Good to merge.
For changes in stability of perf tests, we should compare two new versions with this change (after merge).

@alexey-milovidov
Copy link
Member

Integration tests (thread) — fail: 1, passed: 1040, error: 24

Broken in master.

@alexey-milovidov
Copy link
Member

ClickHouse special build check — 5/6 builds are OK

Need to disable the flag for Darwin build.

@alexey-milovidov alexey-milovidov self-assigned this Mar 4, 2021
@azat
Copy link
Collaborator

azat commented Mar 4, 2021

@azat Need to check it - maybe it started to work.

@alexey-milovidov yep, I've re-checked before writing, it still does not work.

@alexey-milovidov alexey-milovidov changed the title Add compression of debug symbols and function alignment for better performance Add function alignment for better performance Apr 1, 2021
@alexey-milovidov
Copy link
Member

Ok, I removed compression as we cannot use it for now.
Let's validate the changes in performance.

Previous run showed -0.3% in average, it can be a noise.

@alexey-milovidov
Copy link
Member

There is zero difference in performance. Also looks like no difference in test stability.
There are no queries with statistically significant speedup.

@danlark1
Copy link
Contributor Author

danlark1 commented Apr 2, 2021

Then I am ok not to merge it for now

@alexey-milovidov
Copy link
Member

Note that all performance tests are performed on Xeon Gold CPUs. Maybe there is some difference on other CPUs but we don't know.

@danlark1 do you think we should merge it?

@danlark1 danlark1 closed this Apr 2, 2021
@alexey-milovidov
Copy link
Member

Ok.

@alexey-milovidov
Copy link
Member

@danlark1 said that it just should be better.

@alexey-milovidov alexey-milovidov merged commit 7a2ba95 into ClickHouse:master Apr 14, 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.

None yet

4 participants