Move the binary_ops common dispatcher logic to be executed on the CPU #9816
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
robertmaynard
added
! - Hotfix
robertmaynard
requested review from
vyasr,
revans2,
karthikeyann and
jrhemstad
github-actions
bot
added
CMake
|
Performance comparison: |
|
Binary size change is an increase of 80KB ( from |
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9816 +/- ##
=============================================
Coverage 10.60% 10.60%
=============================================
Files 118 118
Lines 20081 20081
=============================================
Hits 2130 2130
Misses 17951 17951 Continue to review full report at Codecov.
|
ttnghia
approved these changes
Dec 2, 2021
|
Awesome. Surprised that binary size didn't increase much! Following operations still take longer time for 100M size.
JIT and 21.08 compiled binops are in 3ms range for 100M size. We need to add benchmark for both common_type combination and non common type combination to benchmark both kernels now. |
harrism
reviewed
Dec 2, 2021
| bool is_rhs_scalar, | ||
| binary_operator op, | ||
| rmm::cuda_stream_view stream) | ||
| __forceinline__ void operator_dispatcher(mutable_column_device_view& out, |
There was a problem hiding this comment.
Have you tested with inline instead of force inline? We should prefer standard keywords wherever possible.
There was a problem hiding this comment.
I specifically requested this as the regression is caused by a lack of inlining due to crossing a threshold of complexity.
There was a problem hiding this comment.
Actually, this is the wrong place to put it because this is host code.
There was a problem hiding this comment.
I have moved the force_inline to the correct place and re-running the benchmarks.
|
@abellina and others ran through a large set of performance tests for the Spark plugin yesterday and last night. This patch improves the performance quite a bit vs the 21.10 release. We are seeing a median improvement in performance for the queries we ran of about 7.4%. This is great because the same tests run against #9802 showed about a 0.8% improvement. That is a great win. The tests we ran last night were mostly targeting the non-decimal use case, as we were concerned about regressions in performance there. We will likely be doing more performance tests today, but overall this is looking really good. |
revans2
approved these changes
Dec 2, 2021
|
Compile time difference: +4s only! 👍 old - touch binary_op.cuh |
|
Update performance numbers comparing this PR with and without forceinline. In short we see the performance regression for For file size we see a reduction of the binary by |
jrhemstad
approved these changes
Dec 2, 2021
jrhemstad
added
the
5 - Ready to Merge
vyasr
approved these changes
Dec 2, 2021
|
Benchmark Details |
karthikeyann
approved these changes
Dec 3, 2021
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.
Fixes #9813
This builds on the work of #9802, and address #9813
In short we see significantly better performance for
IMBALANCED,ADD,SUB,MUL, andDIVwith no significant change in other binary_ops