-
Notifications
You must be signed in to change notification settings - Fork 409
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
Improve performance of number comparison functions #5670
Improve performance of number comparison functions #5670
Conversation
Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
A solid bench is needed |
Running ./dbms/bench_dbms
Run on (40 X 2399.94 MHz CPU s)
CPU Caches:
L1 Data 32 KiB (x20)
L1 Instruction 32 KiB (x20)
L2 Unified 256 KiB (x20)
L3 Unified 25600 KiB (x2)
Load Average: 48.56, 24.48, 23.63
# before
--------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
--------------------------------------------------------------------------------------------------
NumberComparsionBench/constantEqVector/iterations:100 95193 ns 95195 ns 100
NumberComparsionBench/vectorEqVector/iterations:100 112950 ns 112955 ns 100
NumberComparsionBench/constantLessVector/iterations:100 104847 ns 104850 ns 100
NumberComparsionBench/vectorLessVector/iterations:100 112402 ns 112410 ns 100
# after
--------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
--------------------------------------------------------------------------------------------------
NumberComparsionBench/constantEqVector/iterations:100 60521 ns 60521 ns 100
NumberComparsionBench/vectorEqVector/iterations:100 63688 ns 63687 ns 100
NumberComparsionBench/constantLessVector/iterations:100 58405 ns 58405 ns 100
NumberComparsionBench/vectorLessVector/iterations:100 64027 ns 64028 ns 100 |
Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
change the # before
--------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
--------------------------------------------------------------------------------------------------
NumberComparsionBench/constantEqVector/iterations:100 10734751 ns 10734228 ns 100
NumberComparsionBench/vectorEqVector/iterations:100 19793874 ns 19791053 ns 100
NumberComparsionBench/constantLessVector/iterations:100 11959062 ns 11947725 ns 100
NumberComparsionBench/vectorLessVector/iterations:100 19011253 ns 19001708 ns 100
# after
--------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
--------------------------------------------------------------------------------------------------
NumberComparsionBench/constantEqVector/iterations:100 8810097 ns 8810259 ns 100
NumberComparsionBench/vectorEqVector/iterations:100 16357388 ns 16312559 ns 100
NumberComparsionBench/constantLessVector/iterations:100 9277899 ns 9278057 ns 100
NumberComparsionBench/vectorLessVector/iterations:100 15475154 ns 15475493 ns 100 |
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/run-integration-test |
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
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.
LGTM
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.
Excellent
/merge |
@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: fe5a350
|
@Lloyd-Pottiger: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/merge |
@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/merge |
@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/build |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/merge |
@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/hold |
merge into #5748 |
What problem does this PR solve?
Issue Number: ref #5672
Problem Summary:
What is changed and how it works?
using dynamic dispatch
port from ClickHouse/ClickHouse#37399
Check List
Tests
Result
Release note