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

GH-15200: [C++] Created benchmarks for round kernels. #15201

Merged
merged 8 commits into from
Jan 10, 2023

Conversation

EpsilonPrime
Copy link
Contributor

@EpsilonPrime EpsilonPrime commented Jan 5, 2023

The four existing kernel functions Ceil, Floor, Round, and Trunc gain benchmarks with this change.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

⚠️ GitHub issue #15200 has been automatically assigned in GitHub to PR creator.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

⚠️ GitHub issue #15200 has no components, please add labels for components.

@EpsilonPrime
Copy link
Contributor Author

One thought about the number of benchmarks -- there is a benchmark for every type and round mode (10 each). Some alternatives could be to try to fold the round mode tests into the type (for less visibility) or come up with a "less frequently used" benchmark concept if having less benchmarks is ideal.

@cyb70289
Copy link
Contributor

cyb70289 commented Jan 8, 2023

One thought about the number of benchmarks -- there is a benchmark for every type and round mode (10 each). Some alternatives could be to try to fold the round mode tests into the type (for less visibility) or come up with a "less frequently used" benchmark concept if having less benchmarks is ideal.

It does looks too many benchmarks are generated. And I guess most of them are similar.
How long will all these cases run? If the round modes don't impact performance, I agree that we can only test one of them. And probably few data types, e.g., int64/uint64/double.

@cyb70289
Copy link
Contributor

cyb70289 commented Jan 8, 2023

Do you plan to add benchmark for decimal types? It can be in a followup PR.

@EpsilonPrime
Copy link
Contributor Author

Do you plan to add benchmark for decimal types? It can be in a followup PR.

Done.

@EpsilonPrime
Copy link
Contributor Author

All of the tests together are about 3 minutes. I've selected 3 of the round modes to run normally and disabled the rest using a command line flag.

EpsilonPrime and others added 2 commits January 9, 2023 15:21
Add labels for size and inverse_null_proportion arguments as suggested by wjones127.

Co-authored-by: Will Jones <willjones127@gmail.com>
…ithmetic benchmarks which round was derived from.
@EpsilonPrime
Copy link
Contributor Author

I've updated the arithmetic benchmarks to also have labels. Thanks for the suggestion!

…ts as they will require a much larger change (and should encompass the rest of the benchmarks in this directory which also do not have DECIMAL tests).
@cyb70289
Copy link
Contributor

Travis CI error is not related.

@cyb70289
Copy link
Contributor

Not related to this PR, but the benchmark result is not obvious to me.
Why is double faster than int64? I suppose integer rounding does nothing.

Ceil<Int64Type, RoundMode::DOWN>/size:1048576/inverse_null_proportion:0                      323253 ns       323251 ns        
2161 bytes_per_second=3.02107G/s items_per_second=405.481M/s null_percent=0 size=1048.58k

Ceil<DoubleType, RoundMode::DOWN>/size:1048576/inverse_null_proportion:0                      89834 ns        89832 ns         
7795 bytes_per_second=10.871G/s items_per_second=1.45908G/s null_percent=0 size=1048.58k

@cyb70289 cyb70289 merged commit 85b167c into apache:master Jan 10, 2023
@EpsilonPrime EpsilonPrime deleted the round_benchmarks3 branch January 10, 2023 08:29
@ursabot
Copy link

ursabot commented Jan 10, 2023

Benchmark runs are scheduled for baseline = 33d677c and contender = 85b167c. 85b167c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.33% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 85b167c0 ec2-t3-xlarge-us-east-2
[Failed] 85b167c0 test-mac-arm
[Finished] 85b167c0 ursa-i9-9960x
[Finished] 85b167c0 ursa-thinkcentre-m75q
[Finished] 33d677c4 ec2-t3-xlarge-us-east-2
[Finished] 33d677c4 test-mac-arm
[Finished] 33d677c4 ursa-i9-9960x
[Finished] 33d677c4 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

EpsilonPrime added a commit to EpsilonPrime/arrow that referenced this pull request Jan 10, 2023
…5201)

The four existing kernel functions Ceil, Floor, Round, and Trunc gain benchmarks with this change.
* Closes: apache#15200

Lead-authored-by: David Sisson <EpsilonPrime@users.noreply.github.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Yibo Cai <yibo.cai@arm.com>
EpsilonPrime added a commit to EpsilonPrime/arrow that referenced this pull request Jan 10, 2023
…5201)

The four existing kernel functions Ceil, Floor, Round, and Trunc gain benchmarks with this change.
* Closes: apache#15200

Lead-authored-by: David Sisson <EpsilonPrime@users.noreply.github.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Yibo Cai <yibo.cai@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++][Arrow] Add benchmarks for the round functions.
4 participants