Added polynomials benchmark #17695
Open
Added polynomials benchmark #17695
+312
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
bot
added
libcudf
lamarrr
added
feature request
github-actions
bot
added
libcudf
Benchmark Resultsast_polynomials_float32[0] NVIDIA RTX A6000
ast_polynomials_float64[0] NVIDIA RTX A6000
binaryop_polynomials_float32[0] NVIDIA RTX A6000
binaryop_polynomials_float64[0] NVIDIA RTX A6000
transform_polynomials_float32[0] NVIDIA RTX A6000
transform_polynomials_float64[0] NVIDIA RTX A6000
order 1
order 2
order 4
order 8
order 16
order 32
|
mhaseeb123
reviewed
Jan 9, 2025
cpp/benchmarks/ast/polynomials.cpp
Outdated
Comment on lines
46
to
51
| std::vector<cudf::numeric_scalar<key_type>> constants; | ||
|
|
||
| std::transform(thrust::make_counting_iterator(0), | ||
| thrust::make_counting_iterator(order + 1), | ||
| std::back_inserter(constants), | ||
| [](int) { return cudf::numeric_scalar<key_type>(1); }); |
There was a problem hiding this comment.
Suggested change
| std::vector<cudf::numeric_scalar<key_type>> constants; | |
| std::transform(thrust::make_counting_iterator(0), | |
| thrust::make_counting_iterator(order + 1), | |
| std::back_inserter(constants), | |
| [](int) { return cudf::numeric_scalar<key_type>(1); }); | |
| std::vector<cudf::numeric_scalar<key_type>> constants(order + 1); | |
| std::fill(constants.begin(), | |
| constants.end(), | |
| cudf::numeric_scalar<key_type>(1)); |
There was a problem hiding this comment.
it's temporary in the mean time as we intend to use a random constant generator to fill the range
There was a problem hiding this comment.
Ah I see, makes sense. Thanks for confirming!
There was a problem hiding this comment.
Is it possible for us to implement the full solution in this PR? Otherwise, we need a TODO to track this "temporary" solution. I think this will be forgotten otherwise.
mhaseeb123
approved these changes
Jan 10, 2025
bdice
requested changes
Jan 13, 2025
cpp/benchmarks/ast/polynomials.cpp
Outdated
Comment on lines
46
to
51
| std::vector<cudf::numeric_scalar<key_type>> constants; | ||
|
|
||
| std::transform(thrust::make_counting_iterator(0), | ||
| thrust::make_counting_iterator(order + 1), | ||
| std::back_inserter(constants), | ||
| [](int) { return cudf::numeric_scalar<key_type>(1); }); |
There was a problem hiding this comment.
Is it possible for us to implement the full solution in this PR? Otherwise, we need a TODO to track this "temporary" solution. I think this will be forgotten otherwise.
| std::string expr = std::to_string(constants[0]); | ||
|
|
||
| for (cudf::size_type i = 0; i < order; i++) { | ||
| expr = "( " + expr + " ) * x + " + std::to_string(constants[i + 1]); |
There was a problem hiding this comment.
Do we think it's a legitimate benchmark if we hardcode these constants? I think it would be more fair to the other AST/binaryops if we provided an array of device scalar pointers that must be dereferenced for each multiplication.
A different way to think about this: do we want to JIT a new kernel for every possible set of constants and/or every possible order of polynomial? If we compute multiple polynomials, does that JIT overhead pay for itself, or do we need to assume that we will amortize the JIT overhead across multiple polynomials?
There was a problem hiding this comment.
The transform API is pretty limited presently, We don't support multiple input columns/scalars in the transform API. As we discussed with Spark-Rapids, we would need to support that to meet their needs, but I don't think it would make much of a performance/throughput difference for this benchmark.
No, it wouldn't be okay to JIT a new kernel for each constant, but it would be reasonable to JIT a new kernel for each polynomial order.
If we compute multiple polynomials, does that JIT overhead pay for itself, or do we need to assume that we will amortize the JIT overhead across multiple polynomials?
I don't think it would pay for itself across multiple polynomials, Although there's a program cache, I don't have enough insight into its internals, I'll investigate and get back to you.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This merge request implements benchmarks for comparing the AST, UDF Transform, and BINARY_OP methods by computing a polynomial.
Closes #17561
Checklist