Refactor binaryop/compiled/util.cpp #10756
Conversation
bdice
added
libcudf
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10756 +/- ##
================================================
+ Coverage 86.40% 86.45% +0.04%
================================================
Files 143 143
Lines 22448 22491 +43
================================================
+ Hits 19396 19444 +48
+ Misses 3052 3047 -5
Continue to review full report at Codecov.
|
| * @brief Functor that returns optional common type of 2 or 3 given types. | ||
| * | ||
| */ | ||
|
|
||
| struct common_type_functor { |
There was a problem hiding this comment.
This is similar to the old code, except we remove a layer of dispatch (2 instead of 3 types). We make up for this in get_common_type by doing multiple double-dispatches with this functor in a way that is equivalent to the rules of std::common_type_t for multiple input types.
| } | ||
| }; | ||
|
|
||
| struct has_mutable_element_accessor_functor { |
There was a problem hiding this comment.
We introduce two new functors and methods to dispatch them (has_mutable_element_accessor, is_constructible) that we can use at runtime based on the value of data_type out. The method is_constructible is templated on ReturnType because it is known at compile time based on the invoked return type of the binary operator. However, the output type can be used as runtime information with a single dispatch.
|
Any impact to performance -- the binaryop benchmarks? |
Co-authored-by: Robert Maynard <[email protected]>
|
@davidwendt To your question about benchmarks: |
|
This is great! This file is one of the files which could not be compiled in Windows as well, due to sheer template combinations limit and huge memory usage. Can you write a test on all types to check if old code and new code gives same result for the modified code, or top level |
…mpiled-util-refactor
|
@karthikeyann I wrote the test you requested. All data types and operations behave the same between the old and new code. You can see it here on a separate branch, but it's hacky and I don't plan to merge it because it requires compiling the old (huge) file: bdice@4a59d30 Would you (or a second reviewer) be able to approve this PR now? |
|
@gpucibot merge 🚀 🚀 🚀 (edit: oops, the bot requires an exact match |
|
@gpucibot merge |
|
Looks like the CI bot doesn't get along with rockets. |
This PR reduces the complexity of compile-time dispatches to resolve long compile times and massive memory usage in
binaryop/compiled.util.cpp.The file
binaryop/compiled/util.cppexposes two functions:is_supported_operation(out, lhs, rhs, op)andget_common_type(out, lhs, rhs). I refactored both of them, since they were both expensive to compile.In
is_supported_operation, I replaced a quadruple dispatch (!!!!) on (LHS type, RHS type, binary operation, output type) with a triple dispatch (LHS type, RHS type, BinaryOp) and some runtime single-dispatches to handle the output type.In
get_common_type, I replaced a triple type dispatch on (output type, LHS type, RHS type) with a few double type dispatches. I used the definition ofstd::common_typeto simplifystd::common_type_t<A, B, C>intostd::common_type_t<std::common_type_t<A, B>, C>, which means we can double-dispatch twice and use runtimedata_typevalues in between.Impact: Peak memory usage (max resident set size) when compiling this file drops from 14.6 GB (280acdf) to 2.4 GB (ee2c26a), and the time to compile drops from 2:52.48 minutes (280acdf) to 57.91 seconds (ee2c26a).