[FEA] Performance improvement for mixed left semi/anti join - #15288
Conversation
Current implementation of mixed semi/anti join probes the built hash table twice -- once to find the output table size and once to build the output. Since the upper bound on output table size is O(N) where N is the size of the left table, we can avoid probing twice and achieve a faster join implementation. This implementation reserves the required upper memory bound, builds the output, and then collects the relevant output rows. This probes the hash table only once. - Closes NVIDIA#15250
github-actions
Bot
added
libcudf
tgujar
changed the title
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Nice. Overall, this is close to what I was hoping for! I have one algorithmic suggestion that would reduce the lines of code. I haven't benchmarked or built my suggestion, though, so feel free to re-measure (I expect compile time and binary size to decrease, but runtime might be a tiny bit slower?).
There was a problem hiding this comment.
Hi @tgujar, I am new to libcudf team (joined just last Monday) but I have gone through the code changes as thoroughly as possible. I mostly understood your new code and I like the changes and the performance improvements so far. I saw @bdice's suggestion on building gather_map as boolean_mask(thrust::sequence, predicate) instead of with copy_if and I would also like to see if it could be done that way and if there are any performance consequences to it. Thank you!
PointKernel
added
Performance
| // gather_map_end will be the end of valid data in gather_map | ||
| auto gather_map_end = | ||
| thrust::copy_if(rmm::exec_policy(stream), | ||
| thrust::counting_iterator<size_type>(0), | ||
| thrust::counting_iterator<size_type>(probe.num_rows()), | ||
| gather_map->begin(), | ||
| [join_type, d_flagged = flagged->begin()] __device__(size_type const idx) { | ||
| return *(d_flagged + idx) == (join_type == detail::join_kind::LEFT_SEMI_JOIN); | ||
| }); |
There was a problem hiding this comment.
suggestion which may not be better, rather than closing over the flagged iterator in the lambda, how about using the stencil form of copy_if
auto gather_map_end =
thrust::copy_if(rmm::exec_policy(stream),
thrust::counting_iterator<size_type>(0),
thrust::counting_iterator<size_type>(probe.num_rows()),
flagged.begin(),
gather_map->begin(),
[join_type] __device__(bool flag) {
return flag == (join_type == detail::join_kind::LEFT_SEMI_JOIN);
});
WDYT?
There was a problem hiding this comment.
fixed, I think this is better, thanks!
| * be relied upon, simply passed to the corresponding `mixed_left_join` API as | ||
| * is. | ||
| */ | ||
| std::pair<std::size_t, std::unique_ptr<rmm::device_uvector<size_type>>> mixed_left_semi_join_size( |
There was a problem hiding this comment.
question: (cc @revans2) Does spark use these APIs for estimating whether a join must be batched because it would overflow memory?
There was a problem hiding this comment.
I want to note here that this function itself reserves memory equivalent to the max output size of the semi join.
|
/ok to test |
- Use default resource for scratch mem allocation. - Use stencil version of copy_if
|
/ok to test |
Initialization kernel can be skipped if we write to flagged array on non-match in hash table.
Remove function calls for mixed semi/anti join size calculation and remove optional output size parameter to conform with changes to API in libcudf
|
/ok to test |
|
/ok to test |
PointKernel
left a comment
PointKernel
left a comment
There was a problem hiding this comment.
Great work! Thanks for helping improve cudf!
|
As a followup, which would be an API-break and a change from the normal (not semi-) joins, I wonder if it is worthwhile removing the |
If this table is involved in another join operation, we would have to first materialize the rows numbers if I understand correctly. I am not sure if the performance difference would be worth the additional complexity. For memory footprint, we could add a prefix sum kernel and only reserve the required memory, but this may not be a good tradeoff |
Well, the gather map that is currently returned is just the sparse representation of the boolean mask. So you would move from doing: To: That puts (in many cases) the responsibility of constructing the sparse gather map on |
|
/ok to test |
|
/merge |
7 participants
Current implementation of mixed semi/anti join probes the built hash table twice -- once to find the output table size and once to build the output. Since the upper bound on output table size is O(N) where N is the size of the left table, we can avoid probing twice and achieve a faster join implementation.
This implementation reserves the required upper memory bound, builds the output, and then collects the relevant output rows. This probes the hash table only once.
This PR also removes the size kernels for mixed semi join and output size parameters passed to the mixed semi join.
Closes #15250
Benchmark Results from cudf repository
mixed_left_semi_join_32bit (New implementation)
[0] NVIDIA TITAN V
mixed_left_semi_join_32bit (Old implementation)
[0] NVIDIA TITAN V
Checklist