Refactor joins for conditional semis and antis #14646

DanialJavady96 · 2023-12-18T17:20:38Z

Contributes to #10039

Currently conditional_joins for both semi and anti joins rely on an implementation that was designed for taking in results from both tables involved in the join. This leads to wasteful allocation that can be optimized for these two cases.

Description

Add a new kernel to be used for both semi and anti joins.
Add some new device functions for adding only one array of shared_memory for caching.

Tests pass on my 3080.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…cate a second output when it is not needed in that context

copy-pr-bot · 2023-12-18T17:20:41Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

cpp/src/join/join_common_utils.cuh

DanialJavady96 · 2023-12-18T17:25:11Z

CC @bdice @vyasr please let me know if changes needed to be made or if i misunderstood anything. I imagine in desire of keeping PRs smaller that this shouldn't touch the size APIs

cpp/src/join/conditional_join.cu

DanialJavady96 · 2023-12-20T18:48:13Z

Did some benchmarking with branch-24.02 and this branch, performance gains were negligible/statistically insignificant(1-3% gains). However, I made some changes by removing the compute_size kernels, and used a pessimistic assumption that the size would always be the left table size N(compromise memory for runtime speed up), and gains were significant

My specs are as follows

CPU: 12th Gen Intel(R) Core(TM) i9-12900K, 3200 Mhz, 16 Core(s), 24 Logical Processor(s)
GPU: RTX 3080.
RAM: 64gb ddr5
OS: WSL2 Win 11 host os

vuule · 2023-12-20T20:12:47Z

/ok to test

GregoryKimball · 2024-01-03T18:49:37Z

@vyasr would you please take a look when you get back?

GregoryKimball · 2024-01-03T19:42:57Z

Please note that this PR addresses part of #10039

PointKernel · 2024-01-03T20:23:05Z

/ok to test

PointKernel · 2024-01-03T20:23:56Z

@DanialJavady96 Making this ready for review to draw proper attention from reviewers

bdice · 2024-04-17T02:11:19Z

Do we need any expanded tests? I'll try to look into that.

Responding to myself -- I think our testing looks okay for now. I don't know of anything that would need to be changed. https://github.com/rapidsai/cudf/blob/branch-24.06/cpp/tests/join/conditional_join_tests.cu

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Co-authored-by: Yunsong Wang <yunsongw@nvidia.com>

harrism

Need to use device_async_resource_ref for MR parameters. Otherwise just a couple of nits.

cpp/src/join/conditional_join.cu

harrism · 2024-04-18T00:34:04Z

cpp/src/join/conditional_join.cu

@@ -348,14 +443,13 @@ std::unique_ptr<rmm::device_uvector<size_type>> conditional_left_semi_join(
  rmm::mr::device_memory_resource* mr)


☑️ todo: ‏ Please use rmm::device_async_resource_ref (not pointer)

See #15498 for details / examples. It should be a straightforward replacement.

@DanialJavady96 I can launch another round of CI tests once this and the below (line 366) mr is migrated to the new async resource ref.

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

bdice · 2024-04-18T22:37:51Z

/ok to test

bdice

A few comments, and one question about exiting early / work reduction.

bdice · 2024-04-19T22:01:34Z

cpp/src/join/conditional_join.cu

+  auto const right_num_rows{right.num_rows()};
+  auto const left_num_rows{left.num_rows()};


Let's choose a consistent pattern between this and the changes below. Either use auto const variables or always use .num_rows(). It looks like the below was refactored to use the method, but this is still using variables.

cpp/src/join/conditional_join_kernels.cuh

bdice · 2024-04-19T22:15:55Z

cpp/src/join/conditional_join_kernels.cuh

+        if (join_type == join_kind::LEFT_SEMI_JOIN && !found_match) {
+          add_left_to_cache(outer_row_index, current_idx_shared, warp_id, join_shared_l[warp_id]);
+        }
+        found_match = true;


