Improve contains_column by invoking contains_table
#14238
Merged
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
PointKernel
added
tech debt
improvement
|
Performance comparison between before and after this PR shows that the new implementation can bring up to 2.2x speedups over the old for large data and can be 15% slower than the current code in the worst scenario: Benchmark Time CPU Time Old Time New CPU Old CPU New
--------------------------------------------------------------------------------------------------------------------------------------------------
Search/ColumnContains_AllValid/1024/manual_time -0.0201 -0.0147 0 0 0 0
Search/ColumnContains_AllValid/4096/manual_time +0.1479 +0.1027 0 0 0 0
Search/ColumnContains_AllValid/32768/manual_time +0.3774 +0.2879 0 0 0 0
Search/ColumnContains_AllValid/262144/manual_time +0.1825 +0.1631 0 0 0 0
Search/ColumnContains_AllValid/2097152/manual_time -0.4995 -0.4977 4 2 4 2
Search/ColumnContains_AllValid/16777216/manual_time -0.4629 -0.4627 42 23 42 23
Search/ColumnContains_AllValid/67108864/manual_time -0.5701 -0.5700 82 35 82 35
Search/ColumnContains_Nulls/1024/manual_time +0.0138 +0.0081 0 0 0 0
Search/ColumnContains_Nulls/4096/manual_time +0.0807 +0.0570 0 0 0 0
Search/ColumnContains_Nulls/32768/manual_time +0.4188 +0.3202 0 0 0 0
Search/ColumnContains_Nulls/262144/manual_time +0.4772 +0.4089 0 0 0 0
Search/ColumnContains_Nulls/2097152/manual_time -0.4655 -0.4635 4 2 4 2
Search/ColumnContains_Nulls/16777216/manual_time -0.4141 -0.4139 38 22 38 22
Search/ColumnContains_Nulls/67108864/manual_time -0.5455 -0.5453 77 35 77 35
OVERALL_GEOMEAN -0.1737 -0.1887 0 0 0 0 |
ttnghia
reviewed
Sep 30, 2023
ttnghia
approved these changes
Oct 2, 2023
bdice
approved these changes
Oct 4, 2023
There was a problem hiding this comment.
I've wanted this change for a long time! Thanks @PointKernel. The benchmarks look good for large sizes, despite the minor slowdown for medium sizes.
|
/merge |
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
Part of ##12261
This PR simplifies the
contains_columnimplementation by invokingcontains_tableand gets rid of the use of the cudfunordered_multiset. It also removes theunordered_multisetheader file from libcudf.Checklist