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.

Sign up for GitHub

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

Jump to bottom

Add bytes_per_second to compiled binaryop benchmark #13938

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Add bytes_per_second to compiled binaryop benchmark #13938

merged 1 commit into from
Aug 24, 2023

Conversation

Blonck
Copy link
Contributor

@Blonck Blonck commented Aug 23, 2023

To add bytes_per_second, a call to .SetBytesProcessed() with the number of written and read bytes is added to the benchmark.

This patch relates to #13735.

Checklist

@Blonck Blonck requested a review from a team as a code owner August 23, 2023 13:46
@rapids-bot
Copy link

rapids-bot bot commented Aug 23, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write permissions or greater before CI can begin.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 23, 2023
@Blonck
Copy link
Contributor Author

Blonck commented Aug 23, 2023

Hi,
I'm seeking clarity on what qualifies as "processed bytes." While it's clear that for most cases this refers to bytes read and written, certain algorithms might change this definition. Take QUANTILES_BENCH, for instance. The actual number of processed bytes might vary depending on the sorting algorithm used for unsorted input. Although I haven't verified, I suspect it operates on an O(NlogN) complexity here :). However, I guess the calculation of bytes_per_second shouldn't hinge on the algorithm's internals. My suggestion is to count both input and output bytes. Do you have a better recommendation?

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 23, 2023
@davidwendt
Copy link
Contributor

/ok to test

@karthikeyann
Copy link
Contributor

My suggestion is to count both input and output bytes.

Yes. This is right. From the user perspective, only input and output bytes matters.

Add bytes_per_second to compiled binaryop benchmark
937a7c5 
To add `bytes_per_second`, a call to `SetBytesProcessed()` with the
number of written and read bytes is added to the benchmark.

This patch relates to rapidsai#13735.
@Blonck Blonck force-pushed the processed_bytes_bench_binaryop branch from 4e351f9 to 937a7c5 Compare August 24, 2023 16:22
PointKernel
PointKernel reviewed Aug 24, 2023
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

Looks good. Just one small question about type conversion.

cpp/benchmarks/binaryop/compiled_binaryop.cpp Show resolved Hide resolved
@PointKernel
Copy link
Member

/ok to test

@PointKernel
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit d2efb1f into rapidsai:branch-23.10 Aug 24, 2023
@GregoryKimball GregoryKimball mentioned this pull request Nov 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PointKernel PointKernel PointKernel approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@hyperbolic2346 hyperbolic2346 Awaiting requested review from hyperbolic2346 hyperbolic2346 is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Blonck @davidwendt @karthikeyann @PointKernel