Once found_match is true for an outer_row_index, are we allowed to quit evaluating more inner rows for matches? It seems like we should be able to trigger the flush code and then go to the next outer_row_index. Both SEMI and ANTI joins check that found_match is false before adding an outer_row_index, but it seems like they would continue evaluating other inner rows anyway (but they shouldn't have to do so). Does that sound right?

Oh good catch! Yeah there is no need at all for it to continue searching that space after it finds a match. I am really curious about the speed improvements this change will make. Let me benchmark before i push up.

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

ZelboK · 2024-04-22T21:15:08Z

@bdice

Benchmark                                                                                                    Time             CPU   Iterations
----------------------------------------------------------------------------------------------------------------------------------------------
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit/100000/100000/manual_time               311 ms          312 ms            2
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit/100000/400000/manual_time              1126 ms         1126 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit/100000/1000000/manual_time             2748 ms         2748 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit/100000/100000/manual_time               318 ms          318 ms            2
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit/100000/400000/manual_time              1147 ms         1147 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit/100000/1000000/manual_time             2796 ms         2796 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit_nulls/100000/100000/manual_time         415 ms          415 ms            2
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit_nulls/100000/400000/manual_time        1485 ms         1485 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_anti_join_32bit_nulls/100000/1000000/manual_time       3605 ms         3605 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit_nulls/100000/100000/manual_time         417 ms          417 ms            2
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit_nulls/100000/400000/manual_time        1497 ms         1497 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_anti_join_64bit_nulls/100000/1000000/manual_time       3651 ms         3651 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit/100000/100000/manual_time               310 ms          310 ms            2
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit/100000/400000/manual_time              1117 ms         1117 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit/100000/1000000/manual_time             2725 ms         2725 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit/100000/100000/manual_time               316 ms          316 ms            2
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit/100000/400000/manual_time              1142 ms         1142 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit/100000/1000000/manual_time             2782 ms         2782 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit_nulls/100000/100000/manual_time         412 ms          412 ms            2
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit_nulls/100000/400000/manual_time        1482 ms         1482 ms            1
ConditionalJoin<int32_t, int32_t>/conditional_left_semi_join_32bit_nulls/100000/1000000/manual_time       3615 ms         3615 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit_nulls/100000/100000/manual_time         418 ms          418 ms            2
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit_nulls/100000/400000/manual_time        1501 ms         1501 ms            1
ConditionalJoin<int64_t, int64_t>/conditional_left_semi_join_64bit_nulls/100000/1000000/manual_time       3658 ms         3658 ms            1
(pyt_dev) ksm@Kashimo:~/cudf/cpp/build/benchmarks$

Compared to the benchmarks here,

#14646 (comment)

Looks pretty good! Some of the gains are quite significant.

PointKernel

One last styling issue. Otherwise LGTM

PointKernel · 2024-04-23T20:43:06Z

cpp/src/join/conditional_join.cu

-  auto left_num_rows{left.num_rows()};
-  if (right_num_rows == 0) {
+
+  if (right.num_rows() == 0) {


This is the place that @bdice was referring to https://github.com/rapidsai/cudf/pull/14646/files#r1573011742.

Depending on how you see it, we should consistently use either auto const ... or .num_rows() in both functions

I'll change the entire file to be consistent

Ah... Okay. It turns out i was pushing to the wrong fork. That's why there were was some inconsistencies. My bad.

So I won't block merging.

PointKernel · 2024-04-24T17:23:33Z

/ok to test

PointKernel

Thanks for the contribution. Great work!

bdice · 2024-04-30T14:35:49Z

/merge

refactor joins for conditional semis and antis to not wastefully allo…

e9ae4e7

…cate a second output when it is not needed in that context

DanialJavady96 requested a review from a team as a code owner December 18, 2023 17:20

DanialJavady96 requested review from harrism and vuule December 18, 2023 17:20

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Dec 18, 2023

DanialJavady96 commented Dec 18, 2023

View reviewed changes

cpp/src/join/join_common_utils.cuh Outdated Show resolved Hide resolved

DanialJavady96 commented Dec 18, 2023

View reviewed changes

cpp/src/join/join_common_utils.cuh Show resolved Hide resolved

DanialJavady96 commented Dec 18, 2023

View reviewed changes

cpp/src/join/conditional_join.cu Show resolved Hide resolved

DanialJavady96 marked this pull request as draft December 18, 2023 17:31

PointKernel added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed 3 - Ready for Review Ready for review by team labels Dec 18, 2023

Merge branch 'branch-24.02' into feat-joins-refactor-semi-anti

1300bc8

save

6c1030e

GregoryKimball requested a review from vyasr January 3, 2024 18:49

GregoryKimball requested a review from PointKernel January 3, 2024 19:39

PointKernel added Performance Performance related issue 3 - Ready for Review Ready for review by team labels Jan 3, 2024

PointKernel changed the title ~~[DRAFT] refactor joins for conditional semis and antis~~ Refactor joins for conditional semis and antis Jan 3, 2024

Merge branch 'branch-24.02' into feat-joins-refactor-semi-anti

2605529

PointKernel marked this pull request as ready for review January 3, 2024 20:22

DanialJavady96 and others added 3 commits April 17, 2024 09:30

Update cpp/src/join/conditional_join.cu

4bcb922

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Update cpp/src/join/conditional_join_kernels.cuh

6f670f8

Co-authored-by: Yunsong Wang <yunsongw@nvidia.com>

Format and clean up.

000d89d

harrism previously requested changes Apr 18, 2024

View reviewed changes

ZelboK and others added 7 commits April 18, 2024 08:34

Update cpp/src/join/conditional_join.cu

0c2307f

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Update cpp/src/join/conditional_join.cu

756bb4c

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Update cpp/src/join/conditional_join.cu

6dfb7d5

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Address comments, use async memory resource

ebe95f5

Refactor to use async memory resource

4267899

Refactor to async ref mr

ac864f6

Merge branch 'branch-24.06' into feat-joins-refactor-semi-anti

a4457f5

Merge branch 'branch-24.06' into feat-joins-refactor-semi-anti

26feb6f

bdice reviewed Apr 19, 2024

View reviewed changes

ZelboK and others added 2 commits April 19, 2024 19:31

Update cpp/src/join/conditional_join_kernels.cuh

cb89def

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Update cpp/src/join/conditional_join_kernels.cuh

53f23ab

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

Merge branch 'branch-24.06' into feat-joins-refactor-semi-anti

eeecd8a

PointKernel reviewed Apr 23, 2024

View reviewed changes

ZelboK added 2 commits April 23, 2024 17:13

Remove needless iterating over anti/semi joins when condition found.

24c1c52

Formatting

91b79a4

ZelboK and others added 2 commits April 23, 2024 21:40

fmt

6fae49f

Merge branch 'branch-24.06' into feat-joins-refactor-semi-anti

7541016

PointKernel approved these changes Apr 24, 2024

View reviewed changes

bdice approved these changes Apr 30, 2024

View reviewed changes

rapids-bot bot merged commit 5287580 into rapidsai:branch-24.06 Apr 30, 2024
70 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor joins for conditional semis and antis #14646

Refactor joins for conditional semis and antis #14646

DanialJavady96 commented Dec 18, 2023 •

edited by PointKernel

Loading

copy-pr-bot bot commented Dec 18, 2023

DanialJavady96 commented Dec 18, 2023 •

edited

Loading

DanialJavady96 commented Dec 20, 2023

vuule commented Dec 20, 2023

GregoryKimball commented Jan 3, 2024

GregoryKimball commented Jan 3, 2024

PointKernel commented Jan 3, 2024

PointKernel commented Jan 3, 2024

bdice commented Apr 17, 2024

harrism left a comment

harrism Apr 18, 2024

bdice Apr 18, 2024

PointKernel Apr 18, 2024

bdice commented Apr 18, 2024

bdice left a comment

bdice Apr 19, 2024

bdice Apr 19, 2024 •

edited

Loading

ZelboK Apr 20, 2024 •

edited

Loading

ZelboK commented Apr 22, 2024 •

edited

Loading

PointKernel left a comment

PointKernel Apr 23, 2024

ZelboK Apr 23, 2024

ZelboK Apr 23, 2024

PointKernel commented Apr 24, 2024

PointKernel left a comment

bdice commented Apr 30, 2024

		@@ -348,14 +443,13 @@ std::unique_ptr<rmm::device_uvector<size_type>> conditional_left_semi_join(
		rmm::mr::device_memory_resource* mr)

		auto const right_num_rows{right.num_rows()};
		auto const left_num_rows{left.num_rows()};

Refactor joins for conditional semis and antis #14646

Refactor joins for conditional semis and antis #14646

Conversation

DanialJavady96 commented Dec 18, 2023 • edited by PointKernel Loading

Description

Checklist

copy-pr-bot bot commented Dec 18, 2023

DanialJavady96 commented Dec 18, 2023 • edited Loading

DanialJavady96 commented Dec 20, 2023

vuule commented Dec 20, 2023

GregoryKimball commented Jan 3, 2024

GregoryKimball commented Jan 3, 2024

PointKernel commented Jan 3, 2024

PointKernel commented Jan 3, 2024

bdice commented Apr 17, 2024

harrism left a comment

Choose a reason for hiding this comment

harrism Apr 18, 2024

Choose a reason for hiding this comment

bdice Apr 18, 2024

Choose a reason for hiding this comment

PointKernel Apr 18, 2024

Choose a reason for hiding this comment

bdice commented Apr 18, 2024

bdice left a comment

Choose a reason for hiding this comment

bdice Apr 19, 2024

Choose a reason for hiding this comment

bdice Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

ZelboK Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

ZelboK commented Apr 22, 2024 • edited Loading

PointKernel left a comment

Choose a reason for hiding this comment

PointKernel Apr 23, 2024

Choose a reason for hiding this comment

ZelboK Apr 23, 2024

Choose a reason for hiding this comment

ZelboK Apr 23, 2024

Choose a reason for hiding this comment

PointKernel commented Apr 24, 2024

PointKernel left a comment

Choose a reason for hiding this comment

bdice commented Apr 30, 2024

DanialJavady96 commented Dec 18, 2023 •

edited by PointKernel

Loading

DanialJavady96 commented Dec 18, 2023 •

edited

Loading

bdice Apr 19, 2024 •

edited

Loading

ZelboK Apr 20, 2024 •

edited

Loading

ZelboK commented Apr 22, 2024 •

edited

Loading